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

expand: Stop using nonterminals for passing tokens to attribute and derive macros #73345

Merged
merged 3 commits into from
Jul 2, 2020

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jun 14, 2020

Make one more step towards fully token-based expansion and fix issues described in #72545 (comment).

Now struct S; is passed to foo!(struct S;) and #[foo] struct S; in the same way - as a token stream struct S ;, rather than a single non-terminal token NtItem which is then broken into parts later.

The cost is making pretty-printing of token streams less pretty.
Some of the pretty-printing regressions will be recovered by keeping jointness with each token, which we will need to do anyway.

Unfortunately, this is not exactly the same thing as #73102.
One more observable effect is how $crate is printed in the attribute input.
Inside NtItem was printed as crate or that_crate, now as a part of a token stream it's printed as $crate (there are good reasons for these differences, see #62393 and related PRs).
This may break old proc macros (custom derives) written before the main portion of the proc macro API (macros 1.2) was stabilized, those macros did input.to_string() and reparsed the result, now that result can contain $crate which cannot be reparsed.

So, I think we should do this regardless, but we need to run crater first.
r? @Aaron1011

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 14, 2020
@petrochenkov
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Jun 14, 2020

⌛ Trying commit afc5180669a14fa5d65f86e5f65f96976e927bd5 with merge c8bdaa84ec1b37f18f30375e196a65b7602f3790...

@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2020
@bors
Copy link
Contributor

bors commented Jun 14, 2020

☀️ Try build successful - checks-azure
Build commit: c8bdaa84ec1b37f18f30375e196a65b7602f3790 (c8bdaa84ec1b37f18f30375e196a65b7602f3790)

@petrochenkov
Copy link
Contributor Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-73345 created and queued.
🤖 Automatically detected try build c8bdaa84ec1b37f18f30375e196a65b7602f3790
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🚧 Experiment pr-73345 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-73345 is completed!
📊 451 regressed and 1 fixed (108855 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jun 17, 2020
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 17, 2020
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jun 18, 2020

All randomly selected regressions that I've analyzed are caused by some popular proc macro crate using an assert checking that #[allow(unused)] is pretty-printed as is rather than as # [allow(unused)] like in this PR.

It's very easy to fix, but I still need to triage all the regressions to find out other possible root causes.

@petrochenkov
Copy link
Contributor Author

Summary of errors:

448 proc-macro derive panicked
208 unexpected panic
197 cannot find value `MAP` in this scope
197 cannot find value `MAX_LENGTH` in this scope
 11 cannot find value `BYTES` in this scope
  3 cannot find macro `__cpp_closure_impl` in this scope
  1 failed to run custom build command for `rusty_v8 v0.4.2`
  1 expected identifier, found `{`
  1 `main` function not found in crate `demo`
  1 Error parsing the struct: Expected either 'struct' or 'union' keyword.
  1 macros that expand to items must be delimited with braces or followed by a semicolon
  1 cannot find type `DontDropOpt` in this scope
  1 cannot find function, tuple struct or tuple variant `DontDropOpt` in this scope

From those error at least this subset

448 proc-macro derive panicked
208 unexpected panic
197 cannot find value `MAP` in this scope
197 cannot find value `MAX_LENGTH` in this scope
 11 cannot find value `BYTES` in this scope

is caused by these asserts in procedural-masquerade (cc @SimonSapin)

https://github.com/servo/rust-cssparser/blob/master/procedural-masquerade/lib.rs#L201-L232

I'll try to hard-code all this stuff in the compiler to avoid regressions until the proper solution with keeping token jointness is implemented.

@SimonSapin
Copy link
Contributor

I’ve reproduced this error by running this in an older commit the rust-cssparser repository (the current version of cssparser doesn’t use procedural-masquerade anymore):

rustup-toolchain-install-master c8bdaa84ec1b37f18f30375e196a65b7602f3790
cargo +c8bdaa84ec1b37f18f30375e196a65b7602f3790 test

I can publish a new version of procedural-masquerade with the diff below in order to make it resilient to whitespace changes, but parsing still fails in c8bdaa84ec1b37f18f30375e196a65b7602f3790 because it doesn’t emit the trailing comma after the last enum variant that older compilers did:

   Compiling cssparser v0.26.0 (/home/simon/projects/rust-cssparser)
thread '<unnamed>' panicked at 'expected suffix "," not found in "# [allow(unused)] enum ProceduralMasqueradeDummyType\n{\n    Input =\n    (0, stringify !\n     (\"deg\" => v, \"grad\" => v * 360. / 400., \"rad\" => v * 360. / (2. * PI),\n      \"turn\" => v * 360., _ => return\n      Err(location .\n          new_unexpected_token_error(Token :: Ident(unit . clone()))),)) . 0\n}"', procedural-masquerade/lib.rs:233:9
[]
error: proc-macro derive panicked

I can hack it some more to support both with and without the comma, but it gets annoying for a crate that at this point only exists to support old compilers between Rust 1.15 and 1.29. (Those with stable proc macro derive but not other proc macros.)


diff --git procedural-masquerade/lib.rs procedural-masquerade/lib.rs
index 2736ccf..c1a99a1 100644
--- procedural-masquerade/lib.rs
+++ procedural-masquerade/lib.rs
@@ -199,14 +199,23 @@ pub fn _extract_input(derive_input: &str) -> &str {
     let mut input = derive_input;
 
     for expected in &[
-        "#[allow(unused)]",
+        "#",
+        "[",
+        "allow",
+        "(",
+        "unused",
+        ")",
+        "]",
         "enum",
         "ProceduralMasqueradeDummyType",
         "{",
         "Input",
         "=",
-        "(0,",
-        "stringify!",
+        "(",
+        "0",
+        ",",
+        "stringify",
+        "!",
         "(",
     ] {
         input = input.trim_start();
@@ -219,7 +228,7 @@ pub fn _extract_input(derive_input: &str) -> &str {
         input = &input[expected.len()..];
     }
 
-    for expected in [")", ").0,", "}"].iter().rev() {
+    for expected in [")", ")", ".", "0", ",", "}"].iter().rev() {
         input = input.trim_end();
         assert!(
             input.ends_with(expected),

@SimonSapin
Copy link
Contributor

Oh but if I add the trailing comma to the original dummy enum then it’s preserved. I’ve submitted servo/rust-cssparser#274

@petrochenkov
Copy link
Contributor Author

@SimonSapin
Thanks!

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jun 20, 2020

Blocked on merging servo/rust-cssparser#274 (UPD: Done) and publishing a bugfix version of procedural-masquerade.

The asserts checking for whitespace can be fixed up in the compiler, but the assert checking for trailing comma unfortunately can not.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 20, 2020
bors-servo added a commit to servo/rust-cssparser that referenced this pull request Jun 20, 2020
Make procedural-masquerade resilient to macro input pretty-printing changes

CC rust-lang/rust#73345 (comment)
@SimonSapin
Copy link
Contributor

https://crates.io/crates/procedural-masquerade/0.1.7 is up and should be picked up as a recursive dependency of some of those crates that failed the last Crater run.

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020
expand: Stop using nonterminals for passing tokens to attribute and derive macros

Make one more step towards fully token-based expansion and fix issues described in rust-lang#72545 (comment).

Now `struct S;` is passed to `foo!(struct S;)` and `#[foo] struct S;` in the same way - as a token stream `struct S ;`, rather than a single non-terminal token `NtItem` which is then broken into parts later.

The cost is making pretty-printing of token streams less pretty.
Some of the pretty-printing regressions will be recovered by keeping jointness with each token, which we will need to do anyway.

Unfortunately, this is not exactly the same thing as rust-lang#73102.
One more observable effect is how `$crate` is printed in the attribute input.
Inside `NtItem` was printed as `crate` or `that_crate`, now as a part of a token stream it's printed as `$crate` (there are good reasons for these differences, see rust-lang#62393 and related PRs).
This may break old proc macros (custom derives) written before the main portion of the proc macro API (macros 1.2) was stabilized, those macros did `input.to_string()` and reparsed the result, now that result can contain `$crate` which cannot be reparsed.

So, I think we should do this regardless, but we need to run crater first.
r? @Aaron1011
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020
ast_pretty: Pass some token streams and trees by reference

Salvaged from an intermediate version of rust-lang#73345.
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020
ast_pretty: Pass some token streams and trees by reference

Salvaged from an intermediate version of rust-lang#73345.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 2, 2020
…arth

Rollup of 16 pull requests

Successful merges:

 - rust-lang#72569 (Remove legacy InnoSetup GUI installer)
 - rust-lang#73306 (Don't implement Fn* traits for #[target_feature] functions)
 - rust-lang#73345 (expand: Stop using nonterminals for passing tokens to attribute and derive macros)
 - rust-lang#73449 (Provide more information on duplicate lang item error.)
 - rust-lang#73569 (Handle `macro_rules!` tokens consistently across crates)
 - rust-lang#73803 (Recover extra trailing angle brackets in struct definition)
 - rust-lang#73839 (Split and expand nonstandard-style lints unicode unit test.)
 - rust-lang#73841 (Remove defunct `-Z print-region-graph`)
 - rust-lang#73848 (Fix markdown rendering in librustc_lexer docs)
 - rust-lang#73865 (Fix Zulip topic format)
 - rust-lang#73892 (Clean up E0712 explanation)
 - rust-lang#73898 (remove duplicate test for rust-lang#61935)
 - rust-lang#73906 (Add missing backtick in `ty_error_with_message`)
 - rust-lang#73909 (`#[deny(unsafe_op_in_unsafe_fn)]` in libstd/fs.rs)
 - rust-lang#73910 (Rewrite a few manual index loops with while-let)
 - rust-lang#73929 (Fix comment typo)

Failed merges:

r? @ghost
@bors bors merged commit 8ed5c0d into rust-lang:master Jul 2, 2020
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
nnethercote added a commit to nnethercote/rust that referenced this pull request May 27, 2024
We still check for the `rental`/`allsorts-rental` crates. But now if
they are detected we just emit a fatal error, instead of emitting a
warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added
in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative
behaviour is interfering with rust-lang#125174 and follow-on work.
nnethercote added a commit to nnethercote/rust that referenced this pull request May 27, 2024
We still check for the `rental`/`allsorts-rental` crates. But now if
they are detected we just emit a fatal error, instead of emitting a
warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added
in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative
behaviour is interfering with rust-lang#125174 and follow-on work.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 27, 2024
Convert `proc_macro_back_compat` lint to an unconditional error.

We still check for the `rental`/`allsorts-rental` crates. But now if they are detected we just emit a fatal error, instead of emitting a warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative behaviour is interfering with rust-lang#125174 and follow-on work.

r? `@estebank`
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 4, 2024
…stebank

Convert `proc_macro_back_compat` lint to an unconditional error.

We still check for the `rental`/`allsorts-rental` crates. But now if they are detected we just emit a fatal error, instead of emitting a warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative behaviour is interfering with rust-lang#125174 and follow-on work.

r? `@estebank`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 4, 2024
…stebank

Convert `proc_macro_back_compat` lint to an unconditional error.

We still check for the `rental`/`allsorts-rental` crates. But now if they are detected we just emit a fatal error, instead of emitting a warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative behaviour is interfering with rust-lang#125174 and follow-on work.

r? ``@estebank``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
Rollup merge of rust-lang#125596 - nnethercote:rental-hard-error, r=estebank

Convert `proc_macro_back_compat` lint to an unconditional error.

We still check for the `rental`/`allsorts-rental` crates. But now if they are detected we just emit a fatal error, instead of emitting a warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative behaviour is interfering with rust-lang#125174 and follow-on work.

r? ``@estebank``
lcnr pushed a commit to lcnr/rust that referenced this pull request Jun 12, 2024
We still check for the `rental`/`allsorts-rental` crates. But now if
they are detected we just emit a fatal error, instead of emitting a
warning and providing alternative behaviour.

The original "hack" implementing alternative behaviour was added
in rust-lang#73345.

The lint was added in rust-lang#83127.

The tracking issue is rust-lang#83125.

The direct motivation for the change is that providing the alternative
behaviour is interfering with rust-lang#125174 and follow-on work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants