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

Fix a bunch of warnings across the workspace #2066

Merged
merged 3 commits into from
Mar 26, 2024
Merged

Fix a bunch of warnings across the workspace #2066

merged 3 commits into from
Mar 26, 2024

Conversation

aumetra
Copy link
Member

@aumetra aumetra commented Mar 21, 2024

Sorry, the warnings were just annoying me so much when having the project open.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Fixes are fine, but why isn't the CI complaining about those things? Can we make our CI stricter first such that we have those things covered in the future by default?

@aumetra
Copy link
Member Author

aumetra commented Mar 21, 2024

Fixes are fine, but why isn't the CI complaining about those things?

Not sure! My local VSCode config is latest Rust nightly + cargo clippy as the check command.
That's the things it has shown me..

Yeah, I'll have a look at the CI config, that should be caught at least by the clippy step.

@webmaster128
Copy link
Member

Mybe not for all of them, but some of those things are so obvious, they should be cought by Rust or Clippy from 2016. I think we are missing something there on the tooling side if things ...

@aumetra
Copy link
Member Author

aumetra commented Mar 21, 2024

After some googling, I think it's because we don't invoke the clippy tests with cargo clippy --tests since some of the basic warnings (like the unused static assertion traits) are gated behind #[cfg(test)] behind with the basic cargo clippy invocation can't look.

And a bunch of the warnings I fixed were only present after I updated my nightly yesterday, so a bunch of those lints are probably very new.

@webmaster128
Copy link
Member

But we always use cargo clippy --all-targets. Shouldn't that include the test code?

@aumetra aumetra changed the title Fix a bunch of warning across the workspace Fix a bunch of warnings across the workspace Mar 22, 2024
@webmaster128
Copy link
Member

Could you have a look at #2067 first and merge that? Then we can see what's left here coming from nightly. Seems like something related to shadowing for things that are already imported.

@aumetra
Copy link
Member Author

aumetra commented Mar 25, 2024

Shouldn't that include the test code?

I tested it locally and nightly clippy detects it just fine but stable clippy doesn't find a dead code warning for some reason

@aumetra
Copy link
Member Author

aumetra commented Mar 25, 2024

I'm wondering if it's just some new analysis on nightly, where it detects traits as unused if they aren't immediately used as a parameter (i.e. to bind a generic type parameter or to do dynamic dispatch).

I'll have a look into the Rust compiler PRs, I guess

@aumetra
Copy link
Member Author

aumetra commented Mar 25, 2024

Yep, seems like detecting the unused traits is part of rust-lang/rust#118257
So not on stable yet but part of beta

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Thanks

@aumetra aumetra merged commit 7702850 into main Mar 26, 2024
30 of 31 checks passed
@aumetra aumetra deleted the aw/fix-warnings branch March 26, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants