Skip to content

Commit

Permalink
Auto merge of #119105 - dtolnay:paren, r=WaffleLapkin
Browse files Browse the repository at this point in the history
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 #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 #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
  • Loading branch information
bors committed Dec 27, 2023
2 parents 88d69b7 + 17239d9 commit 89e2160
Show file tree
Hide file tree
Showing 3 changed files with 358 additions and 44 deletions.
18 changes: 15 additions & 3 deletions compiler/rustc_ast_pretty/src/pprust/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1096,14 +1096,22 @@ impl<'a> State<'a> {
ast::StmtKind::Item(item) => self.print_item(item),
ast::StmtKind::Expr(expr) => {
self.space_if_not_bol();
self.print_expr_outer_attr_style(expr, false, FixupContext::default());
self.print_expr_outer_attr_style(
expr,
false,
FixupContext { stmt: true, ..FixupContext::default() },
);
if classify::expr_requires_semi_to_be_stmt(expr) {
self.word(";");
}
}
ast::StmtKind::Semi(expr) => {
self.space_if_not_bol();
self.print_expr_outer_attr_style(expr, false, FixupContext::default());
self.print_expr_outer_attr_style(
expr,
false,
FixupContext { stmt: true, ..FixupContext::default() },
);
self.word(";");
}
ast::StmtKind::Empty => {
Expand Down Expand Up @@ -1155,7 +1163,11 @@ impl<'a> State<'a> {
ast::StmtKind::Expr(expr) if i == blk.stmts.len() - 1 => {
self.maybe_print_comment(st.span.lo());
self.space_if_not_bol();
self.print_expr_outer_attr_style(expr, false, FixupContext::default());
self.print_expr_outer_attr_style(
expr,
false,
FixupContext { stmt: true, ..FixupContext::default() },
);
self.maybe_print_trailing_comment(expr.span, Some(blk.span.hi()));
}
_ => self.print_stmt(st),
Expand Down

0 comments on commit 89e2160

Please sign in to comment.