Auto merge of #3729 - illicitonion:match_enum_wildcard, r=flip1995

wildcard_enum_match_arm gives suggestions

And is also more robust

3 files changed, 125 insertions, 15 deletions

diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs
index 9cb160685ca..fcff1e16f38 100644
--- a/clippy_lints/src/matches.rs
+++ b/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,89 @@ 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();
+
+        if suggestion.is_empty() {
+            return;
         }
+
+        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,
+        )
     }
 }
 
diff --git a/tests/ui/wildcard_enum_match_arm.rs b/tests/ui/wildcard_enum_match_arm.rs
index 58daabf4268..94d69d3c8a4 100644
--- a/tests/ui/wildcard_enum_match_arm.rs
+++ b/tests/ui/wildcard_enum_match_arm.rs
@@ -26,6 +26,14 @@ fn main() {
         _ => eprintln!("Not red"),
     };
     match color {
+        Color::Red => println!("Red"),
+        _not_red => eprintln!("Not red"),
+    };
+    let _str = match color {
+        Color::Red => "Red".to_owned(),
+        not_red => format!("{:?}", not_red),
+    };
+    match color {
         Color::Red => {},
         Color::Green => {},
         Color::Blue => {},
@@ -33,6 +41,18 @@ fn main() {
         c if c.is_monochrome() => {},
         Color::Rgb(_, _, _) => {},
     };
+    let _str = match color {
+        Color::Red => "Red",
+        c @ Color::Green | c @ Color::Blue | c @ Color::Rgb(_, _, _) | c @ Color::Cyan => "Not red",
+    };
+    match color {
+        Color::Rgb(r, _, _) if r > 0 => "Some red",
+        _ => "No red",
+    };
+    match color {
+        Color::Red | Color::Green | Color::Blue | Color::Cyan => {},
+        Color::Rgb(..) => {},
+    };
     let x: u8 = unimplemented!();
     match x {
         0 => {},
diff --git a/tests/ui/wildcard_enum_match_arm.stderr b/tests/ui/wildcard_enum_match_arm.stderr
index 6319a3f3d46..999c1693301 100644
--- a/tests/ui/wildcard_enum_match_arm.stderr
+++ b/tests/ui/wildcard_enum_match_arm.stderr
@@ -2,14 +2,31 @@ error: wildcard match will miss any future added variants.
   --> $DIR/wildcard_enum_match_arm.rs:26:9
    |
 LL |         _ => eprintln!("Not red"),
-   |         ^
+   |         ^ help: try this: `Color::Green | Color::Blue | Color::Rgb(..) | Color::Cyan`
    |
 note: lint level defined here
   --> $DIR/wildcard_enum_match_arm.rs:1:9
    |
 LL | #![deny(clippy::wildcard_enum_match_arm)]
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   = note: to resolve, match each variant explicitly
 
-error: aborting due to previous error
+error: wildcard match will miss any future added variants.
+  --> $DIR/wildcard_enum_match_arm.rs:30:9
+   |
+LL |         _not_red => eprintln!("Not red"),
+   |         ^^^^^^^^ help: try this: `_not_red @ Color::Green | _not_red @ Color::Blue | _not_red @ Color::Rgb(..) | _not_red @ Color::Cyan`
+
+error: wildcard match will miss any future added variants.
+  --> $DIR/wildcard_enum_match_arm.rs:34:9
+   |
+LL |         not_red => format!("{:?}", not_red),
+   |         ^^^^^^^ help: try this: `not_red @ Color::Green | not_red @ Color::Blue | not_red @ Color::Rgb(..) | not_red @ Color::Cyan`
+
+error: wildcard match will miss any future added variants.
+  --> $DIR/wildcard_enum_match_arm.rs:50:9
+   |
+LL |         _ => "No red",
+   |         ^ help: try this: `Color::Red | Color::Green | Color::Blue | Color::Rgb(..) | Color::Cyan`
+
+error: aborting due to 4 previous errors