Repository: dtolnay/syn
Pull request #1695 (2024-07-07 01:29:04)

Improve precision of jump parentheses when next token cannot begin expr
Previously syn would forcibly put parentheses when printing something like `(break) + 1` or `(return) + 1`. Although parentheses are required in `(break) - 1` and `(return) - 1`, they should not be required when the next token following the valueless jump is not a legal first token of an expression.


Patch:
diff --git a/src/expr.rs b/src/expr.rs
index 2285084e..9f2cd8b1 100644
--- a/src/expr.rs
+++ b/src/expr.rs
@@ -3119,9 +3119,16 @@ pub(crate) mod printing {
         }
     }
 
-    fn precedence_of_rhs(e: &Expr, #[cfg(feature = "full")] fixup: FixupContext) -> Precedence {
+    fn leading_precedence(e: &Expr, #[cfg(feature = "full")] fixup: FixupContext) -> Precedence {
         #[cfg(feature = "full")]
-        return fixup.precedence_of_rhs(e);
+        return fixup.leading_precedence(e);
+        #[cfg(not(feature = "full"))]
+        return Precedence::of(e);
+    }
+
+    fn trailing_precedence(e: &Expr, #[cfg(feature = "full")] fixup: FixupContext) -> Precedence {
+        #[cfg(feature = "full")]
+        return fixup.trailing_precedence(e);
         #[cfg(not(feature = "full"))]
         return Precedence::of(e);
     }
@@ -3157,7 +3164,7 @@ pub(crate) mod printing {
         e.eq_token.to_tokens(tokens);
         print_subexpression(
             &e.right,
-            precedence_of_rhs(&e.right, fixup) < Precedence::Assign,
+            trailing_precedence(&e.right, fixup) < Precedence::Assign,
             tokens,
             fixup.subsequent_subexpression(),
         );
@@ -3214,13 +3221,31 @@ pub(crate) mod printing {
     ) {
         outer_attrs_to_tokens(&e.attrs, tokens);
 
-        let binop_prec = Precedence::of_binop(&e.op);
-        let left_prec = Precedence::of(&e.left);
-        let right_prec = precedence_of_rhs(
+        #[cfg(feature = "full")]
+        let left_fixup = fixup.leftmost_subexpression_with_begin_operator(match &e.op {
+            BinOp::Sub(_)
+            | BinOp::Mul(_)
+            | BinOp::And(_)
+            | BinOp::Or(_)
+            | BinOp::BitAnd(_)
+            | BinOp::BitOr(_)
+            | BinOp::Shl(_)
+            | BinOp::Lt(_) => true,
+            _ => false,
+        });
+        let left_prec = leading_precedence(
+            &e.left,
+            #[cfg(feature = "full")]
+            left_fixup,
+        );
+
+        let right_prec = trailing_precedence(
             &e.right,
             #[cfg(feature = "full")]
             fixup,
         );
+
+        let binop_prec = Precedence::of_binop(&e.op);
         let (mut left_needs_group, right_needs_group) = if let Precedence::Assign = binop_prec {
             (left_prec <= binop_prec, right_prec < binop_prec)
         } else {
@@ -3259,7 +3284,7 @@ pub(crate) mod printing {
             left_needs_group,
             tokens,
             #[cfg(feature = "full")]
-            fixup.leftmost_subexpression(),
+            left_fixup,
         );
         e.op.to_tokens(tokens);
         print_subexpression(
@@ -3328,17 +3353,24 @@ pub(crate) mod printing {
     ) {
         outer_attrs_to_tokens(&e.attrs, tokens);
 
-        let precedence = if let Expr::Field(_) = &*e.func {
+        let call_precedence = if let Expr::Field(_) = &*e.func {
             Precedence::Any
         } else {
             Precedence::Unambiguous
         };
+        #[cfg(feature = "full")]
+        let func_fixup = fixup.leftmost_subexpression_with_begin_operator(true);
+        let func_precedence = leading_precedence(
+            &e.func,
+            #[cfg(feature = "full")]
+            func_fixup,
+        );
         print_subexpression(
             &e.func,
-            Precedence::of(&e.func) < precedence,
+            func_precedence < call_precedence,
             tokens,
             #[cfg(feature = "full")]
-            fixup.leftmost_subexpression(),
+            func_fixup,
         );
 
         e.paren_token.surround(tokens, |tokens| {
@@ -3535,12 +3567,19 @@ pub(crate) mod printing {
         #[cfg(feature = "full")] fixup: FixupContext,
     ) {
         outer_attrs_to_tokens(&e.attrs, tokens);
+        #[cfg(feature = "full")]
+        let obj_fixup = fixup.leftmost_subexpression_with_begin_operator(true);
+        let obj_precedence = leading_precedence(
+            &e.expr,
+            #[cfg(feature = "full")]
+            obj_fixup,
+        );
         print_subexpression(
             &e.expr,
-            Precedence::of(&e.expr) < Precedence::Unambiguous,
+            obj_precedence < Precedence::Unambiguous,
             tokens,
             #[cfg(feature = "full")]
-            fixup.leftmost_subexpression(),
+            obj_fixup,
         );
         e.bracket_token.surround(tokens, |tokens| {
             e.index.to_tokens(tokens);
@@ -3707,7 +3746,7 @@ pub(crate) mod printing {
         if let Some(end) = &e.end {
             print_subexpression(
                 end,
-                precedence_of_rhs(end, fixup) <= Precedence::Range,
+                trailing_precedence(end, fixup) <= Precedence::Range,
                 tokens,
                 fixup.subsequent_subexpression(),
             );
@@ -3734,7 +3773,7 @@ pub(crate) mod printing {
         outer_attrs_to_tokens(&e.attrs, tokens);
         e.and_token.to_tokens(tokens);
         e.mutability.to_tokens(tokens);
-        let prec = precedence_of_rhs(
+        let prec = trailing_precedence(
             &e.expr,
             #[cfg(feature = "full")]
             fixup,
@@ -3860,7 +3899,7 @@ pub(crate) mod printing {
     ) {
         outer_attrs_to_tokens(&e.attrs, tokens);
         e.op.to_tokens(tokens);
-        let prec = precedence_of_rhs(
+        let prec = trailing_precedence(
             &e.expr,
             #[cfg(feature = "full")]
             fixup,
diff --git a/src/fixup.rs b/src/fixup.rs
index 4b9fe564..63b79aac 100644
--- a/src/fixup.rs
+++ b/src/fixup.rs
@@ -93,6 +93,14 @@ pub(crate) struct FixupContext {
     //     let _ = 1 + (return 1) + 1;  // needs parens
     //
     parenthesize_exterior_jump: bool,
+
+    // This is the difference between:
+    //
+    //     let _ = (return) - 1;  // without paren, this would return -1
+    //
+    //     let _ = return + 1;  // no paren because '+' cannot begin expr
+    //
+    next_operator_can_begin_expr: bool,
 }
 
 impl FixupContext {
@@ -105,6 +113,7 @@ impl FixupContext {
         leftmost_subexpression_in_match_arm: false,
         parenthesize_exterior_struct_lit: false,
         parenthesize_exterior_jump: false,
+        next_operator_can_begin_expr: false,
     };
 
     /// Create the initial fixup for printing an expression in statement
@@ -174,6 +183,19 @@ impl FixupContext {
         }
     }
 
+    /// Transform this fixup into the one that should apply when printing a
+    /// leftmost subexpression followed by punctuation that is legal as the
+    /// first token of an expression.
+    pub fn leftmost_subexpression_with_begin_operator(
+        self,
+        next_operator_can_begin_expr: bool,
+    ) -> Self {
+        FixupContext {
+            next_operator_can_begin_expr,
+            ..self.leftmost_subexpression()
+        }
+    }
+
     /// Transform this fixup into the one that should apply when printing any
     /// subexpression that is neither a leftmost subexpression nor surrounded in
     /// delimiters.
@@ -216,15 +238,32 @@ impl FixupContext {
     ///     "let chain".
     pub fn needs_group_as_let_scrutinee(self, expr: &Expr) -> bool {
         self.parenthesize_exterior_struct_lit && classify::confusable_with_adjacent_block(expr)
-            || self.precedence_of_rhs(expr) <= Precedence::And
+            || self.trailing_precedence(expr) <= Precedence::And
+    }
+
+    /// Determines the effective precedence of a left subexpression. Some
+    /// expressions have lower precedence when adjacent to particular operators.
+    pub fn leading_precedence(self, expr: &Expr) -> Precedence {
+        if self.next_operator_can_begin_expr {
+            match expr {
+                // Decrease precedence of value-less jumps when followed by an
+                // operator that would otherwise get interpreted as beginning a
+                // value for the jump.
+                Expr::Break(_) | Expr::Return(_) | Expr::Yield(_) => return Precedence::Any,
+                _ => {}
+            }
+        }
+        Precedence::of(expr)
     }
 
-    /// Determines the effective precedence of a subexpression. Some expressions
-    /// have higher precedence on the right side of a binary operator than on
-    /// the left.
-    pub fn precedence_of_rhs(self, expr: &Expr) -> Precedence {
+    /// Determines the effective precedence of a right subexpression. Some
+    /// expressions have higher precedence on the right side of a binary
+    /// operator than on the left.
+    pub fn trailing_precedence(self, expr: &Expr) -> Precedence {
         if !self.parenthesize_exterior_jump {
             match expr {
+                // Increase precedence of expressions that extend to the end of
+                // current statement or group.
                 Expr::Break(_) | Expr::Closure(_) | Expr::Return(_) | Expr::Yield(_) => {
                     return Precedence::Prefix;
                 }
diff --git a/src/precedence.rs b/src/precedence.rs
index 14320be8..5f872aab 100644
--- a/src/precedence.rs
+++ b/src/precedence.rs
@@ -1,5 +1,7 @@
 #[cfg(feature = "printing")]
 use crate::expr::Expr;
+#[cfg(all(feature = "printing", feature = "full"))]
+use crate::expr::{ExprBreak, ExprReturn, ExprYield};
 use crate::op::BinOp;
 #[cfg(all(feature = "printing", feature = "full"))]
 use crate::ty::ReturnType;
@@ -82,7 +84,14 @@ impl Precedence {
                 ReturnType::Type(..) => Precedence::Unambiguous,
             },
 
-            Expr::Break(_) | Expr::Return(_) | Expr::Yield(_) => Precedence::Any,
+            #[cfg(feature = "full")]
+            Expr::Break(ExprBreak { expr, .. })
+            | Expr::Return(ExprReturn { expr, .. })
+            | Expr::Yield(ExprYield { expr, .. }) => match expr {
+                Some(_) => Precedence::Any,
+                None => Precedence::Unambiguous,
+            },
+
             Expr::Assign(_) => Precedence::Assign,
             Expr::Range(_) => Precedence::Range,
             Expr::Binary(e) => Precedence::of_binop(&e.op),
@@ -119,7 +128,7 @@ impl Precedence {
             | Expr::While(_) => Precedence::Unambiguous,
 
             #[cfg(not(feature = "full"))]
-            Expr::Closure(_) => unreachable!(),
+            Expr::Break(_) | Expr::Closure(_) | Expr::Return(_) | Expr::Yield(_) => unreachable!(),
         }
     }
 }


Test patch:
diff --git a/tests/test_expr.rs b/tests/test_expr.rs
index a40c0f5d..f48f8ed1 100644
--- a/tests/test_expr.rs
+++ b/tests/test_expr.rs
@@ -674,6 +674,9 @@ fn test_fixup() {
         quote! { if let _ = (S {}) {} },
         quote! { break ('a: loop { break 'a 1 } + 1) },
         quote! { a + (|| b) + c },
+        quote! { if let _ = ((break) - 1 || true) {} },
+        quote! { if let _ = (break + 1 || true) {} },
+        quote! { (break)() },
     ] {
         let original: Expr = syn::parse2(tokens).unwrap();