Auto merge of #6863 - Jarcho:wild_enum_match, r=llogiq

`match_wildcard` improvements

fixes: #6604
fixes: #5733
fixes: #6862

#5733 is only fixed in the normal case, if different paths are used for the variants then the same problem will occur. It's cause by `def_path_str` returning an utterly useless result. I haven't dug into why yet.

For #6604 there should be some discussion before accepting this. It's easy enough to change the message rather than disable the lint for `Option` and `Result`.

changelog: Attempt to find a common path prefix for `match_wildcard_for_single_variants` and `wildcard_enum_match_arm`
changelog: Don't lint op `Option` and `Result` for `match_wildcard_for_single_variants` and `wildcard_enum_match_arm`
changelog: Consider `or` patterns and `Self` prefix for `match_wildcard_for_single_variants` and `wildcard_enum_match_arm`

Author: bors

clippy_lints/src/attrs.rs

@@ -430,7 +430,7 @@ fn is_relevant_block(cx: &LateContext<'_>, typeck_results: &ty::TypeckResults<'_
         |stmt| match &stmt.kind {
             StmtKind::Local(_) => true,
             StmtKind::Expr(expr) | StmtKind::Semi(expr) => is_relevant_expr(cx, typeck_results, expr),
-            _ => false,
+            StmtKind::Item(_) => false,
         },
     )
 }
@@ -613,7 +613,7 @@ fn check_mismatched_target_os(cx: &EarlyContext<'_>, attr: &Attribute) {
                             }
                         }
                     },
-                    _ => {},
+                    MetaItemKind::NameValue(..) => {},
                 }
             }
         }

clippy_lints/src/doc.rs

@@ -583,7 +583,7 @@ fn check_code(cx: &LateContext<'_>, text: &str, edition: Edition, span: Span) {
                                 let returns_nothing = match &sig.decl.output {
                                     FnRetTy::Default(..) => true,
                                     FnRetTy::Ty(ty) if ty.kind.is_unit() => true,
-                                    _ => false,
+                                    FnRetTy::Ty(_) => false,
                                 };
 
                                 if returns_nothing && !is_async && !block.stmts.is_empty() {

clippy_lints/src/eval_order_dependence.rs

@@ -273,7 +273,7 @@ fn check_stmt<'a, 'tcx>(vis: &mut ReadVisitor<'a, 'tcx>, stmt: &'tcx Stmt<'_>) -
             .init
             .as_ref()
             .map_or(StopEarly::KeepGoing, |expr| check_expr(vis, expr)),
-        _ => StopEarly::KeepGoing,
+        StmtKind::Item(..) => StopEarly::KeepGoing,
     }
 }
 

clippy_lints/src/loops/never_loop.rs

@@ -78,7 +78,7 @@ fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<&'tcx Expr<'tcx>> {
     match stmt.kind {
         StmtKind::Semi(ref e, ..) | StmtKind::Expr(ref e, ..) => Some(e),
         StmtKind::Local(ref local) => local.init.as_deref(),
-        _ => None,
+        StmtKind::Item(..) => None,
     }
 }
 

clippy_lints/src/matches.rs

@@ -8,21 +8,21 @@ use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, match_type, pe
 use clippy_utils::visitors::LocalUsedVisitor;
 use clippy_utils::{
     get_parent_expr, in_macro, is_allowed, is_expn_of, is_refutable, is_wild, match_qpath, meets_msrv, path_to_local,
-    path_to_local_id, peel_hir_pat_refs, peel_n_hir_expr_refs, remove_blocks, strip_pat_refs,
+    path_to_local_id, peel_hir_pat_refs, peel_n_hir_expr_refs, recurse_or_patterns, remove_blocks, strip_pat_refs,
 };
 use clippy_utils::{paths, search_same, SpanlessEq, SpanlessHash};
 use if_chain::if_chain;
 use rustc_ast::ast::LitKind;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_errors::Applicability;
-use rustc_hir::def::CtorKind;
+use rustc_hir::def::{CtorKind, DefKind, Res};
 use rustc_hir::{
-    Arm, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, Guard, HirId, Local, MatchSource, Mutability, Node, Pat,
-    PatKind, QPath, RangeEnd,
+    self as hir, Arm, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, Guard, HirId, Local, MatchSource,
+    Mutability, Node, Pat, PatKind, PathSegment, QPath, RangeEnd, TyKind,
 };
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
-use rustc_middle::ty::{self, Ty, TyS};
+use rustc_middle::ty::{self, Ty, TyS, VariantDef};
 use rustc_semver::RustcVersion;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::source_map::{Span, Spanned};
@@ -956,114 +956,181 @@ fn check_wild_err_arm<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'tcx>, arms: &[Arm
     }
 }
 
-fn check_wild_enum_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) {
-    let ty = cx.typeck_results().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;
+enum CommonPrefixSearcher<'a> {
+    None,
+    Path(&'a [PathSegment<'a>]),
+    Mixed,
+}
+impl CommonPrefixSearcher<'a> {
+    fn with_path(&mut self, path: &'a [PathSegment<'a>]) {
+        match path {
+            [path @ .., _] => self.with_prefix(path),
+            [] => (),
+        }
     }
 
+    fn with_prefix(&mut self, path: &'a [PathSegment<'a>]) {
+        match self {
+            Self::None => *self = Self::Path(path),
+            Self::Path(self_path)
+                if path
+                    .iter()
+                    .map(|p| p.ident.name)
+                    .eq(self_path.iter().map(|p| p.ident.name)) => {},
+            Self::Path(_) => *self = Self::Mixed,
+            Self::Mixed => (),
+        }
+    }
+}
+
+#[allow(clippy::too_many_lines)]
+fn check_wild_enum_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) {
+    let ty = cx.typeck_results().expr_ty(ex).peel_refs();
+    let adt_def = match ty.kind() {
+        ty::Adt(adt_def, _)
+            if adt_def.is_enum()
+                && !(is_type_diagnostic_item(cx, ty, sym::option_type)
+                    || is_type_diagnostic_item(cx, ty, sym::result_type)) =>
+        {
+            adt_def
+        },
+        _ => 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;
+    let mut has_non_wild = false;
     for arm in arms {
-        if let PatKind::Wild = arm.pat.kind {
-            wildcard_span = Some(arm.pat.span);
-        } else if let PatKind::Binding(_, _, ident, None) = arm.pat.kind {
-            wildcard_span = Some(arm.pat.span);
-            wildcard_ident = Some(ident);
+        match peel_hir_pat_refs(arm.pat).0.kind {
+            PatKind::Wild => wildcard_span = Some(arm.pat.span),
+            PatKind::Binding(_, _, ident, None) => {
+                wildcard_span = Some(arm.pat.span);
+                wildcard_ident = Some(ident);
+            },
+            _ => has_non_wild = true,
         }
     }
+    let wildcard_span = match wildcard_span {
+        Some(x) if has_non_wild => x,
+        _ => return,
+    };
 
-    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.
+    // Accumulate the variants which should be put in place of the wildcard because they're not
+    // already covered.
+    let mut missing_variants: Vec<_> = adt_def.variants.iter().collect();
 
-        let mut missing_variants = vec![];
-        if let ty::Adt(def, _) = ty.kind() {
-            for variant in &def.variants {
-                missing_variants.push(variant);
+    let mut path_prefix = CommonPrefixSearcher::None;
+    for arm in arms {
+        // 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.
+        recurse_or_patterns(arm.pat, |pat| {
+            let path = match &peel_hir_pat_refs(pat).0.kind {
+                PatKind::Path(path) => {
+                    #[allow(clippy::match_same_arms)]
+                    let id = match cx.qpath_res(path, pat.hir_id) {
+                        Res::Def(DefKind::Const | DefKind::ConstParam | DefKind::AnonConst, _) => return,
+                        Res::Def(_, id) => id,
+                        _ => return,
+                    };
+                    if arm.guard.is_none() {
+                        missing_variants.retain(|e| e.ctor_def_id != Some(id));
+                    }
+                    path
+                },
+                PatKind::TupleStruct(path, patterns, ..) => {
+                    if arm.guard.is_none() && patterns.iter().all(|p| !is_refutable(cx, p)) {
+                        let id = cx.qpath_res(path, pat.hir_id).def_id();
+                        missing_variants.retain(|e| e.ctor_def_id != Some(id));
+                    }
+                    path
+                },
+                PatKind::Struct(path, patterns, ..) => {
+                    if arm.guard.is_none() && patterns.iter().all(|p| !is_refutable(cx, p.pat)) {
+                        let id = cx.qpath_res(path, pat.hir_id).def_id();
+                        missing_variants.retain(|e| e.def_id != id);
+                    }
+                    path
+                },
+                _ => return,
+            };
+            match path {
+                QPath::Resolved(_, path) => path_prefix.with_path(path.segments),
+                QPath::TypeRelative(
+                    hir::Ty {
+                        kind: TyKind::Path(QPath::Resolved(_, path)),
+                        ..
+                    },
+                    _,
+                ) => path_prefix.with_prefix(path.segments),
+                _ => (),
             }
-        }
+        });
+    }
 
-        for arm in arms {
-            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;
-            }
-            if let PatKind::Path(ref path) = arm.pat.kind {
-                if let QPath::Resolved(_, p) = path {
-                    missing_variants.retain(|e| e.ctor_def_id != Some(p.res.def_id()));
-                }
-            } else if let PatKind::TupleStruct(QPath::Resolved(_, p), ref patterns, ..) = arm.pat.kind {
-                // Some simple checks for exhaustive patterns.
-                // There is a room for improvements to detect more cases,
-                // but it can be more expensive to do so.
-                let is_pattern_exhaustive =
-                    |pat: &&Pat<'_>| matches!(pat.kind, PatKind::Wild | PatKind::Binding(.., None));
-                if patterns.iter().all(is_pattern_exhaustive) {
-                    missing_variants.retain(|e| e.ctor_def_id != Some(p.res.def_id()));
+    let format_suggestion = |variant: &VariantDef| {
+        format!(
+            "{}{}{}{}",
+            if let Some(ident) = wildcard_ident {
+                format!("{} @ ", ident.name)
+            } else {
+                String::new()
+            },
+            if let CommonPrefixSearcher::Path(path_prefix) = path_prefix {
+                let mut s = String::new();
+                for seg in path_prefix {
+                    s.push_str(&seg.ident.as_str());
+                    s.push_str("::");
                 }
+                s
+            } else {
+                let mut s = cx.tcx.def_path_str(adt_def.did);
+                s.push_str("::");
+                s
+            },
+            variant.ident.name,
+            match variant.ctor_kind {
+                CtorKind::Fn if variant.fields.len() == 1 => "(_)",
+                CtorKind::Fn => "(..)",
+                CtorKind::Const => "",
+                CtorKind::Fictive => "{ .. }",
             }
-        }
-
-        let mut 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.def_path_str(v.def_id), suffix)
-            })
-            .collect();
-
-        if suggestion.is_empty() {
-            return;
-        }
-
-        let mut message = "wildcard match will miss any future added variants";
+        )
+    };
 
-        if let ty::Adt(def, _) = ty.kind() {
-            if def.is_variant_list_non_exhaustive() {
-                message = "match on non-exhaustive enum doesn't explicitly match all known variants";
-                suggestion.push(String::from("_"));
-            }
-        }
+    match missing_variants.as_slice() {
+        [] => (),
+        [x] if !adt_def.is_variant_list_non_exhaustive() => span_lint_and_sugg(
+            cx,
+            MATCH_WILDCARD_FOR_SINGLE_VARIANTS,
+            wildcard_span,
+            "wildcard matches only a single variant and will also match any future added variants",
+            "try this",
+            format_suggestion(x),
+            Applicability::MaybeIncorrect,
+        ),
+        variants => {
+            let mut suggestions: Vec<_> = variants.iter().cloned().map(format_suggestion).collect();
+            let message = if adt_def.is_variant_list_non_exhaustive() {
+                suggestions.push("_".into());
+                "wildcard matches known variants and will also match future added variants"
+            } else {
+                "wildcard match will also match any future added variants"
+            };
 
-        if suggestion.len() == 1 {
-            // No need to check for non-exhaustive enum as in that case len would be greater than 1
             span_lint_and_sugg(
                 cx,
-                MATCH_WILDCARD_FOR_SINGLE_VARIANTS,
+                WILDCARD_ENUM_MATCH_ARM,
                 wildcard_span,
                 message,
                 "try this",
-                suggestion[0].clone(),
+                suggestions.join(" | "),
                 Applicability::MaybeIncorrect,
             )
-        };
-
-        span_lint_and_sugg(
-            cx,
-            WILDCARD_ENUM_MATCH_ARM,
-            wildcard_span,
-            message,
-            "try this",
-            suggestion.join(" | "),
-            Applicability::MaybeIncorrect,
-        )
-    }
+        },
+    };
 }
 
 // If the block contains only a `panic!` macro (as expression or statement)

clippy_lints/src/redundant_clone.rs

@@ -556,7 +556,7 @@ impl<'a, 'tcx> mir::visit::Visitor<'tcx> for PossibleBorrowerVisitor<'a, 'tcx> {
                     mir::Operand::Copy(p) | mir::Operand::Move(p) => {
                         self.possible_borrower.add(p.local, *dest);
                     },
-                    _ => (),
+                    mir::Operand::Constant(..) => (),
                 }
             }
         }
@@ -578,7 +578,7 @@ fn rvalue_locals(rvalue: &mir::Rvalue<'_>, mut visit: impl FnMut(mir::Local)) {
 
     let mut visit_op = |op: &mir::Operand<'_>| match op {
         mir::Operand::Copy(p) | mir::Operand::Move(p) => visit(p.local),
-        _ => (),
+        mir::Operand::Constant(..) => (),
     };
 
     match rvalue {

clippy_lints/src/types/mod.rs

@@ -276,7 +276,7 @@ impl<'tcx> LateLintPass<'tcx> for Types {
         match item.kind {
             TraitItemKind::Const(ref ty, _) | TraitItemKind::Type(_, Some(ref ty)) => self.check_ty(cx, ty, false),
             TraitItemKind::Fn(ref sig, _) => self.check_fn_decl(cx, &sig.decl),
-            _ => (),
+            TraitItemKind::Type(..) => (),
         }
     }
 
@@ -452,15 +452,15 @@ impl<'tcx> LateLintPass<'tcx> for TypeComplexity {
             TraitItemKind::Const(ref ty, _) | TraitItemKind::Type(_, Some(ref ty)) => self.check_type(cx, ty),
             TraitItemKind::Fn(FnSig { ref decl, .. }, TraitFn::Required(_)) => self.check_fndecl(cx, decl),
             // methods with default impl are covered by check_fn
-            _ => (),
+            TraitItemKind::Type(..) | TraitItemKind::Fn(_, TraitFn::Provided(_)) => (),
         }
     }
 
     fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) {
         match item.kind {
             ImplItemKind::Const(ref ty, _) | ImplItemKind::TyAlias(ref ty) => self.check_type(cx, ty),
             // methods are covered by check_fn
-            _ => (),
+            ImplItemKind::Fn(..) => (),
         }
     }
 

clippy_lints/src/unnecessary_wraps.rs

@@ -71,7 +71,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
                 }
             },
             FnKind::Closure => return,
-            _ => (),
+            FnKind::Method(..) => (),
         }
 
         // Abort if the method is implementing a trait or of it a trait method.