suggest adding parentheses when linting [let_and_return] and [needless_return]

J-ZhengLi · J-ZhengLi · commit 03306b6ab61b · 2024-05-27T11:49:10.000+08:00
diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs
@@ -2,7 +2,9 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
 use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
 use clippy_utils::sugg::has_enclosing_paren;
 use clippy_utils::ty::{implements_trait, is_manually_drop, peel_mid_ty_refs};
-use clippy_utils::{expr_use_ctxt, get_parent_expr, is_lint_allowed, path_to_local, DefinedTy, ExprUseNode};
+use clippy_utils::{
+    expr_use_ctxt, get_parent_expr, is_block_like, is_lint_allowed, path_to_local, DefinedTy, ExprUseNode,
+};
 use core::mem;
 use rustc_ast::util::parser::{PREC_POSTFIX, PREC_PREFIX};
 use rustc_data_structures::fx::FxIndexMap;
@@ -1038,14 +1040,8 @@ fn report<'tcx>(
             );
         },
         State::ExplicitDeref { mutability } => {
-            if matches!(
-                expr.kind,
-                ExprKind::Block(..)
-                    | ExprKind::ConstBlock(_)
-                    | ExprKind::If(..)
-                    | ExprKind::Loop(..)
-                    | ExprKind::Match(..)
-            ) && let ty::Ref(_, ty, _) = data.adjusted_ty.kind()
+            if is_block_like(expr)
+                && let ty::Ref(_, ty, _) = data.adjusted_ty.kind()
                 && ty.is_sized(cx.tcx, cx.param_env)
             {
                 // Rustc bug: auto deref doesn't work on block expression when targeting sized types.
diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs
@@ -6,8 +6,8 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg};
 use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::sugg::Sugg;
 use clippy_utils::{
-    higher, is_else_clause, is_expn_of, is_parent_stmt, peel_blocks, peel_blocks_with_stmt, span_extract_comment,
-    SpanlessEq,
+    higher, is_block_like, is_else_clause, is_expn_of, is_parent_stmt, peel_blocks, peel_blocks_with_stmt,
+    span_extract_comment, SpanlessEq,
 };
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
@@ -121,14 +121,7 @@ fn condition_needs_parentheses(e: &Expr<'_>) -> bool {
     | ExprKind::Type(i, _)
     | ExprKind::Index(i, _, _) = inner.kind
     {
-        if matches!(
-            i.kind,
-            ExprKind::Block(..)
-                | ExprKind::ConstBlock(..)
-                | ExprKind::If(..)
-                | ExprKind::Loop(..)
-                | ExprKind::Match(..)
-        ) {
+        if is_block_like(i) {
             return true;
         }
         inner = i;
diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs
@@ -3,8 +3,8 @@ use clippy_utils::source::{snippet_opt, snippet_with_context};
 use clippy_utils::sugg::has_enclosing_paren;
 use clippy_utils::visitors::{for_each_expr_with_closures, Descend};
 use clippy_utils::{
-    fn_def_id, is_from_proc_macro, is_inside_let_else, is_res_lang_ctor, path_res, path_to_local_id, span_contains_cfg,
-    span_find_starting_semi,
+    binary_expr_needs_parentheses, fn_def_id, is_from_proc_macro, is_inside_let_else, is_res_lang_ctor, path_res,
+    path_to_local_id, span_contains_cfg, span_find_starting_semi,
 };
 use core::ops::ControlFlow;
 use rustc_errors::Applicability;
@@ -129,7 +129,7 @@ enum RetReplacement<'tcx> {
     Empty,
     Block,
     Unit,
-    IfSequence(Cow<'tcx, str>, Applicability),
+    NeedsPar(Cow<'tcx, str>, Applicability),
     Expr(Cow<'tcx, str>, Applicability),
 }
 
@@ -139,13 +139,13 @@ impl<'tcx> RetReplacement<'tcx> {
             Self::Empty | Self::Expr(..) => "remove `return`",
             Self::Block => "replace `return` with an empty block",
             Self::Unit => "replace `return` with a unit value",
-            Self::IfSequence(..) => "remove `return` and wrap the sequence with parentheses",
+            Self::NeedsPar(..) => "remove `return` and wrap the sequence with parentheses",
         }
     }
 
     fn applicability(&self) -> Applicability {
         match self {
-            Self::Expr(_, ap) | Self::IfSequence(_, ap) => *ap,
+            Self::Expr(_, ap) | Self::NeedsPar(_, ap) => *ap,
             _ => Applicability::MachineApplicable,
         }
     }
@@ -157,7 +157,7 @@ impl<'tcx> Display for RetReplacement<'tcx> {
             Self::Empty => write!(f, ""),
             Self::Block => write!(f, "{{}}"),
             Self::Unit => write!(f, "()"),
-            Self::IfSequence(inner, _) => write!(f, "({inner})"),
+            Self::NeedsPar(inner, _) => write!(f, "({inner})"),
             Self::Expr(inner, _) => write!(f, "{inner}"),
         }
     }
@@ -244,7 +244,11 @@ impl<'tcx> LateLintPass<'tcx> for Return {
                     err.span_label(local.span, "unnecessary `let` binding");
 
                     if let Some(mut snippet) = snippet_opt(cx, initexpr.span) {
-                        if !cx.typeck_results().expr_adjustments(retexpr).is_empty() {
+                        if binary_expr_needs_parentheses(initexpr) {
+                            if !has_enclosing_paren(&snippet) {
+                                snippet = format!("({snippet})");
+                            }
+                        } else if !cx.typeck_results().expr_adjustments(retexpr).is_empty() {
                             if !has_enclosing_paren(&snippet) {
                                 snippet = format!("({snippet})");
                             }
@@ -349,8 +353,8 @@ fn check_final_expr<'tcx>(
 
                 let mut applicability = Applicability::MachineApplicable;
                 let (snippet, _) = snippet_with_context(cx, inner_expr.span, ret_span.ctxt(), "..", &mut applicability);
-                if expr_contains_conjunctive_ifs(inner_expr) {
-                    RetReplacement::IfSequence(snippet, applicability)
+                if binary_expr_needs_parentheses(inner_expr) {
+                    RetReplacement::NeedsPar(snippet, applicability)
                 } else {
                     RetReplacement::Expr(snippet, applicability)
                 }
@@ -404,18 +408,6 @@ fn check_final_expr<'tcx>(
     }
 }
 
-fn expr_contains_conjunctive_ifs<'tcx>(expr: &'tcx Expr<'tcx>) -> bool {
-    fn contains_if(expr: &Expr<'_>, on_if: bool) -> bool {
-        match expr.kind {
-            ExprKind::If(..) => on_if,
-            ExprKind::Binary(_, left, right) => contains_if(left, true) || contains_if(right, true),
-            _ => false,
-        }
-    }
-
-    contains_if(expr, false)
-}
-
 fn emit_return_lint(
     cx: &LateContext<'_>,
     ret_span: Span,
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
@@ -3370,3 +3370,25 @@ pub fn is_parent_stmt(cx: &LateContext<'_>, id: HirId) -> bool {
         Node::Stmt(..) | Node::Block(Block { stmts: &[], .. })
     )
 }
+
+/// Returns true if the given `expr` is a block or resembled as a block,
+/// such as `if`, `loop`, `match` expressions etc.
+pub fn is_block_like(expr: &Expr<'_>) -> bool {
+    matches!(
+        expr.kind,
+        ExprKind::Block(..) | ExprKind::ConstBlock(..) | ExprKind::If(..) | ExprKind::Loop(..) | ExprKind::Match(..)
+    )
+}
+
+/// Returns true if the given `expr` is binary expression that needs to be wrapped in parentheses.
+pub fn binary_expr_needs_parentheses(expr: &Expr<'_>) -> bool {
+    fn contains_block(expr: &Expr<'_>, is_operand: bool) -> bool {
+        match expr.kind {
+            ExprKind::Binary(_, lhs, _) => contains_block(lhs, true),
+            _ if is_block_like(expr) => is_operand,
+            _ => false,
+        }
+    }
+
+    contains_block(expr, false)
+}
diff --git a/tests/ui/let_and_return.fixed b/tests/ui/let_and_return.fixed
@@ -210,4 +210,38 @@ fn issue9150() -> usize {
     x
 }
 
+fn issue12801() {
+    fn left_is_if() -> String {
+        
+        (if true { "a".to_string() } else { "b".to_string() } + "c")
+        //~^ ERROR: returning the result of a `let` binding from a block
+    }
+
+    fn no_par_needed() -> String {
+        
+        "c".to_string() + if true { "a" } else { "b" }
+        //~^ ERROR: returning the result of a `let` binding from a block
+    }
+
+    fn conjunctive_blocks() -> String {
+        
+        ({ "a".to_string() } + "b" + { "c" } + "d")
+        //~^ ERROR: returning the result of a `let` binding from a block
+    }
+
+    #[allow(clippy::overly_complex_bool_expr)]
+    fn other_ops() {
+        let _ = || {
+            
+            (if true { 2 } else { 3 } << 4)
+            //~^ ERROR: returning the result of a `let` binding from a block
+        };
+        let _ = || {
+            
+            ({ true } || { false } && { 2 <= 3 })
+            //~^ ERROR: returning the result of a `let` binding from a block
+        };
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/let_and_return.rs b/tests/ui/let_and_return.rs
@@ -210,4 +210,38 @@ fn issue9150() -> usize {
     x
 }
 
+fn issue12801() {
+    fn left_is_if() -> String {
+        let s = if true { "a".to_string() } else { "b".to_string() } + "c";
+        s
+        //~^ ERROR: returning the result of a `let` binding from a block
+    }
+
+    fn no_par_needed() -> String {
+        let s = "c".to_string() + if true { "a" } else { "b" };
+        s
+        //~^ ERROR: returning the result of a `let` binding from a block
+    }
+
+    fn conjunctive_blocks() -> String {
+        let s = { "a".to_string() } + "b" + { "c" } + "d";
+        s
+        //~^ ERROR: returning the result of a `let` binding from a block
+    }
+
+    #[allow(clippy::overly_complex_bool_expr)]
+    fn other_ops() {
+        let _ = || {
+            let s = if true { 2 } else { 3 } << 4;
+            s
+            //~^ ERROR: returning the result of a `let` binding from a block
+        };
+        let _ = || {
+            let s = { true } || { false } && { 2 <= 3 };
+            s
+            //~^ ERROR: returning the result of a `let` binding from a block
+        };
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/let_and_return.stderr b/tests/ui/let_and_return.stderr
@@ -78,5 +78,75 @@ LL +                 E::B(x) => x,
 LL +             }) as _
    |
 
-error: aborting due to 5 previous errors
+error: returning the result of a `let` binding from a block
+  --> tests/ui/let_and_return.rs:216:9
+   |
+LL |         let s = if true { "a".to_string() } else { "b".to_string() } + "c";
+   |         ------------------------------------------------------------------- unnecessary `let` binding
+LL |         s
+   |         ^
+   |
+help: return the expression directly
+   |
+LL ~         
+LL ~         (if true { "a".to_string() } else { "b".to_string() } + "c")
+   |
+
+error: returning the result of a `let` binding from a block
+  --> tests/ui/let_and_return.rs:222:9
+   |
+LL |         let s = "c".to_string() + if true { "a" } else { "b" };
+   |         ------------------------------------------------------- unnecessary `let` binding
+LL |         s
+   |         ^
+   |
+help: return the expression directly
+   |
+LL ~         
+LL ~         "c".to_string() + if true { "a" } else { "b" }
+   |
+
+error: returning the result of a `let` binding from a block
+  --> tests/ui/let_and_return.rs:228:9
+   |
+LL |         let s = { "a".to_string() } + "b" + { "c" } + "d";
+   |         -------------------------------------------------- unnecessary `let` binding
+LL |         s
+   |         ^
+   |
+help: return the expression directly
+   |
+LL ~         
+LL ~         ({ "a".to_string() } + "b" + { "c" } + "d")
+   |
+
+error: returning the result of a `let` binding from a block
+  --> tests/ui/let_and_return.rs:236:13
+   |
+LL |             let s = if true { 2 } else { 3 } << 4;
+   |             -------------------------------------- unnecessary `let` binding
+LL |             s
+   |             ^
+   |
+help: return the expression directly
+   |
+LL ~             
+LL ~             (if true { 2 } else { 3 } << 4)
+   |
+
+error: returning the result of a `let` binding from a block
+  --> tests/ui/let_and_return.rs:241:13
+   |
+LL |             let s = { true } || { false } && { 2 <= 3 };
+   |             -------------------------------------------- unnecessary `let` binding
+LL |             s
+   |             ^
+   |
+help: return the expression directly
+   |
+LL ~             
+LL ~             ({ true } || { false } && { 2 <= 3 })
+   |
+
+error: aborting due to 10 previous errors
 
diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed
@@ -323,4 +323,8 @@ fn allow_works() -> i32 {
     }
 }
 
+fn conjunctive_blocks() -> String {
+    ({ "a".to_string() } + "b" + { "c" })
+}
+
 fn main() {}
diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs
@@ -333,4 +333,8 @@ fn allow_works() -> i32 {
     }
 }
 
+fn conjunctive_blocks() -> String {
+    return { "a".to_string() } + "b" + { "c" };
+}
+
 fn main() {}
diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr
@@ -653,5 +653,17 @@ LL -         return if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4
 LL +         (if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 })
    |
 
-error: aborting due to 52 previous errors
+error: unneeded `return` statement
+  --> tests/ui/needless_return.rs:337:5
+   |
+LL |     return { "a".to_string() } + "b" + { "c" };
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: remove `return` and wrap the sequence with parentheses
+   |
+LL -     return { "a".to_string() } + "b" + { "c" };
+LL +     ({ "a".to_string() } + "b" + { "c" })
+   |
+
+error: aborting due to 53 previous errors
 

Original file line number	Diff line number	Diff line change
`@@ -323,4 +323,8 @@ fn allow_works() -> i32 {`
`323`	`323`	`}`
`324`	`324`	`}`
`325`	`325`
	`326`	`+fn conjunctive_blocks() -> String {`
	`327`	`+ ({ "a".to_string() } + "b" + { "c" })`
	`328`	`+}`
	`329`	`+`
`326`	`330`	`fn main() {}`
Original file line number	Diff line number	Diff line change
`@@ -333,4 +333,8 @@ fn allow_works() -> i32 {`
`333`	`333`	`}`
`334`	`334`	`}`
`335`	`335`
	`336`	`+fn conjunctive_blocks() -> String {`
	`337`	`+ return { "a".to_string() } + "b" + { "c" };`
	`338`	`+}`
	`339`	`+`
`336`	`340`	`fn main() {}`