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 cargo clippy warnings (or --deny warnings we don't like) #80

Open
RyanGlScott opened this issue Feb 24, 2025 · 1 comment
Open

Fix cargo clippy warnings (or --deny warnings we don't like) #80

RyanGlScott opened this issue Feb 24, 2025 · 1 comment
Labels
ci Continuous integration tech debt Technical debt

Comments

@RyanGlScott
Copy link
Contributor

(Spun off from #62.)

We should make an effort to fix cargo clippy warnings where it is reasonable to do so. For warnings that we don't agree with, we should explicitly --deny them so that they do not arise when running cargo clippy.

@RyanGlScott RyanGlScott added ci Continuous integration tech debt Technical debt labels Feb 24, 2025
@spernsteiner
Copy link
Collaborator

My experience with Clippy in the past has generally not been positive. The majority of its suggestions either make no real difference or actively hinder readability. Though I think this was with Clippy's default config - I didn't look into customizing it.

As an example, on another project, we had a MIR analysis with methods for statements, terminators, operands, and rvalues. The body of each method was a big match on the kind, with cases for the variants that analysis cared about, and a catch-all case _ => {} for everything else. Clippy noticed that the match on statements had only one case (the analysis only cared about StatementKind::Assign) and suggested turning into an if let. This would break the parallel structure and give the false impression that the statement visitor is designed differently from the other three visitors. Things that are similar should look similar, but Clippy has no way of knowing which methods are meant to be similar in cases like this.

(I'm opposed to rustfmt for similar reasons - things that are unimportant should look unimportant, but rustfmt often likes to unroll unimportant one-liners into 5+ lines.)

For warnings that we don't agree with, we should explicitly --deny them

--allow them*. --allow suppresses the warning; --deny turns it into a hard error.

I think there's some way to configure Clippy, either through Cargo.toml or a separate config file, to allow (or deny) specific warnings project-wide, so you don't need to pass arguments each time you invoke cargo clippy. You can also put #![allow(clippy::foo, clippy::bar)] at the crate's top level, but mir-json has quite a few binaries that would all need their own copies of this directive, and it would be better to have the settings centralized somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration tech debt Technical debt
Projects
None yet
Development

No branches or pull requests

2 participants