Disable type_complexity lint by default

[alice-i-cecile]

changelog: [`type_complexity`]: disabled by default by moving from the `complexity` group to the `pedantic` group

The type_complexity lint is currently the most globally ignored lint. Across whole ecosystems (Bevy, Diesel and more), it simply must be turned off by every user and every crate in the ecosystem: clear and idiomatic use of type-heavy frameworks constantly triggers this lint.

In these cases, it encourages replacing straightforward dependency injection code with type definitions that are only used once, misleading beginners into masking complexity with indirection.

Feedback requested: I'm new to contributing to clippy, and I wasn't sure of the best way to disable this by default. Should this be pedantic? Style? It's clearly a "complexity" lint still, but it doesn't appear that we can configure it to be off-by-default without moving it out of the group.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[flip1995]

The question is, is the lint the problem or is it just too trigger happy and we should increase the default threshold:

rust-clippy/clippy_lints/src/utils/conf.rs

Lines 294 to 297 in d43714a

	/// Lint: TYPE_COMPLEXITY.
	///
	/// The maximum complexity a type can have
	(type_complexity_threshold: u64 = 250),

Moving it to pedantic is the right approach. But you'll have to add a #![warn(clippy::type_complexity)] to its tests/ui test file for CI to pass.

[alice-i-cecile]

Increasing the threshold probably won't help tremendously: I don't think there's a good threshold for "normal" Rust that also works well for type-heavy frameworks.

If we wanted to keep it enabled by default, but make it less trigger happy, I would actually suggest that it should only trigger on types that are used more than once.

In these frameworks (like Bevy), types are used to construct one-off requests. In cases where the type is only used once, there's no benefit to defining a wrapper type: the author is merely moving the complexity. With that change in place, this lint could probably stay enabled in most cases, and only locally ignored if needed.

[Manishearth]

Personally I would oppose disabling this lint, I think there is insufficient justification for doing so. Clippy lints are meant to be used with a liberal sprinkling of per-use-case allow() ¹, and this smells extremely like a "per use case allow".

I think most of the ecosystem still prefers small types, using aliases where necessary, but there are a couple frameworks where heavy type chains are a core part of how they work, and I think users of those frameworks are correct to disable this lint. It might be worth having this mentioned in your docs, it is not a code smell to disable a clippy lint for codebase-specific reasons.

Footnotes

1. And clippy would not be able to exist without this design principle, things that do not fall in this bucket are good candidates to uplift to rustc itself. ↩

[alice-i-cecile]

@Manishearth, how would you feel about "the type must be used more than once" as a restriction? I think that would reduce the false positive rate significantly, but it may lead to surprising behavior and adds complexity to clippy's code base.

[Manishearth]

That's a pretty tricky heuristic to implement since clippy largely sticks to local passes, not global ones. It's possible, would be a perf issue.

But also; I not actually sure if I agree with that heuristic either, even if a very complex type is used in one place, I think it's reasonable to want to simplify it in the general case, though not in the case of frameworks which are designed with this kind of type in mind.

FWIW we are open to special casing crates as well, we can do that with a second config and we include some crates by default. This would mean that if a bevy type is found inside a complex type the lint would be suppressed as long as the config option includes bevy (or maybe bevy* or something if we want to do families of crates).

I've also posted this to Zulip to get more feedback.

[alice-i-cecile]

But also; I not actually sure if I agree with that heuristic either, even if a very complex type is used in one place, I think it's reasonable to want to simplify it in the general case, though not in the case of frameworks which are designed with this kind of type in mind.

Yeah, that's my concern too. Things like Vec<Vec<&[HashMap<A, B>]>> stuff is a genuine code smell, even if only used once.

FWIW we are open to special casing crates as well, we can do that with a second config and we include some crates by default. This would mean that if a bevy type is found inside a complex type the lint would be suppressed as long as the config option includes bevy (or maybe bevy* or something if we want to do families of crates).

This would work very well for us: in our case, only the types from bevy_ecs should be ignored. Or more precisely, we only want to ignore:

• Query<Q, F>
• SystemState<T>
• ParamSet

If we had a first class tool for variadics (🥺), skipping over "all variadic types" would probably similarly eliminate this class of false positive. One day.

[Manishearth]

@sgrif Do you have opinions on the proposed heuristics above from a Diesel perspective? Would it make sense to be at the crate level, or with specific types?

@alice-i-cecile Another option if you really care about specific types only (rather than blanket for a crate) is that we could add a #[clippy::complexity_allowed] annotation for crates to apply to their own types, so it no longer is a user config option but rather is a thing crates can opt in to.

[alice-i-cecile]

I love the complexity_allowed annotation: that's very targeted, and gives more autonomy to crate authors to opt-out where they see fit.

[sgrif]

Diesel would probably need it at the crate level, the only type that doesn't end up getting built to arbitrary complexity regularly is the connection. @weiznich can probably give more concrete thoughts than I can though

[Manishearth]

Would it be ok if it was item-level and Diesel just applied it to every item?

Note that the lint would be suppressed by the presence of a tagged type anywhere inside a "complex type".

[weiznich]

Thanks for the ping. It might work for diesel if the lint is suppressed of any tagged type appears inside of a "complex type". (It will nevertheless be quite a bit of work to add the relevant #[allow] to all types, but that's doable if required)

[flip1995]

I really like the idea of a clippy:: attribute, that crate authors can add to their crate or type. I generally like this idea and think we should do this for more lints.

Just allowing tagging items with the attribute is easier to implement in Clippy, I think. But we might be able to also allow an inner #![clippy::type_complexity_allowed] attribute that can be added to an entire module or even crate. (But then again, custom inner attributes are still unstable).

Adding a config to Clippy and providing some crates as a default configuration has the downside, that we would have to decide which crate is "important" enough to include in the default configuration. I'd like to avoid that.

[Manishearth]

Alright, we probably should close this PR but open two issues:

• One for the new attribute
• One for improving the heuristics in general. For example, pointer-to-DST should always just count as a single type (barring generics on the DST itself)