Auto merge of #7095 - Y-Nak:match_single_binding, r=giraffate

match_single_binding: Fix invalid suggestion when match scrutinee has side effects fixes #7094 changelog: `match_single_binding`: Fix invalid suggestion when match scrutinee has side effects --- `Expr::can_have_side_effects` is used to determine the scrutinee has side effects, while this method is a little bit conservative for our use case. But I'd like to use it to avoid reimplementation of the method and too much heuristics. If you think this is problematic, then I'll implement a custom visitor to address it.
rust-lang · May 13, 2021 · 1fd9975 · 1fd9975
2 parents 90fb7c4 + 8214bf0
commit 1fd9975
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 35 deletions.
diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs
@@ -1478,15 +1478,34 @@ fn check_match_single_binding<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[A
             );
         },
         PatKind::Wild => {
-            span_lint_and_sugg(
-                cx,
-                MATCH_SINGLE_BINDING,
-                expr.span,
-                "this match could be replaced by its body itself",
-                "consider using the match body instead",
-                snippet_body,
-                Applicability::MachineApplicable,
-            );
+            if ex.can_have_side_effects() {
+                let indent = " ".repeat(indent_of(cx, expr.span).unwrap_or(0));
+                let sugg = format!(
+                    "{};\n{}{}",
+                    snippet_with_applicability(cx, ex.span, "..", &mut applicability),
+                    indent,
+                    snippet_body
+                );
+                span_lint_and_sugg(
+                    cx,
+                    MATCH_SINGLE_BINDING,
+                    expr.span,
+                    "this match could be replaced by its scrutinee and body",
+                    "consider using the scrutinee and body instead",
+                    sugg,
+                    applicability,
+                )
+            } else {
+                span_lint_and_sugg(
+                    cx,
+                    MATCH_SINGLE_BINDING,
+                    expr.span,
+                    "this match could be replaced by its body itself",
+                    "consider using the match body instead",
+                    snippet_body,
+                    Applicability::MachineApplicable,
+                );
+            }
         },
         _ => (),
     }

diff --git a/tests/ui/match_single_binding.fixed b/tests/ui/match_single_binding.fixed
@@ -94,10 +94,7 @@ fn main() {
         0 => println!("Disabled branch"),
         _ => println!("Enabled branch"),
     }
-    // Lint
-    let x = 1;
-    let y = 1;
-    println!("Single branch");
+
     // Ok
     let x = 1;
     let y = 1;

diff --git a/tests/ui/match_single_binding.rs b/tests/ui/match_single_binding.rs
@@ -106,15 +106,7 @@ fn main() {
         0 => println!("Disabled branch"),
         _ => println!("Enabled branch"),
     }
-    // Lint
-    let x = 1;
-    let y = 1;
-    match match y {
-        0 => 1,
-        _ => 2,
-    } {
-        _ => println!("Single branch"),
-    }
+
     // Ok
     let x = 1;
     let y = 1;

diff --git a/tests/ui/match_single_binding.stderr b/tests/ui/match_single_binding.stderr
@@ -167,16 +167,5 @@ LL |             unwrapped
 LL |         })
    |
 
-error: this match could be replaced by its body itself
-  --> $DIR/match_single_binding.rs:112:5
-   |
-LL | /     match match y {
-LL | |         0 => 1,
-LL | |         _ => 2,
-LL | |     } {
-LL | |         _ => println!("Single branch"),
-LL | |     }
-   | |_____^ help: consider using the match body instead: `println!("Single branch");`
-
-error: aborting due to 12 previous errors
+error: aborting due to 11 previous errors
 
diff --git a/tests/ui/match_single_binding2.fixed b/tests/ui/match_single_binding2.fixed
@@ -34,4 +34,20 @@ fn main() {
         },
         None => println!("nothing"),
     }
+
+    fn side_effects() {}
+
+    // Lint (scrutinee has side effects)
+    // issue #7094
+    side_effects();
+    println!("Side effects");
+
+    // Lint (scrutinee has side effects)
+    // issue #7094
+    let x = 1;
+    match x {
+        0 => 1,
+        _ => 2,
+    };
+    println!("Single branch");
 }
diff --git a/tests/ui/match_single_binding2.rs b/tests/ui/match_single_binding2.rs
@@ -34,4 +34,22 @@ fn main() {
         },
         None => println!("nothing"),
     }
+
+    fn side_effects() {}
+
+    // Lint (scrutinee has side effects)
+    // issue #7094
+    match side_effects() {
+        _ => println!("Side effects"),
+    }
+
+    // Lint (scrutinee has side effects)
+    // issue #7094
+    let x = 1;
+    match match x {
+        0 => 1,
+        _ => 2,
+    } {
+        _ => println!("Single branch"),
+    }
 }
diff --git a/tests/ui/match_single_binding2.stderr b/tests/ui/match_single_binding2.stderr
@@ -30,5 +30,39 @@ LL |             let (a, b) = get_tup();
 LL |             println!("a {:?} and b {:?}", a, b);
    |
 
-error: aborting due to 2 previous errors
+error: this match could be replaced by its scrutinee and body
+  --> $DIR/match_single_binding2.rs:42:5
+   |
+LL | /     match side_effects() {
+LL | |         _ => println!("Side effects"),
+LL | |     }
+   | |_____^
+   |
+help: consider using the scrutinee and body instead
+   |
+LL |     side_effects();
+LL |     println!("Side effects");
+   |
+
+error: this match could be replaced by its scrutinee and body
+  --> $DIR/match_single_binding2.rs:49:5
+   |
+LL | /     match match x {
+LL | |         0 => 1,
+LL | |         _ => 2,
+LL | |     } {
+LL | |         _ => println!("Single branch"),
+LL | |     }
+   | |_____^
+   |
+help: consider using the scrutinee and body instead
+   |
+LL |     match x {
+LL |         0 => 1,
+LL |         _ => 2,
+LL |     };
+LL |     println!("Single branch");
+   |
+
+error: aborting due to 4 previous errors