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

Precedence improvements: closures and jumps #133782

Merged
merged 5 commits into from
Dec 21, 2024
Merged

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Dec 3, 2024

This PR fixes some cases where rustc's pretty printers would redundantly parenthesize expressions that didn't need it.

BeforeAfter
return (|x: i32| x)return |x: i32| x
(|| -> &mut () { std::process::abort() }).clone()|| -> &mut () { std::process::abort() }.clone()
(continue) + 1continue + 1

Tested by echo "fn main() { let _ = $AFTER; }" | rustc -Zunpretty=expanded /dev/stdin.

The pretty-printer aims to render the syntax tree as it actually exists in rustc, as faithfully as possible, in Rust syntax. It can insert parentheses where forced by Rust's grammar in order to preserve the meaning of a macro-generated syntax tree, for example in the case of a * $rhs where $rhs is b + c. But for any expression parsed from source code, without a macro involved, there should never be a reason for inserting additional parentheses not present in the original.

For closures and jumps (return, break, continue, yield, do yeet, become) the unneeded parentheses came from the precedence of some of these expressions being misidentified. In the same order as the table above:

  • Jumps and closures are supposed to have equal precedence. The Rust Reference says so, and in Syn they do. There is no Rust syntax that would require making a precedence distinction between jumps and closures. But in rustc these were previously 2 distinct levels with the closure being lower, hence the parentheses around a closure inside a jump (but not a jump inside a closure).

  • When a closure is written with an explicit return type, the grammar requires that the closure body consists of exactly one block expression, not any other arbitrary expression as usual for closures. Parsing of the closure body does not continue after the block expression. So in || { 0 }.clone() the clone is inside the closure body and applies to { 0 }, whereas in || -> _ { 0 }.clone() the clone is outside and applies to the closure as a whole.

  • Continue never needs parentheses. It was previously marked as having the lowest possible precedence but it should have been the highest, next to paths and loops and function calls, not next to jumps.

@rustbot
Copy link
Collaborator

rustbot commented Dec 3, 2024

r? @spastorino

rustbot has assigned @spastorino.
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 Dec 3, 2024
@dtolnay dtolnay added the A-pretty Area: Pretty printing (including `-Z unpretty`) label Dec 3, 2024
@spastorino
Copy link
Member

Sorry that it took very long for me to get into this. I think this is good and makes sense in general and the code looks perfect.
Not sure if lang or any other team need to have a say or something around this cc @traviscross

@dtolnay
Copy link
Member Author

dtolnay commented Dec 20, 2024

This is my 40th pretty-printer change and they have not needed a full team review. There is no stable or irreversible commitment being made. See similar parenthesization PRs in #118726 and #119105 and #119427.

@traviscross
Copy link
Contributor

This all sounds right to me. Agreed there's nothing lang needs to rule on here (but it is interesting work, so cc @rust-lang/lang).

cc @compiler-errors

@traviscross
Copy link
Contributor

@bors r=spastorino,traviscross rollup

@bors
Copy link
Contributor

bors commented Dec 20, 2024

📌 Commit fe06c5d has been approved by spastorino,traviscross

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 Dec 20, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#123604 (Abstract `ProcThreadAttributeList` into its own struct)
 - rust-lang#128780 (Add `--doctest-compilation-args` option to add compilation flags to doctest compilation)
 - rust-lang#133782 (Precedence improvements: closures and jumps)
 - rust-lang#134509 (Arbitrary self types v2: niche deshadowing test)
 - rust-lang#134524 (Arbitrary self types v2: no deshadow pre feature.)
 - rust-lang#134539 (Restrict `#[non_exaustive]` on structs with default field values)
 - rust-lang#134586 (Also lint on option of function pointer comparisons)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f3b19f5 into rust-lang:master Dec 21, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2024
Rollup merge of rust-lang#133782 - dtolnay:closuresjumps, r=spastorino,traviscross

Precedence improvements: closures and jumps

This PR fixes some cases where rustc's pretty printers would redundantly parenthesize expressions that didn't need it.

<table>
<tr><th>Before</th><th>After</th></tr>
<tr><td><code>return (|x: i32| x)</code></td><td><code>return |x: i32| x</code></td></tr>
<tr><td><code>(|| -> &mut () { std::process::abort() }).clone()</code></td><td><code>|| -> &mut () { std::process::abort() }.clone()</code></td></tr>
<tr><td><code>(continue) + 1</code></td><td><code>continue + 1</code></td></tr>
</table>

Tested by `echo "fn main() { let _ = $AFTER; }" | rustc -Zunpretty=expanded /dev/stdin`.

The pretty-printer aims to render the syntax tree as it actually exists in rustc, as faithfully as possible, in Rust syntax. It can insert parentheses where forced by Rust's grammar in order to preserve the meaning of a macro-generated syntax tree, for example in the case of `a * $rhs` where $rhs is `b + c`. But for any expression parsed from source code, without a macro involved, there should never be a reason for inserting additional parentheses not present in the original.

For closures and jumps (return, break, continue, yield, do yeet, become) the unneeded parentheses came from the precedence of some of these expressions being misidentified. In the same order as the table above:

- Jumps and closures are supposed to have equal precedence. The [Rust Reference](https://doc.rust-lang.org/1.83.0/reference/expressions.html#expression-precedence) says so, and in Syn they do. There is no Rust syntax that would require making a precedence distinction between jumps and closures. But in rustc these were previously 2 distinct levels with the closure being lower, hence the parentheses around a closure inside a jump (but not a jump inside a closure).

- When a closure is written with an explicit return type, the grammar [requires](https://doc.rust-lang.org/1.83.0/reference/expressions/closure-expr.html) that the closure body consists of exactly one block expression, not any other arbitrary expression as usual for closures. Parsing of the closure body does not continue after the block expression. So in `|| { 0 }.clone()` the clone is inside the closure body and applies to `{ 0 }`, whereas in `|| -> _ { 0 }.clone()` the clone is outside and applies to the closure as a whole.

- Continue never needs parentheses. It was previously marked as having the lowest possible precedence but it should have been the highest, next to paths and loops and function calls, not next to jumps.
@dtolnay dtolnay deleted the closuresjumps branch December 22, 2024 17:04
@dtolnay dtolnay removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pretty Area: Pretty printing (including `-Z unpretty`) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants