Rollup merge of rust-lang#142476 - dtolnay:attrbinop, r=fmease

Insert parentheses around binary operation with attribute

Fixes the bug found by `@fmease` in rust-lang#134661 (review).

Previously, `-Zunpretty=expanded` would expand this program as follows:

```rust
#![feature(stmt_expr_attributes)]
#![allow(unused_attributes)]

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

macro_rules! extra {
    ($e:expr) => {
        #[allow()] $e
    };
}

fn main() {
    let _ = #[allow()] 1 + 1;
    let _ = group!(#[allow()] 1) + 1;
    let _ = 1 + group!(#[allow()] 1);
    let _ = extra!({ 0 }) + 1;
    let _ = extra!({ 0 } + 1);
}
```

```console
let _ = #[allow()] 1 + 1;
let _ = #[allow()] 1 + 1;
let _ = 1 + #[allow()] 1;
let _ = #[allow()] { 0 } + 1;
let _ = #[allow()] { 0 } + 1;
```

The first 4 statements are the correct expansion, but the last one is not. The attribute is supposed to apply to the entire binary operation, not only to the left operand.

After this PR, the 5th statement will expand to:

```console
let _ = #[allow()] ({ 0 } + 1);
```

In the future, as some subset of `stmt_expr_attributes` approaches stabilization, it is possible that we will need to do parenthesization for a number of additional cases depending on the outcome of rust-lang#127436. But for now, at least this PR makes the pretty-printer align with the current behavior of the parser.

r? fmease

Author: tgross35

compiler/rustc_ast_pretty/src/pprust/state/expr.rs

@@ -386,18 +386,44 @@ impl<'a> State<'a> {
 
         let ib = self.ibox(INDENT_UNIT);
 
-        // The Match subexpression in `match x {} - 1` must be parenthesized if
-        // it is the leftmost subexpression in a statement:
-        //
-        //     (match x {}) - 1;
-        //
-        // But not otherwise:
-        //
-        //     let _ = match x {} - 1;
-        //
-        // Same applies to a small set of other expression kinds which eagerly
-        // terminate a statement which opens with them.
-        let needs_par = fixup.would_cause_statement_boundary(expr);
+        let needs_par = {
+            // The Match subexpression in `match x {} - 1` must be parenthesized
+            // if it is the leftmost subexpression in a statement:
+            //
+            //     (match x {}) - 1;
+            //
+            // But not otherwise:
+            //
+            //     let _ = match x {} - 1;
+            //
+            // Same applies to a small set of other expression kinds which
+            // eagerly terminate a statement which opens with them.
+            fixup.would_cause_statement_boundary(expr)
+        } || {
+            // If a binary operation ends up with an attribute, such as
+            // resulting from the following macro expansion, then parentheses
+            // are required so that the attribute encompasses the right
+            // subexpression and not just the left one.
+            //
+            //     #![feature(stmt_expr_attributes)]
+            //
+            //     macro_rules! add_attr {
+            //         ($e:expr) => { #[attr] $e };
+            //     }
+            //
+            //     let _ = add_attr!(1 + 1);
+            //
+            // We must pretty-print `#[attr] (1 + 1)` not `#[attr] 1 + 1`.
+            !attrs.is_empty()
+                && matches!(
+                    expr.kind,
+                    ast::ExprKind::Binary(..)
+                        | ast::ExprKind::Cast(..)
+                        | ast::ExprKind::Assign(..)
+                        | ast::ExprKind::AssignOp(..)
+                        | ast::ExprKind::Range(..)
+                )
+        };
         if needs_par {
             self.popen();
             fixup = FixupContext::default();

tests/ui-fulldeps/pprust-parenthesis-insertion.rs

@@ -92,6 +92,21 @@ static EXPRS: &[&str] = &[
     "#[attr] loop {}.field",
     "(#[attr] loop {}).field",
     "loop { #![attr] }.field",
+    // Attributes on a Binary, Cast, Assign, AssignOp, and Range expression
+    // require parentheses. Without parentheses `#[attr] lo..hi` means
+    // `(#[attr] lo)..hi`, and `#[attr] ..hi` is invalid syntax.
+    "#[attr] (1 + 1)",
+    "#[attr] (1 as T)",
+    "#[attr] (x = 1)",
+    "#[attr] (x += 1)",
+    "#[attr] (lo..hi)",
+    "#[attr] (..hi)",
+    // If the attribute were not present on the binary operation, it would be
+    // legal to render this without not just the inner parentheses, but also the
+    // outer ones. `return x + .. .field` (Yes, really.) Currently the
+    // pretty-printer does not take advantage of this edge case.
+    "(return #[attr] (x + ..)).field",
+    "(return x + ..).field",
     // Grammar restriction: break value starting with a labeled loop is not
     // allowed, except if the break is also labeled.
     "break 'outer 'inner: loop {} + 2",
@@ -158,7 +173,12 @@ struct Unparenthesize;
 impl MutVisitor for Unparenthesize {
     fn visit_expr(&mut self, e: &mut Expr) {
         while let ExprKind::Paren(paren) = &mut e.kind {
+            let paren_attrs = mem::take(&mut e.attrs);
             *e = mem::replace(paren, Expr::dummy());
+            if !paren_attrs.is_empty() {
+                assert!(e.attrs.is_empty());
+                e.attrs = paren_attrs;
+            }
         }
         mut_visit::walk_expr(self, e);
     }