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

proc_macro: Stop flattening groups with dummy spans #73102

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jun 7, 2020

Reduce the scope of the hack described in #72545 (comment).

We still pass AST fragments to attribute and derive macros as single nonterminal tokens rather than as tokens streams, but now use a precise flag instead of the span-based heuristic that could do lead to incorrect behavior in unrelated cases.

#73345 attempts to fully resolve this issue, but there are some compatibility issues to be addressed.

@petrochenkov
Copy link
Contributor Author

@bors try
cc @Aaron1011

@bors
Copy link
Contributor

bors commented Jun 7, 2020

⌛ Trying commit d754b6fccd23147e425772d925152c7ef19850be with merge 71b81291baafb8a58836272757b28817fe1d8190...

@bors
Copy link
Contributor

bors commented Jun 7, 2020

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

@petrochenkov
Copy link
Contributor Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-73102 created and queued.
🤖 Automatically detected try build 71b81291baafb8a58836272757b28817fe1d8190
🔍 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 craterbot added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jun 8, 2020
@craterbot
Copy link
Collaborator

🚧 Experiment pr-73102 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-73102 is completed!
📊 3 regressed and 0 fixed (107905 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 14, 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 14, 2020
@petrochenkov
Copy link
Contributor Author

Interesting and surprising.
Assuming the implementation is correct, this means syn and other utilities are fixed and all other regressions in #73084 are caused by hygiene issues rather than grouping?
cc @Aaron1011

@petrochenkov
Copy link
Contributor Author

Closing in favor of #73345 which is simpler and more principled.

@petrochenkov petrochenkov reopened this Jun 25, 2020
@petrochenkov petrochenkov changed the title [experiment] proc_macro: Stop flattening groups with dummy spans proc_macro: Stop flattening groups with dummy spans Jun 25, 2020
@petrochenkov
Copy link
Contributor Author

Rebased and added some comments.
r? @Aaron1011

@Aaron1011
Copy link
Member

r=me with the nits fixed

@petrochenkov
Copy link
Contributor Author

@bors r=Aaron1011

@bors
Copy link
Contributor

bors commented Jun 25, 2020

📌 Commit 77b0ed7 has been approved by Aaron1011

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 25, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 26, 2020
proc_macro: Stop flattening groups with dummy spans

Reduce the scope of the hack described in rust-lang#72545 (comment).

We still pass AST fragments to attribute and derive macros as single nonterminal tokens rather than as tokens streams, but now use a precise flag instead of the span-based heuristic that could do lead to incorrect behavior in unrelated cases.

rust-lang#73345 attempts to fully resolve this issue, but there are some compatibility issues to be addressed.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2020
…arth

Rollup of 13 pull requests

Successful merges:

 - rust-lang#72620 (Omit DW_AT_linkage_name when it is the same as DW_AT_name)
 - rust-lang#72967 (Don't move cursor in search box when using arrows to navigate results)
 - rust-lang#73102 (proc_macro: Stop flattening groups with dummy spans)
 - rust-lang#73297 (Support configurable deny-warnings for all in-tree crates.)
 - rust-lang#73507 (Cleanup MinGW LLVM linkage workaround)
 - rust-lang#73588 (Fix handling of reserved registers for ARM inline asm)
 - rust-lang#73597 (Record span of `const` kw in GenericParamKind)
 - rust-lang#73629 (Make AssocOp Copy)
 - rust-lang#73681 (Update Chalk to 0.14)
 - rust-lang#73707 (Fix links in `SliceIndex` documentation)
 - rust-lang#73719 (emitter: column width defaults to 140)
 - rust-lang#73729 (disable collectionbenches for android)
 - rust-lang#73748 (Add code block to code in documentation of `List::rebase_onto`)

Failed merges:

r? @ghost
@bors bors merged commit 2aee608 into rust-lang:master Jun 26, 2020
@nnethercote
Copy link
Contributor

I suspect this may have caused a perf regression. I'm going to try to do a perf CI run after the landing. Not sure if it will work...

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@mati865
Copy link
Contributor

mati865 commented Jun 30, 2020

Bors doesn't listen on closed issues/PRs.

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 1, 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
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
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
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
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
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 14, 2020
…arth

Rollup of 13 pull requests

Successful merges:

 - rust-lang#72620 (Omit DW_AT_linkage_name when it is the same as DW_AT_name)
 - rust-lang#72967 (Don't move cursor in search box when using arrows to navigate results)
 - rust-lang#73102 (proc_macro: Stop flattening groups with dummy spans)
 - rust-lang#73297 (Support configurable deny-warnings for all in-tree crates.)
 - rust-lang#73507 (Cleanup MinGW LLVM linkage workaround)
 - rust-lang#73588 (Fix handling of reserved registers for ARM inline asm)
 - rust-lang#73597 (Record span of `const` kw in GenericParamKind)
 - rust-lang#73629 (Make AssocOp Copy)
 - rust-lang#73681 (Update Chalk to 0.14)
 - rust-lang#73707 (Fix links in `SliceIndex` documentation)
 - rust-lang#73719 (emitter: column width defaults to 140)
 - rust-lang#73729 (disable collectionbenches for android)
 - rust-lang#73748 (Add code block to code in documentation of `List::rebase_onto`)

Failed merges:

r? @ghost
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
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.

8 participants