When should code be tweaked to comply with Clippy lints?

The fears that Rust successfully removes (regarding aliasing control and thereby data races) are not the only relevant ones. Rust has nothing to offer against deadlocks in concurrent programs, which is fine because it is not Rust’s fault, but touting “fearless concurrency” gives a false sense of security. I know very good software engineers who say “just put it into Arc/Mutex to avoid all the difficulties”, and I’ve had to debug the resulting partial program freezes as well.

But let’s end this tangent here, as it is off topic.

True. However I don't feel that discredits the "fearless concurrency" slogan. Deadlocks are a logic error like any other, like inadvertently creating an endless loop or infinite recursion. Of course we don't expect Rust to be able to prevent that. Clearly the very good software engineers you refer to were not as good as they appeared.

I don't think discussing this is off topic at all. The topic is about clippy and claim is that Clippy makes suggestions that change program behaviour. In this case possibly introducing deadlocks. What if it were some other program behaviour that were changed, like introducing endless loops or infinite recursion? I suspect most people would think that is bad, right?

My feeling is that Clippy suggestions should not change program behaviour, that it is a bug in clippy. Perhaps it's impossible to avoid given the possible level of program analysis and without knowledge of what the program is actually supposed to do, which only the programmer knows. I don't know.

While we do not yet agree on the topic of deadlocks (which I still think is a sufficiently different discussion), we do agree that clippy needs to be conservative in the absolute and technical sense. IIUC this makes the needless collect lint illegal in most, possibly all cases, because it changes the iterator’s lifetime (by which I mean when it is dropped, not the erased language lifetime).

My second point is that that is not enough to make it maximally useful: the churn imposed on default-conforming codebases needs to be reduced, the current amount of changes required in this scenario (which I often encounter) is too high.

I do agree that having new lint warnings turn up all the time as the linter changes or the Rust language itself changes could be inconvenient. Is there no way to freeze the set of lints one uses at some point in time and only take on board new ones if and when you want to?

Perhaps it is off topic and requires a new thread but I'm now wondering how removal of collect is causing deadlocks? My understanding of collect is that it is used to take the output of iteration over something into a collection (of possibly something else). Which of course requires allocation of the new collection. If that new collection is not actually used subsequently removing the collect should not cause any change in program behaviour. As in the "bad" example given for the lint.

Do you have a short and sweet example?

Perhaps using collect solely to prevent deadlocks is not the best way to do it?

Actually I can't get the "bad" collect example to produce the needles_collect lint. This:

    let v: Vec<i32> = vec![1, 2, 3];
    let iterator = v.into_iter();
    let len = iterator.clone().collect::<Vec<_>>().len();
    println!("Length = {len}");

produces this:

warning: redundant clone
  --> src/main.rs:23:23
   |
23 |     let len = iterator.clone().collect::<Vec<_>>().len();
   |                       ^^^^^^^^ help: remove this

Actually, the downgrade was back-ported all the way to (about to be released soon) stable 1.67.1 now. Fascinating. The temporary-ness of this change still stands; as far as I understand, the main problem at the moment was bad Rust-Analyzer support, not any general stylistic considerations of “inconsistencies” as the original post in the original topic was concerned with; e.g. operations such as variable renaming would not properly find and modify the name inside a format!.

I don’t have the concrete examples anymore, the general shape of one that I recall is this:

let things = x.lock().iter().filter_map(whatever).collect::<Vec<_>>();
for a in things {
    y.lock().do_the_thing(a);
}

Obeying clippy would have led to the lock on x being held while locking y, which was against the locking protocol for these objects (would have caused deadlock when it happens in conjunction with some other code — getting lock discipline right is tough!). The point is that Rust’s type system does not capture all information necessary to exclude this bug, so there is no way that clippy could make this call. My worry is that while I may have a trained eye to see this problem before it causes damage (in the form of machines stopping in a factory somewhere, causing great losses), some other people on the team would happily apply the clippy suggestion. Tests exposing this particular bug are very hard to write in a deterministic fashion, so a comment was all I could do — and a comment would be useless against a clippy --fix commit (don’t know whether this one would have been “fixed”, but it is conceivable).

1 Like

#[allow(clippy::whatever)] along with the comment would probably be better, I guess?

The issue is that when the code was first written and then debugged, the clippy lint hadn’t yet entered the picture. It was just lucky coincidence that I was the one fixing clippy issues when the lint was added to the default set.

1 Like

Yes, it seems that the direction that clippy has taken is more like "give me all the lints" instead of something principled, in which it can attempt to reason about semantics, and be reasonably confident that its suggestions do not affect program semantics.

Few linters attempt to do that kind of thing, as far as a I know, but most linters are not nearly as aggressive as clippy with their default settings. Whichever type of linter clippy aspires to be, I don't think it makes sense to have both (aggressive lints and weak-to-none formal reasoning of correctness).

Surely y'all aren't blindly checking clippy fixes without examining each change for correctness? right...? Surely the clippy gods could not steer us wrong?

But on a serious note, the prevailing dogma of "just apply clippy fixes and your code will be better" is harmful, but one cannot be blamed for believing this is safe to do! Within this community, clippy is presented as a much more robust tool than it really is. I wonder if this sort of misrepresentation in part contributes to the state of clippy - after all, how are the contributors supposed to know there are these stability/correctness problems, when so much of the community here is emphatic that clippy is always right?

Having been tenaciously reading this forum for three years now I have not detected such "dogma" or such advice. On the contrary, many pass through to dispute clippy suggestions or the very notion of it's universal use.

I like to think that nobody is daft enough to "just apply the fixes". All changes require understanding and testing.

Anyway. Out of curiosity and boredom I'm still trying to trigger the "needles_collect" clippy lint. Can't get the "bad" example in the docs to work. As per When should code be tweaked to comply with Clippy lints? - #24 by ZiCog or anything else.

How is it done?

I think that, while clippy is an amazing project, it only works the way it does because that was the best option for it at the time, not because it's how people really want to see its complaints.

(I probably don't feel as strongly as the below is written, but for didactic purposes it's easier to write the strong version than to try to add various caveats.)

I think the best end-game is that most projects no longer use clippy. Instead, it splits into three parts:

  1. Compiler-supported attributes for common patterns, so that you get the lints from running normal rustc. For example, iterator_step_by_zero is a great lint that's always correct, so we shouldn't make people use clippy to get it. There should be a compiler-supported thing (strawman #[panics_if("step == 0")]) that can be put on the signature of the Iterator method in core, and the normal compiler will check it for you. And then every library can take advantage of lints by adding them to their own code, rather than needing clippy to balloon to "we know about every API in every library ever".

  2. Stylistic suggestions in Rust-Analyzer. I don't ever want my CI to fail for something like collapsible_else_if, nor do I want that spammed onto my command line -- I plausibly wrote it like that on purpose to emphasize similarity between the arms. What I do want for something like that is an easy-and-quick to apply IDE fix for this so that I'm reminded that it's an option that I can do to clean it up quickly as I'm writing it in the first place. This is also the right place for the "remember, you could have used map here" kinds of things.

  3. Then clippy slims down to basically just restriction kinds of lints. Things like "don't use floats" that are opt-in per-project things, and which might be best done with a toml configuration in a workspace, rather than per-crate attribute kinds of things.

5 Likes

Frankly, any project which is gating CI on warnings but not pinning the toolchain version used in CI for those warnings is doing it wrong. All Rust tooling has explicitly stated that adding new nonfatal lints is a minor change[1]. When you bump the toolchain version used for lints in CI is when you would fix (or allow) any outstanding new default lints.

Additionally, since the Cargo team doesn't have sufficient bandwidth, the clippy team is again considering adding workspace-level lint level configuration directly into clippy[1:1]. Alongside that, they're separately considering adding in configuration to only enable lints that existed before a certain toolchain version, to limit the need to pin clippy in CI[2].

Mainly though, clippy relies on user reports to calibrate lints. It's intended for projects to disable lints where they're less helpful, though if it's a large number of projects disabling the lint, that's rightly taken as an indication that the lint is overtuned currently. In the other direction, though, clippy prefers to bias towards turning most reasonable[3] lints on by default, as a nondefault lint may as well not exist in many cases[4].

It's fairly objectively better (modulo IDE support) to replace format!("{name} laughed", name=name) with just format!("{name} laughed"). Between that and format!("{} laughed", name), I prefer the former for consistency, and thus agree with the lint.

However, I do find it plausible that given community feedback, the lint may be split between the "stutter" case and the positional case. If you feel strongly about this (or any other clippy lint) in a global context, please do open an issue with details on how the lint could be split/refined such that you no longer will feel the need to globally disable the entire lint on all of your projects.


  1. The ideal situation is for this to be done at the Cargo level, with cargo passing lint level arguments to the compiler invocation. With Cargo in charge, the configuration would work for rustc and rustdoc lints as well as clippy lints. ↩︎ ↩︎

  2. Although I will continue to tell people to pin style/lint CI, as old lints can still get more accurate to trigger in more cases, and the other toolchain lint sources don't have any such mechanism. Getting access to lint bug fixes automatically is great, and definitely should be preferred for informational CI jobs and local development, but anything used to gate CI should always strive to isolate the success of CI from external state as much as possible. ↩︎

  3. Although reasonable people can of course still disagree on how reasonable a lint is. The cutoff point in false positives and/or signal/noise ratio to classify a lint as pedantic or not is fuzzy at best. ↩︎

  4. There's multiple existing rustc allow-by-default lints which get suggested for "new" lints fairly commonly, because the discovery of these nondefault lints is basically non-existent. ↩︎

2 Likes

The needless_collect lint was moved to the nursery group which indicates it as still in development and allowed by default a few months ago due to issues like this. You need to enable it manually now if you want to use it.

Move needless_collect to nursery by sophiajt · Pull Request #9705 · rust-lang/rust-clippy · GitHub

1 Like

Thanks for your informative post, it does contain useful guidelines for how to better manage a project’s clippy usage.

As far as I can see, while formally replying to my post, you have not answered either of the two points I raised. Having the ability to choose the timing of when to catch up with newer versions is nice, but it doesn’t change the fact that the amount of churn per version is still too high — and the point of the exercise is to follow along, not to defer updating indefinitely.

While you’re here, I would very much like to hear what you think about my third point that so far I merely hinted at. The Rust language is very principled, no-nonsense, precise, sound; it has an ecosystem around it inspired by these qualities. Clippy is the only part of this ecosystem that feels unpleasant, it forces me to make stylistic changes that are arguable, and it sometimes is not conservative (as in: breaks the program). I feel bullied, and I know that more than half the Rust developers I know feel this too. We suffer this because clippy still is a net positive, but obviously this situation does taint the experience.

The density of new lints will by necessity decrease over time, as the linting tool gets better. For the majority of new lints, the suggestion was still idiomatic practice before the lint was implemented, it's just that implementation of the lints necessarily lag behind the development of idiomatic practice. uninlined_format_args is the less common case of pushing to use new style.

The churn of new idioms isn't in clippy, it's in the language and stdlib introducing new, better ways of doing things. These are "handled" by setting the msrv setting in clippy.toml, so clippy won't suggest any idioms which were introduced after the set rust version. This doesn't have to match your actual MSRV, either! You can just set it to whatever version the library started with to bypass new versions' idioms, separately from the rust-version key in Cargo.toml.

Emphatically no, clippy's design is expecting you to #[allow] lints when they don't apply. Rustc lints have a very strict expectation of little/no false positives. Clippy lints have a soft expectation of signal/noise, but explicitly allows false positives.

Lints which suggest changing behavior are generally either explicit about doing so (e.g. replace .or with .or_else says roughly "to only run the operation when present", and if you want it unconditionally it's typically better to hoist the computation out), or considered a bug (e.g. needless_collect being downgraded to nursery).

[joking] There's a reason that there's the meme/stereotype of Rust developers being subs and the compiler/tooling being the dom in the relationship... [/joking] and in truth, it's about expectations. The expected expectation of using clippy is that it will be an opinionated form of automated code review, and that it will be wrong sometimes. If your expectations of the tool differ from this, there will be disconnects.

3 Likes

Consider this program:

fn foo(_: &str) {}

pub fn bar(v: Vec<String>) {
    for s in v {
        foo(&*s);
    }
}

Clippy 1.64 added the explicit_auto_deref lint, enabled by default:

warning: deref which would be done by auto-deref
 --> src/lib.rs:5:13
  |
5 |         foo(&*s);
  |             ^^^ help: try this: `&s`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#explicit_auto_deref
  = note: `#[warn(clippy::explicit_auto_deref)]` on by default

Prior to that, there was no warning, and the “idiom” which I wrote was to not always rely on auto-deref but rather to use &* in many of these situations (note that the use of String in particular above is just for the sake of a minimal example), since it specifies exactly the transformation I expect to happen.

This example is not a (recent) language change, and I doubt that there's an existing consensus among Rust developers that all *s that can be omitted should be. Or perhaps there is, and I merely didn't hear about it when I learned Rust; either way, the result is that I experienced “churn” that was caused by changes in clippy and not changes in the language.


My own take on this situation is that Clippy is generally okay, but I think that it would benefit from some additional review process (for any lints that will be warn-by-default) to gain some amount of consensus of “is this lint a good idea to show to everyone that uses Clippy” rather than the current “sounds nice to the PR reviewer” process. Perhaps the existing Final Comment Period process (which is made visible to the general Rust-using public via TWIR) would be an adequate fit.

5 Likes

I’ll first answer technically and then regarding UX. For exceptional cases (I recently had a case of “this default can be inferred” that was false) we add #[allow] attributes that in the correctness cases also serve as documentation of the edge case. When a lint goes against local coding practice we should just switch it off. But when a lint is sometimes helpful and sometimes not? Littering the code with clippy attributes certainly is not the way (I like what @scottmcm said on this topic!).

In my codebases I don’t tolerate warnings because I want to notice every single new one, and many other people do the same. When you say “clippy is expecting you to #[allow] lints” I must note that the current tooling doesn’t convey this expectation by making it easy to oblige. When using eslint for JS in VS Code the time between seeing a linter warning and confirming that it doesn’t apply (only here or in the whole file) takes three or four finger movements. In contrast, in Rust it takes quite some typing, mouse hovering, copy-pasting, it is a much larger task that interrupts the review flow.

That is fair and logical. There is a social aspect to this, though, that has a technical part: with cargo tooling being by far the best I’ve ever seen for a programming language and its ecosystem, it is natural to expect that starting a project will yield reasonable defaults that guide me in the right direction. This intuition is reinforced by seeing the compiler (with its very helpful warning & error messages) and clippy in action.

The expected expectation you state is unfortunately not conveyed in this process.


My personal takeaway so far is:

  • project setup should include adding clippy.toml (I have rarely seen this file!) to fix the minimum Rust version
  • I am expected to more rigorously slap #[allow] attributes on all crates (which is a lot of work, but you hint that it may become per-workspace at some future time)

The takeaway for clippy is:

  • there is currently no good solution for revisiting rejected clippy suggestions later (i.e. I may want to listen to the opinions from time to time without them being warnings — permitting warnings in CI is definitely not sustainable, we’ve tried that)
  • it needs to become way easier for the developer to ignore a lint or a linter warning
  • default project setup should incorporate the aforementioned best practices automatically

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.