RangeInclusive::contains doesn't consider exhaustion

[cuviper]

I tried this code:

fn main() {
    let mut range = 0..=0;
    assert!(!range.is_empty());
    assert!(range.contains(&0));

    range.next();
    assert!(range.is_empty());
    assert!(!range.contains(&0));
}

I expected to see this happen: all assertions pass.

Instead, this happened: the last assertion fails.

Meta

Playground link

The behavior is the same across stable 1.47.0, 1.48.0-beta.2, and 1.49.0-nightly (2020-10-13 adef9da30f1ecbfeb813).

Analysis

The implementation of is_empty includes a check for the exhausted flag:

    pub fn is_empty(&self) -> bool {
        self.exhausted || !(self.start <= self.end)
    }

But contains only looks at the raw bounds:

    pub fn contains<U>(&self, item: &U) -> bool
    where
        Idx: PartialOrd<U>,
        U: ?Sized + PartialOrd<Idx>,
    {
        <Self as RangeBounds<Idx>>::contains(self, item)
    }

It would be easy to insert !self.exhausted && ... in there, as long as we're OK with the behavior change, which I consider a bug fix. I think the exhausted state of start/end is unspecified, so nobody should be depending on contains being true then.

[Mark-Simulacrum]

It is a bit unfortunate that this is likely also a regression performance wise, but correctness does seem more important :)

I think we should do this as well, though given the (potential) breakage it would want t-libs FCP.

[SkiFire13]

It should be noted that #78109 doesn't fix <RangeInclusive<T> as RangeBounds<T>>::contains.

Digging deeper it looks like the same problem happens when using the range to index after it has been exhausted (which doesn't use contains). Playground link
The problem is likely the implementation of RangeBounds<T>::end_bound for both RangeInclusive<T> and RangeInclusive<&T> which marks the end bound as inclusive even if the range has been exhausted. This then causes the misbehave in RangeBounds<T>::contains and then RangeInclusive::contains.

[cuviper]

The problem is likely the implementation of RangeBounds<T>::end_bound for both RangeInclusive<T> and RangeInclusive<&T> which marks the end bound as inclusive even if the range has been exhausted.

That's an interesting thought -- maybe it should return an exclusive end when the iterator is exhausted?

[cuviper]

Digging deeper it looks like the same problem happens when using the range to index after it has been exhausted (which doesn't use contains). Playground link

This would have to be addressed in SliceIndex<str>/SliceIndex<[T]> for RangeInclusive<usize>.