Auto merge of #4575 - Manishearth:suggestions, r=oli-obk

Make more tests rustfixable

Fixes #3630

changelog: Improve suggestions for many lints in preparation for `cargo fix --clippy`

r? @phansch @yaahc

clippy_lints/src/entry.rs

@@ -1,5 +1,6 @@
 use crate::utils::SpanlessEq;
-use crate::utils::{get_item_name, higher, match_type, paths, snippet, snippet_opt, span_lint_and_then, walk_ptrs_ty};
+use crate::utils::{get_item_name, higher, match_type, paths, snippet, snippet_opt};
+use crate::utils::{snippet_with_applicability, span_lint_and_then, walk_ptrs_ty};
 use if_chain::if_chain;
 use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
 use rustc::hir::*;
@@ -64,6 +65,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for HashMapPass {
                             } else {
                                 true
                             }
+                        // XXXManishearth we can also check for if/else blocks containing `None`.
                     };
 
                     let mut visitor = InsertVisitor {
@@ -145,10 +147,11 @@ impl<'a, 'tcx, 'b> Visitor<'tcx> for InsertVisitor<'a, 'tcx, 'b> {
                 span_lint_and_then(self.cx, MAP_ENTRY, self.span,
                                    &format!("usage of `contains_key` followed by `insert` on a `{}`", self.ty), |db| {
                     if self.sole_expr {
-                        let help = format!("{}.entry({}).or_insert({})",
-                                           snippet(self.cx, self.map.span, "map"),
-                                           snippet(self.cx, params[1].span, ".."),
-                                           snippet(self.cx, params[2].span, ".."));
+                        let mut app = Applicability::MachineApplicable;
+                        let help = format!("{}.entry({}).or_insert({});",
+                                           snippet_with_applicability(self.cx, self.map.span, "map", &mut app),
+                                           snippet_with_applicability(self.cx, params[1].span, "..", &mut app),
+                                           snippet_with_applicability(self.cx, params[2].span, "..", &mut app));
 
                         db.span_suggestion(
                             self.span,
@@ -158,15 +161,13 @@ impl<'a, 'tcx, 'b> Visitor<'tcx> for InsertVisitor<'a, 'tcx, 'b> {
                         );
                     }
                     else {
-                        let help = format!("{}.entry({})",
+                        let help = format!("consider using `{}.entry({})`",
                                            snippet(self.cx, self.map.span, "map"),
                                            snippet(self.cx, params[1].span, ".."));
 
-                        db.span_suggestion(
+                        db.span_label(
                             self.span,
-                            "consider using",
-                            help,
-                            Applicability::MachineApplicable, // snippet
+                            &help,
                         );
                     }
                 });

clippy_lints/src/eq_op.rs

@@ -118,7 +118,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for EqOp {
                                     left.span,
                                     "use the left value directly",
                                     lsnip,
-                                    Applicability::MachineApplicable, // snippet
+                                    Applicability::MaybeIncorrect, // FIXME #2597
                                 );
                             })
                         } else if !lcpy
@@ -136,7 +136,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for EqOp {
                                         right.span,
                                         "use the right value directly",
                                         rsnip,
-                                        Applicability::MachineApplicable, // snippet
+                                        Applicability::MaybeIncorrect, // FIXME #2597
                                     );
                                 },
                             )

clippy_lints/src/loops.rs

@@ -2427,12 +2427,17 @@ fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>
                     let contains_arg = snippet(cx, args[1].span, "??");
                     let span = shorten_needless_collect_span(expr);
                     span_lint_and_then(cx, NEEDLESS_COLLECT, span, NEEDLESS_COLLECT_MSG, |db| {
+                        let (arg, pred) = if contains_arg.starts_with('&') {
+                            ("x", &contains_arg[1..])
+                        } else {
+                            ("&x", &*contains_arg)
+                        };
                         db.span_suggestion(
                             span,
                             "replace with",
                             format!(
-                                ".any(|&x| x == {})",
-                                if contains_arg.starts_with('&') { &contains_arg[1..] } else { &contains_arg }
+                                ".any(|{}| x == {})",
+                                arg, pred
                             ),
                             Applicability::MachineApplicable,
                         );

clippy_lints/src/map_unit_fn.rs

@@ -217,15 +217,15 @@ fn lint_map_unit_fn(cx: &LateContext<'_, '_>, stmt: &hir::Stmt, expr: &hir::Expr
     if is_unit_function(cx, fn_arg) {
         let msg = suggestion_msg("function", map_type);
         let suggestion = format!(
-            "if let {0}({1}) = {2} {{ {3}(...) }}",
+            "if let {0}({binding}) = {1} {{ {2}({binding}) }}",
             variant,
-            let_binding_name(cx, var_arg),
             snippet(cx, var_arg.span, "_"),
-            snippet(cx, fn_arg.span, "_")
+            snippet(cx, fn_arg.span, "_"),
+            binding = let_binding_name(cx, var_arg)
         );
 
         span_lint_and_then(cx, lint, expr.span, &msg, |db| {
-            db.span_suggestion(stmt.span, "try this", suggestion, Applicability::Unspecified);
+            db.span_suggestion(stmt.span, "try this", suggestion, Applicability::MachineApplicable);
         });
     } else if let Some((binding, closure_expr)) = unit_closure(cx, fn_arg) {
         let msg = suggestion_msg("closure", map_type);
@@ -250,9 +250,9 @@ fn lint_map_unit_fn(cx: &LateContext<'_, '_>, stmt: &hir::Stmt, expr: &hir::Expr
                     "if let {0}({1}) = {2} {{ ... }}",
                     variant,
                     snippet(cx, binding.pat.span, "_"),
-                    snippet(cx, var_arg.span, "_")
+                    snippet(cx, var_arg.span, "_"),
                 );
-                db.span_suggestion(stmt.span, "try this", suggestion, Applicability::Unspecified);
+                db.span_suggestion(stmt.span, "try this", suggestion, Applicability::HasPlaceholders);
             }
         });
     }

clippy_lints/src/misc_early.rs

@@ -1,5 +1,6 @@
 use crate::utils::{
-    constants, snippet, snippet_opt, span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then,
+    constants, snippet_opt, snippet_with_applicability, span_help_and_lint, span_lint, span_lint_and_sugg,
+    span_lint_and_then,
 };
 use if_chain::if_chain;
 use rustc::lint::{in_external_macro, EarlyContext, EarlyLintPass, LintArray, LintContext, LintPass};
@@ -414,13 +415,10 @@ impl EarlyLintPass for MiscEarlyLints {
                                 "Try not to call a closure in the expression where it is declared.",
                                 |db| {
                                     if decl.inputs.is_empty() {
-                                        let hint = snippet(cx, block.span, "..").into_owned();
-                                        db.span_suggestion(
-                                            expr.span,
-                                            "Try doing something like: ",
-                                            hint,
-                                            Applicability::MachineApplicable, // snippet
-                                        );
+                                        let mut app = Applicability::MachineApplicable;
+                                        let hint =
+                                            snippet_with_applicability(cx, block.span, "..", &mut app).into_owned();
+                                        db.span_suggestion(expr.span, "Try doing something like: ", hint, app);
                                     }
                                 },
                             );

clippy_lints/src/needless_borrowed_ref.rs

@@ -2,9 +2,9 @@
 //!
 //! This lint is **warn** by default
 
-use crate::utils::{snippet, span_lint_and_then};
+use crate::utils::{snippet_with_applicability, span_lint_and_then};
 use if_chain::if_chain;
-use rustc::hir::{BindingAnnotation, MutImmutable, Pat, PatKind};
+use rustc::hir::{BindingAnnotation, MutImmutable, Node, Pat, PatKind};
 use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use rustc::{declare_lint_pass, declare_tool_lint};
 use rustc_errors::Applicability;
@@ -65,16 +65,25 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessBorrowedRef {
 
             // Check sub_pat got a `ref` keyword (excluding `ref mut`).
             if let PatKind::Binding(BindingAnnotation::Ref, .., spanned_name, _) = sub_pat.node;
+            let parent_id = cx.tcx.hir().get_parent_node(pat.hir_id);
+            if let Some(parent_node) = cx.tcx.hir().find(parent_id);
             then {
+                // do not recurse within patterns, as they may have other references
+                // XXXManishearth we can relax this constraint if we only check patterns
+                // with a single ref pattern inside them
+                if let Node::Pat(_) = parent_node {
+                    return;
+                }
+                let mut applicability = Applicability::MachineApplicable;
                 span_lint_and_then(cx, NEEDLESS_BORROWED_REFERENCE, pat.span,
                                    "this pattern takes a reference on something that is being de-referenced",
                                    |db| {
-                                       let hint = snippet(cx, spanned_name.span, "..").into_owned();
+                                       let hint = snippet_with_applicability(cx, spanned_name.span, "..", &mut applicability).into_owned();
                                        db.span_suggestion(
                                            pat.span,
                                            "try removing the `&ref` part and just keep",
                                            hint,
-                                           Applicability::MachineApplicable, // snippet
+                                           applicability,
                                        );
                                    });
             }

clippy_lints/src/non_copy_const.rs

@@ -10,7 +10,6 @@ use rustc::lint::{LateContext, LateLintPass, Lint, LintArray, LintPass};
 use rustc::ty::adjustment::Adjust;
 use rustc::ty::{Ty, TypeFlags};
 use rustc::{declare_lint_pass, declare_tool_lint};
-use rustc_errors::Applicability;
 use rustc_typeck::hir_ty_to_ty;
 use syntax_pos::{InnerSpan, Span, DUMMY_SP};
 
@@ -125,16 +124,11 @@ fn verify_ty_bound<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>, source: S
         match source {
             Source::Item { .. } => {
                 let const_kw_span = span.from_inner(InnerSpan::new(0, 5));
-                db.span_suggestion(
-                    const_kw_span,
-                    "make this a static item",
-                    "static".to_string(),
-                    Applicability::MachineApplicable,
-                );
+                db.span_label(const_kw_span, "make this a static item (maybe with lazy_static)");
             },
             Source::Assoc { ty: ty_span, .. } => {
                 if ty.flags.contains(TypeFlags::HAS_FREE_LOCAL_NAMES) {
-                    db.span_help(ty_span, &format!("consider requiring `{}` to be `Copy`", ty));
+                    db.span_label(ty_span, &format!("consider requiring `{}` to be `Copy`", ty));
                 }
             },
             Source::Expr { .. } => {

clippy_lints/src/redundant_pattern_matching.rs

@@ -49,14 +49,22 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantPatternMatching {
         if let ExprKind::Match(ref op, ref arms, ref match_source) = expr.node {
             match match_source {
                 MatchSource::Normal => find_sugg_for_match(cx, expr, op, arms),
-                MatchSource::IfLetDesugar { .. } => find_sugg_for_if_let(cx, expr, op, arms),
+                MatchSource::IfLetDesugar { contains_else_clause } => {
+                    find_sugg_for_if_let(cx, expr, op, arms, *contains_else_clause)
+                },
                 _ => return,
             }
         }
     }
 }
 
-fn find_sugg_for_if_let<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr, op: &P<Expr>, arms: &HirVec<Arm>) {
+fn find_sugg_for_if_let<'a, 'tcx>(
+    cx: &LateContext<'a, 'tcx>,
+    expr: &'tcx Expr,
+    op: &P<Expr>,
+    arms: &HirVec<Arm>,
+    has_else: bool,
+) {
     let good_method = match arms[0].pat.node {
         PatKind::TupleStruct(ref path, ref patterns, _) if patterns.len() == 1 => {
             if let PatKind::Wild = patterns[0].node {
@@ -79,6 +87,8 @@ fn find_sugg_for_if_let<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr,
         _ => return,
     };
 
+    let maybe_semi = if has_else { "" } else { ";" };
+
     span_lint_and_then(
         cx,
         REDUNDANT_PATTERN_MATCHING,
@@ -89,7 +99,7 @@ fn find_sugg_for_if_let<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr,
             db.span_suggestion(
                 span,
                 "try this",
-                format!("{}.{}", snippet(cx, op.span, "_"), good_method),
+                format!("{}.{}{}", snippet(cx, op.span, "_"), good_method, maybe_semi),
                 Applicability::MaybeIncorrect, // snippet
             );
         },

tests/ui/entry_fixable.fixed

@@ -0,0 +1,15 @@
+// run-rustfix
+
+#![allow(unused, clippy::needless_pass_by_value)]
+#![warn(clippy::map_entry)]
+
+use std::collections::{BTreeMap, HashMap};
+use std::hash::Hash;
+
+fn foo() {}
+
+fn insert_if_absent0<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
+    m.entry(k).or_insert(v);
+}
+
+fn main() {}

tests/ui/entry_fixable.rs

@@ -0,0 +1,17 @@
+// run-rustfix
+
+#![allow(unused, clippy::needless_pass_by_value)]
+#![warn(clippy::map_entry)]
+
+use std::collections::{BTreeMap, HashMap};
+use std::hash::Hash;
+
+fn foo() {}
+
+fn insert_if_absent0<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
+    if !m.contains_key(&k) {
+        m.insert(k, v);
+    }
+}
+
+fn main() {}

tests/ui/entry_fixable.stderr

@@ -0,0 +1,12 @@
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry_fixable.rs:12:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         m.insert(k, v);
+LL | |     }
+   | |_____^ help: consider using: `m.entry(k).or_insert(v);`
+   |
+   = note: `-D clippy::map-entry` implied by `-D warnings`
+
+error: aborting due to previous error
+

tests/ui/entry.rs → tests/ui/entry_unfixable.rs

@@ -6,19 +6,6 @@ use std::hash::Hash;
 
 fn foo() {}
 
-fn insert_if_absent0<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
-    if !m.contains_key(&k) {
-        m.insert(k, v);
-    }
-}
-
-fn insert_if_absent1<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
-    if !m.contains_key(&k) {
-        foo();
-        m.insert(k, v);
-    }
-}
-
 fn insert_if_absent2<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
     if !m.contains_key(&k) {
         m.insert(k, v)
@@ -62,6 +49,7 @@ fn insert_in_btreemap<K: Ord, V>(m: &mut BTreeMap<K, V>, k: K, v: V) {
     };
 }
 
+// should not trigger
 fn insert_other_if_absent<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, o: K, v: V) {
     if !m.contains_key(&k) {
         m.insert(o, v);