Add collapsible_match lint

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

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<Result<u64, String>>) {
+    ///     let n = match opt {
+    ///         Some(n) => match n {
+    ///             Ok(n) => n,
+    ///             _ => return,
+    ///         }
+    ///         None => return,
+    ///     };
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// fn func(opt: Option<Result<u64, String>>) {
+    ///     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 <local> { .. }
+        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
+        // ..<local>.. => match <local> { .. }
+        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<Span> {
+    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
+}

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),

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<Option<u32>>, res_opt: Result<Option<u32>, 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<Option<u32>, String>, res_res: Result<Result<u32, String>, 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<T> {
+        A(T),
+        B(T),
+        C(T),
+    };
+    match make::<E<Option<u32>>>() {
+        E::A(val) | E::B(val) => match val {
+            Some(n) => foo(n),
+            _ => return,
+        },
+        _ => return,
+    }
+    match make::<Option<E<u32>>>() {
+        Some(val) => match val {
+            E::A(val) | E::B(val) => foo(val),
+            _ => return,
+        },
+        _ => return,
+    }
+}
+
+fn make<T>() -> T {
+    unimplemented!()
+}
+
+fn foo<T, U>(t: T) -> U {
+    unimplemented!()
+}
+
+fn take<T>(t: T) {}
+
+fn main() {}

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
+