Skip to content

Refactor token collection to capture trailing token immediately #81017

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jan 23, 2021

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Jan 14, 2021

Split out from #80689 - when we start capturing more information about attribute targets, we'll need to know in advance if we're capturing a trailing token or not.

r? @ghost

@Aaron1011 Aaron1011 changed the title Set tokens on AST node in collect_tokens Refactor token collection to capture trailing token immediately Jan 14, 2021
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Collaborator

bors commented Jan 14, 2021

⌛ Trying commit c0d77f1c76c8d324c77755cd99908860d4c3eec0 with merge 125fb95a2bc8697595e30ef058c5cc82b2222284...

@bors
Copy link
Collaborator

bors commented Jan 14, 2021

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

@rust-timer
Copy link
Collaborator

Queued 125fb95a2bc8697595e30ef058c5cc82b2222284 with parent d03fe84, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 14, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (125fb95a2bc8697595e30ef058c5cc82b2222284): 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 label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 14, 2021
@Aaron1011 Aaron1011 force-pushed the collect-trailing-token branch from c0d77f1 to 73a976f Compare January 14, 2021 22:51
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Collaborator

bors commented Jan 14, 2021

⌛ Trying commit 73a976ffd206a284f6485baf7a37545ef6cc6343 with merge df2eeabc93790b4803c40f482839a38c56301e3b...

@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 14, 2021

Something like this was my first thought when you introduced the semicolon token collection - "you just need to refactor statement parsing carefully, so the parsed statements already contain semicolons".
Statement parsing is kind of a mess though, and is plagued by ad hoc diagnostic code as well, so it was kind of understandable why adding the add_trailing_semi hack could be less invasive.

The end goal for statement parsing is to use it basically everywhere - for parsing statements not only in blocks, but also in all contexts where items can be written (so e.g. let x = 10; is parsed and gracefully rejected even if written at module level).
Centril started this work in #69366 and related PRs, but didn't finish it unfortunately.
I think we can change (or even slightly break) the behavior of the (almost unused) stmt nonterminal to achieve this goal, if it becomes necessary.

@bors
Copy link
Collaborator

bors commented Jan 14, 2021

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

@rust-timer
Copy link
Collaborator

Queued df2eeabc93790b4803c40f482839a38c56301e3b with parent 4275ef6, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 14, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (df2eeabc93790b4803c40f482839a38c56301e3b): 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 label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 15, 2021
@Aaron1011 Aaron1011 force-pushed the collect-trailing-token branch from 73a976f to b8b7901 Compare January 15, 2021 19:13
@rust-log-analyzer

This comment has been minimized.

@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Collaborator

bors commented Jan 15, 2021

⌛ Trying commit f5acaa1f663e5f0979aad348b2bf1e081c1ef520 with merge 48d68b3b8d788525a0310668dfae564f7edf7e09...

@bors
Copy link
Collaborator

bors commented Jan 15, 2021

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

@Aaron1011
Copy link
Member Author

@petrochenkov: The force_capture changes are needed to avoid exposing the underlying issue in #81007 further. I'll split out the changes first, and then rebase this PR on top of that.

@Aaron1011
Copy link
Member Author

Blocked on #81177.

@Aaron1011 Aaron1011 added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jan 18, 2021
@Aaron1011 Aaron1011 force-pushed the collect-trailing-token branch from 5522328 to c901b42 Compare January 22, 2021 01:48
@Aaron1011 Aaron1011 force-pushed the collect-trailing-token branch from c901b42 to ccfc292 Compare January 22, 2021 05:50
@Aaron1011 Aaron1011 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 22, 2021
@Aaron1011
Copy link
Member Author

@petrochenkov: I've rebased this PR against master. The content is almost the same as before - a small adjustment to maybe_collect_tokens was needed.

@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Collaborator

bors commented Jan 22, 2021

⌛ Trying commit ccfc292 with merge 134fdeae99435ada2f12fd43c12f277914f9a9f7...

@bors
Copy link
Collaborator

bors commented Jan 22, 2021

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

@rust-timer
Copy link
Collaborator

Queued 134fdeae99435ada2f12fd43c12f277914f9a9f7 with parent dc1eee2, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 22, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (134fdeae99435ada2f12fd43c12f277914f9a9f7): 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 label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 22, 2021
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 22, 2021

📌 Commit ccfc292 has been approved by petrochenkov

@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 Jan 22, 2021
@bors
Copy link
Collaborator

bors commented Jan 23, 2021

⌛ Testing commit ccfc292 with merge 693ed05...

@bors
Copy link
Collaborator

bors commented Jan 23, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 693ed05 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 23, 2021
@bors bors merged commit 693ed05 into rust-lang:master Jan 23, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 23, 2021
# for free to join this conversation on GitHub. Already have an account? # 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