Make RangeInclusive just a two-field struct (amend 1192) by scottmcm · Pull Request #1980 · rust-lang/rfcs (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation26 Commits4 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Rationale overview:

Having the variants make trying to use a RangeInclusive unergonomic.
For example, fn clamp(self, range: RangeInclusive<Self>) is forced
to deal with the Empty case.
The variants don't actually provide any offsetting safety or additional
performance, since everything is pub so it's totally possible for a
"NonEmpty" range to contain no elements.
This makes it more consistent with (half-open) Range.
The extra flag/variant is not actually needed in order to make it
iterable, even using the existing Step trait. And supposing a more
cut-down trait, generating a, b such that !(a <= b) is easier
than other fundamental requirements of safe range iterability.

Posting this here after discussion on the inclusive ranges tracking issue: rust-lang/rust#28237

A branch with this change implemented: https://github.com/rust-lang/rust/compare/master...scottmcm:rangeinclusive-struct?expand=1

[Edit] Rendered: https://github.com/scottmcm/rfcs/blob/simpler-rangeinclusive/text/1192-inclusive-ranges.md

Rationale:

Having the variants make trying to use a RangeInclusive unergonomic. For example, fn clamp(self, range: RangeInclusive<Self>) is forced to deal with the Empty case.
The variants don't actually provide any offsetting safety or additional performance, since everything is pub so it's totally possible for a "NonEmpty" range to contain no elements.
This makes it more consistent with (half-open) Range.
The extra flag/variant is not actually needed in order to make it iterable, even using the existing Step trait. And supposing a more cut-down trait, generating a, b such that !(a <= b) is easier than other fundamental requirements of safe range iterability.

For the record, here were the arguments in favor of changing it from a struct to an enum: #1192 (comment)

@durka I agree with #1320 that the enum is superior to the finished field, since it can only be wrong in one way instead of two.

Type history, to save people needing to open all the diffs:

#1192

pub struct RangeInclusive { pub start: T, pub end: T, pub finished: bool, }

#1320

pub enum RangeInclusive { Empty { at: T, }, NonEmpty { start: T, end: T, } }

Proposed here:

pub struct RangeInclusive { pub start: T, pub end: T, }

This was referenced

Apr 25, 2017

Wasn't the finished field added to handle the a...usize::MAX case? You can't have (usize::MAX+1)...usize::MAX.

@Stebalien As stated in the amendment, after reaching n ... n, the whole range will be replaced by 1 ... 0.

@kennytm I see (although I personally didn't expect an important detail like that to be relegated to the drawbacks section).

There’s one use case that is not handled by this proposal: looking at which value the iteration has stopped. This is possible with the previous approach and I suppose that’s why the Empty variant had at field in the first place.

One could try approximating this by having Empty be at ... at-1 instead, but that falls over when at would be 0u64, for example.

That being said, I have never had any need for Empty variant, so good riddance I guess?

@Stebalien Thanks, that's good feedback. I've moved the behaviour of the patch into the design section (from the double-negative phrasing in drawbacks), as well as updated the alternatives with other possibilities.

@nagisa Added that as an explicit drawback. "Good riddance I guess" is pretty much how I felt about it 🙂

Ooh, this is great! I don't know why we didn't think of it before.

@rfcbot fcp merge

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

No concerns currently listed.

Once these reviewers reach consensus, 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.

Would it be possible to make the overflow case turn into MAX...MIN instead of 1...0? That way, we can tell an exhausted n...MAX range apart from an exhausted 0...0.

@clarcharr Under the current proposal, all ranges are replaced by 1 ... 0 at the end, not just 0 ... 0 or MAX ... MAX.

The advantage of using 1 ... 0 unconditionally is that we don't need to add two more methods (.is_max(), .replace_min()) into into the Step trait, and we don't need to add the overflow-check logic. The disadvantage of course is information loss.

I think being able to inspect the exhausted value is a niche use, and is better solved by storing it in an extra variable.

let finish_value = range.end.clone(); 
while let Some(i) = range.next() {
    ..;
}
// no guarantee what `range` will become here.
// but do whatever you like with `finish_value`.

It is much easier than getting a value from (n+1) ... n or MAX ... MIN.

Edit. Perhaps the RFC could be relaxed a bit, saying that after calling (n ... n).next(), it will be guaranteed that r.start > r.end, but it is unspecified what the actual values will be. Ending up with (n+1) ... n could probably benefit the LLVM optimizer.

I have no particular attachment to the 1...0 range; it was chosen mostly to avoid the "what should Step look like" discussion. That said, there are no real uses of replace_one and replace_zero (they didn't even work until recently: rust-lang/rust#41492), so replacing them with one or two methods for this case would probably be fine. I don't think I'd want to mandate MAX...MIN, since that's not possible for RangeInclusive<num::BigInt>, but a trait method like make_empty that left the choice to the type sounds reasonable, and we could choose MAX...MIN in core for numeric types.

One thing that is nice about producing 1...0 (or any "canonical empty" range) at the end is that it strongly discourages people looking at the post-iteration range. A potential trap with it almost always being (n+1)...n is that weird bug the first time it turns out to actually be n...(n-1) or n...0 and some code reads a value it wasn't expecting.

I do like the "but it is unspecified what the actual values will be" option, but worry that any later PR that proposed changing the values would be rejected as potentially-breaking, since code could be relying on the values and just compiling all of crates.io wouldn't find such a regression.

Just to be clear, my proposal for Iterator::next was (in pseudo-Rust):

let ret = start;
match start.cmp(end) {
    Less => {
        start = start.wrapping_inc();
        Some(ret)
    }
    Equal => {
        start = start.wrapping_inc();
        if start < end {
            swap(start, end);
        }
        Some(ret)
    }
    Greater => None
}

This still works for BigInt and any other custom number types without having to explicitly state what max and min are. It only requires that the addition be wrapping; for things like floats I doubt we'd run into an issue like this (e.g. n...f64::INFINITY is bad).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 44: Empty, no such variant now

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

rfcbot added the final-comment-period

Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised.

label

May 11, 2017

@scottmcm would you mind responding to the example code I gave? It would remove the need for replace_zero and replace_one.

@clarcharr Overall, I think what the options here have demonstrated is that they all need some method on Step that ordinary Range iteration doesn't if RangeInclusive is going to be just two fields.

Doing it with overflowing_inc is certainly nice for BigInt, and might be a nice way to make RangeFrom always panic on overflow instead of the current endless loop in release. Though for a weirder range it might be nice to have it turn into something else instead. For example, with a RangeInclusive<*mut T> (where inc is .wrapping_offset(1) since plain offset is unsafe) it might be nice to have it become empty by making the end into ptr::null() (and the start into Unique::empty() if it's currently null). A new method like Step::turn_singleton_inclusive_range_into_empty could allow all these: for BigInt it would just increment, pointers could do the thing above, primitive integers could do the 1...0 thing or swap-if-overflow approach.

Maybe the RFC should go with @kennytm's "unspecified empty range" and leave the details to an implementation question or to Step design...

I like leaving it as "unspecified empty," making the only requirement that end > start. Perhaps we could update the RFC to reflect this, and put the 1...0 implementation as just the current plan for implementation, not the spec?

If we can't come up with a plan here then how is the implementor going to decide? I don't like just throwing up our hands. If I'm writing some code that consumes ranges, I need to be able to check whether the range is empty. Perhaps one way to avoid committing to anything would be to provide a RangeInclusive::is_empty method, and documenting that it's the only reliable way to find out.

…

@durka RangeInclusive already has an is_empty method via the ExactSizeInterator trait. So only the documentation is needed

@durka I should add that end > start is a pretty non-committal way of describing is_empty while allowing flexibility of implementation

Updated to make the only post-iteration requirement !(start <= end) and explicitly suggest ExactSizeIterator::is_empty for checking emptiness.

(Step doesn't require Ord today, so start > end is sufficient but not necessary.)

The final comment period is now complete.

Alright the FCP is now complete, and looks like we've had a few tweaks but appears that otherwise nothing major came up. In that case I'm going to merge, thanks again for the RFC @scottmcm!

Ok I've updated the tracking issue for inclusive ranges to include this RFC as a work item before stabilizing.

scottmcm deleted the simpler-rangeinclusive branch

May 23, 2017 02:39

This was referenced

May 23, 2017

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request

May 24, 2017

…turon

Make RangeInclusive just a two-field struct

Not being an enum improves ergonomics and consistency, especially since NonEmpty variant wasn't prevented from being empty. It can still be iterable without an extra "done" bit by making the range have !(start <= end), which is even possible without changing the Step trait.

Implements merged rust-lang/rfcs#1980; tracking issue rust-lang#28237.

This is definitely a breaking change to anything consuming RangeInclusive directly (not as an Iterator) or constructing it without using the sugar. Is there some change that would make sense before this so compilation failures could be compatibly fixed ahead of time?