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

rustc_ast: Visit tokens stored in AST nodes in mutable visitor #78712

Merged
merged 3 commits into from
Nov 8, 2020

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Nov 3, 2020

After #77271 token visiting is enabled only for one visitor in rustc_expand\src\mbe\transcribe.rs which applies hygiene marks to tokens produced by declarative macros (macro_rules or macro), so this change doesn't affect anything else.

When a macro has some interpolated token from an outer macro in its output

macro inner() {
    $interpolated
}

we can use the usual interpretation of interpolated tokens in token-based model - a None-delimited group - to write this macro in an equivalent form

macro inner() {
    ⟪ a b c d ⟫
}

When we are expanding the macro inner we need to apply hygiene marks to all tokens produced by it, including the tokens inside the group.

Before this PR we did this by visiting the AST piece inside the interpolated token and applying marks to all spans in it.
I'm not sure this is 100% correct (ideally we should apply the marks to tokens and then re-parse the AST from tokens), but it's a very good approximation at least.
We didn't however apply the marks to actual tokens stored in the nonterminal, so if we used the nonterminal as a token rather than as an AST piece (e.g. passed it to a proc macro), then we got hygiene bugs.
This PR applies the marks to tokens in addition to the AST pieces thus fixing the issue.

r? @Aaron1011

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2020
@Aaron1011
Copy link
Member

I'm not sure this is 100% correct (ideally we should apply the marks to tokens and then re-parse the AST from tokens), but it's a very good approximation at least.

That would just affect the computed spans, right? E.g. we might fail to join two spans that now have different SyntaxContexts?

@Aaron1011
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 5, 2020

⌛ Trying commit fcfe3063cbacb070da03eeda8ed88c7bfd24f9de with merge bd450b36446fdb9d6276544cc56174298a890781...

@bors
Copy link
Contributor

bors commented Nov 5, 2020

☀️ Try build successful - checks-actions
Build commit: bd450b36446fdb9d6276544cc56174298a890781 (bd450b36446fdb9d6276544cc56174298a890781)

@rust-timer
Copy link
Collaborator

Queued bd450b36446fdb9d6276544cc56174298a890781 with parent 0fb0025, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (bd450b36446fdb9d6276544cc56174298a890781): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@Aaron1011 Aaron1011 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 Nov 5, 2020
@petrochenkov
Copy link
Contributor Author

@Aaron1011

That would just affect the computed spans, right? E.g. we might fail to join two spans that now have different SyntaxContexts?

Yes, perhaps some spans will be different.
Perhaps it's possible to make an example where edition-specific parsing would be different and produce an error (but that seems to be because our edition hygiene works incorrectly with nested macro definitions - the outermost ExpnData is used instead of the innermost one).

@petrochenkov
Copy link
Contributor Author

Updated.

I've also added the mut visitor micro-optimization commit from #78736 which seems relevant and non-controversial.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 5, 2020
@Aaron1011
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 7, 2020

📌 Commit 8def2fc 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2020
@bors
Copy link
Contributor

bors commented Nov 8, 2020

⌛ Testing commit 8def2fc with merge 1773f60...

@bors
Copy link
Contributor

bors commented Nov 8, 2020

☀️ Test successful - checks-actions
Approved by: Aaron1011
Pushing 1773f60 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 8, 2020
@bors bors merged commit 1773f60 into rust-lang:master Nov 8, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

6 participants