Initial support for `..=` syntax #44709

Badel2 · 2017-09-20T01:19:03Z

This PR adds ..= as a synonym for ... in patterns and expressions.
Since ... in expressions was never stable, we now issue a warning.

cc @durka
r? @aturon

rust-highfive · 2017-09-20T01:19:10Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

scottmcm · 2017-09-20T02:26:41Z

[00:50:58] ---- [compile-fail] compile-fail/E0586.rs stdout ----
[00:50:58] 	
[00:50:58] error: /checkout/src/test/compile-fail/E0586.rs:13: unexpected "warning": '13:19: 13:22: `...` is being replaced by `..=`'
[00:50:58] 
[00:50:58] error: 1 unexpected errors found, 0 expected errors not found

petrochenkov · 2017-09-20T09:13:02Z

src/test/incremental/hashes/indexing_expressions.rs

@@ -153,5 +153,5 @@ fn exclusive_to_inclusive_range(slice: &[u32]) -> &[u32] {
 #[rustc_metadata_clean(cfg="cfail2")]
 #[rustc_metadata_clean(cfg="cfail3")]
 fn exclusive_to_inclusive_range(slice: &[u32]) -> &[u32] {
-    &slice[3...7]
+    &slice[3..=7]


Sigh, the syntax is incredibly ugly.

Now I see all these examples, I am more convinced than ever that this syntax is a bad idea.

kennytm · 2017-09-20T12:54:25Z

src/libsyntax/parse/token.rs

@@ -147,6 +147,8 @@ pub enum Token {
    Dot,
    DotDot,
    DotDotDot,
+    DotDotEquals,
+    DotEq, // HACK(durka42) never produced by the parser, only used for libproc_macro


Why is the token called DotDotEquals, not DotDotEq?

Hey, that's a good name! I will probably change it.

Badel2 · 2017-09-20T13:30:48Z

I should have mentioned this earlier: right now we issue a warning if ... is used in expressions. However the compiler itself uses ... in libcore/slice/mod.rs, which means that it will compile with warnings until we can update it to ..=. Is this a problem?

kennytm · 2017-09-20T13:39:28Z

@Badel2: #![cfg_attr(not(stage0), allow(that_warning))], and add a FIXME saying "replace remaining ... by ..= after next stage0".

kennytm · 2017-09-20T17:05:47Z

src/libcore/slice/mod.rs

@@ -16,6 +16,9 @@

 #![stable(feature = "rust1", since = "1.0.0")]

+// FIXME: replace remaining ... by ..= after next stage0
+// Silence warning: "... is being replaced by ..="
+#![cfg_attr(not(stage0), allow(warnings))]


Errr is there no way to silence just the `...` is being replaced by `..=` message, instead of all warnings?

It's because the warn_dotdoteq function just shows a generic warning without any flag.

durka · 2017-09-20T17:10:10Z

@kennytm how do you output a warning that has a name to use in `#[allow(xxx)]`?

…

On Wed, Sep 20, 2017 at 1:06 PM, kennytm ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/libcore/slice/mod.rs <#44709 (comment)>: > @@ -16,6 +16,9 @@ #![stable(feature = "rust1", since = "1.0.0")] +// FIXME: replace remaining ... by ..= after next stage0 +// Silence warning: "... is being replaced by ..=" +#![cfg_attr(not(stage0), allow(warnings))] Errr is there no way to silence just the `...` is being replaced by `..=` message, instead of all warnings? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44709 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n0SAa_AvtFHv1wjPdDXS-Z8LmnzIks5skUYSgaJpZM4PdN3Z> .

kennytm · 2017-09-20T17:15:12Z

@durka Turn the ... checking into an early lint?

I'm just afraid allow(warning) is too broad, even though it just affects one cycle.

durka · 2017-09-20T17:27:27Z

@kennytm want to join as a co-mentor? I have no idea how to make lints.

…

On Wed, Sep 20, 2017 at 1:16 PM, kennytm ***@***.***> wrote: @durka <https://github.com/durka> Turn the ... checking into an early lint? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44709 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n94QeDtdH-6Fw9yMVYZ0rVB0fgxHks5skUhegaJpZM4PdN3Z> .

kennytm · 2017-09-20T17:50:58Z

Actually a lint may be not desirable, since lints are run after the AST is built, but these warnings are emitted during parsing. So either the AST needs to retain knowledge whether the closed range is of the form x ... y or x ..= y, or the lint pass will need to re-parse the expression to figure out which operator is used.

durka · 2017-09-20T18:03:52Z

Yeah, for the lint we would need to add an "old syntax was used" flag to the AST.

…

On Wed, Sep 20, 2017 at 1:51 PM, kennytm ***@***.***> wrote: Actually a lint may be not desirable, since lints are run after the AST is built, but these warnings are emitted during parsing. So either the AST needs to retain knowledge whether the closed range is of the form x ... y or x ..= y, or the lint pass will need to re-parse the expression to figure out which operator is used. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44709 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n0PlBaBjfMZmamF_rwFSjHZNuwUAks5skVC9gaJpZM4PdN3Z> .

Badel2 · 2017-09-20T18:50:02Z

@kennytm I guess one solution would be temporally replacing a...b with the long form: std::ops::RangeInclusive { start: a, end: b }, would you prefer this workaround?

kennytm · 2017-09-20T18:51:29Z

@Badel2 LGTM. Retain the "Change RangeInclusive { start, end } back to start ..= end" FIXME though.

petrochenkov · 2017-09-20T21:46:35Z

src/libsyntax/parse/parser.rs

@@ -2770,12 +2774,13 @@ impl<'a> Parser<'a> {
                    }
                };
                continue
-            } else if op == AssocOp::DotDot || op == AssocOp::DotDotDot {
-                // If we didn’t have to handle `x..`/`x...`, it would be pretty easy to
+            } else if op == AssocOp::DotDot || op == AssocOp::DotDotEq {


Looks like op == AssocOp::DotDotDot shouldn't be removed from here yet.

Oh, it's AssocOp, nevermind.

petrochenkov · 2017-09-20T21:57:43Z

@Badel2
LGTM.
Could you squash into two commits - one with the actual change and one with fallout from the warning?
r=me after that

Badel2 · 2017-09-21T08:57:10Z

All right, I admit it looks way better now with only two commits.

r? @petrochenkov

petrochenkov · 2017-09-21T09:16:13Z

@bors r+

bors · 2017-09-21T09:16:14Z

📌 Commit ff012da has been approved by petrochenkov

petrochenkov · 2017-09-21T09:22:54Z

@bors r-

Wait a second. It looks like ..= in range patterns becomes insta-stable with this PR.
@Badel2, could you add a flag indicating the used syntax in PatKind::Range and check in in feature_gate.rs?
(AST is supposed to be close to the source, so this information should be kept anyway.)

Badel2 · 2017-09-21T09:43:36Z

@petrochenkov You mean like this?

/// The `bool` is true when the new `..=` syntax is used
Range(P<Expr>, P<Expr>, RangeEnd, bool),

And how I call the feature? dotdoteq_in_patterns?

petrochenkov · 2017-09-21T09:59:12Z

@Badel2
A dedicated enum would probably be better than bool in this case

enum RangeSyntax {
    DotDotDot,
    DotDotEq,
}

and it's better inserted into RangeEnd::Included, not ExprKind::Range.
P.S. No need to lower it in HIR, this is a purely syntactic thing.

dotdoteq_in_patterns

LGTM.

bors · 2017-09-22T22:24:21Z

💔 Test failed - status-appveyor

scottmcm · 2017-09-22T22:30:17Z

[01:05:00] error[E0532]: expected unit struct/variant or constant, found tuple variant `RangeEnd::Included`
[01:05:00]   --> src\tools\rustfmt\src\patterns.rs:62:17
[01:05:00]    |
[01:05:00] 62 |                 RangeEnd::Included => rewrite_pair(&**lhs, &**rhs, "", "...", "", context, shape),
[01:05:00]    |                 ^^^^^^^^^^--------
[01:05:00]    |                           |
[01:05:00]    |                           did you mean `Excluded`?

Does this need the synchronized rustfmt update dance?

Badel2 · 2017-09-22T23:07:29Z

@scottmcm if the "synchronized rustfmt update dance" is too complicated, a simple workaround would be temporarily replacing RangeEnd::Included with _.

petrochenkov · 2017-09-25T08:28:47Z

@Badel2
Have you tried to build rustfmt with this branch? There may be other errors.
(This is usually done with help of rustup - https://github.com/rust-lang-nursery/rustup.rs/#working-with-custom-toolchains-and-local-builds)
If workarounds like replacing with _ works for all of them, then it will be simpler indeed.

Badel2 · 2017-09-25T17:58:38Z

@petrochenkov that's the only error. So I guess now I need to submit a PR to the rustfmt repo?

petrochenkov · 2017-09-25T18:10:13Z

@Badel2

So I guess now I need to submit a PR to the rustfmt repo?

Yes.

petrochenkov · 2017-09-27T11:45:12Z

@bors r+

bors · 2017-09-27T11:45:13Z

📌 Commit 5102309 has been approved by petrochenkov

bors · 2017-09-27T16:04:39Z

⌛ Testing commit 5102309 with merge 0e6f4cf...

@durka

Initial support for `..=` syntax #28237 This PR adds `..=` as a synonym for `...` in patterns and expressions. Since `...` in expressions was never stable, we now issue a warning. cc @durka r? @aturon

bors · 2017-09-27T18:39:51Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 0e6f4cf to master...

leonardo-m · 2017-09-28T11:04:45Z

This shows one warning in a case and no warning in another case:

#![feature(inclusive_range_syntax)]

fn main() {
    let c = b'c';

    match c {
        b'a' ... b'c' => println!("a-c"), // Doesn't warn
        _ => println!("other"),
    }

    for _ in 0 ... 10 { // Warns
    }
}

Badel2 · 2017-09-28T11:08:57Z

@leonardo-m yes, that's the expected behavior, and in the near future the second case will not compile. We can't warn on the first case because that's been stable for a long time.

Flaise · 2017-10-04T13:14:33Z

I don't understand why an unstable ... operator is getting replaced with a stable ..= operator instead of simply stabilizing ...

UtherII · 2017-10-04T13:42:31Z

Because some people fear that ... can be easily too mistaken with ..

b-jonas0 · 2017-10-04T14:30:01Z

@Flaise the unstable ... syntax is utterly confusing to some people, because in Ruby, .. means an inclusive range and ... means a half-inclusive.

durka · 2017-10-04T16:05:10Z

The syntax decision was extensively discussed in the tracking issue (linked at the top of this PR). We don't need to rehash it here.

…

On Oct 4, 2017 16:31, "b-ambrus" ***@***.***> wrote: @Flaise <https://github.com/flaise> the unstable ... syntax is utterly confusing to some people, because in Ruby, .. means an inclusive range and ... means a half-inclusive. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44709 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n_eG31F6K1NtQmhBuIYxdJqwqwx4ks5so5angaJpZM4PdN3Z> .

zengsai · 2017-10-06T00:52:25Z

To Rust Core Team: Regardless of community's opposition and made a wrong decision, You'll regret it!

I'd spell creat with an 'e'. by Ken Thompson

Badel2 · 2017-10-07T17:42:38Z

I just realized there is one test that still uses the old syntax in expressions, doesn't show my warning, and doesn't use the feature gate.

rust/src/test/run-pass-fulldeps/issue-35829.rs

Line 45 in bb4d149

let closed_range = quote_expr!(&cx, 0 ... 1);

petrochenkov · 2017-10-07T17:47:04Z

@Badel2
run-pass tests successfully pass even if there are warnings and output from passing tests is suppressed.

Add error for `...` in expressions Follow-up to #44709 Tracking issue: #28237 * Using `...` in expressions was a warning, now it's an error * The error message suggests using `..` or `..=` instead, and explains the difference * Updated remaining occurrences of `...` to `..=` r? petrochenkov

rust-highfive assigned aturon Sep 20, 2017

petrochenkov reviewed Sep 20, 2017

View reviewed changes

petrochenkov self-assigned this Sep 20, 2017

kennytm reviewed Sep 20, 2017

View reviewed changes

petrochenkov reviewed Sep 20, 2017

View reviewed changes

Badel2 force-pushed the inclusive-range-dotdoteq branch from a42fc89 to ff012da Compare September 20, 2017 22:31

aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 20, 2017

rust-highfive unassigned aturon Sep 21, 2017

Badel2 force-pushed the inclusive-range-dotdoteq branch 3 times, most recently from 720782b to c947e05 Compare September 21, 2017 11:17

Badel2 mentioned this pull request Sep 25, 2017

Temporarily replace RangeEnd::Included with _ rust-lang/rustfmt#1996

Merged

bump rustfmt

5102309

Badel2 force-pushed the inclusive-range-dotdoteq branch from dde0c7c to 5102309 Compare September 26, 2017 19:15

bors merged commit 5102309 into rust-lang:master Sep 27, 2017

This was referenced Sep 27, 2017

Rls and Rustfmt stuff #44769

Closed

Update some minor dependencies #44785

Merged

Badel2 mentioned this pull request Nov 5, 2017

Add error for ... in expressions #45773

Merged

kennytm mentioned this pull request Dec 31, 2017

Tracking issue for ..= inclusive ranges (RFC #1192) -- originally ... #28237

Closed

8 tasks

Initial support for ..= syntax #44709

Initial support for ..= syntax #44709

Conversation

Badel2 commented Sep 20, 2017

rust-highfive commented Sep 20, 2017

scottmcm commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badel2 commented Sep 20, 2017

kennytm commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durka commented Sep 20, 2017 via email

kennytm commented Sep 20, 2017 • edited Loading

durka commented Sep 20, 2017 via email

kennytm commented Sep 20, 2017

durka commented Sep 20, 2017 via email

Badel2 commented Sep 20, 2017

kennytm commented Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Sep 20, 2017

Badel2 commented Sep 21, 2017

petrochenkov commented Sep 21, 2017

bors commented Sep 21, 2017

petrochenkov commented Sep 21, 2017 • edited Loading

Badel2 commented Sep 21, 2017

petrochenkov commented Sep 21, 2017

bors commented Sep 22, 2017

scottmcm commented Sep 22, 2017

Badel2 commented Sep 22, 2017

petrochenkov commented Sep 25, 2017

Badel2 commented Sep 25, 2017

petrochenkov commented Sep 25, 2017

petrochenkov commented Sep 27, 2017

bors commented Sep 27, 2017

bors commented Sep 27, 2017

bors commented Sep 27, 2017

leonardo-m commented Sep 28, 2017

Badel2 commented Sep 28, 2017

Flaise commented Oct 4, 2017

UtherII commented Oct 4, 2017 • edited Loading

b-jonas0 commented Oct 4, 2017

durka commented Oct 4, 2017 via email

zengsai commented Oct 6, 2017

Badel2 commented Oct 7, 2017

petrochenkov commented Oct 7, 2017

Initial support for `..=` syntax #44709

Initial support for `..=` syntax #44709

kennytm commented Sep 20, 2017 •

edited

Loading

kennytm commented Sep 20, 2017 •

edited

Loading

petrochenkov commented Sep 21, 2017 •

edited

Loading

UtherII commented Oct 4, 2017 •

edited

Loading