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

Initial support for ..= syntax #44709

Merged
merged 5 commits into from
Sep 27, 2017
Merged

Conversation

Badel2
Copy link
Contributor

@Badel2 Badel2 commented Sep 20, 2017

#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

@rust-highfive
Copy link
Collaborator

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
Copy link
Member

[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

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

Choose a reason for hiding this comment

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

Sigh, the syntax is incredibly ugly.

Copy link

Choose a reason for hiding this comment

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

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

@petrochenkov petrochenkov self-assigned this Sep 20, 2017
@@ -147,6 +147,8 @@ pub enum Token {
Dot,
DotDot,
DotDotDot,
DotDotEquals,
DotEq, // HACK(durka42) never produced by the parser, only used for libproc_macro
Copy link
Member

Choose a reason for hiding this comment

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

Why is the token called DotDotEquals, not DotDotEq?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Badel2
Copy link
Contributor Author

Badel2 commented Sep 20, 2017

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
Copy link
Member

kennytm commented Sep 20, 2017

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

@@ -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))]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@durka
Copy link
Contributor

durka commented Sep 20, 2017 via email

@kennytm
Copy link
Member

kennytm commented Sep 20, 2017

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

durka commented Sep 20, 2017 via email

@kennytm
Copy link
Member

kennytm commented Sep 20, 2017

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
Copy link
Contributor

durka commented Sep 20, 2017 via email

@Badel2
Copy link
Contributor Author

Badel2 commented Sep 20, 2017

@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
Copy link
Member

kennytm commented Sep 20, 2017

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it's AssocOp, nevermind.

@petrochenkov
Copy link
Contributor

@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 Badel2 force-pushed the inclusive-range-dotdoteq branch from a42fc89 to ff012da Compare September 20, 2017 22:31
@aidanhs 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
@Badel2
Copy link
Contributor Author

Badel2 commented Sep 21, 2017

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

r? @petrochenkov

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 21, 2017

📌 Commit ff012da has been approved by petrochenkov

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 21, 2017

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

Badel2 commented Sep 21, 2017

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

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

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

bors commented Sep 22, 2017

💔 Test failed - status-appveyor

@scottmcm
Copy link
Member

[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
Copy link
Contributor Author

Badel2 commented Sep 22, 2017

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

@petrochenkov
Copy link
Contributor

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

Badel2 commented Sep 25, 2017

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

@petrochenkov
Copy link
Contributor

@Badel2

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

Yes.

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

@bors r+

@bors
Copy link
Contributor

bors commented Sep 27, 2017

📌 Commit 5102309 has been approved by petrochenkov

@bors
Copy link
Contributor

bors commented Sep 27, 2017

⌛ Testing commit 5102309 with merge 0e6f4cf...

bors added a commit that referenced this pull request Sep 27, 2017
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
Copy link
Contributor

bors commented Sep 27, 2017

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

@bors bors merged commit 5102309 into rust-lang:master Sep 27, 2017
This was referenced Sep 27, 2017
@leonardo-m
Copy link

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
Copy link
Contributor Author

Badel2 commented Sep 28, 2017

@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
Copy link

Flaise commented Oct 4, 2017

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

@UtherII
Copy link

UtherII commented Oct 4, 2017

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

@b-jonas0
Copy link

b-jonas0 commented Oct 4, 2017

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

@durka
Copy link
Contributor

durka commented Oct 4, 2017 via email

@zengsai
Copy link

zengsai commented Oct 6, 2017

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
Copy link
Contributor Author

Badel2 commented Oct 7, 2017

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.

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

@petrochenkov
Copy link
Contributor

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

bors added a commit that referenced this pull request Nov 10, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.