expand: Stop using nonterminals for passing tokens to attribute and derive macros#73345
expand: Stop using nonterminals for passing tokens to attribute and derive macros#73345bors merged 3 commits intorust-lang:masterfrom
Conversation
|
@bors try |
|
⌛ Trying commit afc5180669a14fa5d65f86e5f65f96976e927bd5 with merge c8bdaa84ec1b37f18f30375e196a65b7602f3790... |
|
☀️ Try build successful - checks-azure |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
|
All randomly selected regressions that I've analyzed are caused by some popular proc macro crate using an assert checking that It's very easy to fix, but I still need to triage all the regressions to find out other possible root causes. |
|
Summary of errors: From those error at least this subset is caused by these asserts in 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. |
|
I’ve reproduced this error by running this in an older commit the I can publish a new version of 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 panickedI 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), |
|
Oh but if I add the trailing comma to the original dummy enum then it’s preserved. I’ve submitted servo/rust-cssparser#274 |
|
@SimonSapin |
|
Blocked on merging servo/rust-cssparser#274 (UPD: Done) and publishing a bugfix version of The asserts checking for whitespace can be fixed up in the compiler, but the assert checking for trailing comma unfortunately can not. |
Make procedural-masquerade resilient to macro input pretty-printing changes CC rust-lang/rust#73345 (comment)
|
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. |
|
Should be ready now. |
|
Ideally @petrochenkov Can you open an issue to track the eventual removal of |
|
@bors r=Aaron1011 |
|
📌 Commit eb4ba55 has been approved by |
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
ast_pretty: Pass some token streams and trees by reference Salvaged from an intermediate version of rust-lang#73345.
Make one more step towards fully token-based expansion and fix issues described in #72545 (comment).
Now
struct S;is passed tofoo!(struct S;)and#[foo] struct S;in the same way - as a token streamstruct S ;, rather than a single non-terminal tokenNtItemwhich 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
$crateis printed in the attribute input.Inside
NtItemwas printed ascrateorthat_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$cratewhich cannot be reparsed.So, I think we should do this regardless, but we need to run crater first.
r? @Aaron1011