short_circuit_statement: handle macros and parenthesis better (rust-lang#14047)

y21 · web-flow · commit 88d83b6e55ff · 2025-01-22T02:54:49.000Z
- The lint no longer triggers if one of the operands in the boolean
expression comes from a macro expansion.
- Parenthesis are now removed inside the generated block if they are no
longer necessary.
- Error markers have been added.

changelog: [`short_circuit_statement`]: better handling of macros and
better looking suggestions
diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs
@@ -216,9 +216,10 @@ impl<'tcx> LateLintPass<'tcx> for LintPass {
             );
         }
         if let StmtKind::Semi(expr) = stmt.kind
-            && let ExprKind::Binary(ref binop, a, b) = expr.kind
-            && (binop.node == BinOpKind::And || binop.node == BinOpKind::Or)
-            && let Some(sugg) = Sugg::hir_opt(cx, a)
+            && let ExprKind::Binary(binop, a, b) = &expr.kind
+            && matches!(binop.node, BinOpKind::And | BinOpKind::Or)
+            && !stmt.span.from_expansion()
+            && expr.span.eq_ctxt(stmt.span)
         {
             span_lint_hir_and_then(
                 cx,
@@ -227,13 +228,11 @@ impl<'tcx> LateLintPass<'tcx> for LintPass {
                 stmt.span,
                 "boolean short circuit operator in statement may be clearer using an explicit test",
                 |diag| {
-                    let sugg = if binop.node == BinOpKind::Or { !sugg } else { sugg };
-                    diag.span_suggestion(
-                        stmt.span,
-                        "replace it with",
-                        format!("if {sugg} {{ {}; }}", &snippet(cx, b.span, ".."),),
-                        Applicability::MachineApplicable, // snippet
-                    );
+                    let mut app = Applicability::MachineApplicable;
+                    let test = Sugg::hir_with_context(cx, a, expr.span.ctxt(), "_", &mut app);
+                    let test = if binop.node == BinOpKind::Or { !test } else { test };
+                    let then = Sugg::hir_with_context(cx, b, expr.span.ctxt(), "_", &mut app);
+                    diag.span_suggestion(stmt.span, "replace it with", format!("if {test} {{ {then}; }}"), app);
                 },
             );
         }
diff --git a/tests/ui/short_circuit_statement.fixed b/tests/ui/short_circuit_statement.fixed
@@ -3,8 +3,35 @@
 
 fn main() {
     if f() { g(); }
+    //~^ ERROR: boolean short circuit operator in statement
     if !f() { g(); }
+    //~^ ERROR: boolean short circuit operator in statement
     if 1 != 2 { g(); }
+    //~^ ERROR: boolean short circuit operator in statement
+    if f() || g() { H * 2; }
+    //~^ ERROR: boolean short circuit operator in statement
+    if !(f() || g()) { H * 2; }
+    //~^ ERROR: boolean short circuit operator in statement
+
+    macro_rules! mac {
+        ($f:ident or $g:ident) => {
+            $f() || $g()
+        };
+        ($f:ident and $g:ident) => {
+            $f() && $g()
+        };
+        () => {
+            f() && g()
+        };
+    }
+
+    if mac!() { mac!(); }
+    //~^ ERROR: boolean short circuit operator in statement
+    if !mac!() { mac!(); }
+    //~^ ERROR: boolean short circuit operator in statement
+
+    // Do not lint if the expression comes from a macro
+    mac!();
 }
 
 fn f() -> bool {
@@ -14,3 +41,12 @@ fn f() -> bool {
 fn g() -> bool {
     false
 }
+
+struct H;
+
+impl std::ops::Mul<u32> for H {
+    type Output = bool;
+    fn mul(self, other: u32) -> Self::Output {
+        true
+    }
+}
diff --git a/tests/ui/short_circuit_statement.rs b/tests/ui/short_circuit_statement.rs
@@ -3,8 +3,35 @@
 
 fn main() {
     f() && g();
+    //~^ ERROR: boolean short circuit operator in statement
     f() || g();
+    //~^ ERROR: boolean short circuit operator in statement
     1 == 2 || g();
+    //~^ ERROR: boolean short circuit operator in statement
+    (f() || g()) && (H * 2);
+    //~^ ERROR: boolean short circuit operator in statement
+    (f() || g()) || (H * 2);
+    //~^ ERROR: boolean short circuit operator in statement
+
+    macro_rules! mac {
+        ($f:ident or $g:ident) => {
+            $f() || $g()
+        };
+        ($f:ident and $g:ident) => {
+            $f() && $g()
+        };
+        () => {
+            f() && g()
+        };
+    }
+
+    mac!() && mac!();
+    //~^ ERROR: boolean short circuit operator in statement
+    mac!() || mac!();
+    //~^ ERROR: boolean short circuit operator in statement
+
+    // Do not lint if the expression comes from a macro
+    mac!();
 }
 
 fn f() -> bool {
@@ -14,3 +41,12 @@ fn f() -> bool {
 fn g() -> bool {
     false
 }
+
+struct H;
+
+impl std::ops::Mul<u32> for H {
+    type Output = bool;
+    fn mul(self, other: u32) -> Self::Output {
+        true
+    }
+}
diff --git a/tests/ui/short_circuit_statement.stderr b/tests/ui/short_circuit_statement.stderr
@@ -8,16 +8,40 @@ LL |     f() && g();
    = help: to override `-D warnings` add `#[allow(clippy::short_circuit_statement)]`
 
 error: boolean short circuit operator in statement may be clearer using an explicit test
-  --> tests/ui/short_circuit_statement.rs:6:5
+  --> tests/ui/short_circuit_statement.rs:7:5
    |
 LL |     f() || g();
    |     ^^^^^^^^^^^ help: replace it with: `if !f() { g(); }`
 
 error: boolean short circuit operator in statement may be clearer using an explicit test
-  --> tests/ui/short_circuit_statement.rs:7:5
+  --> tests/ui/short_circuit_statement.rs:9:5
    |
 LL |     1 == 2 || g();
    |     ^^^^^^^^^^^^^^ help: replace it with: `if 1 != 2 { g(); }`
 
-error: aborting due to 3 previous errors
+error: boolean short circuit operator in statement may be clearer using an explicit test
+  --> tests/ui/short_circuit_statement.rs:11:5
+   |
+LL |     (f() || g()) && (H * 2);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `if f() || g() { H * 2; }`
+
+error: boolean short circuit operator in statement may be clearer using an explicit test
+  --> tests/ui/short_circuit_statement.rs:13:5
+   |
+LL |     (f() || g()) || (H * 2);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `if !(f() || g()) { H * 2; }`
+
+error: boolean short circuit operator in statement may be clearer using an explicit test
+  --> tests/ui/short_circuit_statement.rs:28:5
+   |
+LL |     mac!() && mac!();
+   |     ^^^^^^^^^^^^^^^^^ help: replace it with: `if mac!() { mac!(); }`
+
+error: boolean short circuit operator in statement may be clearer using an explicit test
+  --> tests/ui/short_circuit_statement.rs:30:5
+   |
+LL |     mac!() || mac!();
+   |     ^^^^^^^^^^^^^^^^^ help: replace it with: `if !mac!() { mac!(); }`
+
+error: aborting due to 7 previous errors
 
