Check for exhaustion in RangeInclusive::contains and slicing

[cuviper]

When a range has finished iteration, is_empty returns true, so it
should also be the case that contains returns false.

Fixes #77941.

[rust-highfive]

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

[cuviper]

This might want @rust-lang/libs FCP per #77941 (comment).

[cuviper]

I've moved the exhausted check to RangeBounds::end_bound, per @SkiFire13's observation in #77941 (comment).

[jyn514]

I don't know why the end should be treated specially. If you think this is useful, shouldn't it apply to all parts of iteration equally, and let mut iter = 0..5; iter.next(); iter.contains(0) return false?

[cuviper]

This is about RangeInclusive, so your example should be 0..=5, but otherwise yes! Once you advance the iterator, contains(&0) will be false, and this is already the case. Well, except if you advance in reverse to exhaustion, then it would be "empty" but still "contain" start==end==0, until this PR.

The inclusive end is special by nature, that's the whole point. Once the iterator reaches start == end, we have two possible states: !exhausted where the next value is that start/end value, then exhausted where we are empty. But contains didn't consider the latter state, which is what I'm addressing now.

[jyn514]

Got it, thanks, I didn't realize this was fixing an omission. Looks good to me then :)

Also, wow, Range being an Iterator is very confusing.

[SkiFire13]

It would also be nice to have some tests that check this behaviour

[dtolnay]

@rust-lang/libs: this PR changes the implementation of RangeBounds for RangeInclusive in such a way that the following assertions succeed. Previously range.contains(end) continued returning true even after range.is_empty() was true.

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));
}

Another example of the behavior change:

fn main() {
    let array = ['w', 'x', 'y', 'z'];
    let mut range = 0..=3;
    while let Some(i) = range.next() {
        println!(
            "after i={}, the remaining elements are {:?}",
            i, &array[range.clone()],
        );
    }
}

Old behavior:

after i=0, the remaining elements are ['x', 'y', 'z']
after i=1, the remaining elements are ['y', 'z']
after i=2, the remaining elements are ['z']
after i=3, the remaining elements are ['z']

New behavior:

after i=0, the remaining elements are ['x', 'y', 'z']
after i=1, the remaining elements are ['y', 'z']
after i=2, the remaining elements are ['z']
after i=3, the remaining elements are []

@rfcbot fcp merge

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @KodrAus
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[cuviper]

@SkiFire13 I figured the doc-test was sufficient -- I don't see any other existing tests of ranged contains.

@dtolnay this doesn't actually affect slicing (yet) because SliceIndex is a separate thing. I was going to followup separately, but I can add that change to this PR if you'd prefer to do it all at once.

[dtolnay]

Let's consider both as part of this FCP. I imagine we'd not want to do one without the other. But the code change for SliceIndex can go in either this PR or separate PR if easier.

[cuviper]

Preserving bounds checking for exhausted SliceIndex was tricky to think about, but I think I've got it working now.

[dtolnay]

Implementation looks good to me. Thank you @cuviper.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[dtolnay]

@bors r+

[bors]

📌 Commit 9202fbd has been approved by dtolnay

[bors]

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened