Auto merge of rust-lang#5034 - ThibsG:MatchWildErrArmImprove5024, r=f…

…lip1995

Match wild err arm improvements

This lint should trigger on other identifiers which have `_` prefix (such as `_e`) and only if they are unused in the panic block.

_Note_: the `is_unused` function is greatly inspired from `pat_is_wild` function in [loops lints](https://github.com/rust-lang/rust-clippy/blob/43ac9416d935942d6c7d2b2e0c876c551652c4ec/clippy_lints/src/loops.rs#L1689).
I've been considering doing some refactoring, maybe in utils. Maybe this PR or a new one. What do you think ?

fixes rust-lang#5024

changelog: none

clippy_lints/src/loops.rs

@@ -1,37 +1,35 @@
+use crate::consts::{constant, Constant};
 use crate::reexport::*;
+use crate::utils::paths;
+use crate::utils::usage::{is_unused, mutated_variables};
+use crate::utils::{
+    get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait,
+    is_integer_const, is_refutable, last_path_segment, match_trait_method, match_type, match_var, multispan_sugg,
+    snippet, snippet_opt, snippet_with_applicability, span_help_and_lint, span_lint, span_lint_and_sugg,
+    span_lint_and_then, SpanlessEq,
+};
+use crate::utils::{is_type_diagnostic_item, qpath_res, same_tys, sext, sugg};
 use if_chain::if_chain;
 use itertools::Itertools;
+use rustc::hir::map::Map;
 use rustc::lint::in_external_macro;
 use rustc::middle::region;
+use rustc::ty::{self, Ty};
+use rustc_data_structures::fx::{FxHashMap, FxHashSet};
+use rustc_errors::Applicability;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id;
 use rustc_hir::intravisit::{walk_block, walk_expr, walk_pat, walk_stmt, NestedVisitorMap, Visitor};
 use rustc_hir::*;
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-// use rustc::middle::region::CodeExtent;
-use crate::consts::{constant, Constant};
-use crate::utils::usage::mutated_variables;
-use crate::utils::{is_type_diagnostic_item, qpath_res, same_tys, sext, sugg};
-use rustc::hir::map::Map;
-use rustc::ty::{self, Ty};
-use rustc_data_structures::fx::{FxHashMap, FxHashSet};
-use rustc_errors::Applicability;
 use rustc_span::source_map::Span;
 use rustc_span::{BytePos, Symbol};
 use rustc_typeck::expr_use_visitor::*;
 use std::iter::{once, Iterator};
 use std::mem;
 use syntax::ast;
 
-use crate::utils::paths;
-use crate::utils::{
-    get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait,
-    is_integer_const, is_refutable, last_path_segment, match_trait_method, match_type, match_var, multispan_sugg,
-    snippet, snippet_opt, snippet_with_applicability, span_help_and_lint, span_lint, span_lint_and_sugg,
-    span_lint_and_then, SpanlessEq,
-};
-
 declare_clippy_lint! {
     /// **What it does:** Checks for for-loops that manually copy items between
     /// slices that could be optimized by having a memcpy.
@@ -1689,39 +1687,11 @@ fn check_for_mutation(
 fn pat_is_wild<'tcx>(pat: &'tcx PatKind<'_>, body: &'tcx Expr<'_>) -> bool {
     match *pat {
         PatKind::Wild => true,
-        PatKind::Binding(.., ident, None) if ident.as_str().starts_with('_') => {
-            let mut visitor = UsedVisitor {
-                var: ident.name,
-                used: false,
-            };
-            walk_expr(&mut visitor, body);
-            !visitor.used
-        },
+        PatKind::Binding(.., ident, None) if ident.as_str().starts_with('_') => is_unused(&ident, body),
         _ => false,
     }
 }
 
-struct UsedVisitor {
-    var: ast::Name, // var to look for
-    used: bool,     // has the var been used otherwise?
-}
-
-impl<'tcx> Visitor<'tcx> for UsedVisitor {
-    type Map = Map<'tcx>;
-
-    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
-        if match_var(expr, self.var) {
-            self.used = true;
-        } else {
-            walk_expr(self, expr);
-        }
-    }
-
-    fn nested_visit_map(&mut self) -> NestedVisitorMap<'_, Self::Map> {
-        NestedVisitorMap::None
-    }
-}
-
 struct LocalUsedVisitor<'a, 'tcx> {
     cx: &'a LateContext<'a, 'tcx>,
     local: HirId,

clippy_lints/src/matches.rs

@@ -1,8 +1,9 @@
 use crate::consts::{constant, miri_to_const, Constant};
 use crate::utils::paths;
 use crate::utils::sugg::Sugg;
+use crate::utils::usage::is_unused;
 use crate::utils::{
-    expr_block, is_allowed, is_expn_of, match_qpath, match_type, multispan_sugg, remove_blocks, snippet,
+    expr_block, is_allowed, is_expn_of, is_wild, match_qpath, match_type, multispan_sugg, remove_blocks, snippet,
     snippet_with_applicability, span_help_and_lint, span_lint_and_sugg, span_lint_and_then, span_note_and_lint,
     walk_ptrs_ty,
 };
@@ -461,33 +462,40 @@ fn check_overlapping_arms<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ex: &'tcx Expr<'
     }
 }
 
-fn is_wild<'tcx>(pat: &impl std::ops::Deref<Target = Pat<'tcx>>) -> bool {
-    match pat.kind {
-        PatKind::Wild => true,
-        _ => false,
-    }
-}
-
 fn check_wild_err_arm(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>]) {
     let ex_ty = walk_ptrs_ty(cx.tables.expr_ty(ex));
     if match_type(cx, ex_ty, &paths::RESULT) {
         for arm in arms {
             if let PatKind::TupleStruct(ref path, ref inner, _) = arm.pat.kind {
                 let path_str = print::to_string(print::NO_ANN, |s| s.print_qpath(path, false));
-                if_chain! {
-                    if path_str == "Err";
-                    if inner.iter().any(is_wild);
-                    if let ExprKind::Block(ref block, _) = arm.body.kind;
-                    if is_panic_block(block);
-                    then {
-                        // `Err(_)` arm with `panic!` found
-                        span_note_and_lint(cx,
-                                           MATCH_WILD_ERR_ARM,
-                                           arm.pat.span,
-                                           "`Err(_)` will match all errors, maybe not a good idea",
-                                           arm.pat.span,
-                                           "to remove this warning, match each error separately \
-                                            or use `unreachable!` macro");
+                if path_str == "Err" {
+                    let mut matching_wild = inner.iter().any(is_wild);
+                    let mut ident_bind_name = String::from("_");
+                    if !matching_wild {
+                        // Looking for unused bindings (i.e.: `_e`)
+                        inner.iter().for_each(|pat| {
+                            if let PatKind::Binding(.., ident, None) = &pat.kind {
+                                if ident.as_str().starts_with('_') && is_unused(ident, arm.body) {
+                                    ident_bind_name = (&ident.name.as_str()).to_string();
+                                    matching_wild = true;
+                                }
+                            }
+                        });
+                    }
+                    if_chain! {
+                        if matching_wild;
+                        if let ExprKind::Block(ref block, _) = arm.body.kind;
+                        if is_panic_block(block);
+                        then {
+                            // `Err(_)` or `Err(_e)` arm with `panic!` found
+                            span_note_and_lint(cx,
+                                MATCH_WILD_ERR_ARM,
+                                arm.pat.span,
+                                &format!("`Err({})` matches all errors", &ident_bind_name),
+                                arm.pat.span,
+                                "match each error separately or use the error output",
+                            );
+                        }
                     }
                 }
             }

clippy_lints/src/utils/mod.rs

@@ -127,6 +127,14 @@ pub fn is_present_in_source<T: LintContext>(cx: &T, span: Span) -> bool {
     true
 }
 
+/// Checks if given pattern is a wildcard (`_`)
+pub fn is_wild<'tcx>(pat: &impl std::ops::Deref<Target = Pat<'tcx>>) -> bool {
+    match pat.kind {
+        PatKind::Wild => true,
+        _ => false,
+    }
+}
+
 /// Checks if type is struct, enum or union type with the given def path.
 pub fn match_type(cx: &LateContext<'_, '_>, ty: Ty<'_>, path: &[&str]) -> bool {
     match ty.kind {

clippy_lints/src/utils/usage.rs

@@ -1,9 +1,14 @@
+use crate::utils::match_var;
+use rustc::hir::map::Map;
 use rustc::ty;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_hir::def::Res;
+use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
 use rustc_hir::*;
 use rustc_lint::LateContext;
+use rustc_span::symbol::Ident;
 use rustc_typeck::expr_use_visitor::*;
+use syntax::ast;
 
 /// Returns a set of mutated local variable IDs, or `None` if mutations could not be determined.
 pub fn mutated_variables<'a, 'tcx>(expr: &'tcx Expr<'_>, cx: &'a LateContext<'a, 'tcx>) -> Option<FxHashSet<HirId>> {
@@ -70,3 +75,33 @@ impl<'tcx> Delegate<'tcx> for MutVarsDelegate {
         self.update(&cmt)
     }
 }
+
+pub struct UsedVisitor {
+    pub var: ast::Name, // var to look for
+    pub used: bool,     // has the var been used otherwise?
+}
+
+impl<'tcx> Visitor<'tcx> for UsedVisitor {
+    type Map = Map<'tcx>;
+
+    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
+        if match_var(expr, self.var) {
+            self.used = true;
+        } else {
+            walk_expr(self, expr);
+        }
+    }
+
+    fn nested_visit_map(&mut self) -> NestedVisitorMap<'_, Self::Map> {
+        NestedVisitorMap::None
+    }
+}
+
+pub fn is_unused<'tcx>(ident: &'tcx Ident, body: &'tcx Expr<'_>) -> bool {
+    let mut visitor = UsedVisitor {
+        var: ident.name,
+        used: false,
+    };
+    walk_expr(&mut visitor, body);
+    !visitor.used
+}

tests/ui/matches.rs

@@ -28,6 +28,19 @@ fn match_wild_err_arm() {
         },
     }
 
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_e) => panic!(),
+    }
+
+    // Allowed when used in `panic!`.
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_e) => panic!("{}", _e),
+    }
+
     // Allowed when not with `panic!` block.
     match x {
         Ok(3) => println!("ok"),