implement RFC 1192 inclusive ranges

[durka]

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]

r? @aturon

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

[Stebalien]

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]

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

[durka]

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]

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]

Edit: I'm wrong

[durka]

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]

Oh gah yes, sorry about that, my mistake!

[durka]

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]

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]

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

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

[durka]

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]

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

[durka]

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]

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

[Stebalien]

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

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

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

[nrc]

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

Rebased.

[Stebalien]

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

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]

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]

@aturon should be good to go.

[aturon]

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

@bors: r+

[bors]

📌 Commit 430b3e1 has been approved by aturon

[bors]

⌛ Testing commit 430b3e1 with merge deb9109...

[bors]

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

[durka]

Looks like it just timed out?

[aturon]

@bors: retry

[bors]

⌛ Testing commit 430b3e1 with merge 8484831...

[bors]

☀️ 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