Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make RangeInclusive just a two-field struct (amend 1192) #1980

Merged
merged 4 commits into from
May 22, 2017

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Apr 25, 2017

Rationale overview:

  1. 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.
  2. 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.
  3. This makes it more consistent with (half-open) Range.
  4. 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:

1. 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.
2. 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.
3. This makes it more consistent with (half-open) `Range`.
4. 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.
@durka
Copy link
Contributor

durka commented Apr 25, 2017

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

@scottmcm
Copy link
Member Author

scottmcm commented Apr 25, 2017

@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<T> {
    pub start: T,
    pub end: T,
    pub finished: bool,
}

#1320

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

Proposed here:

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

@Stebalien
Copy link
Contributor

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

@kennytm
Copy link
Member

kennytm commented Apr 26, 2017

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

@Stebalien
Copy link
Contributor

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

@nagisa
Copy link
Member

nagisa commented Apr 26, 2017

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?

@scottmcm
Copy link
Member Author

@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 🙂

@aturon aturon self-assigned this Apr 29, 2017
@aturon
Copy link
Member

aturon commented Apr 29, 2017

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

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 29, 2017

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.

@clarfonthey
Copy link
Contributor

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.

@kennytm
Copy link
Member

kennytm commented May 1, 2017

@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.

@scottmcm
Copy link
Member Author

scottmcm commented May 1, 2017

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.

@clarfonthey
Copy link
Contributor

clarfonthey commented May 2, 2017

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).

Copy link
Contributor

@liigo liigo left a comment

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

@rfcbot
Copy link
Collaborator

rfcbot commented May 11, 2017

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

@rfcbot 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
@clarfonthey
Copy link
Contributor

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

@scottmcm
Copy link
Member Author

@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...

@clarfonthey
Copy link
Contributor

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?

@durka
Copy link
Contributor

durka commented May 15, 2017 via email

@kennytm
Copy link
Member

kennytm commented May 15, 2017

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

@clarfonthey
Copy link
Contributor

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

@scottmcm
Copy link
Member Author

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.)

@rfcbot
Copy link
Collaborator

rfcbot commented May 21, 2017

The final comment period is now complete.

@alexcrichton
Copy link
Member

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!

@alexcrichton
Copy link
Member

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

@alexcrichton alexcrichton reopened this May 22, 2017
@alexcrichton alexcrichton merged commit a01974b into rust-lang:master May 22, 2017
@scottmcm scottmcm deleted the simpler-rangeinclusive branch May 23, 2017 02:39
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?

r? @aturon (as FCP proposer on the RFC)
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?

r? @aturon (as FCP proposer on the RFC)
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?

r? @aturon (as FCP proposer on the RFC)
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 25, 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?

r? @aturon (as FCP proposer on the RFC)
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 25, 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?

r? @aturon (as FCP proposer on the RFC)
@crlf0710
Copy link
Member

crlf0710 commented May 27, 2017

Just a thought... While 1...0 is ok for representing Empty for say, RangeInclusive<u8>, how can one represent Empty for

enum S {
    OnlyVariant,
}

in principle? And what can an implementation do to change an NonEmpty one into Empty state RangeInclusive for non numeric types?

kennytm added a commit to kennytm/rust that referenced this pull request Feb 12, 2018
…alexcrichton Add Range[Inclusive]::is_empty During rust-lang/rfcs#1980, it was discussed that figuring out whether a range is empty was subtle, and thus there should be a clear and obvious way to do it. It can't just be ExactSizeIterator::is_empty (also unstable) because not all ranges are ExactSize -- such as `Range<i64>` and `RangeInclusive<usize>`. Things to ponder: - Unless this is stabilized first, this makes stabilizing ExactSizeIterator::is_empty more icky, since this hides that. - This is only on `Range` and `RangeInclusive`, as those are the only ones where it's interesting. But one could argue that it should be on more for consistency, or on RangeArgument instead. - The bound on this is PartialOrd, since that works ok (see tests for float examples) and is consistent with `contains`. But ranges like `NAN..=NAN`_are_ kinda weird. - [x] ~~There's not a real issue number on this yet~~
kennytm added a commit to kennytm/rust that referenced this pull request Feb 13, 2018
…alexcrichton

Add Range[Inclusive]::is_empty

During rust-lang/rfcs#1980, it was discussed that figuring out whether a range is empty was subtle, and thus there should be a clear and obvious way to do it.  It can't just be ExactSizeIterator::is_empty (also unstable) because not all ranges are ExactSize -- such as `Range<i64>` and `RangeInclusive<usize>`.

Things to ponder:
- Unless this is stabilized first, this makes stabilizing ExactSizeIterator::is_empty more icky, since this hides that.
- This is only on `Range` and `RangeInclusive`, as those are the only ones where it's interesting.  But one could argue that it should be on more for consistency, or on RangeArgument instead.
- The bound on this is PartialOrd, since that works ok (see tests for float examples) and is consistent with `contains`.  But ranges like `NAN..=NAN`_are_ kinda weird.
- [x] ~~There's not a real issue number on this yet~~
kennytm added a commit to kennytm/rust that referenced this pull request Feb 14, 2018
…alexcrichton

Add Range[Inclusive]::is_empty

During rust-lang/rfcs#1980, it was discussed that figuring out whether a range is empty was subtle, and thus there should be a clear and obvious way to do it.  It can't just be ExactSizeIterator::is_empty (also unstable) because not all ranges are ExactSize -- such as `Range<i64>` and `RangeInclusive<usize>`.

Things to ponder:
- Unless this is stabilized first, this makes stabilizing ExactSizeIterator::is_empty more icky, since this hides that.
- This is only on `Range` and `RangeInclusive`, as those are the only ones where it's interesting.  But one could argue that it should be on more for consistency, or on RangeArgument instead.
- The bound on this is PartialOrd, since that works ok (see tests for float examples) and is consistent with `contains`.  But ranges like `NAN..=NAN`_are_ kinda weird.
- [x] ~~There's not a real issue number on this yet~~
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.