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.

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

Pretty-print parenthesis around binary in postfix match #124269

Merged
merged 1 commit into from
Apr 29, 2024
Merged

Pretty-print parenthesis around binary in postfix match #124269

merged 1 commit into from
Apr 29, 2024

Conversation

scrabsha
Copy link
Contributor

Fixes #124206.

@rustbot
Copy link
Collaborator

rustbot commented Apr 22, 2024

r? @michaelwoerister

rustbot has assigned @michaelwoerister.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 22, 2024
@RossSmyth
Copy link
Contributor

Could you please either give the test name something descriptive about what the test tests (idk pfix-match-paren or something) or make a new directory for postfix match within the pretty directory. Adding tests with issue numbers, while common, makes maintenance frustrating.

@scrabsha scrabsha force-pushed the sasha/fix-124206 branch 2 times, most recently from 027d6f0 to 6d6bc93 Compare April 22, 2024 18:29
@scrabsha
Copy link
Contributor Author

Could you please either give the test name something descriptive about what the test tests (idk pfix-match-paren or something) or make a new directory for postfix match within the pretty directory. Adding tests with issue numbers, while common, makes maintenance frustrating.

I mkdired tests/pretty/postfix-match and moved every postfix-match-related test in it. Thanks for the suggestion!

scrabsha
scrabsha commented Apr 25, 2024
View reviewed changes
tests/pretty/postfix-match/precedence.pp
Comment on lines +16 to +27
(
{ 1 } + 1).match {
_ => {}
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks incorrect. Is it an issue in my code or in the pretty printer?

@dtolnay dtolnay assigned dtolnay and unassigned michaelwoerister Apr 28, 2024
dtolnay
dtolnay requested changes Apr 28, 2024
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to cause double parens to be put around cases like repro!(1 + Struct {}). ((1 + Struct {})).match { _ => {} }

In general print_expr_as_cond is not a correct thing to be using here. That is for syntactic positions where a { is ambiguous between being a part of the matched expression / if condition / while condition, vs being the body for the match/if/while. It isn't applicable to postfix match.

Postfix match needs to be printed like very other expression kind containing ., such as field accesses and method call expressions and postfix await.

@dtolnay dtolnay 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 Apr 28, 2024
@scrabsha
Copy link
Contributor Author

@dtolnay I modified my changes according to your comment and added your example in the test suite. Thanks!

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot 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 Apr 29, 2024
@scrabsha scrabsha requested a review from dtolnay April 29, 2024 09:39
dtolnay
dtolnay approved these changes Apr 29, 2024
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Apr 29, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 29, 2024

📌 Commit c8ff8a4 has been approved by dtolnay

It is now in the queue for this repository.

@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 Apr 29, 2024
@jieyouxu jieyouxu mentioned this pull request Apr 29, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2024
@bors
Auto merge of rust-lang#124527 - jieyouxu:rollup-eslzncy, r=jieyouxu
a8a1d3a 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#124269 (Pretty-print parenthesis around binary in postfix match)
 - rust-lang#124415 (Use probes more aggressively in new solver)
 - rust-lang#124475 (Remove direct dependencies on lazy_static, once_cell and byteorder)
 - rust-lang#124484 (Fix rust-lang#124478 - offset_of! returns a temporary)
 - rust-lang#124504 (Mark unions non-const-propagatable in `KnownPanicsLint` without calling layout)
 - rust-lang#124508 (coverage: Avoid hard-coded values when visiting logical ops)
 - rust-lang#124522 ([Refactor] Rename `Lint` and `LintGroup`'s `is_loaded` to `is_externally_loaded` )

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f1c53da into rust-lang:master Apr 29, 2024
10 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Apr 29, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2024
@rust-timer
Unrolled build for rust-lang#124269
e25491c 
Rollup merge of rust-lang#124269 - scrabsha:sasha/fix-124206, r=dtolnay

Pretty-print parenthesis around binary in postfix match

Fixes rust-lang#124206.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.80.0
Development

Successfully merging this pull request may close these issues.

AST pretty-printer produces invalid syntax for postfix match
7 participants
@scrabsha @rustbot @RossSmyth @dtolnay @bors @compiler-errors @michaelwoerister