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

Make more tests rustfixable

Burning through #3630

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

r? @phansch @yaahc

Cargo.toml

@@ -46,7 +46,7 @@ rustc_tools_util = { version = "0.2.0", path = "rustc_tools_util"}
 
 [dev-dependencies]
 cargo_metadata = "0.8.0"
-compiletest_rs = { version = "0.3.22", features = ["tmp"] }
+compiletest_rs = { version = "0.3.23", features = ["tmp"] }
 lazy_static = "1.0"
 clippy-mini-macro-test = { version = "0.2", path = "mini-macro" }
 serde = { version = "1.0", features = ["derive"] }

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/loops.rs

@@ -2429,12 +2429,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,14 @@ 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) {
     if arms[0].pats.len() == 1 {
         let good_method = match arms[0].pats[0].node {
             PatKind::TupleStruct(ref path, ref patterns, _) if patterns.len() == 1 => {
@@ -80,6 +80,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,
@@ -90,7 +92,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.stderr

@@ -15,7 +15,16 @@ LL | /     if !m.contains_key(&k) {
 LL | |         foo();
 LL | |         m.insert(k, v);
 LL | |     }
-   | |_____^ help: consider using: `m.entry(k)`
+   | |_____^
+   |
+help: consider using `m.entry(k)`
+  --> $DIR/entry.rs:16:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         foo();
+LL | |         m.insert(k, v);
+LL | |     }
+   | |_____^
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry.rs:23:5
@@ -25,7 +34,17 @@ LL | |         m.insert(k, v)
 LL | |     } else {
 LL | |         None
 LL | |     };
-   | |_____^ help: consider using: `m.entry(k)`
+   | |_____^
+   |
+help: consider using `m.entry(k)`
+  --> $DIR/entry.rs:23:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         m.insert(k, v)
+LL | |     } else {
+LL | |         None
+LL | |     };
+   | |_____^
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry.rs:31:5
@@ -35,7 +54,17 @@ LL | |         None
 LL | |     } else {
 LL | |         m.insert(k, v)
 LL | |     };
-   | |_____^ help: consider using: `m.entry(k)`
+   | |_____^
+   |
+help: consider using `m.entry(k)`
+  --> $DIR/entry.rs:31:5
+   |
+LL | /     if m.contains_key(&k) {
+LL | |         None
+LL | |     } else {
+LL | |         m.insert(k, v)
+LL | |     };
+   | |_____^
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry.rs:39:5
@@ -46,7 +75,18 @@ LL | |         m.insert(k, v)
 LL | |     } else {
 LL | |         None
 LL | |     };
-   | |_____^ help: consider using: `m.entry(k)`
+   | |_____^
+   |
+help: consider using `m.entry(k)`
+  --> $DIR/entry.rs:39:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         foo();
+LL | |         m.insert(k, v)
+LL | |     } else {
+LL | |         None
+LL | |     };
+   | |_____^
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
   --> $DIR/entry.rs:48:5
@@ -57,7 +97,18 @@ LL | |     } else {
 LL | |         foo();
 LL | |         m.insert(k, v)
 LL | |     };
-   | |_____^ help: consider using: `m.entry(k)`
+   | |_____^
+   |
+help: consider using `m.entry(k)`
+  --> $DIR/entry.rs:48:5
+   |
+LL | /     if m.contains_key(&k) {
+LL | |         None
+LL | |     } else {
+LL | |         foo();
+LL | |         m.insert(k, v)
+LL | |     };
+   | |_____^
 
 error: usage of `contains_key` followed by `insert` on a `BTreeMap`
   --> $DIR/entry.rs:57:5
@@ -68,7 +119,18 @@ LL | |         m.insert(k, v)
 LL | |     } else {
 LL | |         None
 LL | |     };
-   | |_____^ help: consider using: `m.entry(k)`
+   | |_____^
+   |
+help: consider using `m.entry(k)`
+  --> $DIR/entry.rs:57:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         foo();
+LL | |         m.insert(k, v)
+LL | |     } else {
+LL | |         None
+LL | |     };
+   | |_____^
 
 error: aborting due to 7 previous errors
 

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() {}