2018 edition ? Kleene operator by mark-i-m · Pull Request #51587 · 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
Conversation95 Commits7 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 }})
This is my first attempt at implementing the migration lint + 2018 behavior as discussed in #48075
Also, I think I should add some more tests for 2015 vs 2018 behavior.
2018 edition [WIP] 2018 edition ? Kleene operator? Kleene operator
Nice work, @mark-i-m! Hoping we can get this merged in soon.
I'm not quite sure I got the lint right... it's currently a straight up warning...
@mark-i-m Yes, there look to be 1001 different ways of linting in the compiler code. This is the sort of thing that could really do with a chapter in the rustc book!
@mark-i-m Oh gosh, I really should have checked before making that comment! The rustc book has come a long way since I last read it. The one thing still really deficient is the macros chapter, sadly... Also, it reminds me I should submit a PR about incremental compilation tests.
| } |
|---|
| return (Some(token::Question), op); |
| } |
| Ok(Ok(op)) => return (Some(token::Question), op), |
| Ok(Ok(op)) => { |
| if !features.macro_at_most_once_rep |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the feature gate is not quite acting like I expect. My expectation is:
- In Edition 2015:
- If you do
$()?[*+], then you get "?as separator". - Else, if you do
$(foo)?[^*+], treat as "optional":
* But if feature gate is not enabled, error
- If you do
- In Edition 2018:
- If you do
$(foo)?, then treat as "optional":
* But if feature gate is not enabled, error.
- If you do
My goal is that, when it comes time to stabilize, we can just "constant propagate" as if the feature gate was always enabled and get the final behavior we want:
- in Rust 2015 we fallback to old behavior
- in Rust 2018 we use new behavior all the time
(Is this the plan? Do we have an official, FCP'd proposal somewhere, anyway?)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Is this the plan? Do we have an official, FCP'd proposal somewhere, anyway?)
No, we don't TMK
in Rust 2015 we fallback to old behavior
Given that this is behavior that will only last for another couple of months, my preference is to
- just keep this unstable on 2015 and only enable in 2018
- make the feature flag always turn on 2018 behavior
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to clarify:
- In edition 2015:
- Without feature flag:
?is a separator.?is not a Kleene op. - With feature flag: same as 2018
- Without feature flag:
- In edition 2018
?is not a separator.?is a Kleene op that does not accept a separator.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer this:
- In edition 2015:
- With or without feature flag:
?is a separator and there is no? - However, using
?as a separator issues arust_2018_migrationwarning
- With or without feature flag:
- In edition 2018:
?is always a separator
* and feature-gate is required until we stabilize
Point is, as before, that the feature-gate should just be a "error or no error" filter -- the behavior should model what we eventually want when all is said and done otherwise. And we never want Rust 2015 to support ?.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In edition 2018:
? is always a separator
You mean "in 2018, ? is always a kleene op", right?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'd sure hope so heh!)
@nikomatsakis I think I have implemented what you specified with one major exception: currently, the warning for using ? as a separator in 2015 edition always fires (i.e. it's not informed by #[warn()]).
Do you know how I can hook up the warning to fire when the lint group is set? Also, should it be a rust_2018_compatibility warning or a rust_2018_idioms warning, as mentioned in #50620? My inclination is towards the former.
This comment has been minimized.
This still needs some work. I am currently fixing/writing some tests.
@nikomatsakis I pushed a few tests. Do these tests represent the behavior you were expecting?
This comment has been minimized.
This comment has been minimized.
Ok, I think I've implemented and added tests for everything except that the migration warning is currently an unconditional warning.
All feedback is welcome!
[WIP] 2018 edition [Almost not WIP] 2018 edition ? Kleene operator? Kleene operator
This comment has been minimized.
Doh. I forgot that I started using this in places in the compiler 😛
Hmm... some of these are going to be hard to fix...
This comment has been minimized.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
| // Feature gate test for macro_at_most_once_rep under 2018 edition. |
|---|
| // gate-test-macro_at_most_once_rep |
| // compile-flags: --edition=2018 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated, but we should really add a // edition: 2018 option, shouldn't we?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
| @@ -0,0 +1,71 @@ |
|---|
| error[E0658]: Using the `?` macro Kleene operator for "at most one" repetition is unstable (see issue #48075) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: start compiler messages w/ lower-case letters
@mark-i-m travis is unhappy, but this code looks good to me overall
Great! @mark-i-m want to rebase and I'll do a final once-over and r+?
📌 Commit 10ee0f6 has been approved by alexcrichton
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-blocked
Status: Blocked on something else such as an RFC or other implementation work.
labels
bors added a commit that referenced this pull request
2018 edition ? Kleene operator
This is my first attempt at implementing the migration lint + 2018 behavior as discussed in #48075
📣 Toolstate changed by #51587!
Tested on commit f498e4e.
Direct link to PR: #51587
💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra).
💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).
💔 rustfmt on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra).
💔 rustfmt on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).
rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request
Tested on commit rust-lang/rust@f498e4e. Direct link to PR: <rust-lang/rust#51587>
💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rustfmt on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rustfmt on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).
[01:09:02] --> /cargo/registry/src/github.com-1ecc6299db9ec823/rustc-ap-syntax-182.0.0/ext/expand.rs:47:60
[01:09:02] |
[01:09:02] 47 | <span class="katex"><span class="katex-mathml"><math xmlns="http://www.w3.org/1998/Math/MathML"><semantics><mrow><mo stretchy="false">(</mo><mi>o</mi><mi>n</mi><mi>e</mi><mi>f</mi><mi>n</mi></mrow><annotation encoding="application/x-tex">(one fn </annotation></semantics></math></span><span class="katex-html" aria-hidden="true"><span class="base"><span class="strut" style="height:1em;vertical-align:-0.25em;"></span><span class="mopen">(</span><span class="mord mathnormal">o</span><span class="mord mathnormal">n</span><span class="mord mathnormal">e</span><span class="mord mathnormal" style="margin-right:0.10764em;">f</span><span class="mord mathnormal">n</span></span></span></span>fold_ast:ident; fn $visit_ast:ident;)?
[01:09:02] | ^
[01:09:02] |
[01:09:02] = note: `?` is not a macro repetition operator
We need to update rustc-ap-syntax?
Edit: We'll need to wait for rustc-ap-syntax v209 to include this PR that fixes the use of ? inside libsyntax.
kennytm added a commit to rust-lang/rustfmt that referenced this pull request
This was referenced
Jul 26, 2018
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.