Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty 👎 on silencing clippy lints in code. I think this is a point where it clearly becomes a hindrance rather than a helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if I take a fully opposite position, but I do see value in locally allowing lints.
For one, it's explicitly saying "yes, this may be weird, but it's intended."
Secondly, and more importantly to me, is that a linter is most useful when it's silent. Warnings are a lot more useful when you only have a few and plan to address them all. Not suppressing false positives is how you get warning fatigue and ignoring new lints.
Anecdotal case in point is that the 10 or so clippy warnings this PR fixes were already enough to make me change my check workflow when working with rowan from
clippy
tocheck
. With multiple warnings, some of which I have to ignore, it makes it harder to find the ones that are an improvement to fix.In another formulation, imho, the lack of silencing the clippy lint harms clippy's usefulness more than some
#[allow]
s get in the way of reading code. (In fact, I think they have a (minor) positive that approximately pays for the vertical space they occupy.)The TL;DR is that clippy has false positives, as a consequence of its design. The options are either to allow some lints sometimes or allow the false positives to overwhelm any useful information the linter provides.
That said, I'd be fine yeeting the two problematic commits and either globally allowing those lints or just stopping trying to use clippy on rowan, depending on your preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to get a tad too philosophical, so bear with me.... (but the TL;DR is that I don't find clippy useful in this context).
I feel that the general "use linters" advice is too religiously carried over to Rust. In languages like
TypeScript
, for example, you basically cannot exist without linter, as even the simplest things, like "did I just drop this promise on the floor, really?" are not checked by the compiler. Basically, linterns tend to paper over compiler deficiencies.In Rust, however, everything really important is already handled by compiler. For a user, who is already familiar with Rust idioms, majority of clippy suggestions are just a wash. Like in this PR:
into
s is great (and should really be moved into the compiler)fold
's is great (and probably should be moved into the compiler as well)unwrap_or_else
is, again, slightly negative change, but the differences in utility is tiny in comparisong to the fact that this is a change.The overall theme here is that clippy makes me change my code, without making it better. It creates noise, and I think that noise has a pretty high cost. Similarly, explicit
allows
create noise. They don't show "something's fishy is going on" (b/c these are caught by compiler), they disproportionally highlight code trivialities.What I can get behind is conservative clippy profile, consisting of a lints which are candidates for inclusion in the compiler. IIRC, there's no such clippy profile yet. What we do in rust-analyzer is that we define, globally, the list of disabled lints:
https://github.com/rust-analyzer/rust-analyzer/blob/7a9ba1657daa9fd90c639dcd937da11b4f526675/xtask/src/lib.rs#L107-L117
But we still don't run clippy on CI, as I find that would be mostly waste of time.
Where clippy shines is when teaching new users Rust idioms, but this use case is different.
I guess the bottom line is that I am 👎 on running clippy on CI, and, for this reason in particular, I don't want to see anything just to appease clippy in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chiming in from Clippy: The Clippy philosophy is that sprinkling of allows is encouraged, for the reason @CAD97 already stated: "The following code is intentional."
I think disabling
clippy::style
andclippy::complexity
lints already gets pretty close to what you want from Clippy. Theclippy::correctness
group exists for lints, that catch faulty code and I would recommend to everyone to at least use that lint group in every project (But I'm obviously biased).