wildcard_enum_match_arm gives suggestions

And is also more robust

Author: illicitonion

clippy_lints/src/matches.rs

@@ -6,13 +6,15 @@ use crate::utils::{
     snippet_with_applicability, span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty,
 };
 use if_chain::if_chain;
+use rustc::hir::def::CtorKind;
 use rustc::hir::*;
 use rustc::lint::{in_external_macro, LateContext, LateLintPass, LintArray, LintContext, LintPass};
-use rustc::ty::{self, Ty};
+use rustc::ty::{self, Ty, TyKind};
 use rustc::{declare_tool_lint, lint_array};
 use rustc_errors::Applicability;
 use std::cmp::Ordering;
 use std::collections::Bound;
+use std::ops::Deref;
 use syntax::ast::LitKind;
 use syntax::source_map::Span;
 
@@ -191,7 +193,8 @@ declare_clippy_lint! {
 ///
 /// **Why is this bad?** New enum variants added by library updates can be missed.
 ///
-/// **Known problems:** Nested wildcards a la `Foo(_)` are currently not detected.
+/// **Known problems:** Suggested replacements may be incorrect if guards exhaustively cover some
+/// variants, and also may not use correct path to enum if it's not present in the current scope.
 ///
 /// **Example:**
 /// ```rust
@@ -464,19 +467,85 @@ fn check_wild_err_arm(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm]) {
 }
 
 fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm]) {
-    if cx.tables.expr_ty(ex).is_enum() {
+    let ty = cx.tables.expr_ty(ex);
+    if !ty.is_enum() {
+        // If there isn't a nice closed set of possible values that can be conveniently enumerated,
+        // don't complain about not enumerating the mall.
+        return;
+    }
+
+    // First pass - check for violation, but don't do much book-keeping because this is hopefully
+    // the uncommon case, and the book-keeping is slightly expensive.
+    let mut wildcard_span = None;
+    let mut wildcard_ident = None;
+    for arm in arms {
+        for pat in &arm.pats {
+            if let PatKind::Wild = pat.node {
+                wildcard_span = Some(pat.span);
+            } else if let PatKind::Binding(_, _, ident, None) = pat.node {
+                wildcard_span = Some(pat.span);
+                wildcard_ident = Some(ident);
+            }
+        }
+    }
+
+    if let Some(wildcard_span) = wildcard_span {
+        // Accumulate the variants which should be put in place of the wildcard because they're not
+        // already covered.
+
+        let mut missing_variants = vec![];
+        if let TyKind::Adt(def, _) = ty.sty {
+            for variant in &def.variants {
+                missing_variants.push(variant);
+            }
+        }
+
         for arm in arms {
-            if is_wild(&arm.pats[0]) {
-                span_note_and_lint(
-                    cx,
-                    WILDCARD_ENUM_MATCH_ARM,
-                    arm.pats[0].span,
-                    "wildcard match will miss any future added variants.",
-                    arm.pats[0].span,
-                    "to resolve, match each variant explicitly",
-                );
+            if arm.guard.is_some() {
+                // Guards mean that this case probably isn't exhaustively covered. Technically
+                // this is incorrect, as we should really check whether each variant is exhaustively
+                // covered by the set of guards that cover it, but that's really hard to do.
+                continue;
+            }
+            for pat in &arm.pats {
+                if let PatKind::Path(ref path) = pat.deref().node {
+                    if let QPath::Resolved(_, p) = path {
+                        missing_variants.retain(|e| e.did != p.def.def_id());
+                    }
+                } else if let PatKind::TupleStruct(ref path, ..) = pat.deref().node {
+                    if let QPath::Resolved(_, p) = path {
+                        missing_variants.retain(|e| e.did != p.def.def_id());
+                    }
+                }
             }
         }
+
+        let suggestion: Vec<String> = missing_variants
+            .iter()
+            .map(|v| {
+                let suffix = match v.ctor_kind {
+                    CtorKind::Fn => "(..)",
+                    CtorKind::Const | CtorKind::Fictive => "",
+                };
+                let ident_str = if let Some(ident) = wildcard_ident {
+                    format!("{} @ ", ident.name)
+                } else {
+                    String::new()
+                };
+                // This path assumes that the enum type is imported into scope.
+                format!("{}{}{}", ident_str, cx.tcx.item_path_str(v.did), suffix)
+            })
+            .collect();
+
+        span_lint_and_sugg(
+            cx,
+            WILDCARD_ENUM_MATCH_ARM,
+            wildcard_span,
+            "wildcard match will miss any future added variants.",
+            "try this",
+            suggestion.join(" | "),
+            Applicability::MachineApplicable,
+        )
     }
 }
 

tests/ui/wildcard_enum_match_arm.rs

@@ -1,42 +1,73 @@
-#![deny(clippy::wildcard_enum_match_arm)]
-
-#[derive(Clone, Copy, Debug, Eq, PartialEq)]
-enum Color {
-    Red,
-    Green,
-    Blue,
-    Rgb(u8, u8, u8),
-    Cyan,
-}
-
-impl Color {
-    fn is_monochrome(self) -> bool {
-        match self {
-            Color::Red | Color::Green | Color::Blue => true,
-            Color::Rgb(r, g, b) => r | g == 0 || r | b == 0 || g | b == 0,
-            Color::Cyan => false,
-        }
+#![warn(clippy::wildcard_enum_match_arm)]
+
+#[derive(Debug)]
+enum Maybe<T> {
+    Some(T),
+    Probably(T),
+    None,
+}
+
+fn is_it_wildcard<T>(m: Maybe<T>) -> &'static str {
+    match m {
+        Maybe::Some(_) => "Some",
+        _ => "Could be",
+    }
+}
+
+fn is_it_bound<T>(m: Maybe<T>) -> &'static str {
+    match m {
+        Maybe::None => "None",
+        _other => "Could be",
+    }
+}
+
+fn is_it_binding(m: Maybe<u32>) -> String {
+    match m {
+        Maybe::Some(v) => "Large".to_string(),
+        n => format!("{:?}", n),
+    }
+}
+
+fn is_it_binding_exhaustive(m: Maybe<u32>) -> String {
+    match m {
+        Maybe::Some(v) => "Large".to_string(),
+        n @ Maybe::Probably(_) | n @ Maybe::None => format!("{:?}", n),
+    }
+}
+
+fn is_it_with_guard(m: Maybe<u32>) -> &'static str {
+    match m {
+        Maybe::Some(v) if v > 100 => "Large",
+        _ => "Who knows",
+    }
+}
+
+fn is_it_exhaustive<T>(m: Maybe<T>) -> &'static str {
+    match m {
+        Maybe::None => "None",
+        Maybe::Some(_) | Maybe::Probably(..) => "Could be",
+    }
+}
+
+fn is_one_or_three(i: i32) -> bool {
+    match i {
+        1 | 3 => true,
+        _ => false,
     }
 }
 
 fn main() {
-    let color = Color::Rgb(0, 0, 127);
-    match color {
-        Color::Red => println!("Red"),
-        _ => eprintln!("Not red"),
-    };
-    match color {
-        Color::Red => {},
-        Color::Green => {},
-        Color::Blue => {},
-        Color::Cyan => {},
-        c if c.is_monochrome() => {},
-        Color::Rgb(_, _, _) => {},
-    };
-    let x: u8 = unimplemented!();
-    match x {
-        0 => {},
-        140 => {},
-        _ => {},
-    };
+    println!("{}", is_it_wildcard(Maybe::Some("foo")));
+
+    println!("{}", is_it_bound(Maybe::Some("foo")));
+
+    println!("{}", is_it_binding(Maybe::Some(1)));
+
+    println!("{}", is_it_binding_exhaustive(Maybe::Some(1)));
+
+    println!("{}", is_it_with_guard(Maybe::Some(1)));
+
+    println!("{}", is_it_exhaustive(Maybe::Some("foo")));
+
+    println!("{}", is_one_or_three(2));
 }

tests/ui/wildcard_enum_match_arm.stderr

@@ -1,15 +1,28 @@
 error: wildcard match will miss any future added variants.
-  --> $DIR/wildcard_enum_match_arm.rs:26:9
+  --> $DIR/wildcard_enum_match_arm.rs:13:9
    |
-LL |         _ => eprintln!("Not red"),
-   |         ^
+LL |         _ => "Could be",
+   |         ^ help: try this: `Maybe::Probably(..) | Maybe::None`
    |
-note: lint level defined here
-  --> $DIR/wildcard_enum_match_arm.rs:1:9
+   = note: `-D clippy::wildcard-enum-match-arm` implied by `-D warnings`
+
+error: wildcard match will miss any future added variants.
+  --> $DIR/wildcard_enum_match_arm.rs:20:9
+   |
+LL |         _other => "Could be",
+   |         ^^^^^^ help: try this: `_other @ Maybe::Some(..) | _other @ Maybe::Probably(..)`
+
+error: wildcard match will miss any future added variants.
+  --> $DIR/wildcard_enum_match_arm.rs:27:9
+   |
+LL |         n => format!("{:?}", n),
+   |         ^ help: try this: `n @ Maybe::Probably(..) | n @ Maybe::None`
+
+error: wildcard match will miss any future added variants.
+  --> $DIR/wildcard_enum_match_arm.rs:41:9
    |
-LL | #![deny(clippy::wildcard_enum_match_arm)]
-   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   = note: to resolve, match each variant explicitly
+LL |         _ => "Who knows",
+   |         ^ help: try this: `Maybe::Some(..) | Maybe::Probably(..) | Maybe::None`
 
-error: aborting due to previous error
+error: aborting due to 4 previous errors