Linter pushing it too far: `return_self_not_must_use`

While the title may sound like a detail, this topic has been on my mind for a while and this was just the last drop. Rust’s linter is firmly integrated with the language and the whole developer experience, hence I think it is fair to say that it is part of the language design — beginners will see the same remarks as I do (since I don’t usually deviate from the default settings).

The general point I’d like to make is that for lints to remain generally useful (i.e. not just switched off by most people), they must err on the side of caution: only suggest a change to the programmer if there is a compelling reason to do so — a “good” reason should not be enough in my opinion.

Now, the trigger named above may be just an actual bug in beta (which I’m using to work around a compile-time regression that cannot be ignored), namely that the builder pattern now requires #[must_use] annotations all over the place even though there is no benefit in having them (since self is consumed, the programmer cannot possibly use the API incorrectly).

Another case that frequently irks me is being nudged to always compute fallback values in closures — even when the function being called does not perform any harmful amount of work, doesn’t allocate, etc.

.unwrap_or_else(|| Config::new(12))

is just increased noise when compared to

.unwrap_or(Config::new(12))

So what I’m asking is that we demote lints that are “good but not compelling” from default to pedantic and install this high bar for additions to the default set of lints. Am I far off track?

2 Likes

Rustc lints already have an high bar for additions, Clippy lints on the other hand are far more opinionated. I don't think this is in general a problem, and I also don't think this is in particular your problem.

return_self_not_must_use has only been added less than 2 months ago, so people just found the many false negatives recently. And in fact less than a week ago it has been moved to pedantic Move `return_self_not_must_use` to `pedantic` by xFrednet · Pull Request #8302 · rust-lang/rust-clippy · GitHub

12 Likes

That’s good to hear — I somehow missed that clippy is not in the main repo, mainly because I consider the whole package as “the Rust experience”. One thing I didn’t write above is that I find it tremendously useful to require -D warnings in CI so that our code always stays clean and consistent — the vast majority of lints (regardless of technical tool source) are beneficial.

So, to refine my statement in light of what you reminded me of (thanks!): not only rustc lints should have a high bar, clippy lints should have a high bar too (higher than in the past). My argument is that clippy lints are very useful, but only as long as they have a rather small false positive rate.

Note how even though I contributed to clippy some time ago, even I forgot the difference between rustc and clippy. This could be evidence for my forgetfulness or for the fact that the technical split into two tools is arbitrary and mostly irrelevant for end users.

FWIW, it's incredibly relevant to me because I usually don't run clippy on my code. I'm not convinced that it's a good idea to -D clippy::warnings in CI, since so many of them are "well, maybe".

I think what I really want is for many of them to become rust-analyzer things, where I get a subtle hint in an IDE that there's something I might want to change in that point, but not something that I ever see all of them on the command line.

Is it a const fn? If so, it might be reasonable to allow const fns through that lint.

6 Likes

My opinion on this is we should be very careful with any lint which has second order effects, i.e. even users who don't run clippy still suffer the wrath of #[must_use] it is recommending.

3 Likes

While const fn is a useful indicator that this can probably be const folded away, it's not a guarantee. If the const fn doesn't get inlined (e.g. it crosses compilation units without #[inline]) then there's still going to be an amount of work done to call the function at runtime. And we're quickly approaching the point where this is an actual concern, with const in traits and if/when we get const-internal allocation.

An actual const { } block, however, forces const evaluation and is just copying from a const item. (That said, it could probably be a good MIR optimization pass to take const fn which are not in a const context but which could be (wrapping them in const { } would not cause a compilation error) and const fold them before it ever even hits LLVM. This of course has the standard inlining tradeoff of that you have to store the inlined value, which could be much larger than the values needed to generate it, and rustc doesn't have access to the heuristics LLVM uses for when inlining is a good idea, but the availability of such a pass I think would be beneficial. And then this lint could ask "does it get trivially const folded by MIR optimizations" before warning.)


edit: :cake: day

2 Likes

That's a good point. Once we get const {}s allowing those would be fine.

That said, there's enough nullary const fns where it's really not worth linting like this -- such as Vec::new(). It might be fine to just let those be caught by a profiler if they're important enough to matter. To me the important part of that lint is in avoiding observable side effects more than about the speed.

2 Likes

@scottmcm What is your argument for deeming only const fn permissible in computing alternative values? I do agree that such computation’s side effects need to be controlled (either way), but Rust’s type system doesn’t have the tools to automate this process (well, no language I know of really has a grip on this problem, yet), so it can’t be clippy’s job to do this — it would be an incompetent advisor.

My point is that the programmer should choose. Making the code slightly more readable may be more desirable than saving 5 cycles during execution — it depends entirely on what the program is doing.

The same goes for the other lint that prompted me to write this thread (there’s also a discussion at rust-clippy now). On the psychological side, I’ve seen clippy become more and more “like a nanny” over the past four years, which doesn’t feel right. I’m an adult programmer who can make their own choices, and I’m glad that I don’t have to explain all of them to the compiler. I just want the compiler to check for some classes of mistakes (which classes I want corresponds to my choice of programming language), and I want the linter to check for some common pitfalls, but I don’t want to be nudged towards someone else’s tradeoffs that are likely to be incorrect for the project I’m working on.

There’s a whole class of linter uses which I’m leaving out of this discussion: the other purpose of a linter can be to locally enforce style rules for a project or team, which is opted into by running checks on CI. That — and the corresponding lints — is a separate concern, not relevant here.


The point @ratmice raises deserves careful consideration as well. A linter’s hints may make the API of a library more cumbersome to use, like a hammer that has too many protective circuits and entirely refuses to hit the nail because the latter must be held in place by two fingers.

2 Likes

I'm using it as a plausible heuristic to help be less noisy in places where it's more likely to have minimal impact.

Is it perfect? No. But it exists today in stable, so it could easily be used.


To the other things,

  1. As I previously said, I solve this by just not using clippy. To me the really critical ones are in rustc, and more and more the really important ones are moving anyway. I can always run clippy once in a while and make my own choices of whether I want to apply them, then go back to ignoring it against most of the time.

  2. It's expected that you choose to ignore some clippy lints. If you don't like this one you can always just #![allow] it at the crate level.

  3. The extra || stuff really doesn't bother me that much. It's just not that bad. And in places where I do consider it bad, I'd generally want to solve it not by computing it anyway, but by using something like coalesce!(a, b, c) (instead of a.unwrap_or_else(|| b.unwrap_or_else(|| c))) anyway. No, that macro doesn't exist in std, sadly.

This strategy still requires opt-outs (somewhere, presumably in the code so it doesn't get outdated) to avoid constantly being reminded of the same thing. Considering that + there are people who use plain text editors, I don't see a significant difference — save whether Clippy warnings are resolved immediately or at some arbitrary point in the future.

Resharper handles this well, actually, for C#. Every lint you get to pick a severity, including just "hint" which only shows it as a subtle ellipsis under the text and doesn't show it as a problem in the file.

So, for example, it has a "rewrite for loop as iterator" option that I just leave as a hint. Sometimes I click it on the for to see what it does, but usually I find it uglier (since I'll generally have written the iterator version in the first place when it's reasonable), so undo it. And then the little ellipsis on it is fine for it to just stay there.

And yes, that's not something that would work well as a lint display on the command line. But that's fine. We can have features that only work in IDEs, so long as they're not essential.

The first point is evidence that clippy went too far for you, just as this thread is evidence that it is about to go too far for me, too. This would be a shame since clippy was a very useful tool when I started with Rust (around 2017), and I think it could be useful to more people again.

On the second point there is a long-winded discussion and as far as I know no solution: since the selection of lints to opt out of is a personal (as evidenced by your third point) or team choice, it is tedious and error-prone to require every crate’s lib.rs to contain this list. The difficulties mentioned in that thread are real, of course, like how to get reproducible build results across different people’s machines. To my mind, this finding suggests that opt-in would be more suitable than opt-out for those lints that are not compellingly or ubiquitously useful, because less people are annoyed by the absence of a useful lint than by the presence of a useless or (locally) harmful one.

I just came across another example of a clippy hint that is actually harmful:

let things = the_thing.lock().iter().map(...).collect::<Vec<_>>();
for thing in things {
    ....
}

Clippy suggests to directly iterate instead of collecting — which will of course hold the lock while the loop is executed. In the best case this causes other threads to wait, in the worst case it can deadlock.

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.