ACP: Intuitive alternative for `.min()` and `.max()`

[Kyuuhachi]

Proposal

Problem statement

It is a fairly common mistake to write x.max(8) and expect "x but no more than 8", when it's in fact the opposite.

Motivating examples or use cases

While I don't have any concrete examples from my own codebases (because I've fixed any I've found), I think everyone has written something like arr[..x.max(arr.len())] at some point.

There are also a few clippy lints related to similar errors, such as min_max with x.max(100).min(0), and manual_clamp which mentions a rather confusing input.max(min).min(max). These indicate that there is precedent for accidentally swapping min and max.

Solution sketch

I propose adding a function .limit() to Ord (and to float types), which serves the role of both .min(), .max(), and .clamp(). In brief:

x.limit(5..) == x.max(5)
x.limit(..=10) == x.min(10)
x.limit(5..=10) == x.clamp(5, 10)
x.limit(..) == x // because why not?

and yes I did confuse min and max while writing the above. Case in point.

With this, the above examples would be written as arr[..x.limit(arr.len()..)], x.limit(100..).limit(..=0), and input.limit(min..).limit(..=max), respectively. I think all three make the intention (and the mistakes) significantly clearer.

I also imagine a clippy lint that suggests replacing .min() and .max(), and maybe also .clamp(), with their corresponding .limit() forms.

Implementation sketch

pub trait Ord {
    ...
    fn limit<B: LimitBounds<Self>>(self, bounds: B) -> Self {
        bounds.limit_bounds(self)
    }
}

pub trait LimitBounds<T: Ord>: Sized + Sealed {
    fn limit_bounds(self, value: T) -> T;
}

impl<T: Ord> LimitBounds<T> for RangeFrom<T> {
    fn limit_bounds(self, value: T) -> T {
        value.max(self.start)
    }
}

impl<T: Ord> LimitBounds<T> for RangeToInclusive<T> {
    fn limit_bounds(self, value: T) -> T {
        value.min(self.end)
    }
}

impl<T: Ord> LimitBounds<T> for RangeInclusive<T> {
    fn limit_bounds(self, value: T) -> T {
        let (start, end) = self.into_inner();
        value.clamp(start, end)
    }
}

impl<T: Ord> LimitBounds<T> for RangeFull {
    fn limit_bounds(self, value: T) -> T {
        value
    }
}


impl f32 {
    ...
    fn limit<B: LimitBounds<f32>>(self, bounds: B) -> f32 {
        bounds.limit_bounds(self)
    }
}

impl LimitBounds<f32> for RangeFrom<f32> {
    fn limit_bounds(self, value: f32) -> f32 {
        value.max(self.start)
    }
}
// and so on for each range and float type

Alternatives

This could totally be a separate crate, and I would have made one if I could come up with a name. But I think it's a common enough annoyance that it would fit in std.

The operations can be written less error-pronely with function form (usize::min(x, arr.len())) or with clamp (x.clamp(0, arr.len())). In my opinion these are both unsatisfactory: both are rather verbose, functoin form doesn't fit into method chains, and clamp requires the type to be bounded.

Unfortunately the name clamp is already taken, and the function cannot be retrofitted due to different number of arguments. Otherwise, that name would be much better for this operation.

Links and related work

IRLO thread.

The clamp RFC considered making clamp take a range, but as I understand it, that was rejected because inclusive ranges were rather immature at the time. This is no longer the case.

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

• We think this problem seems worth solving, and the standard library might be the right place to solve it.
• We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

• We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
• We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

[tgross35]

Couldn't this use RangeBounds rather than a new LimitBounds?

[Kyuuhachi]

That would require figuring out what to do with exclusive ranges. Those could maybe be dealt with with Step or similar, but that's a significantly stronger bound than just Ord.

[scottmcm]

Another change from the initial clamp conversations is that https://doc.rust-lang.org/std/primitive.f32.html#method.next_down is stable now. So something like 123.456_f32.limit(0.0..1.0) => 0.99999994_f32 could be easily defined via things like

impl LimitBounds<f32> for Range<f32> {
    fn limit_bounds(self, value: f32) -> f32 {
        value.limit(self.start..=self.end.next_down())
    }
} 

[kennytm]

I don't know if allowing ..y & x..y would enable some people to confuse value.limit(..upper) and value.limit(..=upper), the former would mean value.min(upper-1).

Those could maybe be dealt with with Step or similar, but that's a significantly stronger bound than just Ord.

It can be supported easily with

impl<T: Ord + Step> LimitBounds<T> for RangeTo<T> {
    fn limit_bounds(self, value: T) -> T {
        T::backward(self.end, 1).min(value)
        // ^ or use T::backward_checked(self.end, 1).unwrap().min(value)
        //   because T::backward() wraps around on overflow in release mode.
    }
}

[Kyuuhachi]

Yes, exclusive ranges could be added to LimitBounds without too much trouble. But we still can't use RangeBounds for it, since we would want different trait bounds for exclusive and inclusive ranges.

[Amanieu]

We discussed this in the @rust-lang/libs-api meeting today. We preferred the name clamp_range over limit: it's better to explicitly call out the difference between this function and clamp rather than using 2 synonyms.

We also think that having the bounds reject ..X is a feature since it prevents people from accidentally using it instead of ..=.

Finally, an alternative example was also proposed which adds 2 methods clamp_max and clamp_min as aliases for min and max respectively. These methods would have the intuitive behavior of clamping the input value to the given minimum/maximum.

[tisonkun]

Finally, an alternative example was also proposed which adds 2 methods clamp_max and clamp_min as aliases for min and max respectively.

This sounds reasonable. Given that clamp_range(x..y) has the ambiguity as stated in #665 (comment).

[Kyuuhachi]

I think clamp_range is a bit of a mouthful, but yeah, it's certainly more descriptive than limit. clamp_min/max does sound like a reasonable alternative.