Implement manual_clamp lint

[Xaeroxe]

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

[rust-highfive]

r? @Alexendoo

(rust-highfive has picked a reviewer for you, use r? to override)

[kraktus]

Will this lint also close #6751 ?

[Xaeroxe]

Yup!

[tbu-]

This changes behavior if the clamped variable is NaN. Are clippy lints supposed to suggest things that change the behavior?

[Alexendoo]

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

[Xaeroxe]

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.

[Xaeroxe]

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.

[heinrich5991]

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.

[Xaeroxe]

@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

[heinrich5991]

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.

[Xaeroxe]

Ah, okay thank you for the demonstration. I'll note that.

[Xaeroxe]

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

[bors]

☔ The latest upstream changes (presumably #9233) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #9516) made this pull request unmergeable. Please resolve the merge conflicts.

[Alexendoo]

Thanks for waiting, had a good look at it today

The logic is great. Sorry for the large dump, most of it is style related 😄

[Xaeroxe]

@Alexendoo all of the responses to your review are in this commit c65d765

[Alexendoo]

Looks great thanks!

If you could give the commits a squash I think it's ready to be merged

[Xaeroxe]

@Alexendoo Squished!

[Alexendoo]

Awesome, thanks @Xaeroxe!

@bors r+

[dhardy]

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?

[Xaeroxe]

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.

[dhardy]

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.

[Alexendoo]

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

[jneem]

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.

[Alexendoo]

Do you have a link to the run where the note is missing? That sounds problematic

[Xaeroxe]

Here https://github.com/linebender/druid/actions/runs/3544685050/jobs/5952188360#step:6:1

It looks like the note was displayed.

[jneem]

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.

[flip1995]

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.

[Xaeroxe]

If the group wants to adjust this lint to avoid this then I'd suggest splitting into two lints. One lint, which is warn by default, will only trigger if it can be determined at compile time that max is always greater than or equal to min, and never NaN. The other lint, which is allow by default, will retain the current behavior of this lint.

[hbina]

Does clippy not have an as-if policy for their lint suggestions? I am going over the logic and this definitely have different semantic right?

[dhardy]

Replacing x.max(lower).min(upper) with x.clamp(lower, upper) is equivalent if and only if it is known lower ≤ upper. Otherwise the latter may panic while the former never does (whether the result is intended is another matter, but Clippy has no right to assume that it is not).

Then again, this is the primary reason why Clippy lints are not in rustc, so no complaints there. Perhaps the lint simply isn't clear enough that the suggestion is only applicable when it is expected that lower ≤ upper? (I.e. my recommendation would be to ignore the lint unless it is known that this should be the case.)

[flip1995]

Does clippy not have an as-if policy for their lint suggestions?

There is no written policy like that. But when producing a suggestion we usually try to only suggest things with the same semantic behavior. There are a few cases, where we suggest different semantic behavior (panic where it wouldn't panic with the previous code. But if we do this, the original code would have caused really unexpected behavior if it had hit that case. I don't have an example for that at hand, because it is really rare.

However we agreed in the last meeting that the suggestion of this lint, is a bit beyond that criterion. So we decided to move it to nursery until we had time to figure out how we want to treat this lint. (We haven't done that yet though, so if you want to help...)

[rukai]

Looks like this is already being reevaluated but just wanted to register a datapoint:
I blindly followed this lint when upgrading to rust 1.66 and ended up hitting a panic later on when working on another feature :/
Upon closer inspection the original code was doing the right thing by not panicking.

[zesterer]

Another voice to add to the 'disable this by default' crowd (I see this has already been done, but I figured it's still worth a mention): the advice given by this lint was taken in a refactor for Veloren, leading to intermittent crashes for likely a few dozen users: https://gitlab.com/veloren/veloren/-/issues/1769

[udoprog]

Another bug caused by following this lint: iced-rs/iced#1619
Change introduced here: iced-rs/iced#1611
Fixed later here: iced-rs/iced#1637

[flip1995]

Thanks all for pointing this out and really sorry for the inconveniences caused by this. I'll make sure that this lint is no longer warn-by-default in 1.67.

There's nothing else to do here, so +1 comments won't help here. Thanks all for giving us examples what we should look for when trying to improve this lint in the future!

[Xaeroxe]

So first off @dhardy I owe you an apology. You tried to call this out before it was released and I thought it would be okay. I was wrong. I also owe an apology to the people who experienced a panic as a result of following this lint.

Second off, I've thought about my proposal to split the lint some more. Earlier I suggested having two lints, one which would only fire if it can be determined at compile time that max is always greater than or equal to min, and never NaN, and the other with the current behavior of this lint. The first would be warn by default, and the second would be allow by default. Currently I think only the first lint is useful, and thus I'd like to propose adjusting this lint to use those checks and then setting it back to warn by default, once we know it will never trigger a false positive.

[matejcik]

not sure if this is within clippy's capabilities, but it would be nice not to suggest clamp in const functions (in particular when i'm reimplementing clamp precisely so that the result is const :) )

[flip1995]

Please open an issue about this. This PR is not the bug tracker for this lint, even if it seems like it has become the same.