Skip to content

Refactor AST pretty-printing to allow skipping insertion of extra parens #77135

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Oct 14, 2020
Merged

Refactor AST pretty-printing to allow skipping insertion of extra parens #77135

merged 5 commits into from
Oct 14, 2020

Conversation

Aaron1011
Copy link
Contributor

@Aaron1011 Aaron1011 commented Sep 24, 2020 •
edited
Loading

Fixes #75734
Makes progress towards #43081
Unblocks PR #76130

When pretty-printing an AST node, we may insert additional parenthesis
to ensure that precedence is properly preserved in code we output.
However, the proc macro implementation relies on comparing a
pretty-printed AST node to the captured TokenStream. Inserting extra
parenthesis changes the structure of the reparsed TokenStream, making
the comparison fail.

This PR refactors the AST pretty-printing code to allow skipping the
insertion of additional parenthesis. Several freestanding methods are
moved to trait methods on PrintState, which keep track of an internal
insert_extra_parens flag. This flag is normally true, but we expose
a public method which allows pretty-printing a nonterminal with
insert_extra_parens = false.

To avoid changing the public interface of rustc_ast_pretty, the
freestanding _to_string methods are changed to delegate to a
newly-crated State. The main pretty-printing code is moved to a new
state module to ensure that it does not accidentally call any of these
public helper functions (instead, the internal functions with the same
name should be used).

@rust-highfive
Copy link
Contributor

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

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

r? @petrochenkov

@Aaron1011
Copy link
Contributor Author

Let's do a Crater run to be on the safe side.

@bors try

@bors
Copy link
Collaborator

bors commented Sep 26, 2020

⌛ Trying commit 88ef6bd26cd4d9d864295faf26ddd890c51f39f1 with merge 89a6ec545e5f85c102ce8eae663f1abae5e6e8b8...

@bors
Copy link
Collaborator

bors commented Sep 26, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 89a6ec545e5f85c102ce8eae663f1abae5e6e8b8 (89a6ec545e5f85c102ce8eae663f1abae5e6e8b8)

@Aaron1011
Copy link
Contributor Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-77135 created and queued.
🤖 Automatically detected try build 89a6ec545e5f85c102ce8eae663f1abae5e6e8b8
🔍 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 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 Sep 26, 2020
@petrochenkov petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 26, 2020
@petrochenkov
Copy link
Contributor

@Aaron1011
Could you avoid moving code (e.g. to pprust/state.rs) and changing code in the same commit? It's hard to review this way. The move can be split into a separate commit.

petrochenkov
petrochenkov reviewed Sep 26, 2020
View reviewed changes
@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 Sep 26, 2020
@Aaron1011
Copy link
Contributor Author

@petrochenkov: I've addressed your comments

@Aaron1011
Copy link
Contributor Author

@craterbot abort

@craterbot
Copy link
Collaborator

🗑️ Experiment pr-77135 deleted!

ℹ️ 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-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Sep 26, 2020
petrochenkov
petrochenkov reviewed Sep 26, 2020
View reviewed changes
@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 26, 2020 •
edited
Loading

Could you avoid moving code

Moving foo_to_string to inherent methods also qualifies as a code move here.
Moving just the file renaming into a separate commit doesn't really change the diff (and its readability) compared to the old state.

@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 Sep 26, 2020
@Aaron1011
Copy link
Contributor Author

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-77135/retry-regressed-list.txt p=1

@craterbot
Copy link
Collaborator

👌 Experiment pr-77135-1 created and queued.
🤖 Automatically detected try build ee7018e6202e3da222c3f222a29cc4a5abe4a19d
🔍 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 S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 11, 2020
@craterbot
Copy link
Collaborator

🚧 Experiment pr-77135-1 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-77135-1 is completed!
📊 7 regressed and 2 fixed (417 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 Oct 13, 2020
@Aaron1011
Copy link
Contributor Author

Aaron1011 commented Oct 13, 2020 •
edited
Loading

There are a few actix-web regressions that were missed by the check, due to a weird directory structure or being in another fork (e.g. karyx).

@petrochenkov I could extend the check, but it looks like we've covered nearly all of the regressions. This should should be ready to merge.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 13, 2020

📌 Commit 9a6ea38 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 Oct 13, 2020
@Aaron1011
Copy link
Contributor Author

@bors rollup=never

This will cause a (small) amount of breakage, so let's make it easy to bisect.

@bors
Copy link
Collaborator

bors commented Oct 14, 2020

⌛ Testing commit 9a6ea38 with merge 4ba5068...

@bors
Copy link
Collaborator

bors commented Oct 14, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing 4ba5068 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 14, 2020
@bors bors mentioned this pull request Oct 14, 2020
Closed
@bors bors merged commit 4ba5068 into rust-lang:master Oct 14, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 14, 2020
This was referenced Oct 14, 2020
Closed
Merged
Merged
Merged
Closed
Merged
@jebrosen jebrosen mentioned this pull request Oct 30, 2020
Closed
@Aaron1011 Aaron1011 mentioned this pull request Jan 19, 2021
Closed
This was referenced May 6, 2021
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

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
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.

Pretty-printing adds additional parentheses, breaking the proc-macro reparse check
8 participants
@Aaron1011 @rust-highfive @petrochenkov @bors @craterbot @taiki-e @matthewjasper @rustbot