Initial support for `..=` syntax by Badel2 · Pull Request #44709 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation53 Commits5 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

#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

clarfonthey, adelarsq, BrianOn99, Eijebong, LYP951018, and nsmaciej reacted with thumbs up emoji UtherII, Flaise, zengsai, and samoylovfp reacted with thumbs down emoji durka, QuietMisdreavus, adelarsq, vitiral, and Kerollmops reacted with hooray emoji quininer, UtherII, tjkirch, stepancheg, Flaise, zengsai, and tengwar reacted with confused emoji scottmcm and adelarsq reacted with heart emoji

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.

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

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.

svartalf, goodmanjonathan, UtherII, tomaka, tjkirch, estebank, ActuallyaDeviloper, IBUzPE9, ryanwilliams, dumindu, and 9 more reacted with thumbs up emoji kennytm, tomaka, ParadoxSpiral, tbillington, r0xsh, and rawrafox reacted with laugh emoji

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.

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

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?

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.

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?

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

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?

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.

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

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

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

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.

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

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

@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 \|
// If we didn’t have to handle `x..`/`x...`, it would be pretty easy to
} else if op == AssocOp::DotDot \|

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.

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.

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

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

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

r? @petrochenkov

📌 Commit ff012da has been approved by petrochenkov

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

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

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

[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?

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

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

@Badel2

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

Yes.

📌 Commit 5102309 has been approved by petrochenkov

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

This was referenced

Sep 27, 2017

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
    }
}

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

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

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

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

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

Labels

S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

Initial support for ..= syntax by Badel2 · Pull Request #44709 · rust-lang/rust (original) (raw)

Conversation

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

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

Initial support for `..=` syntax by Badel2 · Pull Request #44709 · rust-lang/rust (original) (raw)