From 45c41dbe38630d9d1ea9812bffc8b617f42dc7b6 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sat, 6 Mar 2021 19:25:09 -0500 Subject: [PATCH] Improvements to `match_wildcard_for_single_variants` and `wildcard_enum_match_arm` lints Don't lint on `Result` and `Option` types. Considers `or` patterns. Considers variants prefixed with `Self` Suggestions will try to find a common prefix rather than just using the full path --- clippy_lints/src/matches.rs | 247 +++++++++++------- clippy_utils/src/lib.rs | 10 + .../match_wildcard_for_single_variants.fixed | 40 +++ .../ui/match_wildcard_for_single_variants.rs | 40 +++ .../match_wildcard_for_single_variants.stderr | 44 +++- tests/ui/wildcard_enum_match_arm.fixed | 2 +- tests/ui/wildcard_enum_match_arm.stderr | 2 +- 7 files changed, 282 insertions(+), 103 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 0d79ffbe944e..f157c95422f2 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -4,23 +4,23 @@ use crate::utils::visitors::LocalUsedVisitor; use crate::utils::{ expr_block, get_parent_expr, implements_trait, in_macro, indent_of, is_allowed, is_expn_of, is_refutable, is_type_diagnostic_item, is_wild, match_qpath, match_type, meets_msrv, multispan_sugg, path_to_local, - path_to_local_id, peel_hir_pat_refs, peel_mid_ty_refs, peel_n_hir_expr_refs, remove_blocks, snippet, snippet_block, - snippet_opt, snippet_with_applicability, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, - span_lint_and_then, strip_pat_refs, + path_to_local_id, peel_hir_pat_refs, peel_mid_ty_refs, peel_n_hir_expr_refs, recurse_or_patterns, remove_blocks, + snippet, snippet_block, snippet_opt, snippet_with_applicability, span_lint_and_help, span_lint_and_note, + span_lint_and_sugg, span_lint_and_then, strip_pat_refs, }; use crate::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}; @@ -954,114 +954,179 @@ 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) => { + 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 => "(..)", + CtorKind::Const => "", + CtorKind::Fictive => "{ .. }", } - } - - let mut suggestion: Vec = 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, + "match on non-exhaustive enum doesn't explicitly match all known 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()); + "match on non-exhaustive enum doesn't explicitly match all known variants" + } else { + "wildcard match will miss 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) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 0395079901cb..89cf67955eae 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1179,6 +1179,16 @@ pub fn is_refutable(cx: &LateContext<'_>, pat: &Pat<'_>) -> bool { } } +/// If the pattern is an `or` pattern, call the function once for each sub pattern. Otherwise, call +/// the function once on the given pattern. +pub fn recurse_or_patterns<'tcx, F: FnMut(&'tcx Pat<'tcx>)>(pat: &'tcx Pat<'tcx>, mut f: F) { + if let PatKind::Or(pats) = pat.kind { + pats.iter().cloned().for_each(f) + } else { + f(pat) + } +} + /// Checks for the `#[automatically_derived]` attribute all `#[derive]`d /// implementations have. pub fn is_automatically_derived(attrs: &[ast::Attribute]) -> bool { diff --git a/tests/ui/match_wildcard_for_single_variants.fixed b/tests/ui/match_wildcard_for_single_variants.fixed index 519200977a79..f101e144a13c 100644 --- a/tests/ui/match_wildcard_for_single_variants.fixed +++ b/tests/ui/match_wildcard_for_single_variants.fixed @@ -15,6 +15,16 @@ enum Color { Blue, Rgb(u8, u8, u8), } +impl Color { + fn f(self) { + match self { + Self::Red => (), + Self::Green => (), + Self::Blue => (), + Self::Rgb(..) => (), + }; + } +} fn main() { let f = Foo::A; @@ -56,4 +66,34 @@ fn main() { Color::Rgb(255, _, _) => {}, _ => {}, } + + // References shouldn't change anything + match &color { + &Color::Red => (), + Color::Green => (), + &Color::Rgb(..) => (), + Color::Blue => (), + } + + use self::Color as C; + + match color { + C::Red => (), + C::Green => (), + C::Rgb(..) => (), + C::Blue => (), + } + + match color { + C::Red => (), + Color::Green => (), + Color::Rgb(..) => (), + Color::Blue => (), + } + + match Some(0) { + Some(0) => 0, + Some(_) => 1, + _ => 2, + }; } diff --git a/tests/ui/match_wildcard_for_single_variants.rs b/tests/ui/match_wildcard_for_single_variants.rs index 1df917e085c7..1ddba87e78f3 100644 --- a/tests/ui/match_wildcard_for_single_variants.rs +++ b/tests/ui/match_wildcard_for_single_variants.rs @@ -15,6 +15,16 @@ enum Color { Blue, Rgb(u8, u8, u8), } +impl Color { + fn f(self) { + match self { + Self::Red => (), + Self::Green => (), + Self::Blue => (), + _ => (), + }; + } +} fn main() { let f = Foo::A; @@ -56,4 +66,34 @@ fn main() { Color::Rgb(255, _, _) => {}, _ => {}, } + + // References shouldn't change anything + match &color { + &Color::Red => (), + Color::Green => (), + &Color::Rgb(..) => (), + &_ => (), + } + + use self::Color as C; + + match color { + C::Red => (), + C::Green => (), + C::Rgb(..) => (), + _ => (), + } + + match color { + C::Red => (), + Color::Green => (), + Color::Rgb(..) => (), + _ => (), + } + + match Some(0) { + Some(0) => 0, + Some(_) => 1, + _ => 2, + }; } diff --git a/tests/ui/match_wildcard_for_single_variants.stderr b/tests/ui/match_wildcard_for_single_variants.stderr index 82790aa9e80b..e0900c970c31 100644 --- a/tests/ui/match_wildcard_for_single_variants.stderr +++ b/tests/ui/match_wildcard_for_single_variants.stderr @@ -1,28 +1,52 @@ -error: wildcard match will miss any future added variants - --> $DIR/match_wildcard_for_single_variants.rs:24:9 +error: match on non-exhaustive enum doesn't explicitly match all known variants + --> $DIR/match_wildcard_for_single_variants.rs:24:13 | -LL | _ => {}, - | ^ help: try this: `Foo::C` +LL | _ => (), + | ^ help: try this: `Self::Rgb(..)` | = note: `-D clippy::match-wildcard-for-single-variants` implied by `-D warnings` -error: wildcard match will miss any future added variants +error: match on non-exhaustive enum doesn't explicitly match all known variants --> $DIR/match_wildcard_for_single_variants.rs:34:9 | +LL | _ => {}, + | ^ help: try this: `Foo::C` + +error: match on non-exhaustive enum doesn't explicitly match all known variants + --> $DIR/match_wildcard_for_single_variants.rs:44:9 + | LL | _ => {}, | ^ help: try this: `Color::Blue` -error: wildcard match will miss any future added variants - --> $DIR/match_wildcard_for_single_variants.rs:42:9 +error: match on non-exhaustive enum doesn't explicitly match all known variants + --> $DIR/match_wildcard_for_single_variants.rs:52:9 | LL | _ => {}, | ^ help: try this: `Color::Blue` -error: wildcard match will miss any future added variants - --> $DIR/match_wildcard_for_single_variants.rs:48:9 +error: match on non-exhaustive enum doesn't explicitly match all known variants + --> $DIR/match_wildcard_for_single_variants.rs:58:9 | LL | _ => {}, | ^ help: try this: `Color::Blue` -error: aborting due to 4 previous errors +error: match on non-exhaustive enum doesn't explicitly match all known variants + --> $DIR/match_wildcard_for_single_variants.rs:75:9 + | +LL | &_ => (), + | ^^ help: try this: `Color::Blue` + +error: match on non-exhaustive enum doesn't explicitly match all known variants + --> $DIR/match_wildcard_for_single_variants.rs:84:9 + | +LL | _ => (), + | ^ help: try this: `C::Blue` + +error: match on non-exhaustive enum doesn't explicitly match all known variants + --> $DIR/match_wildcard_for_single_variants.rs:91:9 + | +LL | _ => (), + | ^ help: try this: `Color::Blue` + +error: aborting due to 8 previous errors diff --git a/tests/ui/wildcard_enum_match_arm.fixed b/tests/ui/wildcard_enum_match_arm.fixed index c266f684a36f..fd754e4c794f 100644 --- a/tests/ui/wildcard_enum_match_arm.fixed +++ b/tests/ui/wildcard_enum_match_arm.fixed @@ -77,7 +77,7 @@ fn main() { let error_kind = ErrorKind::NotFound; match error_kind { ErrorKind::NotFound => {}, - std::io::ErrorKind::PermissionDenied | std::io::ErrorKind::ConnectionRefused | std::io::ErrorKind::ConnectionReset | std::io::ErrorKind::ConnectionAborted | std::io::ErrorKind::NotConnected | std::io::ErrorKind::AddrInUse | std::io::ErrorKind::AddrNotAvailable | std::io::ErrorKind::BrokenPipe | std::io::ErrorKind::AlreadyExists | std::io::ErrorKind::WouldBlock | std::io::ErrorKind::InvalidInput | std::io::ErrorKind::InvalidData | std::io::ErrorKind::TimedOut | std::io::ErrorKind::WriteZero | std::io::ErrorKind::Interrupted | std::io::ErrorKind::Other | std::io::ErrorKind::UnexpectedEof | _ => {}, + ErrorKind::PermissionDenied | ErrorKind::ConnectionRefused | ErrorKind::ConnectionReset | ErrorKind::ConnectionAborted | ErrorKind::NotConnected | ErrorKind::AddrInUse | ErrorKind::AddrNotAvailable | ErrorKind::BrokenPipe | ErrorKind::AlreadyExists | ErrorKind::WouldBlock | ErrorKind::InvalidInput | ErrorKind::InvalidData | ErrorKind::TimedOut | ErrorKind::WriteZero | ErrorKind::Interrupted | ErrorKind::Other | ErrorKind::UnexpectedEof | _ => {}, } match error_kind { ErrorKind::NotFound => {}, diff --git a/tests/ui/wildcard_enum_match_arm.stderr b/tests/ui/wildcard_enum_match_arm.stderr index 0da2b68ba0b2..8d880b7ce0a3 100644 --- a/tests/ui/wildcard_enum_match_arm.stderr +++ b/tests/ui/wildcard_enum_match_arm.stderr @@ -32,7 +32,7 @@ error: match on non-exhaustive enum doesn't explicitly match all known variants --> $DIR/wildcard_enum_match_arm.rs:80:9 | LL | _ => {}, - | ^ help: try this: `std::io::ErrorKind::PermissionDenied | std::io::ErrorKind::ConnectionRefused | std::io::ErrorKind::ConnectionReset | std::io::ErrorKind::ConnectionAborted | std::io::ErrorKind::NotConnected | std::io::ErrorKind::AddrInUse | std::io::ErrorKind::AddrNotAvailable | std::io::ErrorKind::BrokenPipe | std::io::ErrorKind::AlreadyExists | std::io::ErrorKind::WouldBlock | std::io::ErrorKind::InvalidInput | std::io::ErrorKind::InvalidData | std::io::ErrorKind::TimedOut | std::io::ErrorKind::WriteZero | std::io::ErrorKind::Interrupted | std::io::ErrorKind::Other | std::io::ErrorKind::UnexpectedEof | _` + | ^ help: try this: `ErrorKind::PermissionDenied | ErrorKind::ConnectionRefused | ErrorKind::ConnectionReset | ErrorKind::ConnectionAborted | ErrorKind::NotConnected | ErrorKind::AddrInUse | ErrorKind::AddrNotAvailable | ErrorKind::BrokenPipe | ErrorKind::AlreadyExists | ErrorKind::WouldBlock | ErrorKind::InvalidInput | ErrorKind::InvalidData | ErrorKind::TimedOut | ErrorKind::WriteZero | ErrorKind::Interrupted | ErrorKind::Other | ErrorKind::UnexpectedEof | _` error: aborting due to 5 previous errors