rust-clippy
rust-clippy copied to clipboard
rust-lang
Implement `manual_clamp` lint
Fixes #9477 Fixes #6751
Identifies common patterns where usage of the clamp function would be more succinct and clear, and suggests using the clamp function instead.
changelog: [manual_clamp]: Implement manual_clamp lint
r? @Alexendoo
(rust-highfive has picked a reviewer for you, use r? to override)
Will this lint also close https://github.com/rust-lang/rust-clippy/issues/6751 ?
This changes behavior if the clamped variable is NaN. Are clippy lints supposed to suggest things that change the behavior?
It also adds a panic if min is greater than max
It's okay for a lint to suggest something that is different yeah, but the applicability should be set to MaybeIncorrect so that cargo fix doesn't apply it automatically. A note attached to the diagnostic explaining that it may change behaviour would also be good
If this lint introduces a panic it's highly likely the code wasn't functioning as intended to begin with. I think breakage due to new panics is rare enough that MachineApplicable is still preferable. I can add a note to the diagnostic. This concern was already noted in the Known Issues section of the documentation.
I changed my mind, MaybeIncorrect is fine. Automatically introducing panics is a bad user experience and should be avoided even if our confidence level is high.
This concern was already noted in the Known Issues section of the documentation.
The case where the clamped variable (but not min and max) is NaN isn't noted in the Known Issues.
@heinrich5991 Ah, that would be because it doesn't panic if the clamped variable is NaN.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=584a534981248089843c531b4f5b996d
It changes behavior though: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=85aa230985a60f5b17017b4f658a0e35.
NaN turns into NaN with clamp, but into either the lower or the upper bound with .min().max(), and sometimes also in the if case, depending on the order of the operations.
New output
error: clamp-like pattern without using clamp function
--> $DIR/manual_clamp.rs:139:19
|
LL | let x23 = cmp_min(CONST_MAX, cmp_max(CONST_MIN, input));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)`
|
= note: clamp will panic if max < min, min.is_nan(), or max.is_nan()
= note: clamp returns NaN if the input is NaN
:umbrella: The latest upstream changes (presumably #9233) made this pull request unmergeable. Please resolve the merge conflicts.
:umbrella: The latest upstream changes (presumably #9516) made this pull request unmergeable. Please resolve the merge conflicts.
@Alexendoo all of the responses to your review are in this commit https://github.com/rust-lang/rust-clippy/commit/c65d7659c130492d449e7c52c6f0f3204ce46679
Looks great thanks!
If you could give the commits a squash I think it's ready to be merged
:pushpin: Commit b22118457245cdd074daaaaea950ec49397f8712 has been approved by Alexendoo
It is now in the queue for this repository.
:hourglass: Testing commit b22118457245cdd074daaaaea950ec49397f8712 with merge 64243c6f998df7cea3c86488c67481de09fec31c...
:sunny: Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test Approved by: Alexendoo Pushing 64243c6f998df7cea3c86488c67481de09fec31c to master...
Should we have a discussion issue for this lint?
If this lint introduces a panic it's highly likely the code wasn't functioning as intended to begin with
I disagree: x.min(b).max(a) need not imply a <= b. In fact, the order of application of min and max may be important in this case.
As such, I'm not really sure what to do about this lint. It is a useful lint in some cases, while in others it is complaining about deliberate behaviour.
Should users really be expected to write #[allow(clippy::manual_clamp)] on each of these cases? I guess there is one advantage to doing so: documenting that it is deliberately not equivalent to clamp.
My biggest concern however is that this lint suggests a possibly incorrect "fix". Perhaps this lint should be placed into a new (default disabled) category to indicate that it might not be correct?
The lint notes accompanying the warning describe the changes in behavior that the fix it suggests would introduce, and in its final merged form it does not auto apply the suggestion. It's still up to the engineer to determine if clamp is right for them or not.
If a project finds this lint to be undesirable for their entire code base they can always put #![allow(clippy::manual_clamp)] in their root file, though I imagine circumstances where that will be necessary are uncommon.
Clippy is highly configurable which means the default lint set is merely a recommendation. I think this lint is correct often enough to be useful as a default. It doesn't need to be perfectly correct to be a reasonable default, just mostly correct.
Sigh. The social perspective is the problem: a tool suggests a "fix", which can easily lead to people lazily following instructions. I guess at least any problem introduced is observable thanks to the panic.
By the way, this lint fails to detect min/max chains on user-defined types also supporting clamp. Is this intentional? Perhaps (one shouldn't assume too much from method names), but it does limit utility.
Yeah we don't generally lint based on method names alone. For this case specifically types that can't be Ord but still have min, max and clamp inherent methods aren't going to be super common
Sigh
This is no need for this
a tool suggests a "fix", which can easily lead to people lazily following instructions
I can confirm that this happened to us here. Luckily our test coverage was good enough to catch the panic. Worth mentioning, perhaps, that we run clippy in CI and the CI messages don't include the "notes" about panics and NaNs.
Do you have a link to the run where the note is missing? That sounds problematic
Here https://github.com/linebender/druid/actions/runs/3544685050/jobs/5952188360#step:6:1
It looks like the note was displayed.
If you click though to the logs, it is indeed displayed. But on this page, for example, it doesn't show them. And if you click on one of the links in the diagnostics, it brings you to this page, which also doesn't show the notes.
This is possibly an issue with the UX on the CI rather than a problem with clippy itself, but I do think it's worth noting that not all consumers of clippy's messages will be getting the detailed info.
I nominated this PR/lint to talk about in the next Clippy meeting on Tuesday. Having a lint that suggests different semantic behavior warn-by-default has a weird taste to it IMO.
