Disabled clippy type complexity lints globally for all crates. #7050
Conversation
|
Note: I still get a few lints from |
alice-i-cecile
added
C-Code-Quality
|
I would prefer to not just allow it everywhere. It was decided to ignore this lint in CI to not slow down development by needing to think about this lint, but as Bevy reach a more stable state I would prefer ignoring the lint to be done on a case by case basis rather than just blanket it. It may make sense to ignore it on some crates like |
alice-i-cecile
added
the
X-Controversial
In my experience, following the type complexity lint when writing Bevy code results in worse code. The only real way around it is judicious use of SystemParam and WorldQuery derives, which increase compile times and reduces clarity. |
I was of the opinion that ecs was a logical target but depending on your editor setup it will lint from the entire root crate.
I can't foresee addressing/disabling it on a case by case basis ever becoming useful. Why try account for it now? Its a very premature action for a scenario I don't think anyone can see arising that results in a very annoying DX. But lets imagine for a second that this scenario arises. In the presence of those future additions the current crates having this lint globally disabled still makes sense because its only in those future additions that addressing that lint would make sense. Those future additions would likely be their own crates ie. they could not have the |
alice-i-cecile
added
the
S-Ready-For-Final-Review
There was a problem hiding this comment.
Generally on board for this global rule (as it is already our policy according to CI). However this feels "redundant". We currently don't use hard-coded clippy lints in each crate and consider the CI-script settings the source of truth.
This PR is attempting to solve the problem of "code editor clippy configuration not matching CI configuration", which feels like a problem we should be solving globally, not hacking around for a specific case. And I don't think the solution should be "mirror global settings into each library", as it creates the challenge of keeping everything in sync and bloats the codebase unnecessarily. And even if we agree that is the solution (and I hope not), then we should treat each library as the source of truth and remove the global CI config. Trying to keep both "in sync" with each other is nasty.
Ideally this would be solved with "global clippy config files" but the Clippy and Cargo teams have been designing / punting that for ages:
rust-lang/rust-clippy#1313
rust-lang/cargo#5034
The embark folks did find a workaround that we should consider:
EmbarkStudios/rust-ecosystem#22 (comment)
|
I've applied that. It's a much better alternative. Even stops the type complexity lints I was getting from the |
| @@ -0,0 +1,2 @@ | |||
| [target.'cfg(all())'] | |||
| rustflags = ["-Aclippy::type_complexity"] | |||
There was a problem hiding this comment.
This still forks the configuration we have in the CI script. We should have one source of truth for global lint configuration. Either it is all here, or it is all in the CI script.
There was a problem hiding this comment.
If we're moving config here, we should also verify that the CI script is still picking up these flags.
| @@ -0,0 +1,2 @@ | |||
| [target.'cfg(all())'] | |||
| rustflags = ["-Aclippy::type_complexity"] | |||
There was a problem hiding this comment.
The big downside to this approach is that config.toml is considered "local user configuration".
Notably, we encourage people to include platform specific linker optimizations here:
[target.x86_64-unknown-linux-gnu]
linker = "/usr/bin/clang"
rustflags = ["-C", "link-arg=--ld-path=/usr/bin/mold"]These linker settings will be per-system config and shouldn't be enabled by default. Currently we .gitignore this file. Checking this in is "controversial" and will almost certainly negatively affect these workflows.
# Objective The clippy lint `type_complexity` is known not to play well with bevy. It frequently triggers when writing complex queries, and taking the lint's advice of using a type alias almost always just obfuscates the code with no benefit. Because of this, this lint is currently ignored in CI, but unfortunately it still shows up when viewing bevy code in an IDE. As someone who's made a fair amount of pull requests to this repo, I will say that this issue has been a consistent thorn in my side. Since bevy code is filled with spurious, ignorable warnings, it can be very difficult to spot the *real* warnings that must be fixed -- most of the time I just ignore all warnings, only to later find out that one of them was real after I'm done when CI runs. ## Solution Suppress this lint in all bevy crates. This was previously attempted in #7050, but the review process ended up making it more complicated than it needs to be and landed on a subpar solution. The discussion in rust-lang/rust-clippy#10571 explores some better long-term solutions to this problem. Since there is no timeline on when these solutions may land, we should resolve this issue in the meantime by locally suppressing these lints. ### Unresolved issues Currently, these lints are not suppressed in our examples, since that would require suppressing the lint in every single source file. They are still ignored in CI.
Objective
Solution
Added#![allow(clippy::type_complexity)]to alllib.rsfiles incrates.cargo/config.tomlwith the following as suggested here.Rationale
Addressing it just fragments already existing information.
When every deviation from a default is for the same reason that default is not a suitable default any more. This clippy defaults is not suitable for bevy as what would typically be considered "complex" in most rust projects is very common in bevy. This I think is a better default.