Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suppress the clippy::type_complexity lint #8313

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Apr 6, 2023

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.

@JoJoJet JoJoJet added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Meta About the project itself labels Apr 6, 2023
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actively despise needing to copy this everywhere / keep it in sync. But Rust really doesn't give us any other better options here (which is why #7050 got locked up). Maybe rust-lang/rfcs#3389 will save us eventually.

That being said, I still think we should have exactly one source of truth for this. If we're committing to this suppression in every crate, lets remove this from the cargo run -p ci validation list to avoid disparities.

Fix that and I'm on board.

@cart
Copy link
Member

cart commented Apr 6, 2023

That being said, I still think we should have exactly one source of truth for this. If we're committing to this suppression in every crate, lets remove this from the cargo run -p ci validation list to avoid disparities.

Ahhh this is how we suppress example validation in CI. Sadly we need to live in this nasty middle ground world.

@cart cart added this pull request to the merge queue Apr 6, 2023
Merged via the queue into bevyengine:main with commit 3ead10a Apr 6, 2023
@JoJoJet JoJoJet deleted the suppress-lints branch April 7, 2023 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Meta About the project itself C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants