implement RFC 1192 inclusive ranges #30884

durka · 2016-01-13T22:56:05Z

This PR implements RFC 1192, which is triple-dot syntax for inclusive range expressions. The new stuff is behind two feature gates (one for the syntax and one for the std::ops types). This replaces the deprecated functionality in std::iter. Along the way I simplified the desugaring for all ranges.

This is my first contribution to rust which changes more than one character outside of a test or comment, so please review carefully! Some of the individual commit messages have more of my notes. Also thanks for putting up with my dumb questions in #rust-internals.

Notes

For implementing std::ops::RangeInclusive, I took @Stebalien's suggestion from RFC for inclusive ranges with ... rfcs#1192 (comment). It seemed to me to make the implementation easier and increase type safety. If that stands, the RFC should be amended to avoid confusion.
I also kind of like @glaebhoerl's idea, which is unified inclusive/exclusive range syntax something like x>..=y. We can experiment with this while everything is behind a feature gate.
There are a couple of FIXMEs left (see the last commit). I didn't know what to do about RangeArgument and I haven't added Index impls yet. Those should be discussed/finished before merging.

cc @gankro since you complained
cc #27777 #30877 #1192 rust-lang/rfcs#1254
relevant to #28237 (tracking issue)

rust-highfive · 2016-01-13T22:56:10Z

r? @aturon

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

Stebalien · 2016-01-13T23:25:10Z

There's actually an rfc for that: rust-lang/rfcs#1320

The current proposed syntax is actually:

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

This way, we don't loose any information or throw anything away (see the discussion on the RFC). It also means we should also be able to implement From<Range<T>> for RangeInclusive<T>.

durka · 2016-01-13T23:27:17Z

@Stebalien oh I completely missed that RFC! I will add that field and impl later tonight.

durka · 2016-01-14T06:38:03Z

Fixed tidy issues. The things @Stebalien brought up will need to wait until tomorrow. But those only affect the libcore part, not the rest of the PR. What's the policy on accepting RFC amendments?

durka · 2016-01-14T16:50:22Z

The failing test is the one that ensures HIR node IDs are reproducible. For debugging I removed some custom Debug impls in favor of #[derive]s which will print the ID. I isolated the difference (full output here):

$ diff -us hir{1,2}
--- hir1    2016-01-14 12:06:39.000000000 -0500
+++ hir2    2016-01-14 12:06:47.000000000 -0500
@@ -16,8 +16,8 @@
                                             ),
                                             Spanned {
                                                 node: Ident {
-                                                    name: _result(81),
-                                                    unhygienic_name: _result(81)
+                                                    name: _result(83),
+                                                    unhygienic_name: _result(83)
                                                 },
                                                 span: Span { lo: BytePos(0), hi: BytePos(0), expn_id: ExpnId(4294967295) }
                                             },
@@ -192,8 +192,8 @@
                                                                     ),
                                                                     Spanned {
                                                                         node: Ident {
-                                                                            name: iter(72),
-                                                                            unhygienic_name: iter(72)
+                                                                            name: iter(82),
+                                                                            unhygienic_name: iter(82)
                                                                         },
                                                                         span: Span { lo: BytePos(0), hi: BytePos(0), expn_id: ExpnId(4294967295) }
                                                                     },
@@ -283,8 +283,8 @@
                                                                                                                 segments: [
                                                                                                                     PathSegment {
                                                                                                                         identifier: Ident {
-                                                                                                                            name: iter(72),
-                                                                                                                            unhygienic_name: iter(72)
+                                                                                                                            name: iter(82),
+                                                                                                                            unhygienic_name: iter(82)
                                                                                                                         },
                                                                                                                         parameters: AngleBracketedParameters(
                                                                                                                             AngleBracketedParameterData {
@@ -587,8 +587,8 @@
                             segments: [
                                 PathSegment {
                                     identifier: Ident {
-                                        name: _result(81),
-                                        unhygienic_name: _result(81)
+                                        name: _result(83),
+                                        unhygienic_name: _result(83)
                                     },
                                     parameters: AngleBracketedParameters(
                                         AngleBracketedParameterData {
@@ -613,4 +613,3 @@
     span: Span { lo: BytePos(0), hi: BytePos(0), expn_id: ExpnId(4294967295) },
     attrs: None
 �[00m�[37m}
-

But I don't have time to investigate right now.

alexcrichton · 2016-01-14T19:09:56Z

Edit: I'm wrong

durka · 2016-01-14T19:15:20Z

Do you mean in the HIR? I did not remove ExprRange from the AST.
On Jan 14, 2016 2:10 PM, "Alex Crichton" notifications@github.com wrote:

I think that we may want to preserve the ExprRange variant in the AST as
it will likely assist with pretty printing (e.g. it disambiguates whether
you have a literal struct or used the sugar syntax)

—
Reply to this email directly or view it on GitHub
#30884 (comment).

alexcrichton · 2016-01-14T23:20:21Z

Oh gah yes, sorry about that, my mistake!

durka · 2016-01-15T01:42:55Z

So just to be clear, pretty printing of the AST is not affected, but pretty
printing the HIR does show the plain struct now (I don't know under what
circumstances if any the HIR is pretty printed). This is similar to other
desugarings like for loops and if-let which are done during HIR lowering.
On Jan 14, 2016 18:20, "Alex Crichton" notifications@github.com wrote:

Oh gah yes, sorry about that, my mistake!

—
Reply to this email directly or view it on GitHub
#30884 (comment).

durka · 2016-01-15T21:08:42Z

Regarding the ID caching stuff, I need help from @nrc or someone else familiar with this code.

I believe it was only working accidentally before, but I'm not sure. "Fixing" the bug I think I found causes an ICE elsewhere. Notably, in cache_ids() here I see that gensym_key gets set but cached_id does not. This then causes later code to assume that caching is not active, and so the gensyms don't match on consecutive lowerings.

Here is a trace of some debugging output I added, with manual indentation to show nested cache_ids() calls:

cache_ids: was not in cache, next_id is 44, now id_cache[18] = 44, cached_id = 0, gensym_key = 44 (will reset)
    cache_ids: was not in cache, next_id is 44, now id_cache[20] = 44, cached_id = 0, gensym_key = 44 (will reset)
    cache_ids: resetting, now cached_id = 0, gensym_key = 0
    str_to_ident(iter) = Ident { name: iter(72), unhygienic_name: iter(72) } (not caching)
    str_to_ident(_result) = Ident { name: _result(81), unhygienic_name: _result(81) } (not caching)
cache_ids: resetting, now cached_id = 0, gensym_key = 0
cache_ids: was in cache, cached_id was 0, now id_cache[18] = 44, cached_id = 44, gensym_key = 44 (will reset)
    cache_ids: was in cache, cached_id was 44, now id_cache[20] = 44, cached_id = 44, gensym_key = 44 (will not reset)
    cache_ids, not resetting, now cached_id = 45, gensym_key = 44
    str_to_ident(iter) = Ident { name: iter(82), unhygienic_name: iter(82) } (uncached, cached_id=44)
    str_to_ident(_result) = Ident { name: _result(83), unhygienic_name: _result(83) } (uncached, cached_id=44)
cache_ids: resetting, now cached_id = 0, gensym_key = 0

This is from the test that lowers a for loop twice to check the HIR equivalence. You can see that the first call to cache_ids() did not set cached_id, so the nested call to cache_ids() decides to reset the keys, so the gensyms of iter and _result are not cached. Oops.

Okay, but I tried to fix this, by adding this line, but uncommenting that causes an ICE somewhere else (backtrace):

thread 'rustc' panicked at 'adding a def'n for node-id 200897 and data Binding(_result(53163)) but a previous def'n exists:
DefData {
    key: DefKey {
        parent: Some(DefIndex(1701)),
        disambiguated_data: DisambiguatedDefPathData {
            data: Binding(_result(53163)),
            disambiguator: 1
        }
    },
    node_id: 200897
}', src/librustc/front/map/definitions.rs:146

Wat do?

durka · 2016-01-16T19:09:34Z

@Stebalien I added RangeInclusive::Empty::at and the From impl. It still isn't going to pass Travis because of the issue above, but take a look and see if I got it right.

Stebalien · 2016-01-16T20:40:57Z

src/libcore/ops.rs

+/// An inclusive range which is only bounded above.
+#[derive(Copy, Clone, PartialEq, Eq)]
+#[unstable(feature = "inclusive_range", reason = "recently added, follows RFC", issue = "28237")]
+pub struct RangeToInclusive<Idx> {


It should be possible to impl From<RangeTo> for this.

Agreed, should be pretty much the same.

On Sat, Jan 16, 2016 at 3:41 PM, Steven Allen notifications@github.com
wrote:

In src/libcore/ops.rs
#30884 (comment):

+impl<Idx: PartialOrd + One + Sub<Output=Idx>> From<Range> for RangeInclusive {

fn from(range: Range) -> RangeInclusive {

use self::RangeInclusive::*;

if range.start < range.end {

NonEmpty { start: range.start, end: range.end - Idx::one() }

} else {

Empty { at: range.start } // FIXME range.start or range.end?

}

}
+}

+/// An inclusive range which is only bounded above.
+#[derive(Copy, Clone, PartialEq, Eq)]
+#[unstable(feature = "inclusive_range", reason = "recently added, follows RFC", issue = "28237")]
+pub struct RangeToInclusive {

It should be possible to impl From for this.

—
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/30884/files#r49936902.

Stebalien · 2016-01-16T20:41:42Z

@durka, looks good. However, I don't know if we want to implement Iterator directly or go though some intermediate (step etc...).

durka · 2016-01-16T21:06:52Z

I note that the extant ops::Range implements Iterator directly, but there is also an impl for StepBy<Range>. That could be done for RangeInclusive as well.

durka · 2016-01-16T21:39:45Z

@Stebalien what do you think about underflow on the conversions? For example what should (..0u8).into() do?

Stebalien · 2016-01-16T21:51:24Z

@durka hm. Yeah, we probably shouldn't implement From<RangeTo>. This isn't a problem for From<Range> because we have an empty variant and 0..0 would map to RangeInclusive { Empty { at: 0 }}.

durka · 2016-01-16T22:47:36Z

@Stebalien I suppose so. Just to be clear, the intention for the From impls is that they should produce a range over the same values, so 0..10 => 0...9? I could also imagine a world in which From converts [a, b) to [a, b] so that 0..10 => 0...10. Then these overflow problems would go away. Not quite sure how to apply the Principle of Least Surprise here.

For From<Range> it's still an issue because you could have an empty range like 5..0u8, which my current implementation would try to convert to RangeInclusive::Empty { at: 0u8-1 } which would either panic or underflow. I guess I could use saturating_sub or something.

bors · 2016-01-17T05:11:50Z

☔ The latest upstream changes (presumably #30567) made this pull request unmergeable. Please resolve the merge conflicts.

nrc · 2016-01-17T23:14:05Z

@durka Hi, I'm not sure off the top of my head what to do. TBH, I'm not sure we need to keep all that stuff, but I need to think about that. Fixing it would be nice. Let me get back to you a bit later...

durka · 2016-01-18T05:47:06Z

Rebased.

Stebalien · 2016-01-18T21:19:46Z

@durka

First, I would expect the conversion should convert the endpoints to keep the same meaning. As for negative/empty ranges, 5..0u8 should be Empty { at: 5 } (an empty range starting at 5).

if end <= start {
    RangeInclusive::Empty { at: start }
} else {
    // Can't underflow as `end > start >= MIN`
    RangeInclusive::NonEmpty {
        start: start,
        end: end - 1,
}

durka · 2016-01-19T02:05:42Z

Another question (foreshadowed by a FIXME buried in my code). Are we settled on NonEmpty as the variant name or is there bikeshedding left to be done? It seems slightly weird to me to have the common case named with a negation. But I can't come up with anything better.

aturon · 2016-03-04T04:07:19Z

Hi, more apologies: been away much of this week due to family catching flu. I looked at the underflow commit and left a comment. Once that's been addressed, I think this is good to go!

durka · 2016-03-05T16:39:21Z

@aturon should be good to go.

aturon · 2016-03-05T21:40:43Z

@durka Great! Thanks for sticking with it, and let's talk RFC soon. :-)

@bors: r+

bors · 2016-03-05T21:40:44Z

📌 Commit 430b3e1 has been approved by aturon

bors · 2016-03-05T22:07:41Z

⌛ Testing commit 430b3e1 with merge deb9109...

bors · 2016-03-05T23:44:50Z

💔 Test failed - auto-win-msvc-32-opt

durka · 2016-03-06T06:21:50Z

Looks like it just timed out?

aturon · 2016-03-06T06:45:52Z

@bors: retry

@Stebalien

This PR implements [RFC 1192](https://github.com/rust-lang/rfcs/blob/master/text/1192-inclusive-ranges.md), which is triple-dot syntax for inclusive range expressions. The new stuff is behind two feature gates (one for the syntax and one for the std::ops types). This replaces the deprecated functionality in std::iter. Along the way I simplified the desugaring for all ranges. This is my first contribution to rust which changes more than one character outside of a test or comment, so please review carefully! Some of the individual commit messages have more of my notes. Also thanks for putting up with my dumb questions in #rust-internals. - For implementing `std::ops::RangeInclusive`, I took @Stebalien's suggestion from rust-lang/rfcs#1192 (comment). It seemed to me to make the implementation easier and increase type safety. If that stands, the RFC should be amended to avoid confusion. - I also kind of like @glaebhoerl's [idea](rust-lang/rfcs#1254 (comment)), which is unified inclusive/exclusive range syntax something like `x>..=y`. We can experiment with this while everything is behind a feature gate. - There are a couple of FIXMEs left (see the last commit). I didn't know what to do about `RangeArgument` and I haven't added `Index` impls yet. Those should be discussed/finished before merging. cc @gankro since you [complained](https://www.reddit.com/r/rust/comments/3xkfro/what_happened_to_inclusive_ranges/cy5j0yq) cc #27777 #30877 #1192 rust-lang/rfcs#1254 relevant to #28237 (tracking issue)

bors · 2016-03-06T07:16:42Z

⌛ Testing commit 430b3e1 with merge 8484831...

bors · 2016-03-06T09:16:30Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

This variant became unused in rust-lang#30884.

…r=alexcrichton Remove TypeOrigin::RangeExpression This variant became unused in rust-lang#30884.

rust-highfive assigned aturon Jan 13, 2016

durka force-pushed the inclusive-ranges branch from f2cb048 to e4ea932 Compare January 13, 2016 23:00

aturon assigned aturon and unassigned aturon Jan 15, 2016

durka force-pushed the inclusive-ranges branch from f7b0624 to 7f40006 Compare January 16, 2016 19:08

durka force-pushed the inclusive-ranges branch from 7f40006 to dfb4293 Compare January 16, 2016 19:28

Stebalien reviewed Jan 16, 2016
View reviewed changes

durka force-pushed the inclusive-ranges branch from dfb4293 to 96b15c5 Compare January 18, 2016 05:46

durka force-pushed the inclusive-ranges branch from 96b15c5 to 4c3ab86 Compare January 18, 2016 05:53

update snapshot comments

54cb2d9

durka force-pushed the inclusive-ranges branch from 1473d16 to 54cb2d9 Compare February 27, 2016 07:03

durka added 3 commits March 4, 2016 18:39

add more unstable annotations

003120a

add underflow/overflow tests

a928c83

remove under/overflow from next_back/next

430b3e1

durka mentioned this pull request Mar 5, 2016

Require stability annotations on fields of tuple variants #30898

Merged

bors merged commit 430b3e1 into rust-lang:master Mar 6, 2016

This was referenced Mar 6, 2016

Save/load incremental compilation dep graph #32016

Merged

Consistent no_prelude attribute #32025

Closed

libcore: add Debug implementations to most missing types #32054

Merged

implement the ? operator #31954

Merged

llogiq mentioned this pull request Mar 6, 2016

TWIR 121's CotW + PRs rust-lang/this-week-in-rust#176

Closed

mcarton mentioned this pull request Mar 7, 2016

[WIP] Rustup to rustc 1.9.0-nightly (998a6720b 2016-03-07) rust-lang/rust-clippy#744

Merged

durka mentioned this pull request Mar 18, 2016

melt the ICE when lowering an impossible range #32267

Merged

jfirebaugh added a commit to jfirebaugh/rust that referenced this pull request Oct 9, 2016

Remove TypeOrigin::RangeExpression

16a979c

This variant became unused in rust-lang#30884.

jfirebaugh mentioned this pull request Oct 9, 2016

Remove TypeOrigin::RangeExpression #37059

Merged

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 1, 2016

Rollup merge of rust-lang#37059 - jfirebaugh:unused-RangeExpression, …

9820bd0

…r=alexcrichton Remove TypeOrigin::RangeExpression This variant became unused in rust-lang#30884.

carols10cents mentioned this pull request Mar 22, 2017

Inclusive ranges are not stable, don't tell people to use them rust-lang/book#553

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement RFC 1192 inclusive ranges #30884

implement RFC 1192 inclusive ranges #30884

durka commented Jan 13, 2016

rust-highfive commented Jan 13, 2016

Stebalien commented Jan 13, 2016

durka commented Jan 13, 2016

durka commented Jan 14, 2016

durka commented Jan 14, 2016

alexcrichton commented Jan 14, 2016

durka commented Jan 14, 2016

alexcrichton commented Jan 14, 2016

durka commented Jan 15, 2016

durka commented Jan 15, 2016

durka commented Jan 16, 2016

Stebalien Jan 16, 2016

durka Jan 16, 2016

Stebalien commented Jan 16, 2016

durka commented Jan 16, 2016

durka commented Jan 16, 2016

Stebalien commented Jan 16, 2016

durka commented Jan 16, 2016

bors commented Jan 17, 2016

nrc commented Jan 17, 2016

durka commented Jan 18, 2016

Stebalien commented Jan 18, 2016

durka commented Jan 19, 2016

aturon commented Mar 4, 2016

durka commented Mar 5, 2016

aturon commented Mar 5, 2016

bors commented Mar 5, 2016

bors commented Mar 5, 2016

bors commented Mar 5, 2016

durka commented Mar 6, 2016

aturon commented Mar 6, 2016

bors commented Mar 6, 2016

bors commented Mar 6, 2016

implement RFC 1192 inclusive ranges #30884

implement RFC 1192 inclusive ranges #30884

Conversation

durka commented Jan 13, 2016

Notes

rust-highfive commented Jan 13, 2016

Stebalien commented Jan 13, 2016

durka commented Jan 13, 2016

durka commented Jan 14, 2016

durka commented Jan 14, 2016

alexcrichton commented Jan 14, 2016

durka commented Jan 14, 2016

alexcrichton commented Jan 14, 2016

durka commented Jan 15, 2016

durka commented Jan 15, 2016

durka commented Jan 16, 2016

Stebalien Jan 16, 2016

Choose a reason for hiding this comment

durka Jan 16, 2016

Choose a reason for hiding this comment

Stebalien commented Jan 16, 2016

durka commented Jan 16, 2016

durka commented Jan 16, 2016

Stebalien commented Jan 16, 2016

durka commented Jan 16, 2016

bors commented Jan 17, 2016

nrc commented Jan 17, 2016

durka commented Jan 18, 2016

Stebalien commented Jan 18, 2016

durka commented Jan 19, 2016

aturon commented Mar 4, 2016

durka commented Mar 5, 2016

aturon commented Mar 5, 2016

bors commented Mar 5, 2016

bors commented Mar 5, 2016

bors commented Mar 5, 2016

durka commented Mar 6, 2016

aturon commented Mar 6, 2016

bors commented Mar 6, 2016

bors commented Mar 6, 2016