Do not parenthesize exterior struct lit inside match guards #118726

dtolnay · 2023-12-07T23:06:18Z

Before this PR, the AST pretty-printer injects parentheses around expressions any time parens could be needed depending on what else is in the code that surrounds that expression. But the pretty-printer did not pass around enough context to understand whether parentheses really are needed on any particular expression. As a consequence, there are false positives where unneeded parentheses are being inserted.

Example:

#![feature(if_let_guard)]

macro_rules! pp {
    ($e:expr) => {
        stringify!($e)
    };
}

fn main() {
    println!("{}", pp!(match () { () if let _ = Struct {} => {} }));
}

Before:

match () { () if let _ = (Struct {}) => {} }

After:

match () { () if let _ = Struct {} => {} }

This PR introduces a bit of state that is passed across various expression printing methods to help understand accurately whether particular situations require parentheses injected by the pretty printer, and it fixes one such false positive involving match guards as shown above.

There are other parenthesization false positive cases not fixed by this PR. I intend to address these in follow-up PRs. For example here is one: the expression { let _ = match x {} + 1; } is pretty-printed as { let _ = (match x {}) + 1; } despite there being no reason for parentheses to appear there.

rustbot · 2023-12-07T23:06:26Z

r? @compiler-errors

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

compiler/rustc_ast_pretty/src/pprust/state.rs

bors · 2023-12-08T19:13:59Z

☔ The latest upstream changes (presumably #118527) made this pull request unmergeable. Please resolve the merge conflicts.

True || needs_par_as_let_scrutinee(...) is always true.

In all four of Break, Closure, Ret, Yeet, the needs_par_as_let_scrutinee is guaranteed to return true because the .precedence().order() of those expr kinds is <= AssocOp::LAnd.precedence(). The relevant functions in rustc_ast::util::parser are: fn needs_par_as_let_scrutinee(order: i8) -> bool { order <= prec_let_scrutinee_needs_par() as i8 } fn prec_let_scrutinee_needs_par() -> usize { AssocOp::LAnd.precedence() } The .precedence().order() of Closure is PREC_CLOSURE (-40) and of Break, Ret, Yeet is PREC_JUMP (-30). The value of AssocOp::LAnd.precedence() is 6. So this commit causes no change in behavior, only potentially performance by doing a redundant call to contains_exterior_struct_lit in those four cases. This is fine because Break, Closure, Ret, Yeet should be exceedingly rare in the position of a let scrutinee.

tidy error: /git/rust/compiler/rustc_ast_pretty/src/pprust/state.rs:1165: unexplained "```ignore" doctest; try one: * make the test actually pass, by adding necessary imports and declarations, or * use "```text", if the code is not Rust code, or * use "```compile_fail,Ennnn", if the code is expected to fail at compile time, or * use "```should_panic", if the code is expected to fail at run time, or * use "```no_run", if the code should type-check but not necessary linkable/runnable, or * explain it like "```ignore (cannot-test-this-because-xxxx)", if the annotation cannot be avoided. tidy error: /git/rust/compiler/rustc_ast_pretty/src/pprust/state.rs:1176: unexplained "```ignore" doctest; try one: * make the test actually pass, by adding necessary imports and declarations, or * use "```text", if the code is not Rust code, or * use "```compile_fail,Ennnn", if the code is expected to fail at compile time, or * use "```should_panic", if the code is expected to fail at run time, or * use "```no_run", if the code should type-check but not necessary linkable/runnable, or * explain it like "```ignore (cannot-test-this-because-xxxx)", if the annotation cannot be avoided.

dtolnay · 2023-12-08T23:19:55Z

Rebased to resolve conflict with never_patterns: Parse match arms with no body #118527

compiler-errors

This is great, thanks

compiler-errors · 2023-12-11T18:13:30Z

@bors r+

bors · 2023-12-11T18:13:33Z

📌 Commit 8997215 has been approved by compiler-errors

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#118620 (resolve: Use `def_kind` query to cleanup some code) - rust-lang#118647 (dump bootstrap shims) - rust-lang#118726 (Do not parenthesize exterior struct lit inside match guards) - rust-lang#118818 (llvm-wrapper: adapt for LLVM API change) - rust-lang#118822 (Extract exhaustiveness into its own crate) - rust-lang#118826 (Edit target doc template to remove email) - rust-lang#118827 (Update table for linker-plugin-lto docs) - rust-lang#118835 (Fix again `rustc_codegen_gcc` tests) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#118726 - dtolnay:matchguardlet, r=compiler-errors Do not parenthesize exterior struct lit inside match guards Before this PR, the AST pretty-printer injects parentheses around expressions any time parens _could_ be needed depending on what else is in the code that surrounds that expression. But the pretty-printer did not pass around enough context to understand whether parentheses really _are_ needed on any particular expression. As a consequence, there are false positives where unneeded parentheses are being inserted. Example: ```rust #![feature(if_let_guard)] macro_rules! pp { ($e:expr) => { stringify!($e) }; } fn main() { println!("{}", pp!(match () { () if let _ = Struct {} => {} })); } ``` **Before:** ```console match () { () if let _ = (Struct {}) => {} } ``` **After:** ```console match () { () if let _ = Struct {} => {} } ``` This PR introduces a bit of state that is passed across various expression printing methods to help understand accurately whether particular situations require parentheses injected by the pretty printer, and it fixes one such false positive involving match guards as shown above. There are other parenthesization false positive cases not fixed by this PR. I intend to address these in follow-up PRs. For example here is one: the expression `{ let _ = match x {} + 1; }` is pretty-printed as `{ let _ = (match x {}) + 1; }` despite there being no reason for parentheses to appear there.

Fix parenthesization of subexprs containing statement boundary This PR fixes a multitude of false negatives and false positives in the AST pretty printer's parenthesis insertion related to statement boundaries — statements which terminate unexpectedly early if there aren't parentheses. Without this fix, the AST pretty printer (including both `stringify!` and `rustc -Zunpretty=expanded`) is prone to producing output which is not syntactically valid Rust. Invalid output is problematic because it means Rustfmt is unable to parse the output of `cargo expand`, for example, causing friction by forcing someone trying to debug a macro into reading poorly formatted code. I believe the set of bugs fixed in this PR account for the most prevalent reason that `cargo expand` produces invalid output in real-world usage. Fixes rust-lang#98790. ## False negatives The following is a correct program — `cargo check` succeeds. ```rust macro_rules! m { ($e:expr) => { match () { _ => $e } }; } fn main() { m!({ 1 } - 1); } ``` But `rustc -Zunpretty=expanded main.rs` produces output that is invalid Rust syntax, because parenthesization is needed and not being done by the pretty printer. ```rust fn main() { match () { _ => { 1 } - 1, }; } ``` Piping this expanded code to rustfmt, it fails to parse. ```console error: unexpected `,` in pattern --> <stdin>:1:38 | 1 | fn main() { match () { _ => { 1 } - 1, }; } | ^ | help: try adding parentheses to match on a tuple... | 1 | fn main() { match () { _ => { 1 } (- 1,) }; } | + + help: ...or a vertical bar to match on multiple alternatives | 1 | fn main() { match () { _ => { 1 } - 1 | }; } | ~~~~~ ``` Fixed output after this PR: ```rust fn main() { match () { _ => ({ 1 }) - 1, }; } ``` ## False positives Less problematic, but worth fixing (just like rust-lang#118726). ```rust fn main() { let _ = match () { _ => 1 } - 1; } ``` Output of `rustc -Zunpretty=expanded lib.rs` before this PR. There is no reason parentheses would need to be inserted there. ```rust fn main() { let _ = (match () { _ => 1, }) - 1; } ``` After this PR: ```rust fn main() { let _ = match () { _ => 1, } - 1; } ``` ## Alternatives considered In this PR I opted to parenthesize only the leading subexpression causing the statement boundary, rather than the entire statement. Example: ```rust macro_rules! m { ($e:expr) => { $e }; } fn main() { m!(loop { break [1]; }[0] - 1); } ``` This PR produces the following pretty-printed contents for fn main: ```rust (loop { break [1]; })[0] - 1; ``` A different equally correct output would be: ```rust (loop { break [1]; }[0] - 1); ``` I chose the one I did because it is the *only* approach used by handwritten code in the standard library and compiler. There are 4 places where parenthesization is being used to prevent a statement boundary, and in all 4, the developer has chosen to parenthesize the smallest subexpression rather than the whole statement: https://github.com/rust-lang/rust/blob/b37d43efd9c5a7a3d76ed21c454dd0f40945d77d/compiler/rustc_codegen_cranelift/example/alloc_system.rs#L102 https://github.com/rust-lang/rust/blob/b37d43efd9c5a7a3d76ed21c454dd0f40945d77d/compiler/rustc_parse/src/errors.rs#L1021-L1029 https://github.com/rust-lang/rust/blob/b37d43efd9c5a7a3d76ed21c454dd0f40945d77d/library/core/src/future/poll_fn.rs#L151 https://github.com/rust-lang/rust/blob/b37d43efd9c5a7a3d76ed21c454dd0f40945d77d/library/core/src/ops/range.rs#L824-L828

Fix, document, and test parser and pretty-printer edge cases related to braced macro calls _Review note: this is a deceptively small PR because it comes with 145 lines of docs and 196 lines of tests, and only 25 lines of compiler code changed. However, I recommend reviewing it 1 commit at a time because much of the effect of the code changes is non-local i.e. affecting code that is not visible in the final state of the PR. I have paid attention that reviewing the PR one commit at a time is as easy as I can make it. All of the code you need to know about is touched in those commits, even if some of those changes disappear by the end of the stack._ This is a follow-up to rust-lang#119105. One case that is not relevant to `-Zunpretty=expanded`, but which came up as I'm porting rust-lang#119105 and rust-lang#118726 into `syn`'s printer and `prettyplease`'s printer where it **is** relevant, and is also relevant to rustc's `stringify!`, is statement boundaries in the vicinity of braced macro calls. Rustc's AST pretty-printer produces invalid syntax for statements that begin with a braced macro call: ```rust macro_rules! stringify_item { ($i:item) => { stringify!($i) }; } macro_rules! repro { ($e:expr) => { stringify_item!(fn main() { $e + 1; }) }; } fn main() { println!("{}", repro!(m! {})); } ``` **Before this PR:** output is not valid Rust syntax. ```console fn main() { m! {} + 1; } ``` ```console error: leading `+` is not supported --> <anon>:1:19 | 1 | fn main() { m! {} + 1; } | ^ unexpected `+` | help: try removing the `+` | 1 - fn main() { m! {} + 1; } 1 + fn main() { m! {} 1; } | ``` **After this PR:** valid syntax. ```console fn main() { (m! {}) + 1; } ```

rustbot assigned compiler-errors Dec 7, 2023

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 7, 2023

This comment has been minimized.

Sign in to view

dtolnay mentioned this pull request Dec 7, 2023

Tracking issue for RFC 2294, "if let guard" #51114

Open

8 tasks

Veykril reviewed Dec 8, 2023

View reviewed changes

compiler/rustc_ast_pretty/src/pprust/state.rs Outdated Show resolved Hide resolved

dtolnay force-pushed the matchguardlet branch from 5a9be48 to 2db4e31 Compare December 8, 2023 11:54

dtolnay added 7 commits December 8, 2023 15:14

Add if_let_guard and let_chains pretty printer tests

419b269

Inline npals closure

d2b7bd4

Inline cond_needs_par into print_let

8d64961

Rearrange logic of needs_par computation in print_let

dc5ec72

True || needs_par_as_let_scrutinee(...) is always true.

Do not parenthesize exterior struct lit inside match guards

6f1d763

dtolnay force-pushed the matchguardlet branch from 2db4e31 to 8997215 Compare December 8, 2023 23:19

compiler-errors approved these changes Dec 11, 2023

View reviewed changes

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 11, 2023

matthiaskrgr mentioned this pull request Dec 11, 2023

Rollup of 8 pull requests #118838

Merged

bors merged commit 3a0562b into rust-lang:master Dec 11, 2023
11 checks passed

rustbot added this to the 1.76.0 milestone Dec 11, 2023

dtolnay deleted the matchguardlet branch December 11, 2023 23:21

dtolnay mentioned this pull request Dec 19, 2023

Fix parenthesization of subexprs containing statement boundary #119105

Merged

dtolnay added the A-pretty Area: Pretty printing (including `-Z unpretty`) label Dec 28, 2023

dtolnay mentioned this pull request Dec 30, 2023

Fix, document, and test parser and pretty-printer edge cases related to braced macro calls #119427

Merged

dtolnay mentioned this pull request May 16, 2024

Automatically insert necessary parentheses in ToTokens for Expr dtolnay/syn#1659

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not parenthesize exterior struct lit inside match guards #118726

Do not parenthesize exterior struct lit inside match guards #118726

dtolnay commented Dec 7, 2023

rustbot commented Dec 7, 2023

This comment has been minimized.

bors commented Dec 8, 2023

dtolnay commented Dec 8, 2023

compiler-errors left a comment

compiler-errors commented Dec 11, 2023

bors commented Dec 11, 2023

Do not parenthesize exterior struct lit inside match guards #118726

Do not parenthesize exterior struct lit inside match guards #118726

Conversation

dtolnay commented Dec 7, 2023

rustbot commented Dec 7, 2023

This comment has been minimized.

bors commented Dec 8, 2023

dtolnay commented Dec 8, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

compiler-errors commented Dec 11, 2023

bors commented Dec 11, 2023