From 75923dff5baa8de3acdec3026aac487033fe68fe Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 6 Feb 2022 16:18:05 -0500 Subject: [PATCH] Split out `wildcard_enum_match_arm` and `match_wildcard_for_single_variants` --- clippy_lints/src/matches/match_wild_enum.rs | 198 +++++++++++++++++++ clippy_lints/src/matches/mod.rs | 200 +------------------- 2 files changed, 203 insertions(+), 195 deletions(-) create mode 100644 clippy_lints/src/matches/match_wild_enum.rs diff --git a/clippy_lints/src/matches/match_wild_enum.rs b/clippy_lints/src/matches/match_wild_enum.rs new file mode 100644 index 00000000000..3515286d5b4 --- /dev/null +++ b/clippy_lints/src/matches/match_wild_enum.rs @@ -0,0 +1,198 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{is_refutable, peel_hir_pat_refs, recurse_or_patterns}; +use rustc_errors::Applicability; +use rustc_hir::def::{CtorKind, DefKind, Res}; +use rustc_hir::{Arm, Expr, PatKind, PathSegment, QPath, Ty, TyKind}; +use rustc_lint::LateContext; +use rustc_middle::ty::{self, VariantDef}; +use rustc_span::sym; + +use super::{MATCH_WILDCARD_FOR_SINGLE_VARIANTS, WILDCARD_ENUM_MATCH_ARM}; + +#[allow(clippy::too_many_lines)] +pub(crate) fn check(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) || is_type_diagnostic_item(cx, ty, sym::Result)) => + { + 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 { + 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, + }; + + // Accumulate the variants which should be put in place of the wildcard because they're not + // already covered. + let has_hidden = adt_def.variants.iter().any(|x| is_hidden(cx, x)); + let mut missing_variants: Vec<_> = adt_def.variants.iter().filter(|x| !is_hidden(cx, x)).collect(); + + 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 | DefKind::InlineConst, + _, + ) => 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 let Some(id) = cx.qpath_res(path, pat.hir_id).opt_def_id() { + if arm.guard.is_none() && patterns.iter().all(|p| !is_refutable(cx, p)) { + missing_variants.retain(|e| e.ctor_def_id != Some(id)); + } + } + path + }, + PatKind::Struct(path, patterns, ..) => { + if let Some(id) = cx.qpath_res(path, pat.hir_id).opt_def_id() { + if arm.guard.is_none() && patterns.iter().all(|p| !is_refutable(cx, p.pat)) { + missing_variants.retain(|e| e.def_id != id); + } + } + path + }, + _ => return, + }; + match path { + QPath::Resolved(_, path) => path_prefix.with_path(path.segments), + QPath::TypeRelative( + Ty { + kind: TyKind::Path(QPath::Resolved(_, path)), + .. + }, + _, + ) => path_prefix.with_prefix(path.segments), + _ => (), + } + }); + } + + 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.name, + match variant.ctor_kind { + CtorKind::Fn if variant.fields.len() == 1 => "(_)", + CtorKind::Fn => "(..)", + CtorKind::Const => "", + CtorKind::Fictive => "{ .. }", + } + ) + }; + + match missing_variants.as_slice() { + [] => (), + [x] if !adt_def.is_variant_list_non_exhaustive() && !has_hidden => 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().copied().map(format_suggestion).collect(); + let message = if adt_def.is_variant_list_non_exhaustive() || has_hidden { + suggestions.push("_".into()); + "wildcard matches known variants and will also match future added variants" + } else { + "wildcard match will also match any future added variants" + }; + + span_lint_and_sugg( + cx, + WILDCARD_ENUM_MATCH_ARM, + wildcard_span, + message, + "try this", + suggestions.join(" | "), + Applicability::MaybeIncorrect, + ); + }, + }; +} + +enum CommonPrefixSearcher<'a> { + None, + Path(&'a [PathSegment<'a>]), + Mixed, +} +impl<'a> 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 => (), + } + } +} + +fn is_hidden(cx: &LateContext<'_>, variant_def: &VariantDef) -> bool { + let attrs = cx.tcx.get_attrs(variant_def.def_id); + clippy_utils::attrs::is_doc_hidden(attrs) || clippy_utils::attrs::is_unstable(attrs) +} diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index e2a103c58d1..8c67380ed52 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -1,29 +1,26 @@ use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_help, span_lint_and_sugg, span_lint_and_then}; use clippy_utils::source::{indent_of, snippet, snippet_block, snippet_opt, snippet_with_applicability}; use clippy_utils::sugg::Sugg; -use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::{ get_parent_expr, is_lang_ctor, is_refutable, is_wild, meets_msrv, msrvs, path_to_local_id, peel_blocks, - peel_hir_pat_refs, recurse_or_patterns, strip_pat_refs, + strip_pat_refs, }; use core::iter::once; use if_chain::if_chain; use rustc_errors::Applicability; -use rustc_hir::def::{CtorKind, DefKind, Res}; use rustc_hir::LangItem::{OptionNone, OptionSome}; use rustc_hir::{ - self as hir, Arm, BindingAnnotation, BorrowKind, Expr, ExprKind, Local, MatchSource, Mutability, Node, Pat, - PatKind, PathSegment, QPath, TyKind, + Arm, BindingAnnotation, BorrowKind, Expr, ExprKind, Local, MatchSource, Mutability, Node, Pat, PatKind, QPath, }; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::{self, VariantDef}; +use rustc_middle::ty; use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::sym; mod match_bool; mod match_like_matches; mod match_same_arms; +mod match_wild_enum; mod match_wild_err_arm; mod overlapping_arms; mod redundant_pattern_match; @@ -629,7 +626,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches { match_bool::check(cx, ex, arms, expr); overlapping_arms::check(cx, ex, arms); match_wild_err_arm::check(cx, ex, arms); - check_wild_enum_match(cx, ex, arms); + match_wild_enum::check(cx, ex, arms); check_match_as_ref(cx, ex, arms, expr); check_wild_in_or_pats(cx, arms); @@ -705,193 +702,6 @@ impl<'tcx> LateLintPass<'tcx> for Matches { extract_msrv_attr!(LateContext); } -enum CommonPrefixSearcher<'a> { - None, - Path(&'a [PathSegment<'a>]), - Mixed, -} -impl<'a> 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 => (), - } - } -} - -fn is_hidden(cx: &LateContext<'_>, variant_def: &VariantDef) -> bool { - let attrs = cx.tcx.get_attrs(variant_def.def_id); - clippy_utils::attrs::is_doc_hidden(attrs) || clippy_utils::attrs::is_unstable(attrs) -} - -#[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) || is_type_diagnostic_item(cx, ty, sym::Result)) => - { - 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 { - 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, - }; - - // Accumulate the variants which should be put in place of the wildcard because they're not - // already covered. - let has_hidden = adt_def.variants.iter().any(|x| is_hidden(cx, x)); - let mut missing_variants: Vec<_> = adt_def.variants.iter().filter(|x| !is_hidden(cx, x)).collect(); - - 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 | DefKind::InlineConst, - _, - ) => 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 let Some(id) = cx.qpath_res(path, pat.hir_id).opt_def_id() { - if arm.guard.is_none() && patterns.iter().all(|p| !is_refutable(cx, p)) { - missing_variants.retain(|e| e.ctor_def_id != Some(id)); - } - } - path - }, - PatKind::Struct(path, patterns, ..) => { - if let Some(id) = cx.qpath_res(path, pat.hir_id).opt_def_id() { - if arm.guard.is_none() && patterns.iter().all(|p| !is_refutable(cx, p.pat)) { - 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), - _ => (), - } - }); - } - - 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.name, - match variant.ctor_kind { - CtorKind::Fn if variant.fields.len() == 1 => "(_)", - CtorKind::Fn => "(..)", - CtorKind::Const => "", - CtorKind::Fictive => "{ .. }", - } - ) - }; - - match missing_variants.as_slice() { - [] => (), - [x] if !adt_def.is_variant_list_non_exhaustive() && !has_hidden => 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().copied().map(format_suggestion).collect(); - let message = if adt_def.is_variant_list_non_exhaustive() || has_hidden { - suggestions.push("_".into()); - "wildcard matches known variants and will also match future added variants" - } else { - "wildcard match will also match any future added variants" - }; - - span_lint_and_sugg( - cx, - WILDCARD_ENUM_MATCH_ARM, - wildcard_span, - message, - "try this", - suggestions.join(" | "), - Applicability::MaybeIncorrect, - ); - }, - }; -} - fn check_match_ref_pats<'a, 'b, I>(cx: &LateContext<'_>, ex: &Expr<'_>, pats: I, expr: &Expr<'_>) where 'b: 'a,