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

2018 edition `?` Kleene operator #51587

Merged
merged 7 commits into from Jul 24, 2018

Conversation

@mark-i-m
Copy link
Contributor

commented Jun 16, 2018

This is my first attempt at implementing the migration lint + 2018 behavior as discussed in #48075

r? @nikomatsakis

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2018

@nikomatsakis Is this close to what you had in mind?

@mark-i-m mark-i-m referenced this pull request Jun 16, 2018

Closed

tracking issue for `?` macro repetition #48075

6 of 6 tasks complete
@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2018

Also, I think I should add some more tests for 2015 vs 2018 behavior.

@mark-i-m mark-i-m changed the title 2018 edition `?` Kleene operator [WIP] 2018 edition `?` Kleene operator Jun 16, 2018

@alexreg

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2018

Nice work, @mark-i-m! Hoping we can get this merged in soon.

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2018

I'm not quite sure I got the lint right... it's currently a straight up warning...

@alexreg

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2018

@alexreg

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2018

@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

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jun 18, 2018

Contributor

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
  • In Edition 2018:
    • If you do $(foo)?, then treat as "optional":
      • But if feature gate is not enabled, error.

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

This comment has been minimized.

Copy link
@mark-i-m

mark-i-m Jun 18, 2018

Author Contributor

(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

This comment has been minimized.

Copy link
@mark-i-m

mark-i-m Jun 18, 2018

Author Contributor

So to clarify:

  • In edition 2015:
    • Without feature flag: ? is a separator. ? is not a Kleene op.
    • With feature flag: same as 2018
  • In edition 2018
    • ? is not a separator. ? is a Kleene op that does not accept a separator.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jun 21, 2018

Contributor

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 a rust_2018_migration warning
  • 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 ?.

This comment has been minimized.

Copy link
@mark-i-m

mark-i-m Jun 22, 2018

Author Contributor

In edition 2018:
? is always a separator

You mean "in 2018, ? is always a kleene op", right?

This comment has been minimized.

Copy link
@alexreg

alexreg Jun 22, 2018

Contributor

(I'd sure hope so heh!)

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2018

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

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented Jun 23, 2018

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
    100% |████████████████████████████████| 1.3MB 964kB/s 
Collecting botocore==1.10.45 (from awscli)
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/2a/c8/b180fb83fa46d2b56ea059177dc3c00647d622987daf5e7ffbc658555ede/botocore-1.10.45-py2.py3-none-any.whl (4.4MB)
    0% |                                | 10kB 43.2MB/s eta 0:00:01
    0% |▏                               | 20kB 18.6MB/s eta 0:00:01
    0% |▎                               | 30kB 23.5MB/s eta 0:00:01
    0% |▎                               | 40kB 14.2MB/s eta 0:00:01
---
[00:23:15]    Compiling syntax_pos v0.0.0 (file:///checkout/src/libsyntax_pos)
[00:23:18]    Compiling rustc_errors v0.0.0 (file:///checkout/src/librustc_errors)
[00:24:14]    Compiling proc_macro v0.0.0 (file:///checkout/src/libproc_macro)
[00:24:25]    Compiling syntax_ext v0.0.0 (file:///checkout/src/libsyntax_ext)
[00:24:25] error: `?` is not a macro repetition operator
[00:24:25]    --> librustc/lint/mod.rs:111:52
[00:24:25]     |
[00:24:25] 111 |      $lint_edition: expr => $edition_level: ident $(,)?
[00:24:25] 
[00:24:25] 
[00:24:25] error: expected `*` or `+`
[00:24:25]    --> librustc/lint/mod.rs:111:52
[00:24:25]     |
[00:24:25] 111 |      $lint_edition: expr => $edition_level: ident $(,)?
[00:24:25] 
[00:24:25] 
[00:24:25] error: `?` is not a macro repetition operator
[00:24:25]    --> librustc/lint/mod.rs:125:25
[00:24:25]     |
[00:24:25] 125 |     ($( $lint:expr ),* $(,)?) => {{
[00:24:25] 
[00:24:25] 
[00:24:25] error: expected `*` or `+`
[00:24:25]    --> librustc/lint/mod.rs:125:25
[00:24:25]     |
[00:24:25] 125 |     ($( $lint:expr ),* $(,)?) => {{
[00:24:25] 
[00:24:55] error: aborting due to 4 previous errors
[00:24:55] 
[00:24:55] error: Could not compile `rustc`.
[00:24:55] error: Could not compile `rustc`.
[00:24:55] 
[00:24:55] Caused by:
[00:24:55]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name rustc librustc/lib.rs --color always --error-format json --crate-type dylib --emit=dep-info,link -C prefer-dynamic -C opt-level=2 -C metadata=976f6e3c1eb047f8 -C extra-filename=-976f6e3c1eb047f8 --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/release/deps --extern polonius_engine=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps/libpolonius_engine-c8176dd12ca6552b.rlib --extern arena=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps/libarena-44e761f8b954b499.so --extern rustc_errors=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_errors-22e1fd34ac64b008.so --extern serialize=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps/libserialize-c8132f366b701b34.so --extern serialize=/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps/libserialize-c8132f366b701b34.rlib --extern graphviz=/checkout/obj/build/x86_64-unknown-linux-gnu/s

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2018

This still needs some work. I am currently fixing/writing some tests.

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2018

@nikomatsakis I pushed a few tests. Do these tests represent the behavior you were expecting?

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented Jun 24, 2018

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:start:tidy
tidy check
[00:06:05] * 553 error codes
[00:06:05] * highest error code: E0906
[00:06:05] tidy error: Found 1 features without a gate test.
[00:06:05] Expected a gate test for the feature 'macro_at_most_once_rep'.
[00:06:05] Hint: create a failing test file named 'feature-gate-macro_at_most_once_rep.rs'
[00:06:05]       in the 'ui' test suite, with its failures due to
[00:06:05]       missing usage of #![feature(macro_at_most_once_rep)].
[00:06:05] Hint: If you already have such a test and don't want to rename it,
[00:06:05]       you can also add a // gate-test-macro_at_most_once_rep line to the test file.
[00:06:06] some tidy checks failed
[00:06:06] 
[00:06:06] 
[00:06:06] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:06:06] 
[00:06:06] 
[00:06:06] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:06:06] Build completed unsuccessfully in 0:01:54
[00:06:06] Build completed unsuccessfully in 0:01:54
[00:06:06] Makefile:79: recipe for target 'tidy' failed
[00:06:06] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:02a807e4
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:00f9ac0b:start=1529804763115039036,finish=1529804763120905889,duration=5866853
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1b0e3cc9
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:09573790
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors

This comment was marked as resolved.

Copy link
Contributor

commented Jun 24, 2018

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

@mark-i-m mark-i-m force-pushed the mark-i-m:at_most_once_rep_2018 branch from 21562f6 to e5ba108 Jun 24, 2018

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2018

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!

@mark-i-m mark-i-m changed the title [WIP] 2018 edition `?` Kleene operator [Almost not WIP] 2018 edition `?` Kleene operator Jun 25, 2018

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented Jun 25, 2018

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2018

Doh. I forgot that I started using this in places in the compiler 😛

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2018

Hmm... some of these are going to be hard to fix...

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented Jun 25, 2018

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:55:52] ---- [run-pass] run-pass/macro-at-most-once-rep.rs stdout ----
[00:55:52] 
[00:55:52] error: compilation failed!
[00:55:52] status: exit code: 101
[00:55:52] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/macro-at-most-once-rep.rs" "--target=x86_64-unknown-linux-gnu" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/macro-at-most-once-rep/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/macro-at-most-once-rep/auxiliary"
[00:55:52] ------------------------------------------
[00:55:52] 
[00:55:52] ------------------------------------------
[00:55:52] stderr:
[00:55:52] stderr:
[00:55:52] ------------------------------------------
[00:55:52] error: expected `*` or `+`
[00:55:52]    |
[00:55:52]    |
[00:55:52] 24 |     ($($a:ident)? ; $num:expr) => { {
[00:55:52]    |
[00:55:52]    |
[00:55:52]    = note: `?` is not a macro repetition operator
[00:55:52] 
[00:55:52] error: expected `*` or `+`
[00:55:52]    |
[00:55:52] 29 |          )?
[00:55:52]    |           ^
[00:55:52]    |
[00:55:52]    |
[00:55:52]    = note: `?` is not a macro repetition operator
[00:55:52] error: aborting due to 2 previous errors
[00:55:52] 
[00:55:52] 
[00:55:52] ------------------------------------------
---
[00:55:52] 
[00:55:52] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:498:22
[00:55:52] 
[00:55:52] 
[00:55:52] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "run-pass" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-3.9/bin/File3380992 .
2490436 ./obj/build
1886572 ./obj/build/x86_64-unknown-linux-gnu
729176 ./src
549328 ./obj/build/x86_64-unknown-linux-gnu/stage0
---
143784 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu
143780 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
138744 ./obj/build/bootstrap/debug/incremental
124172 ./obj/build/bootstrap/debug/incremental/bootstrap-1r3bppl29tbrj
124168 ./obj/build/bootstrap/debug/incremental/bootstrap-1r3bppl29tbrj/s-f2aum0tdwn-13fia35-183djf9a4im2w
107672 ./obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release
103612 ./obj/build/x86_64-unknown-linux-gnu/stage0/lib/ruibcompiler_builtins/modules
32180 ./.git/modules/src/libcompiler_builtins/modules/compiler-rt
31752 ./.git/modules/src/libcompiler_builtins/modules/compiler-rt/objects

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@nikomatsakis
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jun 25, 2018

Contributor

unrelated, but we should really add a // edition: 2018 option, shouldn't we?

This comment has been minimized.

Copy link
@mark-i-m

mark-i-m Jun 26, 2018

Author Contributor

Created #51800

This comment has been minimized.

Copy link
@mark-i-m

mark-i-m Jun 28, 2018

Author Contributor

Done!

@@ -0,0 +1,71 @@
error[E0658]: Using the `?` macro Kleene operator for "at most one" repetition is unstable (see issue #48075)

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jun 25, 2018

Contributor

Nit: start compiler messages w/ lower-case letters

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2018

@mark-i-m travis is unhappy, but this code looks good to me overall

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

@nikomatsakis Is it ok that the warning is unconditional?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

Great! @mark-i-m want to rebase and I'll do a final once-over and r+?

@mark-i-m mark-i-m force-pushed the mark-i-m:at_most_once_rep_2018 branch from 42d4142 to 10ee0f6 Jul 24, 2018

@mark-i-m

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

🎉 Rebased :)

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 24, 2018

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

📌 Commit 10ee0f6 has been approved by alexcrichton

@bors bors added S-waiting-on-bors and removed S-blocked labels Jul 24, 2018

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

⌛️ Testing commit 10ee0f6 with merge f498e4e...

bors added a commit that referenced this pull request Jul 24, 2018

Auto merge of #51587 - mark-i-m:at_most_once_rep_2018, r=alexcrichton
2018 edition `?` Kleene operator

This is my first attempt at implementing the migration lint + 2018 behavior as discussed in #48075

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing f498e4e to master...

@bors bors merged commit 10ee0f6 into rust-lang:master Jul 24, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2018

📣 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 Jul 24, 2018

📣 Toolstate changed by rust-lang/rust#51587!
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).
@kennytm

This comment has been minimized.

Copy link
Member

commented Jul 24, 2018

[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 |             $(one fn $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 Jul 26, 2018

Update rustc-ap-syntax to 209.0.0.
This should fix the breakage caused by rust-lang/rust#51587.

@mark-i-m mark-i-m deleted the mark-i-m:at_most_once_rep_2018 branch Nov 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.