Skip to content

Commit 7387b87

Browse files
committed
Auto merge of #6197 - ThibsG:ImproveFilterNext, r=ebroto
Improve suggestions for several lints This PR is a follow-up of this [Zulip discussion](https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/filter_next.20lint). It unifies placeholders for `methods` module and improves several suggestions for `filter_next`, `filter_map_next` and `map_unwrap_or` lints. changelog: none
2 parents 0be6544 + c0dd1f9 commit 7387b87

22 files changed

+333
-188
lines changed

clippy_lints/src/methods/mod.rs

Lines changed: 45 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,7 @@ use crate::utils::{
3232
is_copy, is_expn_of, is_type_diagnostic_item, iter_input_pats, last_path_segment, match_def_path, match_qpath,
3333
match_trait_method, match_type, match_var, method_calls, method_chain_args, paths, remove_blocks, return_ty,
3434
single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint,
35-
span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, sugg, walk_ptrs_ty_depth,
36-
SpanlessEq,
35+
span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, walk_ptrs_ty_depth, SpanlessEq,
3736
};
3837

3938
declare_clippy_lint! {
@@ -1735,7 +1734,7 @@ fn lint_or_fun_call<'tcx>(
17351734
"try this",
17361735
format!(
17371736
"{}.unwrap_or_default()",
1738-
snippet_with_applicability(cx, self_expr.span, "_", &mut applicability)
1737+
snippet_with_applicability(cx, self_expr.span, "..", &mut applicability)
17391738
),
17401739
applicability,
17411740
);
@@ -2142,7 +2141,7 @@ fn lint_clone_on_ref_ptr(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::
21422141
return;
21432142
};
21442143

2145-
let snippet = snippet_with_macro_callsite(cx, arg.span, "_");
2144+
let snippet = snippet_with_macro_callsite(cx, arg.span, "..");
21462145

21472146
span_lint_and_sugg(
21482147
cx,
@@ -2178,9 +2177,9 @@ fn lint_string_extend(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::E
21782177
"try this",
21792178
format!(
21802179
"{}.push_str({}{})",
2181-
snippet_with_applicability(cx, args[0].span, "_", &mut applicability),
2180+
snippet_with_applicability(cx, args[0].span, "..", &mut applicability),
21822181
ref_str,
2183-
snippet_with_applicability(cx, target.span, "_", &mut applicability)
2182+
snippet_with_applicability(cx, target.span, "..", &mut applicability)
21842183
),
21852184
applicability,
21862185
);
@@ -2427,7 +2426,7 @@ fn lint_get_unwrap<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, get_args:
24272426
let mut applicability = Applicability::MachineApplicable;
24282427
let expr_ty = cx.typeck_results().expr_ty(&get_args[0]);
24292428
let get_args_str = if get_args.len() > 1 {
2430-
snippet_with_applicability(cx, get_args[1].span, "_", &mut applicability)
2429+
snippet_with_applicability(cx, get_args[1].span, "..", &mut applicability)
24312430
} else {
24322431
return; // not linting on a .get().unwrap() chain or variant
24332432
};
@@ -2487,7 +2486,7 @@ fn lint_get_unwrap<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, get_args:
24872486
format!(
24882487
"{}{}[{}]",
24892488
borrow_str,
2490-
snippet_with_applicability(cx, get_args[0].span, "_", &mut applicability),
2489+
snippet_with_applicability(cx, get_args[0].span, "..", &mut applicability),
24912490
get_args_str
24922491
),
24932492
applicability,
@@ -2503,7 +2502,7 @@ fn lint_iter_skip_next(cx: &LateContext<'_>, expr: &hir::Expr<'_>, skip_args: &[
25032502
cx,
25042503
ITER_SKIP_NEXT,
25052504
expr.span.trim_start(caller.span).unwrap(),
2506-
"called `skip(x).next()` on an iterator",
2505+
"called `skip(..).next()` on an iterator",
25072506
"use `nth` instead",
25082507
hint,
25092508
Applicability::MachineApplicable,
@@ -2706,11 +2705,11 @@ fn lint_map_unwrap_or_else<'tcx>(
27062705

27072706
// lint message
27082707
let msg = if is_option {
2709-
"called `map(f).unwrap_or_else(g)` on an `Option` value. This can be done more directly by calling \
2710-
`map_or_else(g, f)` instead"
2708+
"called `map(<f>).unwrap_or_else(<g>)` on an `Option` value. This can be done more directly by calling \
2709+
`map_or_else(<g>, <f>)` instead"
27112710
} else {
2712-
"called `map(f).unwrap_or_else(g)` on a `Result` value. This can be done more directly by calling \
2713-
`.map_or_else(g, f)` instead"
2711+
"called `map(<f>).unwrap_or_else(<g>)` on a `Result` value. This can be done more directly by calling \
2712+
`.map_or_else(<g>, <f>)` instead"
27142713
};
27152714
// get snippets for args to map() and unwrap_or_else()
27162715
let map_snippet = snippet(cx, map_args[1].span, "..");
@@ -2720,16 +2719,15 @@ fn lint_map_unwrap_or_else<'tcx>(
27202719
let multiline = map_snippet.lines().count() > 1 || unwrap_snippet.lines().count() > 1;
27212720
let same_span = map_args[1].span.ctxt() == unwrap_args[1].span.ctxt();
27222721
if same_span && !multiline {
2723-
span_lint_and_note(
2722+
let var_snippet = snippet(cx, map_args[0].span, "..");
2723+
span_lint_and_sugg(
27242724
cx,
27252725
MAP_UNWRAP_OR,
27262726
expr.span,
27272727
msg,
2728-
None,
2729-
&format!(
2730-
"replace `map({0}).unwrap_or_else({1})` with `map_or_else({1}, {0})`",
2731-
map_snippet, unwrap_snippet,
2732-
),
2728+
"try this",
2729+
format!("{}.map_or_else({}, {})", var_snippet, unwrap_snippet, map_snippet),
2730+
Applicability::MachineApplicable,
27332731
);
27342732
return true;
27352733
} else if same_span && multiline {
@@ -2776,8 +2774,8 @@ fn lint_map_or_none<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, map
27762774
if is_option {
27772775
let self_snippet = snippet(cx, map_or_args[0].span, "..");
27782776
let func_snippet = snippet(cx, map_or_args[2].span, "..");
2779-
let msg = "called `map_or(None, f)` on an `Option` value. This can be done more directly by calling \
2780-
`and_then(f)` instead";
2777+
let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \
2778+
`and_then(..)` instead";
27812779
(
27822780
OPTION_MAP_OR_NONE,
27832781
msg,
@@ -2815,18 +2813,20 @@ fn lint_map_or_none<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, map
28152813
fn lint_filter_next<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, filter_args: &'tcx [hir::Expr<'_>]) {
28162814
// lint if caller of `.filter().next()` is an Iterator
28172815
if match_trait_method(cx, expr, &paths::ITERATOR) {
2818-
let msg = "called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling \
2819-
`.find(p)` instead.";
2816+
let msg = "called `filter(..).next()` on an `Iterator`. This is more succinctly expressed by calling \
2817+
`.find(..)` instead.";
28202818
let filter_snippet = snippet(cx, filter_args[1].span, "..");
28212819
if filter_snippet.lines().count() <= 1 {
2820+
let iter_snippet = snippet(cx, filter_args[0].span, "..");
28222821
// add note if not multi-line
2823-
span_lint_and_note(
2822+
span_lint_and_sugg(
28242823
cx,
28252824
FILTER_NEXT,
28262825
expr.span,
28272826
msg,
2828-
None,
2829-
&format!("replace `filter({0}).next()` with `find({0})`", filter_snippet),
2827+
"try this",
2828+
format!("{}.find({})", iter_snippet, filter_snippet),
2829+
Applicability::MachineApplicable,
28302830
);
28312831
} else {
28322832
span_lint(cx, FILTER_NEXT, expr.span, msg);
@@ -2846,9 +2846,9 @@ fn lint_skip_while_next<'tcx>(
28462846
cx,
28472847
SKIP_WHILE_NEXT,
28482848
expr.span,
2849-
"called `skip_while(p).next()` on an `Iterator`",
2849+
"called `skip_while(<p>).next()` on an `Iterator`",
28502850
None,
2851-
"this is more succinctly expressed by calling `.find(!p)` instead",
2851+
"this is more succinctly expressed by calling `.find(!<p>)` instead",
28522852
);
28532853
}
28542854
}
@@ -2862,7 +2862,7 @@ fn lint_filter_map<'tcx>(
28622862
) {
28632863
// lint if caller of `.filter().map()` is an Iterator
28642864
if match_trait_method(cx, expr, &paths::ITERATOR) {
2865-
let msg = "called `filter(p).map(q)` on an `Iterator`";
2865+
let msg = "called `filter(..).map(..)` on an `Iterator`";
28662866
let hint = "this is more succinctly expressed by calling `.filter_map(..)` instead";
28672867
span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint);
28682868
}
@@ -2871,17 +2871,19 @@ fn lint_filter_map<'tcx>(
28712871
/// lint use of `filter_map().next()` for `Iterators`
28722872
fn lint_filter_map_next<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, filter_args: &'tcx [hir::Expr<'_>]) {
28732873
if match_trait_method(cx, expr, &paths::ITERATOR) {
2874-
let msg = "called `filter_map(p).next()` on an `Iterator`. This is more succinctly expressed by calling \
2875-
`.find_map(p)` instead.";
2874+
let msg = "called `filter_map(..).next()` on an `Iterator`. This is more succinctly expressed by calling \
2875+
`.find_map(..)` instead.";
28762876
let filter_snippet = snippet(cx, filter_args[1].span, "..");
28772877
if filter_snippet.lines().count() <= 1 {
2878-
span_lint_and_note(
2878+
let iter_snippet = snippet(cx, filter_args[0].span, "..");
2879+
span_lint_and_sugg(
28792880
cx,
28802881
FILTER_MAP_NEXT,
28812882
expr.span,
28822883
msg,
2883-
None,
2884-
&format!("replace `filter_map({0}).next()` with `find_map({0})`", filter_snippet),
2884+
"try this",
2885+
format!("{}.find_map({})", iter_snippet, filter_snippet),
2886+
Applicability::MachineApplicable,
28852887
);
28862888
} else {
28872889
span_lint(cx, FILTER_MAP_NEXT, expr.span, msg);
@@ -2898,7 +2900,7 @@ fn lint_find_map<'tcx>(
28982900
) {
28992901
// lint if caller of `.filter().map()` is an Iterator
29002902
if match_trait_method(cx, &map_args[0], &paths::ITERATOR) {
2901-
let msg = "called `find(p).map(q)` on an `Iterator`";
2903+
let msg = "called `find(..).map(..)` on an `Iterator`";
29022904
let hint = "this is more succinctly expressed by calling `.find_map(..)` instead";
29032905
span_lint_and_help(cx, FIND_MAP, expr.span, msg, None, hint);
29042906
}
@@ -2913,7 +2915,7 @@ fn lint_filter_map_map<'tcx>(
29132915
) {
29142916
// lint if caller of `.filter().map()` is an Iterator
29152917
if match_trait_method(cx, expr, &paths::ITERATOR) {
2916-
let msg = "called `filter_map(p).map(q)` on an `Iterator`";
2918+
let msg = "called `filter_map(..).map(..)` on an `Iterator`";
29172919
let hint = "this is more succinctly expressed by only calling `.filter_map(..)` instead";
29182920
span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint);
29192921
}
@@ -2928,7 +2930,7 @@ fn lint_filter_flat_map<'tcx>(
29282930
) {
29292931
// lint if caller of `.filter().flat_map()` is an Iterator
29302932
if match_trait_method(cx, expr, &paths::ITERATOR) {
2931-
let msg = "called `filter(p).flat_map(q)` on an `Iterator`";
2933+
let msg = "called `filter(..).flat_map(..)` on an `Iterator`";
29322934
let hint = "this is more succinctly expressed by calling `.flat_map(..)` \
29332935
and filtering by returning `iter::empty()`";
29342936
span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint);
@@ -2944,7 +2946,7 @@ fn lint_filter_map_flat_map<'tcx>(
29442946
) {
29452947
// lint if caller of `.filter_map().flat_map()` is an Iterator
29462948
if match_trait_method(cx, expr, &paths::ITERATOR) {
2947-
let msg = "called `filter_map(p).flat_map(q)` on an `Iterator`";
2949+
let msg = "called `filter_map(..).flat_map(..)` on an `Iterator`";
29482950
let hint = "this is more succinctly expressed by calling `.flat_map(..)` \
29492951
and filtering by returning `iter::empty()`";
29502952
span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint);
@@ -3115,9 +3117,9 @@ fn lint_chars_cmp(
31153117
"like this",
31163118
format!("{}{}.{}({})",
31173119
if info.eq { "" } else { "!" },
3118-
snippet_with_applicability(cx, args[0][0].span, "_", &mut applicability),
3120+
snippet_with_applicability(cx, args[0][0].span, "..", &mut applicability),
31193121
suggest,
3120-
snippet_with_applicability(cx, arg_char[0].span, "_", &mut applicability)),
3122+
snippet_with_applicability(cx, arg_char[0].span, "..", &mut applicability)),
31213123
applicability,
31223124
);
31233125

@@ -3164,7 +3166,7 @@ fn lint_chars_cmp_with_unwrap<'tcx>(
31643166
"like this",
31653167
format!("{}{}.{}('{}')",
31663168
if info.eq { "" } else { "!" },
3167-
snippet_with_applicability(cx, args[0][0].span, "_", &mut applicability),
3169+
snippet_with_applicability(cx, args[0][0].span, "..", &mut applicability),
31683170
suggest,
31693171
c),
31703172
applicability,
@@ -3239,7 +3241,7 @@ fn lint_single_char_pattern(cx: &LateContext<'_>, _expr: &hir::Expr<'_>, arg: &h
32393241
fn lint_single_char_push_string(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
32403242
let mut applicability = Applicability::MachineApplicable;
32413243
if let Some(extension_string) = get_hint_if_single_char_arg(cx, &args[1], &mut applicability) {
3242-
let base_string_snippet = snippet_with_applicability(cx, args[0].span, "_", &mut applicability);
3244+
let base_string_snippet = snippet_with_applicability(cx, args[0].span, "..", &mut applicability);
32433245
let sugg = format!("{}.push({})", base_string_snippet, extension_string);
32443246
span_lint_and_sugg(
32453247
cx,
@@ -3282,7 +3284,7 @@ fn lint_asref(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str, as_re
32823284
expr.span,
32833285
&format!("this call to `{}` does nothing", call_name),
32843286
"try this",
3285-
snippet_with_applicability(cx, recvr.span, "_", &mut applicability).to_string(),
3287+
snippet_with_applicability(cx, recvr.span, "..", &mut applicability).to_string(),
32863288
applicability,
32873289
);
32883290
}

clippy_lints/src/methods/option_map_unwrap_or.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,15 +53,15 @@ pub(super) fn lint<'tcx>(
5353
// lint message
5454
// comparing the snippet from source to raw text ("None") below is safe
5555
// because we already have checked the type.
56-
let arg = if unwrap_snippet == "None" { "None" } else { "a" };
56+
let arg = if unwrap_snippet == "None" { "None" } else { "<a>" };
5757
let unwrap_snippet_none = unwrap_snippet == "None";
5858
let suggest = if unwrap_snippet_none {
59-
"and_then(f)"
59+
"and_then(<f>)"
6060
} else {
61-
"map_or(a, f)"
61+
"map_or(<a>, <f>)"
6262
};
6363
let msg = &format!(
64-
"called `map(f).unwrap_or({})` on an `Option` value. \
64+
"called `map(<f>).unwrap_or({})` on an `Option` value. \
6565
This can be done more directly by calling `{}` instead",
6666
arg, suggest
6767
);

tests/ui/filter_map_next.rs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,6 @@
33
fn main() {
44
let a = ["1", "lol", "3", "NaN", "5"];
55

6-
let element: Option<i32> = a.iter().filter_map(|s| s.parse().ok()).next();
7-
assert_eq!(element, Some(1));
8-
96
#[rustfmt::skip]
107
let _: Option<u32> = vec![1, 2, 3, 4, 5, 6]
118
.into_iter()

tests/ui/filter_map_next.stderr

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,5 @@
1-
error: called `filter_map(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find_map(p)` instead.
2-
--> $DIR/filter_map_next.rs:6:32
3-
|
4-
LL | let element: Option<i32> = a.iter().filter_map(|s| s.parse().ok()).next();
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6-
|
7-
= note: `-D clippy::filter-map-next` implied by `-D warnings`
8-
= note: replace `filter_map(|s| s.parse().ok()).next()` with `find_map(|s| s.parse().ok())`
9-
10-
error: called `filter_map(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find_map(p)` instead.
11-
--> $DIR/filter_map_next.rs:10:26
1+
error: called `filter_map(..).next()` on an `Iterator`. This is more succinctly expressed by calling `.find_map(..)` instead.
2+
--> $DIR/filter_map_next.rs:7:26
123
|
134
LL | let _: Option<u32> = vec![1, 2, 3, 4, 5, 6]
145
| __________________________^
@@ -19,6 +10,8 @@ LL | | if x == 2 {
1910
LL | | })
2011
LL | | .next();
2112
| |_______________^
13+
|
14+
= note: `-D clippy::filter-map-next` implied by `-D warnings`
2215

23-
error: aborting due to 2 previous errors
16+
error: aborting due to previous error
2417

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// run-rustfix
2+
3+
#![warn(clippy::all, clippy::pedantic)]
4+
5+
fn main() {
6+
let a = ["1", "lol", "3", "NaN", "5"];
7+
8+
let element: Option<i32> = a.iter().find_map(|s| s.parse().ok());
9+
assert_eq!(element, Some(1));
10+
}

tests/ui/filter_map_next_fixable.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// run-rustfix
2+
3+
#![warn(clippy::all, clippy::pedantic)]
4+
5+
fn main() {
6+
let a = ["1", "lol", "3", "NaN", "5"];
7+
8+
let element: Option<i32> = a.iter().filter_map(|s| s.parse().ok()).next();
9+
assert_eq!(element, Some(1));
10+
}
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
error: called `filter_map(..).next()` on an `Iterator`. This is more succinctly expressed by calling `.find_map(..)` instead.
2+
--> $DIR/filter_map_next_fixable.rs:8:32
3+
|
4+
LL | let element: Option<i32> = a.iter().filter_map(|s| s.parse().ok()).next();
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `a.iter().find_map(|s| s.parse().ok())`
6+
|
7+
= note: `-D clippy::filter-map-next` implied by `-D warnings`
8+
9+
error: aborting due to previous error
10+

tests/ui/filter_methods.stderr

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
error: called `filter(p).map(q)` on an `Iterator`
1+
error: called `filter(..).map(..)` on an `Iterator`
22
--> $DIR/filter_methods.rs:5:21
33
|
44
LL | let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect();
@@ -7,7 +7,7 @@ LL | let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x *
77
= note: `-D clippy::filter-map` implied by `-D warnings`
88
= help: this is more succinctly expressed by calling `.filter_map(..)` instead
99

10-
error: called `filter(p).flat_map(q)` on an `Iterator`
10+
error: called `filter(..).flat_map(..)` on an `Iterator`
1111
--> $DIR/filter_methods.rs:7:21
1212
|
1313
LL | let _: Vec<_> = vec![5_i8; 6]
@@ -19,7 +19,7 @@ LL | | .flat_map(|x| x.checked_mul(2))
1919
|
2020
= help: this is more succinctly expressed by calling `.flat_map(..)` and filtering by returning `iter::empty()`
2121

22-
error: called `filter_map(p).flat_map(q)` on an `Iterator`
22+
error: called `filter_map(..).flat_map(..)` on an `Iterator`
2323
--> $DIR/filter_methods.rs:13:21
2424
|
2525
LL | let _: Vec<_> = vec![5_i8; 6]
@@ -31,7 +31,7 @@ LL | | .flat_map(|x| x.checked_mul(2))
3131
|
3232
= help: this is more succinctly expressed by calling `.flat_map(..)` and filtering by returning `iter::empty()`
3333

34-
error: called `filter_map(p).map(q)` on an `Iterator`
34+
error: called `filter_map(..).map(..)` on an `Iterator`
3535
--> $DIR/filter_methods.rs:19:21
3636
|
3737
LL | let _: Vec<_> = vec![5_i8; 6]

0 commit comments

Comments
 (0)