From 28dec3b708c3e0d5e45b6c70f054860cbd53d624 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Tue, 24 Nov 2020 20:37:07 -0600 Subject: [PATCH] Add collapsible_match lint --- CHANGELOG.md | 1 + clippy_lints/src/collapsible_match.rs | 172 ++++++++++++++++ clippy_lints/src/lib.rs | 5 + tests/ui/collapsible_match.rs | 278 ++++++++++++++++++++++++++ tests/ui/collapsible_match.stderr | 237 ++++++++++++++++++++++ 5 files changed, 693 insertions(+) create mode 100644 clippy_lints/src/collapsible_match.rs create mode 100644 tests/ui/collapsible_match.rs create mode 100644 tests/ui/collapsible_match.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index e76a781f13b..e65e7cc639f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1770,6 +1770,7 @@ Released 2018-09-13 [`cmp_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_owned [`cognitive_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity [`collapsible_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if +[`collapsible_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_match [`comparison_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain [`comparison_to_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_to_empty [`copy_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#copy_iterator diff --git a/clippy_lints/src/collapsible_match.rs b/clippy_lints/src/collapsible_match.rs new file mode 100644 index 00000000000..a34ba2d00a8 --- /dev/null +++ b/clippy_lints/src/collapsible_match.rs @@ -0,0 +1,172 @@ +use crate::utils::visitors::LocalUsedVisitor; +use crate::utils::{span_lint_and_then, SpanlessEq}; +use if_chain::if_chain; +use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res}; +use rustc_hir::{Arm, Expr, ExprKind, Guard, HirId, Pat, PatKind, QPath, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{DefIdTree, TyCtxt}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{MultiSpan, Span}; + +declare_clippy_lint! { + /// **What it does:** Finds nested `match` or `if let` expressions where the patterns may be "collapsed" together + /// without adding any branches. + /// + /// Note that this lint is not intended to find _all_ cases where nested match patterns can be merged, but only + /// cases where merging would most likely make the code more readable. + /// + /// **Why is this bad?** It is unnecessarily verbose and complex. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// fn func(opt: Option>) { + /// let n = match opt { + /// Some(n) => match n { + /// Ok(n) => n, + /// _ => return, + /// } + /// None => return, + /// }; + /// } + /// ``` + /// Use instead: + /// ```rust + /// fn func(opt: Option>) { + /// let n = match opt { + /// Some(Ok(n)) => n, + /// _ => return, + /// }; + /// } + /// ``` + pub COLLAPSIBLE_MATCH, + style, + "Nested `match` or `if let` expressions where the patterns may be \"collapsed\" together." +} + +declare_lint_pass!(CollapsibleMatch => [COLLAPSIBLE_MATCH]); + +impl<'tcx> LateLintPass<'tcx> for CollapsibleMatch { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + if let ExprKind::Match(_expr, arms, _source) = expr.kind { + if let Some(wild_arm) = arms.iter().rfind(|arm| arm_is_wild_like(arm, cx.tcx)) { + for arm in arms { + check_arm(arm, wild_arm, cx); + } + } + } + } +} + +fn check_arm(arm: &Arm<'_>, wild_outer_arm: &Arm<'_>, cx: &LateContext<'_>) { + if_chain! { + let expr = strip_singleton_blocks(arm.body); + if let ExprKind::Match(expr_in, arms_inner, _) = expr.kind; + // the outer arm pattern and the inner match + if expr_in.span.ctxt() == arm.pat.span.ctxt(); + // there must be no more than two arms in the inner match for this lint + if arms_inner.len() == 2; + // no if guards on the inner match + if arms_inner.iter().all(|arm| arm.guard.is_none()); + // match expression must be a local binding + // match { .. } + if let ExprKind::Path(QPath::Resolved(None, path)) = expr_in.kind; + if let Res::Local(binding_id) = path.res; + // one of the branches must be "wild-like" + if let Some(wild_inner_arm_idx) = arms_inner.iter().rposition(|arm_inner| arm_is_wild_like(arm_inner, cx.tcx)); + let (wild_inner_arm, non_wild_inner_arm) = + (&arms_inner[wild_inner_arm_idx], &arms_inner[1 - wild_inner_arm_idx]); + if !pat_contains_or(non_wild_inner_arm.pat); + // the binding must come from the pattern of the containing match arm + // .... => match { .. } + if let Some(binding_span) = find_pat_binding(arm.pat, binding_id); + // the "wild-like" branches must be equal + if SpanlessEq::new(cx).eq_expr(wild_inner_arm.body, wild_outer_arm.body); + // the binding must not be used in the if guard + if !matches!(arm.guard, Some(Guard::If(guard)) if LocalUsedVisitor::new(binding_id).check_expr(guard)); + // ...or anywhere in the inner match + if !arms_inner.iter().any(|arm| LocalUsedVisitor::new(binding_id).check_arm(arm)); + then { + span_lint_and_then( + cx, + COLLAPSIBLE_MATCH, + expr.span, + "Unnecessary nested match", + |diag| { + let mut help_span = MultiSpan::from_spans(vec![binding_span, non_wild_inner_arm.pat.span]); + help_span.push_span_label(binding_span, "Replace this binding".into()); + help_span.push_span_label(non_wild_inner_arm.pat.span, "with this pattern".into()); + diag.span_help(help_span, "The outer pattern can be modified to include the inner pattern."); + }, + ); + } + } +} + +fn strip_singleton_blocks<'hir>(mut expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> { + while let ExprKind::Block(block, _) = expr.kind { + match (block.stmts, block.expr) { + ([stmt], None) => match stmt.kind { + StmtKind::Expr(e) | StmtKind::Semi(e) => expr = e, + _ => break, + }, + ([], Some(e)) => expr = e, + _ => break, + } + } + expr +} + +/// A "wild-like" pattern is wild ("_") or `None`. +/// For this lint to apply, both the outer and inner match expressions +/// must have "wild-like" branches that can be combined. +fn arm_is_wild_like(arm: &Arm<'_>, tcx: TyCtxt<'_>) -> bool { + if arm.guard.is_some() { + return false; + } + match arm.pat.kind { + PatKind::Binding(..) | PatKind::Wild => true, + PatKind::Path(QPath::Resolved(None, path)) if is_none_ctor(path.res, tcx) => true, + _ => false, + } +} + +fn find_pat_binding(pat: &Pat<'_>, hir_id: HirId) -> Option { + let mut span = None; + pat.walk_short(|p| match &p.kind { + // ignore OR patterns + PatKind::Or(_) => false, + PatKind::Binding(_bm, _, _ident, _) => { + let found = p.hir_id == hir_id; + if found { + span = Some(p.span); + } + !found + }, + _ => true, + }); + span +} + +fn pat_contains_or(pat: &Pat<'_>) -> bool { + let mut result = false; + pat.walk(|p| { + let is_or = matches!(p.kind, PatKind::Or(_)); + result |= is_or; + !is_or + }); + result +} + +fn is_none_ctor(res: Res, tcx: TyCtxt<'_>) -> bool { + if let Some(none_id) = tcx.lang_items().option_none_variant() { + if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), id) = res { + if let Some(variant_id) = tcx.parent(id) { + return variant_id == none_id; + } + } + } + false +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 6eb5f6a7f48..ca190986cdb 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -172,6 +172,7 @@ mod cargo_common_metadata; mod checked_conversions; mod cognitive_complexity; mod collapsible_if; +mod collapsible_match; mod comparison_chain; mod copies; mod copy_iterator; @@ -531,6 +532,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &checked_conversions::CHECKED_CONVERSIONS, &cognitive_complexity::COGNITIVE_COMPLEXITY, &collapsible_if::COLLAPSIBLE_IF, + &collapsible_match::COLLAPSIBLE_MATCH, &comparison_chain::COMPARISON_CHAIN, &copies::IFS_SAME_COND, &copies::IF_SAME_THEN_ELSE, @@ -960,6 +962,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box len_zero::LenZero); store.register_late_pass(|| box attrs::Attributes); store.register_late_pass(|| box blocks_in_if_conditions::BlocksInIfConditions); + store.register_late_pass(|| box collapsible_match::CollapsibleMatch); store.register_late_pass(|| box unicode::Unicode); store.register_late_pass(|| box unit_return_expecting_ord::UnitReturnExpectingOrd); store.register_late_pass(|| box strings::StringAdd); @@ -1351,6 +1354,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&booleans::NONMINIMAL_BOOL), LintId::of(&bytecount::NAIVE_BYTECOUNT), LintId::of(&collapsible_if::COLLAPSIBLE_IF), + LintId::of(&collapsible_match::COLLAPSIBLE_MATCH), LintId::of(&comparison_chain::COMPARISON_CHAIN), LintId::of(&copies::IFS_SAME_COND), LintId::of(&copies::IF_SAME_THEN_ELSE), @@ -1617,6 +1621,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&blacklisted_name::BLACKLISTED_NAME), LintId::of(&blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS), LintId::of(&collapsible_if::COLLAPSIBLE_IF), + LintId::of(&collapsible_match::COLLAPSIBLE_MATCH), LintId::of(&comparison_chain::COMPARISON_CHAIN), LintId::of(&default::FIELD_REASSIGN_WITH_DEFAULT), LintId::of(&doc::MISSING_SAFETY_DOC), diff --git a/tests/ui/collapsible_match.rs b/tests/ui/collapsible_match.rs new file mode 100644 index 00000000000..75640b70ff0 --- /dev/null +++ b/tests/ui/collapsible_match.rs @@ -0,0 +1,278 @@ +#![warn(clippy::collapsible_match)] +#![allow(clippy::needless_return, clippy::no_effect, clippy::single_match)] + +fn lint_cases(opt_opt: Option>, res_opt: Result, String>) { + // match without block + match res_opt { + Ok(val) => match val { + Some(n) => foo(n), + _ => return, + }, + _ => return, + } + + // match with block + match res_opt { + Ok(val) => match val { + Some(n) => foo(n), + _ => return, + }, + _ => return, + } + + // if let, if let + if let Ok(val) = res_opt { + if let Some(n) = val { + take(n); + } + } + + // if let else, if let else + if let Ok(val) = res_opt { + if let Some(n) = val { + take(n); + } else { + return; + } + } else { + return; + } + + // if let, match + if let Ok(val) = res_opt { + match val { + Some(n) => foo(n), + _ => (), + } + } + + // match, if let + match res_opt { + Ok(val) => { + if let Some(n) = val { + take(n); + } + }, + _ => {}, + } + + // if let else, match + if let Ok(val) = res_opt { + match val { + Some(n) => foo(n), + _ => return, + } + } else { + return; + } + + // match, if let else + match res_opt { + Ok(val) => { + if let Some(n) = val { + take(n); + } else { + return; + } + }, + _ => return, + } + + // None in inner match same as outer wild branch + match res_opt { + Ok(val) => match val { + Some(n) => foo(n), + None => return, + }, + _ => return, + } + + // None in outer match same as inner wild branch + match opt_opt { + Some(val) => match val { + Some(n) => foo(n), + _ => return, + }, + None => return, + } + + // if guards on outer match + { + match res_opt { + Ok(val) if make() => match val { + Some(n) => foo(n), + _ => return, + }, + _ => return, + } + match res_opt { + Ok(val) => match val { + Some(n) => foo(n), + _ => return, + }, + _ if make() => return, + _ => return, + } + } + + // macro + { + macro_rules! mac { + ($outer:expr => $pat:pat, $e:expr => $inner_pat:pat, $then:expr) => { + match $outer { + $pat => match $e { + $inner_pat => $then, + _ => return, + }, + _ => return, + } + }; + } + // Lint this since the patterns are not defined by the macro. + // Allows the lint to work on if_chain! for example. + // Fixing the lint requires knowledge of the specific macro, but we optimistically assume that + // there is still a better way to write this. + mac!(res_opt => Ok(val), val => Some(n), foo(n)); + } +} + +fn negative_cases(res_opt: Result, String>, res_res: Result, String>) { + // no wild pattern in outer match + match res_opt { + Ok(val) => match val { + Some(n) => foo(n), + _ => return, + }, + Err(_) => return, + } + + // inner branch is not wild or None + match res_res { + Ok(val) => match val { + Ok(n) => foo(n), + Err(_) => return, + }, + _ => return, + } + + // statement before inner match + match res_opt { + Ok(val) => { + "hi buddy"; + match val { + Some(n) => foo(n), + _ => return, + } + }, + _ => return, + } + + // statement after inner match + match res_opt { + Ok(val) => { + match val { + Some(n) => foo(n), + _ => return, + } + "hi buddy"; + }, + _ => return, + } + + // wild branches do not match + match res_opt { + Ok(val) => match val { + Some(n) => foo(n), + _ => { + "sup"; + return; + }, + }, + _ => return, + } + + // binding used in if guard + match res_opt { + Ok(val) if val.is_some() => match val { + Some(n) => foo(n), + _ => return, + }, + _ => return, + } + + // binding used in inner match body + match res_opt { + Ok(val) => match val { + Some(_) => take(val), + _ => return, + }, + _ => return, + } + + // if guard on inner match + { + match res_opt { + Ok(val) => match val { + Some(n) if make() => foo(n), + _ => return, + }, + _ => return, + } + match res_opt { + Ok(val) => match val { + _ => make(), + _ if make() => return, + }, + _ => return, + } + } + + // differing macro contexts + { + macro_rules! mac { + ($val:ident) => { + match $val { + Some(n) => foo(n), + _ => return, + } + }; + } + match res_opt { + Ok(val) => mac!(val), + _ => return, + } + } + + // OR pattern + enum E { + A(T), + B(T), + C(T), + }; + match make::>>() { + E::A(val) | E::B(val) => match val { + Some(n) => foo(n), + _ => return, + }, + _ => return, + } + match make::>>() { + Some(val) => match val { + E::A(val) | E::B(val) => foo(val), + _ => return, + }, + _ => return, + } +} + +fn make() -> T { + unimplemented!() +} + +fn foo(t: T) -> U { + unimplemented!() +} + +fn take(t: T) {} + +fn main() {} diff --git a/tests/ui/collapsible_match.stderr b/tests/ui/collapsible_match.stderr new file mode 100644 index 00000000000..a9e4f911914 --- /dev/null +++ b/tests/ui/collapsible_match.stderr @@ -0,0 +1,237 @@ +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:7:20 + | +LL | Ok(val) => match val { + | ____________________^ +LL | | Some(n) => foo(n), +LL | | _ => return, +LL | | }, + | |_________^ + | + = note: `-D clippy::collapsible-match` implied by `-D warnings` +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:7:12 + | +LL | Ok(val) => match val { + | ^^^ Replace this binding +LL | Some(n) => foo(n), + | ^^^^^^^ with this pattern + +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:16:20 + | +LL | Ok(val) => match val { + | ____________________^ +LL | | Some(n) => foo(n), +LL | | _ => return, +LL | | }, + | |_________^ + | +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:16:12 + | +LL | Ok(val) => match val { + | ^^^ Replace this binding +LL | Some(n) => foo(n), + | ^^^^^^^ with this pattern + +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:25:9 + | +LL | / if let Some(n) = val { +LL | | take(n); +LL | | } + | |_________^ + | +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:24:15 + | +LL | if let Ok(val) = res_opt { + | ^^^ Replace this binding +LL | if let Some(n) = val { + | ^^^^^^^ with this pattern + +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:32:9 + | +LL | / if let Some(n) = val { +LL | | take(n); +LL | | } else { +LL | | return; +LL | | } + | |_________^ + | +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:31:15 + | +LL | if let Ok(val) = res_opt { + | ^^^ Replace this binding +LL | if let Some(n) = val { + | ^^^^^^^ with this pattern + +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:43:9 + | +LL | / match val { +LL | | Some(n) => foo(n), +LL | | _ => (), +LL | | } + | |_________^ + | +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:42:15 + | +LL | if let Ok(val) = res_opt { + | ^^^ Replace this binding +LL | match val { +LL | Some(n) => foo(n), + | ^^^^^^^ with this pattern + +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:52:13 + | +LL | / if let Some(n) = val { +LL | | take(n); +LL | | } + | |_____________^ + | +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:51:12 + | +LL | Ok(val) => { + | ^^^ Replace this binding +LL | if let Some(n) = val { + | ^^^^^^^ with this pattern + +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:61:9 + | +LL | / match val { +LL | | Some(n) => foo(n), +LL | | _ => return, +LL | | } + | |_________^ + | +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:60:15 + | +LL | if let Ok(val) = res_opt { + | ^^^ Replace this binding +LL | match val { +LL | Some(n) => foo(n), + | ^^^^^^^ with this pattern + +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:72:13 + | +LL | / if let Some(n) = val { +LL | | take(n); +LL | | } else { +LL | | return; +LL | | } + | |_____________^ + | +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:71:12 + | +LL | Ok(val) => { + | ^^^ Replace this binding +LL | if let Some(n) = val { + | ^^^^^^^ with this pattern + +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:83:20 + | +LL | Ok(val) => match val { + | ____________________^ +LL | | Some(n) => foo(n), +LL | | None => return, +LL | | }, + | |_________^ + | +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:83:12 + | +LL | Ok(val) => match val { + | ^^^ Replace this binding +LL | Some(n) => foo(n), + | ^^^^^^^ with this pattern + +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:92:22 + | +LL | Some(val) => match val { + | ______________________^ +LL | | Some(n) => foo(n), +LL | | _ => return, +LL | | }, + | |_________^ + | +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:92:14 + | +LL | Some(val) => match val { + | ^^^ Replace this binding +LL | Some(n) => foo(n), + | ^^^^^^^ with this pattern + +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:102:34 + | +LL | Ok(val) if make() => match val { + | __________________________________^ +LL | | Some(n) => foo(n), +LL | | _ => return, +LL | | }, + | |_____________^ + | +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:102:16 + | +LL | Ok(val) if make() => match val { + | ^^^ Replace this binding +LL | Some(n) => foo(n), + | ^^^^^^^ with this pattern + +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:109:24 + | +LL | Ok(val) => match val { + | ________________________^ +LL | | Some(n) => foo(n), +LL | | _ => return, +LL | | }, + | |_____________^ + | +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:109:16 + | +LL | Ok(val) => match val { + | ^^^ Replace this binding +LL | Some(n) => foo(n), + | ^^^^^^^ with this pattern + +error: Unnecessary nested match + --> $DIR/collapsible_match.rs:123:29 + | +LL | $pat => match $e { + | _____________________________^ +LL | | $inner_pat => $then, +LL | | _ => return, +LL | | }, + | |_____________________^ +... +LL | mac!(res_opt => Ok(val), val => Some(n), foo(n)); + | ------------------------------------------------- in this macro invocation + | +help: The outer pattern can be modified to include the inner pattern. + --> $DIR/collapsible_match.rs:135:28 + | +LL | mac!(res_opt => Ok(val), val => Some(n), foo(n)); + | ^^^ ^^^^^^^ with this pattern + | | + | Replace this binding + = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to 13 previous errors +