Skip to content

Commit f3e82fc

Browse files
borsbors
committed
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
2 parents d5570e4 + eb1e9cd commit f3e82fc

File tree

84 files changed

+1574
-790
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

84 files changed

+1574
-790
lines changed

Diff for: ‎clippy_lints/src/entry.rs

+11-10
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use crate::utils::SpanlessEq;
2-
use crate::utils::{get_item_name, higher, match_type, paths, snippet, snippet_opt, span_lint_and_then, walk_ptrs_ty};
2+
use crate::utils::{get_item_name, higher, match_type, paths, snippet, snippet_opt};
3+
use crate::utils::{snippet_with_applicability, span_lint_and_then, walk_ptrs_ty};
34
use if_chain::if_chain;
45
use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
56
use rustc::hir::*;
@@ -64,6 +65,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for HashMapPass {
6465
} else {
6566
true
6667
}
68+
// XXXManishearth we can also check for if/else blocks containing `None`.
6769
};
6870

6971
let mut visitor = InsertVisitor {
@@ -145,10 +147,11 @@ impl<'a, 'tcx, 'b> Visitor<'tcx> for InsertVisitor<'a, 'tcx, 'b> {
145147
span_lint_and_then(self.cx, MAP_ENTRY, self.span,
146148
&format!("usage of `contains_key` followed by `insert` on a `{}`", self.ty), |db| {
147149
if self.sole_expr {
148-
let help = format!("{}.entry({}).or_insert({})",
149-
snippet(self.cx, self.map.span, "map"),
150-
snippet(self.cx, params[1].span, ".."),
151-
snippet(self.cx, params[2].span, ".."));
150+
let mut app = Applicability::MachineApplicable;
151+
let help = format!("{}.entry({}).or_insert({});",
152+
snippet_with_applicability(self.cx, self.map.span, "map", &mut app),
153+
snippet_with_applicability(self.cx, params[1].span, "..", &mut app),
154+
snippet_with_applicability(self.cx, params[2].span, "..", &mut app));
152155

153156
db.span_suggestion(
154157
self.span,
@@ -158,15 +161,13 @@ impl<'a, 'tcx, 'b> Visitor<'tcx> for InsertVisitor<'a, 'tcx, 'b> {
158161
);
159162
}
160163
else {
161-
let help = format!("{}.entry({})",
164+
let help = format!("consider using `{}.entry({})`",
162165
snippet(self.cx, self.map.span, "map"),
163166
snippet(self.cx, params[1].span, ".."));
164167

165-
db.span_suggestion(
168+
db.span_label(
166169
self.span,
167-
"consider using",
168-
help,
169-
Applicability::MachineApplicable, // snippet
170+
&help,
170171
);
171172
}
172173
});

Diff for: ‎clippy_lints/src/loops.rs

+7-2
Original file line numberDiff line numberDiff line change
@@ -2427,12 +2427,17 @@ fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>
24272427
let contains_arg = snippet(cx, args[1].span, "??");
24282428
let span = shorten_needless_collect_span(expr);
24292429
span_lint_and_then(cx, NEEDLESS_COLLECT, span, NEEDLESS_COLLECT_MSG, |db| {
2430+
let (arg, pred) = if contains_arg.starts_with('&') {
2431+
("x", &contains_arg[1..])
2432+
} else {
2433+
("&x", &*contains_arg)
2434+
};
24302435
db.span_suggestion(
24312436
span,
24322437
"replace with",
24332438
format!(
2434-
".any(|&x| x == {})",
2435-
if contains_arg.starts_with('&') { &contains_arg[1..] } else { &contains_arg }
2439+
".any(|{}| x == {})",
2440+
arg, pred
24362441
),
24372442
Applicability::MachineApplicable,
24382443
);

Diff for: ‎clippy_lints/src/map_unit_fn.rs

+6-6
Original file line numberDiff line numberDiff line change
@@ -217,15 +217,15 @@ fn lint_map_unit_fn(cx: &LateContext<'_, '_>, stmt: &hir::Stmt, expr: &hir::Expr
217217
if is_unit_function(cx, fn_arg) {
218218
let msg = suggestion_msg("function", map_type);
219219
let suggestion = format!(
220-
"if let {0}({1}) = {2} {{ {3}(...) }}",
220+
"if let {0}({binding}) = {1} {{ {2}({binding}) }}",
221221
variant,
222-
let_binding_name(cx, var_arg),
223222
snippet(cx, var_arg.span, "_"),
224-
snippet(cx, fn_arg.span, "_")
223+
snippet(cx, fn_arg.span, "_"),
224+
binding = let_binding_name(cx, var_arg)
225225
);
226226

227227
span_lint_and_then(cx, lint, expr.span, &msg, |db| {
228-
db.span_suggestion(stmt.span, "try this", suggestion, Applicability::Unspecified);
228+
db.span_suggestion(stmt.span, "try this", suggestion, Applicability::MachineApplicable);
229229
});
230230
} else if let Some((binding, closure_expr)) = unit_closure(cx, fn_arg) {
231231
let msg = suggestion_msg("closure", map_type);
@@ -250,9 +250,9 @@ fn lint_map_unit_fn(cx: &LateContext<'_, '_>, stmt: &hir::Stmt, expr: &hir::Expr
250250
"if let {0}({1}) = {2} {{ ... }}",
251251
variant,
252252
snippet(cx, binding.pat.span, "_"),
253-
snippet(cx, var_arg.span, "_")
253+
snippet(cx, var_arg.span, "_"),
254254
);
255-
db.span_suggestion(stmt.span, "try this", suggestion, Applicability::Unspecified);
255+
db.span_suggestion(stmt.span, "try this", suggestion, Applicability::HasPlaceholders);
256256
}
257257
});
258258
}

Diff for: ‎clippy_lints/src/misc_early.rs

+6-8
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use crate::utils::{
2-
constants, snippet, snippet_opt, span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then,
2+
constants, snippet_opt, snippet_with_applicability, span_help_and_lint, span_lint, span_lint_and_sugg,
3+
span_lint_and_then,
34
};
45
use if_chain::if_chain;
56
use rustc::lint::{in_external_macro, EarlyContext, EarlyLintPass, LintArray, LintContext, LintPass};
@@ -414,13 +415,10 @@ impl EarlyLintPass for MiscEarlyLints {
414415
"Try not to call a closure in the expression where it is declared.",
415416
|db| {
416417
if decl.inputs.is_empty() {
417-
let hint = snippet(cx, block.span, "..").into_owned();
418-
db.span_suggestion(
419-
expr.span,
420-
"Try doing something like: ",
421-
hint,
422-
Applicability::MachineApplicable, // snippet
423-
);
418+
let mut app = Applicability::MachineApplicable;
419+
let hint =
420+
snippet_with_applicability(cx, block.span, "..", &mut app).into_owned();
421+
db.span_suggestion(expr.span, "Try doing something like: ", hint, app);
424422
}
425423
},
426424
);

Diff for: ‎clippy_lints/src/needless_borrowed_ref.rs

+13-4
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22
//!
33
//! This lint is **warn** by default
44
5-
use crate::utils::{snippet, span_lint_and_then};
5+
use crate::utils::{snippet_with_applicability, span_lint_and_then};
66
use if_chain::if_chain;
7-
use rustc::hir::{BindingAnnotation, MutImmutable, Pat, PatKind};
7+
use rustc::hir::{BindingAnnotation, MutImmutable, Node, Pat, PatKind};
88
use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
99
use rustc::{declare_lint_pass, declare_tool_lint};
1010
use rustc_errors::Applicability;
@@ -65,16 +65,25 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessBorrowedRef {
6565

6666
// Check sub_pat got a `ref` keyword (excluding `ref mut`).
6767
if let PatKind::Binding(BindingAnnotation::Ref, .., spanned_name, _) = sub_pat.node;
68+
let parent_id = cx.tcx.hir().get_parent_node(pat.hir_id);
69+
if let Some(parent_node) = cx.tcx.hir().find(parent_id);
6870
then {
71+
// do not recurse within patterns, as they may have other references
72+
// XXXManishearth we can relax this constraint if we only check patterns
73+
// with a single ref pattern inside them
74+
if let Node::Pat(_) = parent_node {
75+
return;
76+
}
77+
let mut applicability = Applicability::MachineApplicable;
6978
span_lint_and_then(cx, NEEDLESS_BORROWED_REFERENCE, pat.span,
7079
"this pattern takes a reference on something that is being de-referenced",
7180
|db| {
72-
let hint = snippet(cx, spanned_name.span, "..").into_owned();
81+
let hint = snippet_with_applicability(cx, spanned_name.span, "..", &mut applicability).into_owned();
7382
db.span_suggestion(
7483
pat.span,
7584
"try removing the `&ref` part and just keep",
7685
hint,
77-
Applicability::MachineApplicable, // snippet
86+
applicability,
7887
);
7988
});
8089
}

Diff for: ‎clippy_lints/src/non_copy_const.rs

+2-8
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ use rustc::lint::{LateContext, LateLintPass, Lint, LintArray, LintPass};
1010
use rustc::ty::adjustment::Adjust;
1111
use rustc::ty::{Ty, TypeFlags};
1212
use rustc::{declare_lint_pass, declare_tool_lint};
13-
use rustc_errors::Applicability;
1413
use rustc_typeck::hir_ty_to_ty;
1514
use syntax_pos::{InnerSpan, Span, DUMMY_SP};
1615

@@ -125,16 +124,11 @@ fn verify_ty_bound<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>, source: S
125124
match source {
126125
Source::Item { .. } => {
127126
let const_kw_span = span.from_inner(InnerSpan::new(0, 5));
128-
db.span_suggestion(
129-
const_kw_span,
130-
"make this a static item",
131-
"static".to_string(),
132-
Applicability::MachineApplicable,
133-
);
127+
db.span_label(const_kw_span, "make this a static item (maybe with lazy_static)");
134128
},
135129
Source::Assoc { ty: ty_span, .. } => {
136130
if ty.flags.contains(TypeFlags::HAS_FREE_LOCAL_NAMES) {
137-
db.span_help(ty_span, &format!("consider requiring `{}` to be `Copy`", ty));
131+
db.span_label(ty_span, &format!("consider requiring `{}` to be `Copy`", ty));
138132
}
139133
},
140134
Source::Expr { .. } => {

Diff for: ‎clippy_lints/src/redundant_pattern_matching.rs

+13-3
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,22 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantPatternMatching {
4949
if let ExprKind::Match(ref op, ref arms, ref match_source) = expr.node {
5050
match match_source {
5151
MatchSource::Normal => find_sugg_for_match(cx, expr, op, arms),
52-
MatchSource::IfLetDesugar { .. } => find_sugg_for_if_let(cx, expr, op, arms),
52+
MatchSource::IfLetDesugar { contains_else_clause } => {
53+
find_sugg_for_if_let(cx, expr, op, arms, *contains_else_clause)
54+
},
5355
_ => return,
5456
}
5557
}
5658
}
5759
}
5860

59-
fn find_sugg_for_if_let<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr, op: &P<Expr>, arms: &HirVec<Arm>) {
61+
fn find_sugg_for_if_let<'a, 'tcx>(
62+
cx: &LateContext<'a, 'tcx>,
63+
expr: &'tcx Expr,
64+
op: &P<Expr>,
65+
arms: &HirVec<Arm>,
66+
has_else: bool,
67+
) {
6068
let good_method = match arms[0].pat.node {
6169
PatKind::TupleStruct(ref path, ref patterns, _) if patterns.len() == 1 => {
6270
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,
7987
_ => return,
8088
};
8189

90+
let maybe_semi = if has_else { "" } else { ";" };
91+
8292
span_lint_and_then(
8393
cx,
8494
REDUNDANT_PATTERN_MATCHING,
@@ -89,7 +99,7 @@ fn find_sugg_for_if_let<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr,
8999
db.span_suggestion(
90100
span,
91101
"try this",
92-
format!("{}.{}", snippet(cx, op.span, "_"), good_method),
102+
format!("{}.{}{}", snippet(cx, op.span, "_"), good_method, maybe_semi),
93103
Applicability::MaybeIncorrect, // snippet
94104
);
95105
},

Diff for: ‎tests/ui/entry.stderr

+68-6
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,16 @@ LL | / if !m.contains_key(&k) {
1515
LL | | foo();
1616
LL | | m.insert(k, v);
1717
LL | | }
18-
| |_____^ help: consider using: `m.entry(k)`
18+
| |_____^
19+
|
20+
help: consider using `m.entry(k)`
21+
--> $DIR/entry.rs:16:5
22+
|
23+
LL | / if !m.contains_key(&k) {
24+
LL | | foo();
25+
LL | | m.insert(k, v);
26+
LL | | }
27+
| |_____^
1928

2029
error: usage of `contains_key` followed by `insert` on a `HashMap`
2130
--> $DIR/entry.rs:23:5
@@ -25,7 +34,17 @@ LL | | m.insert(k, v)
2534
LL | | } else {
2635
LL | | None
2736
LL | | };
28-
| |_____^ help: consider using: `m.entry(k)`
37+
| |_____^
38+
|
39+
help: consider using `m.entry(k)`
40+
--> $DIR/entry.rs:23:5
41+
|
42+
LL | / if !m.contains_key(&k) {
43+
LL | | m.insert(k, v)
44+
LL | | } else {
45+
LL | | None
46+
LL | | };
47+
| |_____^
2948

3049
error: usage of `contains_key` followed by `insert` on a `HashMap`
3150
--> $DIR/entry.rs:31:5
@@ -35,7 +54,17 @@ LL | | None
3554
LL | | } else {
3655
LL | | m.insert(k, v)
3756
LL | | };
38-
| |_____^ help: consider using: `m.entry(k)`
57+
| |_____^
58+
|
59+
help: consider using `m.entry(k)`
60+
--> $DIR/entry.rs:31:5
61+
|
62+
LL | / if m.contains_key(&k) {
63+
LL | | None
64+
LL | | } else {
65+
LL | | m.insert(k, v)
66+
LL | | };
67+
| |_____^
3968

4069
error: usage of `contains_key` followed by `insert` on a `HashMap`
4170
--> $DIR/entry.rs:39:5
@@ -46,7 +75,18 @@ LL | | m.insert(k, v)
4675
LL | | } else {
4776
LL | | None
4877
LL | | };
49-
| |_____^ help: consider using: `m.entry(k)`
78+
| |_____^
79+
|
80+
help: consider using `m.entry(k)`
81+
--> $DIR/entry.rs:39:5
82+
|
83+
LL | / if !m.contains_key(&k) {
84+
LL | | foo();
85+
LL | | m.insert(k, v)
86+
LL | | } else {
87+
LL | | None
88+
LL | | };
89+
| |_____^
5090

5191
error: usage of `contains_key` followed by `insert` on a `HashMap`
5292
--> $DIR/entry.rs:48:5
@@ -57,7 +97,18 @@ LL | | } else {
5797
LL | | foo();
5898
LL | | m.insert(k, v)
5999
LL | | };
60-
| |_____^ help: consider using: `m.entry(k)`
100+
| |_____^
101+
|
102+
help: consider using `m.entry(k)`
103+
--> $DIR/entry.rs:48:5
104+
|
105+
LL | / if m.contains_key(&k) {
106+
LL | | None
107+
LL | | } else {
108+
LL | | foo();
109+
LL | | m.insert(k, v)
110+
LL | | };
111+
| |_____^
61112

62113
error: usage of `contains_key` followed by `insert` on a `BTreeMap`
63114
--> $DIR/entry.rs:57:5
@@ -68,7 +119,18 @@ LL | | m.insert(k, v)
68119
LL | | } else {
69120
LL | | None
70121
LL | | };
71-
| |_____^ help: consider using: `m.entry(k)`
122+
| |_____^
123+
|
124+
help: consider using `m.entry(k)`
125+
--> $DIR/entry.rs:57:5
126+
|
127+
LL | / if !m.contains_key(&k) {
128+
LL | | foo();
129+
LL | | m.insert(k, v)
130+
LL | | } else {
131+
LL | | None
132+
LL | | };
133+
| |_____^
72134

73135
error: aborting due to 7 previous errors
74136

Diff for: ‎tests/ui/entry_fixable.fixed

+15
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// run-rustfix
2+
3+
#![allow(unused, clippy::needless_pass_by_value)]
4+
#![warn(clippy::map_entry)]
5+
6+
use std::collections::{BTreeMap, HashMap};
7+
use std::hash::Hash;
8+
9+
fn foo() {}
10+
11+
fn insert_if_absent0<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
12+
m.entry(k).or_insert(v);
13+
}
14+
15+
fn main() {}

Diff for: ‎tests/ui/entry_fixable.rs

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// run-rustfix
2+
3+
#![allow(unused, clippy::needless_pass_by_value)]
4+
#![warn(clippy::map_entry)]
5+
6+
use std::collections::{BTreeMap, HashMap};
7+
use std::hash::Hash;
8+
9+
fn foo() {}
10+
11+
fn insert_if_absent0<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
12+
if !m.contains_key(&k) {
13+
m.insert(k, v);
14+
}
15+
}
16+
17+
fn main() {}

0 commit comments

Comments
 (0)