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

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

Merged
merged 5 commits into from
Oct 14, 2020

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Sep 24, 2020

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
Collaborator

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
Member Author

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

@bors try

@bors
Copy link
Contributor

bors commented Sep 26, 2020

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

@bors
Copy link
Contributor

bors commented Sep 26, 2020

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

@Aaron1011
Copy link
Member 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.

compiler/rustc_ast_pretty/src/pprust/mod.rs Show resolved Hide resolved
compiler/rustc_parse/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/lib.rs Outdated Show resolved Hide resolved
@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
Member Author

@petrochenkov: I've addressed your comments

@Aaron1011
Copy link
Member 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
Copy link
Contributor

petrochenkov commented Sep 26, 2020

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
Member Author

@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
Member Author

Aaron1011 commented Oct 13, 2020

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
Contributor

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
Member Author

@bors rollup=never

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

@bors
Copy link
Contributor

bors commented Oct 14, 2020

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

@bors
Copy link
Contributor

bors commented Oct 14, 2020

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

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.

Pretty-printing adds additional parentheses, breaking the proc-macro reparse check
8 participants