Auto merge of rust-lang#6619 - camsteffen:collapsible-match, r=camsteffen

Improve collapsible_match

changelog: Fix collapsible_match false negatives

Allow `&` and/or `*` on the binding and make sure the type still matches.

Author: bors

clippy_lints/src/collapsible_match.rs

@@ -2,7 +2,7 @@ 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_hir::{Arm, Expr, ExprKind, Guard, HirId, Pat, PatKind, QPath, StmtKind, UnOp};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::{DefIdTree, TyCtxt};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -72,8 +72,7 @@ fn check_arm(arm: &Arm<'_>, wild_outer_arm: &Arm<'_>, cx: &LateContext<'_>) {
         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;
+        if let Some(binding_id) = addr_adjusted_binding(expr_in, cx);
         // 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) =
@@ -85,7 +84,12 @@ fn check_arm(arm: &Arm<'_>, wild_outer_arm: &Arm<'_>, cx: &LateContext<'_>) {
         // 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));
+        if match arm.guard {
+            None => true,
+            Some(Guard::If(expr) | Guard::IfLet(_, expr)) => {
+                !LocalUsedVisitor::new(binding_id).check_expr(expr)
+            }
+        };
         // ...or anywhere in the inner match
         if !arms_inner.iter().any(|arm| LocalUsedVisitor::new(binding_id).check_arm(arm));
         then {
@@ -170,3 +174,20 @@ fn is_none_ctor(res: Res, tcx: TyCtxt<'_>) -> bool {
     }
     false
 }
+
+/// Retrieves a binding ID with optional `&` and/or `*` operators removed. (e.g. `&**foo`)
+/// Returns `None` if a non-reference type is de-referenced.
+/// For example, if `Vec` is de-referenced to a slice, `None` is returned.
+fn addr_adjusted_binding(mut expr: &Expr<'_>, cx: &LateContext<'_>) -> Option<HirId> {
+    loop {
+        match expr.kind {
+            ExprKind::AddrOf(_, _, e) => expr = e,
+            ExprKind::Path(QPath::Resolved(None, path)) => match path.res {
+                Res::Local(binding_id) => break Some(binding_id),
+                _ => break None,
+            },
+            ExprKind::Unary(UnOp::UnDeref, e) if cx.typeck_results().expr_ty(e).is_ref() => expr = e,
+            _ => break None,
+        }
+    }
+}

clippy_lints/src/main_recursion.rs

@@ -43,8 +43,7 @@ impl LateLintPass<'_> for MainRecursion {
 
         if_chain! {
             if let ExprKind::Call(func, _) = &expr.kind;
-            if let ExprKind::Path(path) = &func.kind;
-            if let QPath::Resolved(_, path) = &path;
+            if let ExprKind::Path(QPath::Resolved(_, path)) = &func.kind;
             if let Some(def_id) = path.res.opt_def_id();
             if is_entrypoint_fn(cx, def_id);
             then {

clippy_lints/src/ptr.rs

@@ -263,8 +263,7 @@ fn check_fn(cx: &LateContext<'_>, decl: &FnDecl<'_>, fn_id: HirId, opt_body_id:
             } else if match_type(cx, ty, &paths::COW) {
                 if_chain! {
                     if let TyKind::Rptr(_, MutTy { ref ty, ..} ) = arg.kind;
-                    if let TyKind::Path(ref path) = ty.kind;
-                    if let QPath::Resolved(None, ref pp) = *path;
+                    if let TyKind::Path(QPath::Resolved(None, ref pp)) = ty.kind;
                     if let [ref bx] = *pp.segments;
                     if let Some(ref params) = bx.args;
                     if !params.parenthesized;

clippy_lints/src/utils/higher.rs

@@ -158,8 +158,7 @@ pub fn for_loop<'tcx>(
 /// `while cond { body }` becomes `(cond, body)`.
 pub fn while_loop<'tcx>(expr: &'tcx hir::Expr<'tcx>) -> Option<(&'tcx hir::Expr<'tcx>, &'tcx hir::Expr<'tcx>)> {
     if_chain! {
-        if let hir::ExprKind::Loop(block, _, hir::LoopSource::While) = &expr.kind;
-        if let hir::Block { expr: Some(expr), .. } = &**block;
+        if let hir::ExprKind::Loop(hir::Block { expr: Some(expr), .. }, _, hir::LoopSource::While) = &expr.kind;
         if let hir::ExprKind::Match(cond, arms, hir::MatchSource::WhileDesugar) = &expr.kind;
         if let hir::ExprKind::DropTemps(cond) = &cond.kind;
         if let [hir::Arm { body, .. }, ..] = &arms[..];

clippy_lints/src/utils/mod.rs

@@ -1126,8 +1126,7 @@ pub fn is_self(slf: &Param<'_>) -> bool {
 
 pub fn is_self_ty(slf: &hir::Ty<'_>) -> bool {
     if_chain! {
-        if let TyKind::Path(ref qp) = slf.kind;
-        if let QPath::Resolved(None, ref path) = *qp;
+        if let TyKind::Path(QPath::Resolved(None, ref path)) = slf.kind;
         if let Res::SelfTy(..) = path.res;
         then {
             return true

tests/ui/collapsible_match2.rs

@@ -40,6 +40,35 @@ fn lint_cases(opt_opt: Option<Option<u32>>, res_opt: Result<Option<u32>, String>
         // there is still a better way to write this.
         mac!(res_opt => Ok(val), val => Some(n), foo(n));
     }
+
+    // deref reference value
+    match Some(&[1]) {
+        Some(s) => match *s {
+            [n] => foo(n),
+            _ => (),
+        },
+        _ => (),
+    }
+
+    // ref pattern and deref
+    match Some(&[1]) {
+        Some(ref s) => match &*s {
+            [n] => foo(n),
+            _ => (),
+        },
+        _ => (),
+    }
+}
+
+fn no_lint() {
+    // deref inner value (cannot pattern match with Vec)
+    match Some(vec![1]) {
+        Some(s) => match *s {
+            [n] => foo(n),
+            _ => (),
+        },
+        _ => (),
+    }
 }
 
 fn make<T>() -> T {

tests/ui/collapsible_match2.stderr

@@ -57,5 +57,41 @@ LL |         mac!(res_opt => Ok(val), val => Some(n), foo(n));
    |                            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 3 previous errors
+error: Unnecessary nested match
+  --> $DIR/collapsible_match2.rs:46:20
+   |
+LL |           Some(s) => match *s {
+   |  ____________________^
+LL | |             [n] => foo(n),
+LL | |             _ => (),
+LL | |         },
+   | |_________^
+   |
+help: The outer pattern can be modified to include the inner pattern.
+  --> $DIR/collapsible_match2.rs:46:14
+   |
+LL |         Some(s) => match *s {
+   |              ^ Replace this binding
+LL |             [n] => foo(n),
+   |             ^^^ with this pattern
+
+error: Unnecessary nested match
+  --> $DIR/collapsible_match2.rs:55:24
+   |
+LL |           Some(ref s) => match &*s {
+   |  ________________________^
+LL | |             [n] => foo(n),
+LL | |             _ => (),
+LL | |         },
+   | |_________^
+   |
+help: The outer pattern can be modified to include the inner pattern.
+  --> $DIR/collapsible_match2.rs:55:14
+   |
+LL |         Some(ref s) => match &*s {
+   |              ^^^^^ Replace this binding
+LL |             [n] => foo(n),
+   |             ^^^ with this pattern
+
+error: aborting due to 5 previous errors