Auto merge of rust-lang#8985 - botahamec:single-match-option, r=llogiq

Lint `[single_match]` on `Option` matches

fixes rust-lang#8928

changelog: did some cleanup of the logic for ``[`single_match`]`` and ``[`single_match_else`]`` which fixes the bug where `Option` matches were not linted unless a wildcard was used for one of the arms.

Author: bors

clippy_lints/src/matches/single_match.rs

@@ -140,70 +140,45 @@ fn check_opt_like<'a>(
     ty: Ty<'a>,
     els: Option<&Expr<'_>>,
 ) {
-    // list of candidate `Enum`s we know will never get any more members
-    let candidates = &[
-        (&paths::COW, "Borrowed"),
-        (&paths::COW, "Cow::Borrowed"),
-        (&paths::COW, "Cow::Owned"),
-        (&paths::COW, "Owned"),
-        (&paths::OPTION, "None"),
-        (&paths::RESULT, "Err"),
-        (&paths::RESULT, "Ok"),
-    ];
-
-    // We want to suggest to exclude an arm that contains only wildcards or forms the exhaustive
-    // match with the second branch, without enum variants in matches.
-    if !contains_only_wilds(arms[1].pat) && !form_exhaustive_matches(arms[0].pat, arms[1].pat) {
-        return;
+    // We don't want to lint if the second arm contains an enum which could
+    // have more variants in the future.
+    if form_exhaustive_matches(cx, ty, arms[0].pat, arms[1].pat) {
+        report_single_pattern(cx, ex, arms, expr, els);
     }
+}
 
+/// Returns `true` if all of the types in the pattern are enums which we know
+/// won't be expanded in the future
+fn pat_in_candidate_enum<'a>(cx: &LateContext<'a>, ty: Ty<'a>, pat: &Pat<'_>) -> bool {
     let mut paths_and_types = Vec::new();
-    if !collect_pat_paths(&mut paths_and_types, cx, arms[1].pat, ty) {
-        return;
-    }
+    collect_pat_paths(&mut paths_and_types, cx, pat, ty);
+    paths_and_types.iter().all(|ty| in_candidate_enum(cx, *ty))
+}
 
-    let in_candidate_enum = |path_info: &(String, Ty<'_>)| -> bool {
-        let (path, ty) = path_info;
-        for &(ty_path, pat_path) in candidates {
-            if path == pat_path && match_type(cx, *ty, ty_path) {
-                return true;
-            }
+/// Returns `true` if the given type is an enum we know won't be expanded in the future
+fn in_candidate_enum<'a>(cx: &LateContext<'a>, ty: Ty<'_>) -> bool {
+    // list of candidate `Enum`s we know will never get any more members
+    let candidates = [&paths::COW, &paths::OPTION, &paths::RESULT];
+
+    for candidate_ty in candidates {
+        if match_type(cx, ty, candidate_ty) {
+            return true;
         }
-        false
-    };
-    if paths_and_types.iter().all(in_candidate_enum) {
-        report_single_pattern(cx, ex, arms, expr, els);
     }
+    false
 }
 
-/// Collects paths and their types from the given patterns. Returns true if the given pattern could
-/// be simplified, false otherwise.
-fn collect_pat_paths<'a>(acc: &mut Vec<(String, Ty<'a>)>, cx: &LateContext<'a>, pat: &Pat<'_>, ty: Ty<'a>) -> bool {
+/// Collects types from the given pattern
+fn collect_pat_paths<'a>(acc: &mut Vec<Ty<'a>>, cx: &LateContext<'a>, pat: &Pat<'_>, ty: Ty<'a>) {
     match pat.kind {
-        PatKind::Wild => true,
-        PatKind::Tuple(inner, _) => inner.iter().all(|p| {
+        PatKind::Tuple(inner, _) => inner.iter().for_each(|p| {
             let p_ty = cx.typeck_results().pat_ty(p);
-            collect_pat_paths(acc, cx, p, p_ty)
+            collect_pat_paths(acc, cx, p, p_ty);
         }),
-        PatKind::TupleStruct(ref path, ..) => {
-            let path = rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| {
-                s.print_qpath(path, false);
-            });
-            acc.push((path, ty));
-            true
-        },
-        PatKind::Binding(BindingAnnotation::Unannotated, .., ident, None) => {
-            acc.push((ident.to_string(), ty));
-            true
+        PatKind::TupleStruct(..) | PatKind::Binding(BindingAnnotation::Unannotated, .., None) | PatKind::Path(_) => {
+            acc.push(ty);
         },
-        PatKind::Path(ref path) => {
-            let path = rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| {
-                s.print_qpath(path, false);
-            });
-            acc.push((path, ty));
-            true
-        },
-        _ => false,
+        _ => {},
     }
 }
 
@@ -218,7 +193,7 @@ fn contains_only_wilds(pat: &Pat<'_>) -> bool {
 
 /// Returns true if the given patterns forms only exhaustive matches that don't contain enum
 /// patterns without a wildcard.
-fn form_exhaustive_matches(left: &Pat<'_>, right: &Pat<'_>) -> bool {
+fn form_exhaustive_matches<'a>(cx: &LateContext<'a>, ty: Ty<'a>, left: &Pat<'_>, right: &Pat<'_>) -> bool {
     match (&left.kind, &right.kind) {
         (PatKind::Wild, _) | (_, PatKind::Wild) => true,
         (PatKind::Tuple(left_in, left_pos), PatKind::Tuple(right_in, right_pos)) => {
@@ -264,6 +239,10 @@ fn form_exhaustive_matches(left: &Pat<'_>, right: &Pat<'_>) -> bool {
             }
             true
         },
+        (PatKind::TupleStruct(..), PatKind::Path(_)) => pat_in_candidate_enum(cx, ty, right),
+        (PatKind::TupleStruct(..), PatKind::TupleStruct(_, inner, _)) => {
+            pat_in_candidate_enum(cx, ty, right) && inner.iter().all(contains_only_wilds)
+        },
         _ => false,
     }
 }

tests/ui/single_match.stderr

@@ -38,6 +38,15 @@ LL | |         _ => {},
 LL | |     };
    | |_____^ help: try this: `if let (2..=3, 7..=9) = z { dummy() }`
 
+error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
+  --> $DIR/single_match.rs:54:5
+   |
+LL | /     match x {
+LL | |         Some(y) => dummy(),
+LL | |         None => (),
+LL | |     };
+   | |_____^ help: try this: `if let Some(y) = x { dummy() }`
+
 error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
   --> $DIR/single_match.rs:59:5
    |
@@ -146,5 +155,5 @@ LL | |         (..) => {},
 LL | |     }
    | |_____^ help: try this: `if let (.., Some(E::V), _) = (Some(42), Some(E::V), Some(42)) {}`
 
-error: aborting due to 15 previous errors
+error: aborting due to 16 previous errors
 

tests/ui/single_match_else.rs

@@ -97,4 +97,23 @@ fn main() {
             return;
         },
     }
+
+    // lint here
+    use std::convert::Infallible;
+    match Result::<i32, Infallible>::Ok(1) {
+        Ok(a) => println!("${:?}", a),
+        Err(_) => {
+            println!("else block");
+            return;
+        }
+    }
+
+    use std::borrow::Cow;
+    match Cow::from("moo") {
+        Cow::Owned(a) => println!("${:?}", a),
+        Cow::Borrowed(_) => {
+            println!("else block");
+            return;
+        }
+    }
 }

tests/ui/single_match_else.stderr

@@ -20,5 +20,85 @@ LL +         None
 LL ~     };
    |
 
-error: aborting due to previous error
+error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
+  --> $DIR/single_match_else.rs:84:5
+   |
+LL | /     match Some(1) {
+LL | |         Some(a) => println!("${:?}", a),
+LL | |         None => {
+LL | |             println!("else block");
+LL | |             return
+LL | |         },
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL ~     if let Some(a) = Some(1) { println!("${:?}", a) } else {
+LL +         println!("else block");
+LL +         return
+LL +     }
+   |
+
+error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
+  --> $DIR/single_match_else.rs:93:5
+   |
+LL | /     match Some(1) {
+LL | |         Some(a) => println!("${:?}", a),
+LL | |         None => {
+LL | |             println!("else block");
+LL | |             return;
+LL | |         },
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL ~     if let Some(a) = Some(1) { println!("${:?}", a) } else {
+LL +         println!("else block");
+LL +         return;
+LL +     }
+   |
+
+error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
+  --> $DIR/single_match_else.rs:103:5
+   |
+LL | /     match Result::<i32, Infallible>::Ok(1) {
+LL | |         Ok(a) => println!("${:?}", a),
+LL | |         Err(_) => {
+LL | |             println!("else block");
+LL | |             return;
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL ~     if let Ok(a) = Result::<i32, Infallible>::Ok(1) { println!("${:?}", a) } else {
+LL +         println!("else block");
+LL +         return;
+LL +     }
+   |
+
+error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
+  --> $DIR/single_match_else.rs:112:5
+   |
+LL | /     match Cow::from("moo") {
+LL | |         Cow::Owned(a) => println!("${:?}", a),
+LL | |         Cow::Borrowed(_) => {
+LL | |             println!("else block");
+LL | |             return;
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL ~     if let Cow::Owned(a) = Cow::from("moo") { println!("${:?}", a) } else {
+LL +         println!("else block");
+LL +         return;
+LL +     }
+   |
+
+error: aborting due to 5 previous errors