Suggest `foo.clamp(y, x)` for `foo.min(x).max(y)` and `foo.max(y).min(x)`

[Arnavion]

trafficstars

What it does

Rust 1.50 stabilized std::cmp::Ord::clamp, f32::clamp and f64::clamp.

When the user writes foo.min(x).max(y) or foo.max(y).min(x) where foo is T: Ord or f32 or f64, clippy should suggest foo.clamp(y, x)

Also, the existing lint min_max should be extended to check for instances of foo.clamp(big, small), such as if the user manually translated foo.min(big).max(small) to foo.clamp(big, small) instead of foo.clamp(small, big)

Categories (optional)

• Kind: pedantic or complexity - similar to other "could write it shorter" lints like the ones for Iterator combinators.

What is the advantage of the recommended code over the original code

Shorter code.

Drawbacks

• Easier to silently mess up the order and call foo.clamp(big, small) without the instructive names "min" and "max" visible. That's why I suggest the min_max lint should be updated to also catch this.

• foo.clamp(y, x) is not strictly equal to foo.min(x).max(y) or foo.max(y).min(x); it panics if x < y but the original code would return y or x respectively. But I'm not sure if there can be a situation where the user wants this behavior, rather than either replacing the whole expr with y / x or fixing the order of the operands.

• JP-Ellis points out in this comment that f32::clamp and f64::clamp panic when either parameter is NaN, which is also a deviation from the behavior of f32::min and f32::max which always return the non-NaN value.

Example

As above.

[TaKO8Ki]

@rustbot claim

[JP-Ellis]

This should also apply to the case where someone has written:

if x < a {
  a
} else if x > b {
  b
} else {
  x
}

though I don't know if this is too niche.

Another thing to note is how NaN values are handled. Specifically:

• min and max return the other value if either one is NaN
• clamp panics if either bounds is NaN, and returns NaN if the value being clamped if NaN
• Any comparison with a NaN (with the usual <, <= == >= and >) returns false.

This means that in edge cases where NaN values might appear, the three options are not equivalent.

I'm not really sure how infinites are handled, though I doubt there's anything suprising there.

[Arnavion]

Hmm, I didn't think of the NaN behavior. That makes me lean towards removing that the f32::clamp and f64::clamp suggestions from this lint. Clippy shouldn't assume the user has non-NaN floats.

[JP-Ellis]

I don't see any issue with Clippy suggesting a change, as long as it is made abundantly clear that NaN are handled differently.

[giraffate]

Hmm, I didn't think of the NaN behavior. That makes me lean towards removing that the f32::clamp and f64::clamp suggestions from this lint.

IMHO it is a good point to start this. If we want to address f32::clamp and f64::clamp, we can make a follow-up PR later.

[bengsparks]

@rustbot claim

[bengsparks]

Progress update for the new lint: Method Syntax:

error: chaining calls to `min` and `max`
  --> $DIR/clamp.rs:4:25
   |
LL |     let minmax_method = 10.min(5).max(15);
   |                         ^^^^^^^^^^^^^^^^^ help: replace with `clamp`: `10.clamp(5, 15)`
   |
   = note: `-D clippy::clamp` implied by `-D warnings`

error: chaining calls to `max` and `min`
  --> $DIR/clamp.rs:5:25
   |
LL |     let maxmin_method = 10.max(15).min(5);
   |                         ^^^^^^^^^^^^^^^^^ help: replace with `clamp`: `10.clamp(5, 15)`

error: chaining calls to `min` and `max`
  --> $DIR/clamp.rs:9:29
   |
LL |     let minmax_var_method = y.min(x).max(z);
   |                             ^^^^^^^^^^^^^^^ help: replace with `clamp`: `y.clamp(x, z)`

error: chaining calls to `max` and `min`
  --> $DIR/clamp.rs:10:29
   |
LL |     let maxmin_var_method = y.max(z).min(x);
   |                             ^^^^^^^^^^^^^^^ help: replace with `clamp`: `y.clamp(x, z)`

Is it possible to accurately add support for function invocations of min and max, seeing as clamp will panic if arguments are passed in the wrong order?

[Arnavion]

error: chaining calls to `min` and `max`
  --> $DIR/clamp.rs:4:25
   |
LL |     let minmax_method = 10.min(5).max(15);
   |                         ^^^^^^^^^^^^^^^^^ help: replace with `clamp`: `10.clamp(5, 15)`
   |
   = note: `-D clippy::clamp` implied by `-D warnings`

I don't think this should be suggested, since it's silently fixing the original broken code under the guise of replacing min-max with clamp. Basically in situations where the min_max lint fires, the new one shouldn't.

[bengsparks]

error: chaining calls to `min` and `max`
  --> $DIR/clamp.rs:4:25
   |
LL |     let minmax_method = 10.min(5).max(15);
   |                         ^^^^^^^^^^^^^^^^^ help: replace with `clamp`: `10.clamp(5, 15)`
   |
   = note: `-D clippy::clamp` implied by `-D warnings`

I don't think this should be suggested, since it's silently fixing the original broken code under the guise of replacing min-max with clamp. Basically in situations where the min_max lint fires, the new one shouldn't.

Ah yes, I see. I've read documentation, but I've been unable to find a way to specify a dependency upon whether or not another lint fires for an expression. Would it be best to merge my implementation into that of the already existing min_max lint?

[Arnavion]

No. One lint should do one thing. min_max should lint about the arguments being backwards. clamp should warn about using clamp.

You don't need any "dependency" between lints. You just need to wrap the condition that min_max uses in a reusable function and then invert its output.

[bengsparks]

Progress Update

This lint does not fire on calls like, as this would change broken code, i.e. this is desired.

let false_minmax_method = 10.min(5).max(15);
let false_maxmin_function_r1 = max(15, min(5, 10));
let false_minmax_method = var.min(5).max(15);
let false_maxmin_function_l2 = max(min(10, var), 15);

It DOES fire on calls like these, as they are be converted to clamps:

error: chaining calls to `min` and `max`
  --> $DIR/clamp.rs:35:25
   |
LL |     let minmax_method = 10.min(15).max(5);
   |                         ^^^^^^^^^^^^^^^^^ help: replace with `clamp`: `10.clamp(5, 15)`
   |
   = note: `-D clippy::clamp` implied by `-D warnings`

error: chaining calls to `max` and `min`
  --> $DIR/clamp.rs:36:25
   |
LL |     let minmax_method = 10.max(5).min(15);
   |                         ^^^^^^^^^^^^^^^^^ help: replace with `clamp`: `10.clamp(5, 15)`

error: chaining calls to `min` and `max`
  --> $DIR/clamp.rs:40:29
   |
LL |     let minmax_var_method = y.min(z).max(x);
   |                             ^^^^^^^^^^^^^^^ help: replace with `clamp`: `y.clamp(x, z)`

error: chaining calls to `max` and `min`
  --> $DIR/clamp.rs:41:29
   |
LL |     let maxmin_var_method = y.max(x).min(z);
   |                             ^^^^^^^^^^^^^^^ help: replace with `clamp`: `y.clamp(x, z)`

error: chaining calls to `max` and `min`
  --> $DIR/clamp.rs:55:31
   |
LL |     let minmax_functions_r2 = min(z, max(y, x));
   |                               ^^^^^^^^^^^^^^^^^ help: replace with `clamp`: `y.clamp(x, z)`

error: chaining calls to `max` and `min`
  --> $DIR/clamp.rs:52:31
   |
LL |     let minmax_functions_r1 = min(z, max(x, y));
   |                               ^^^^^^^^^^^^^^^^^ help: replace with `clamp`: `x.clamp(y, z)`

[Xaeroxe]

Implemented in https://github.com/rust-lang/rust-clippy/pull/9484