diff --git a/src/classify.rs b/src/classify.rs index 32e491dc11..8eab19dbc3 100644 --- a/src/classify.rs +++ b/src/classify.rs @@ -67,145 +67,6 @@ pub(crate) fn requires_comma_to_be_match_arm(expr: &Expr) -> bool { } } -#[cfg(all(feature = "printing", feature = "full"))] -pub(crate) fn confusable_with_adjacent_block(expr: &Expr) -> bool { - let allow_struct = false; - let optional_leftmost_subexpression = false; - let rightmost_subexpression = true; - return confusable( - expr, - allow_struct, - optional_leftmost_subexpression, - rightmost_subexpression, - ); - - fn confusable( - expr: &Expr, - allow_struct: bool, - optional_leftmost_subexpression: bool, - rightmost_subexpression: bool, - ) -> bool { - match expr { - Expr::Assign(e) => { - confusable( - &e.left, - allow_struct, - optional_leftmost_subexpression, - false, - ) || confusable(&e.right, allow_struct, false, rightmost_subexpression) - } - Expr::Await(e) => confusable( - &e.base, - allow_struct, - optional_leftmost_subexpression, - false, - ), - Expr::Binary(e) => { - confusable( - &e.left, - allow_struct, - optional_leftmost_subexpression, - false, - ) || confusable(&e.right, allow_struct, false, rightmost_subexpression) - } - Expr::Block(e) => { - optional_leftmost_subexpression && e.attrs.is_empty() && e.label.is_none() - } - Expr::Break(e) => { - if let Some(value) = &e.expr { - confusable(value, true, !allow_struct, rightmost_subexpression) - } else { - allow_struct && rightmost_subexpression - } - } - Expr::Call(e) => confusable( - &e.func, - allow_struct, - optional_leftmost_subexpression, - false, - ), - Expr::Cast(e) => confusable( - &e.expr, - allow_struct, - optional_leftmost_subexpression, - false, - ), - Expr::Closure(e) => confusable(&e.body, allow_struct, false, rightmost_subexpression), - Expr::Field(e) => confusable( - &e.base, - allow_struct, - optional_leftmost_subexpression, - false, - ), - Expr::Index(e) => confusable( - &e.expr, - allow_struct, - optional_leftmost_subexpression, - false, - ), - Expr::MethodCall(e) => confusable( - &e.receiver, - allow_struct, - optional_leftmost_subexpression, - false, - ), - Expr::Path(_) => allow_struct && rightmost_subexpression, - Expr::Range(e) => { - (match &e.start { - Some(start) => { - confusable(start, allow_struct, optional_leftmost_subexpression, false) - } - None => false, - } || match &e.end { - Some(end) => { - confusable(end, allow_struct, !allow_struct, rightmost_subexpression) - } - None => allow_struct && rightmost_subexpression, - }) - } - Expr::RawAddr(e) => confusable(&e.expr, allow_struct, false, rightmost_subexpression), - Expr::Reference(e) => confusable(&e.expr, allow_struct, false, rightmost_subexpression), - Expr::Return(e) => match &e.expr { - Some(expr) => confusable(expr, true, false, rightmost_subexpression), - None => rightmost_subexpression, - }, - Expr::Struct(_) => !allow_struct, - Expr::Try(e) => confusable( - &e.expr, - allow_struct, - optional_leftmost_subexpression, - false, - ), - Expr::Unary(e) => confusable(&e.expr, allow_struct, false, rightmost_subexpression), - Expr::Yield(e) => match &e.expr { - Some(expr) => confusable(expr, true, false, rightmost_subexpression), - None => rightmost_subexpression, - }, - - Expr::Array(_) - | Expr::Async(_) - | Expr::Const(_) - | Expr::Continue(_) - | Expr::ForLoop(_) - | Expr::Group(_) - | Expr::If(_) - | Expr::Infer(_) - | Expr::Let(_) - | Expr::Lit(_) - | Expr::Loop(_) - | Expr::Macro(_) - | Expr::Match(_) - | Expr::Paren(_) - | Expr::Repeat(_) - | Expr::TryBlock(_) - | Expr::Tuple(_) - | Expr::Unsafe(_) - | Expr::Verbatim(_) - | Expr::While(_) => false, - } - } -} - #[cfg(feature = "printing")] pub(crate) fn trailing_unparameterized_path(mut ty: &Type) -> bool { loop { diff --git a/src/expr.rs b/src/expr.rs index 49dcb18c3f..03f243a057 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -3151,16 +3151,6 @@ pub(crate) mod printing { #[cfg(not(feature = "full"))] pub(crate) fn outer_attrs_to_tokens(_attrs: &[Attribute], _tokens: &mut TokenStream) {} - #[cfg(feature = "full")] - fn print_condition(expr: &Expr, tokens: &mut TokenStream) { - print_subexpression( - expr, - classify::confusable_with_adjacent_block(expr), - tokens, - FixupContext::new_condition(), - ); - } - pub(crate) fn print_subexpression( expr: &Expr, needs_group: bool, @@ -3193,7 +3183,7 @@ pub(crate) mod printing { pub(crate) fn print_expr(expr: &Expr, tokens: &mut TokenStream, mut fixup: FixupContext) { #[cfg(feature = "full")] - let needs_group = fixup.would_cause_statement_boundary(expr); + let needs_group = fixup.parenthesize(expr); #[cfg(not(feature = "full"))] let needs_group = false; @@ -3430,7 +3420,7 @@ pub(crate) mod printing { // ^---------------------------------^ e.label.is_none() && classify::expr_leading_label(value), tokens, - fixup.rightmost_subexpression_fixup(Precedence::Jump), + fixup.rightmost_subexpression_fixup(true, true, Precedence::Jump), ); } } @@ -3513,7 +3503,7 @@ pub(crate) mod printing { print_expr( &e.body, tokens, - fixup.rightmost_subexpression_fixup(Precedence::Jump), + fixup.rightmost_subexpression_fixup(false, false, Precedence::Jump), ); } else { token::Brace::default().surround(tokens, |tokens| { @@ -3574,7 +3564,7 @@ pub(crate) mod printing { self.for_token.to_tokens(tokens); self.pat.to_tokens(tokens); self.in_token.to_tokens(tokens); - print_condition(&self.expr, tokens); + print_expr(&self.expr, tokens, FixupContext::new_condition()); self.body.brace_token.surround(tokens, |tokens| { inner_attrs_to_tokens(&self.attrs, tokens); tokens.append_all(&self.body.stmts); @@ -3601,7 +3591,7 @@ pub(crate) mod printing { let mut expr = self; loop { expr.if_token.to_tokens(tokens); - print_condition(&expr.cond, tokens); + print_expr(&expr.cond, tokens, FixupContext::new_condition()); expr.then_branch.to_tokens(tokens); let (else_token, else_) = match &expr.else_branch { @@ -3682,12 +3672,8 @@ pub(crate) mod printing { e.let_token.to_tokens(tokens); e.pat.to_tokens(tokens); e.eq_token.to_tokens(tokens); - print_subexpression( - &e.expr, - fixup.needs_group_as_let_scrutinee(&e.expr), - tokens, - FixupContext::NONE, - ); + let (right_prec, right_fixup) = fixup.rightmost_subexpression(&e.expr, Precedence::Let); + print_subexpression(&e.expr, right_prec < Precedence::Let, tokens, right_fixup); } #[cfg_attr(docsrs, doc(cfg(feature = "printing")))] @@ -3726,7 +3712,7 @@ pub(crate) mod printing { fn to_tokens(&self, tokens: &mut TokenStream) { outer_attrs_to_tokens(&self.attrs, tokens); self.match_token.to_tokens(tokens); - print_condition(&self.expr, tokens); + print_expr(&self.expr, tokens, FixupContext::new_condition()); self.brace_token.surround(tokens, |tokens| { inner_attrs_to_tokens(&self.attrs, tokens); for (i, arm) in self.arms.iter().enumerate() { @@ -3811,7 +3797,8 @@ pub(crate) mod printing { } e.limits.to_tokens(tokens); if let Some(end) = &e.end { - let (right_prec, right_fixup) = fixup.rightmost_subexpression(end, Precedence::Range); + let right_fixup = fixup.rightmost_subexpression_fixup(false, true, Precedence::Range); + let right_prec = right_fixup.rightmost_subexpression_precedence(end); print_subexpression(end, right_prec <= Precedence::Range, tokens, right_fixup); } } @@ -3892,7 +3879,7 @@ pub(crate) mod printing { print_expr( expr, tokens, - fixup.rightmost_subexpression_fixup(Precedence::Jump), + fixup.rightmost_subexpression_fixup(true, false, Precedence::Jump), ); } } @@ -4003,7 +3990,7 @@ pub(crate) mod printing { outer_attrs_to_tokens(&self.attrs, tokens); self.label.to_tokens(tokens); self.while_token.to_tokens(tokens); - print_condition(&self.cond, tokens); + print_expr(&self.cond, tokens, FixupContext::new_condition()); self.body.brace_token.surround(tokens, |tokens| { inner_attrs_to_tokens(&self.attrs, tokens); tokens.append_all(&self.body.stmts); @@ -4027,7 +4014,7 @@ pub(crate) mod printing { print_expr( expr, tokens, - fixup.rightmost_subexpression_fixup(Precedence::Jump), + fixup.rightmost_subexpression_fixup(true, false, Precedence::Jump), ); } } diff --git a/src/fixup.rs b/src/fixup.rs index 2604fd86d8..126700cc8e 100644 --- a/src/fixup.rs +++ b/src/fixup.rs @@ -1,7 +1,9 @@ use crate::classify; use crate::expr::Expr; #[cfg(feature = "full")] -use crate::expr::{ExprBreak, ExprRawAddr, ExprReference, ExprReturn, ExprUnary, ExprYield}; +use crate::expr::{ + ExprBreak, ExprRange, ExprRawAddr, ExprReference, ExprReturn, ExprUnary, ExprYield, +}; use crate::precedence::Precedence; #[cfg(feature = "full")] use crate::ty::ReturnType; @@ -98,7 +100,25 @@ pub(crate) struct FixupContext { // } // #[cfg(feature = "full")] - parenthesize_exterior_struct_lit: bool, + condition: bool, + + // This is the difference between: + // + // if break Struct {} == (break) {} // needs parens + // + // if break break == Struct {} {} // no parens + // + #[cfg(feature = "full")] + rightmost_subexpression_in_condition: bool, + + // This is the difference between: + // + // if break ({ x }).field + 1 {} needs parens + // + // if break 1 + { x }.field {} // no parens + // + #[cfg(feature = "full")] + leftmost_subexpression_in_optional_operand: bool, // This is the difference between: // @@ -145,7 +165,11 @@ impl FixupContext { #[cfg(feature = "full")] leftmost_subexpression_in_match_arm: false, #[cfg(feature = "full")] - parenthesize_exterior_struct_lit: false, + condition: false, + #[cfg(feature = "full")] + rightmost_subexpression_in_condition: false, + #[cfg(feature = "full")] + leftmost_subexpression_in_optional_operand: false, #[cfg(feature = "full")] next_operator_can_begin_expr: false, #[cfg(feature = "full")] @@ -180,7 +204,8 @@ impl FixupContext { #[cfg(feature = "full")] pub fn new_condition() -> Self { FixupContext { - parenthesize_exterior_struct_lit: true, + condition: true, + rightmost_subexpression_in_condition: true, ..FixupContext::NONE } } @@ -216,6 +241,8 @@ impl FixupContext { leftmost_subexpression_in_match_arm: self.match_arm || self.leftmost_subexpression_in_match_arm, #[cfg(feature = "full")] + rightmost_subexpression_in_condition: false, + #[cfg(feature = "full")] next_operator_can_begin_expr, #[cfg(feature = "full")] next_operator_can_continue_expr: true, @@ -243,6 +270,8 @@ impl FixupContext { #[cfg(feature = "full")] leftmost_subexpression_in_match_arm: false, #[cfg(feature = "full")] + rightmost_subexpression_in_condition: false, + #[cfg(feature = "full")] next_operator_can_begin_expr: false, #[cfg(feature = "full")] next_operator_can_continue_expr: true, @@ -283,6 +312,10 @@ impl FixupContext { #[cfg(feature = "full")] precedence: Precedence, ) -> (Precedence, Self) { let fixup = self.rightmost_subexpression_fixup( + #[cfg(feature = "full")] + false, + #[cfg(feature = "full")] + false, #[cfg(feature = "full")] precedence, ); @@ -291,6 +324,8 @@ impl FixupContext { pub fn rightmost_subexpression_fixup( self, + #[cfg(feature = "full")] reset_allow_struct: bool, + #[cfg(feature = "full")] optional_operand: bool, #[cfg(feature = "full")] precedence: Precedence, ) -> Self { FixupContext { @@ -304,11 +339,15 @@ impl FixupContext { match_arm: false, #[cfg(feature = "full")] leftmost_subexpression_in_match_arm: false, + #[cfg(feature = "full")] + condition: self.condition && !reset_allow_struct, + #[cfg(feature = "full")] + leftmost_subexpression_in_optional_operand: self.condition && optional_operand, ..self } } - fn rightmost_subexpression_precedence(self, expr: &Expr) -> Precedence { + pub fn rightmost_subexpression_precedence(self, expr: &Expr) -> Precedence { let default_prec = self.precedence(expr); #[cfg(feature = "full")] @@ -332,33 +371,30 @@ impl FixupContext { } /// Determine whether parentheses are needed around the given expression to - /// head off an unintended statement boundary. - /// - /// The documentation on `FixupContext::leftmost_subexpression_in_stmt` has - /// examples. + /// head off the early termination of a statement or condition. #[cfg(feature = "full")] - pub fn would_cause_statement_boundary(self, expr: &Expr) -> bool { + pub fn parenthesize(self, expr: &Expr) -> bool { (self.leftmost_subexpression_in_stmt && !classify::requires_semi_to_be_stmt(expr)) || ((self.stmt || self.leftmost_subexpression_in_stmt) && matches!(expr, Expr::Let(_))) || (self.leftmost_subexpression_in_match_arm && !classify::requires_comma_to_be_match_arm(expr)) - } - - /// Determine whether parentheses are needed around the given `let` - /// scrutinee. - /// - /// In `if let _ = $e {}`, some examples of `$e` that would need parentheses - /// are: - /// - /// - `Struct {}.f()`, because otherwise the `{` would be misinterpreted - /// as the opening of the if's then-block. - /// - /// - `true && false`, because otherwise this would be misinterpreted as a - /// "let chain". - #[cfg(feature = "full")] - pub fn needs_group_as_let_scrutinee(self, expr: &Expr) -> bool { - self.parenthesize_exterior_struct_lit && classify::confusable_with_adjacent_block(expr) - || self.precedence(expr) < Precedence::Let + || (self.condition && matches!(expr, Expr::Struct(_))) + || (self.rightmost_subexpression_in_condition + && matches!( + expr, + Expr::Return(ExprReturn { expr: None, .. }) + | Expr::Yield(ExprYield { expr: None, .. }) + )) + || (self.rightmost_subexpression_in_condition + && !self.condition + && matches!( + expr, + Expr::Break(ExprBreak { expr: None, .. }) + | Expr::Path(_) + | Expr::Range(ExprRange { end: None, .. }) + )) + || (self.leftmost_subexpression_in_optional_operand + && matches!(expr, Expr::Block(expr) if expr.attrs.is_empty() && expr.label.is_none())) } /// Determines the effective precedence of a subexpression. Some expressions @@ -452,6 +488,9 @@ fn scan_right( fail_offset: u8, bailout_offset: u8, ) -> Scan { + if fixup.parenthesize(expr) { + return Scan::Consume; + } match expr { Expr::Assign(e) => { if match fixup.next_operator { @@ -460,7 +499,7 @@ fn scan_right( } { return Scan::Consume; } - let right_fixup = fixup.rightmost_subexpression_fixup(Precedence::Assign); + let right_fixup = fixup.rightmost_subexpression_fixup(false, false, Precedence::Assign); let scan = scan_right( &e.right, right_fixup, @@ -490,7 +529,7 @@ fn scan_right( return Scan::Consume; } let binop_prec = Precedence::of_binop(&e.op); - let right_fixup = fixup.rightmost_subexpression_fixup(binop_prec); + let right_fixup = fixup.rightmost_subexpression_fixup(false, false, binop_prec); let scan = scan_right( &e.right, right_fixup, @@ -535,7 +574,7 @@ fn scan_right( } { return Scan::Consume; } - let right_fixup = fixup.rightmost_subexpression_fixup(Precedence::Prefix); + let right_fixup = fixup.rightmost_subexpression_fixup(false, false, Precedence::Prefix); let scan = scan_right( expr, right_fixup, @@ -569,7 +608,8 @@ fn scan_right( if fail_offset >= 2 { return Scan::Consume; } - let right_fixup = fixup.rightmost_subexpression_fixup(Precedence::Range); + let right_fixup = + fixup.rightmost_subexpression_fixup(false, true, Precedence::Range); let scan = scan_right( end, right_fixup, @@ -603,7 +643,7 @@ fn scan_right( if bailout_offset >= 1 || e.label.is_none() && classify::expr_leading_label(value) { return Scan::Consume; } - let right_fixup = fixup.rightmost_subexpression_fixup(Precedence::Jump); + let right_fixup = fixup.rightmost_subexpression_fixup(true, true, Precedence::Jump); match scan_right(value, right_fixup, false, 1, 1) { Scan::Fail => Scan::Bailout, Scan::Bailout | Scan::Consume => Scan::Consume, @@ -619,7 +659,8 @@ fn scan_right( if bailout_offset >= 1 { return Scan::Consume; } - let right_fixup = fixup.rightmost_subexpression_fixup(Precedence::Jump); + let right_fixup = + fixup.rightmost_subexpression_fixup(true, false, Precedence::Jump); match scan_right(e, right_fixup, false, 1, 1) { Scan::Fail => Scan::Bailout, Scan::Bailout | Scan::Consume => Scan::Consume, @@ -637,7 +678,8 @@ fn scan_right( if bailout_offset >= 1 { return Scan::Consume; } - let right_fixup = fixup.rightmost_subexpression_fixup(Precedence::Jump); + let right_fixup = + fixup.rightmost_subexpression_fixup(false, false, Precedence::Jump); match scan_right(&e.body, right_fixup, false, 1, 1) { Scan::Fail => Scan::Bailout, Scan::Bailout | Scan::Consume => Scan::Consume, diff --git a/tests/test_expr.rs b/tests/test_expr.rs index edee0bdc91..cfbf16236f 100644 --- a/tests/test_expr.rs +++ b/tests/test_expr.rs @@ -802,25 +802,26 @@ fn test_fixup() { quote! { (&mut fut).await }, quote! { &mut (x as i32) }, quote! { -(x as i32) }, - quote! { if (S {} == 1) {} }, + quote! { if (S {}) == 1 {} }, quote! { { (m! {}) - 1 } }, quote! { match m { _ => ({}) - 1 } }, quote! { if let _ = (a && b) && c {} }, quote! { if let _ = (S {}) {} }, + quote! { if (S {}) == 0 && let Some(_) = x {} }, 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! { if (break break) {} }, + quote! { if break (break) {} }, quote! { if break break {} {} }, - quote! { if (return ..) {} }, + quote! { if return (..) {} }, quote! { if return .. {} {} }, - quote! { if (|| Struct {}) {} }, - quote! { if (|| Struct {}.await) {} }, + quote! { if || (Struct {}) {} }, + quote! { if || (Struct {}).await {} }, quote! { if break || Struct {}.await {} }, quote! { if break 'outer 'block: {} {} }, quote! { if ..'block: {} {} }, - quote! { if (break {}.await) {} }, + quote! { if break ({}).await {} }, quote! { (break)() }, quote! { (..) = () }, quote! { (..) += () },