Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve suggestions for several lints #6197

Merged
merged 5 commits into from
Oct 30, 2020
Merged

Improve suggestions for several lints #6197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2911d9c
Use better placeholders for some methods lint messages
ThibsG Oct 18, 2020
3fec6f5
Improve some suggestions for `filter_map_next`, `filter_next` and `ma…
ThibsG Oct 19, 2020
2a3ae11
Move fixable `map_unwrap_or` cases to rustfixed test
ThibsG Oct 23, 2020
e2d86b5
Move fixable `filter_next` and `filter_map_next` cases to rustfixed t…
ThibsG Oct 23, 2020
c0dd1f9
Fix tests for `map_unwrap_or*`
ThibsG Oct 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 45 additions & 43 deletions clippy_lints/src/methods/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ use crate::utils::{
is_copy, is_expn_of, is_type_diagnostic_item, iter_input_pats, last_path_segment, match_def_path, match_qpath,
match_trait_method, match_type, match_var, method_calls, method_chain_args, paths, remove_blocks, return_ty,
single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint,
span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, sugg, walk_ptrs_ty_depth,
SpanlessEq,
span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, walk_ptrs_ty_depth, SpanlessEq,
};

declare_clippy_lint! {
Expand Down Expand Up @@ -1748,7 +1747,7 @@ fn lint_or_fun_call<'tcx>(
"try this",
format!(
"{}.unwrap_or_default()",
snippet_with_applicability(cx, self_expr.span, "_", &mut applicability)
snippet_with_applicability(cx, self_expr.span, "..", &mut applicability)
),
applicability,
);
Expand Down Expand Up @@ -2155,7 +2154,7 @@ fn lint_clone_on_ref_ptr(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::
return;
};

let snippet = snippet_with_macro_callsite(cx, arg.span, "_");
let snippet = snippet_with_macro_callsite(cx, arg.span, "..");

span_lint_and_sugg(
cx,
Expand Down Expand Up @@ -2191,9 +2190,9 @@ fn lint_string_extend(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::E
"try this",
format!(
"{}.push_str({}{})",
snippet_with_applicability(cx, args[0].span, "_", &mut applicability),
snippet_with_applicability(cx, args[0].span, "..", &mut applicability),
ref_str,
snippet_with_applicability(cx, target.span, "_", &mut applicability)
snippet_with_applicability(cx, target.span, "..", &mut applicability)
),
applicability,
);
Expand Down Expand Up @@ -2460,7 +2459,7 @@ fn lint_get_unwrap<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, get_args:
let mut applicability = Applicability::MachineApplicable;
let expr_ty = cx.typeck_results().expr_ty(&get_args[0]);
let get_args_str = if get_args.len() > 1 {
snippet_with_applicability(cx, get_args[1].span, "_", &mut applicability)
snippet_with_applicability(cx, get_args[1].span, "..", &mut applicability)
} else {
return; // not linting on a .get().unwrap() chain or variant
};
Expand Down Expand Up @@ -2520,7 +2519,7 @@ fn lint_get_unwrap<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, get_args:
format!(
"{}{}[{}]",
borrow_str,
snippet_with_applicability(cx, get_args[0].span, "_", &mut applicability),
snippet_with_applicability(cx, get_args[0].span, "..", &mut applicability),
get_args_str
),
applicability,
Expand All @@ -2536,7 +2535,7 @@ fn lint_iter_skip_next(cx: &LateContext<'_>, expr: &hir::Expr<'_>, skip_args: &[
cx,
ITER_SKIP_NEXT,
expr.span.trim_start(caller.span).unwrap(),
"called `skip(x).next()` on an iterator",
"called `skip(..).next()` on an iterator",
"use `nth` instead",
hint,
Applicability::MachineApplicable,
Expand Down Expand Up @@ -2739,11 +2738,11 @@ fn lint_map_unwrap_or_else<'tcx>(

// lint message
let msg = if is_option {
"called `map(f).unwrap_or_else(g)` on an `Option` value. This can be done more directly by calling \
`map_or_else(g, f)` instead"
"called `map(<f>).unwrap_or_else(<g>)` on an `Option` value. This can be done more directly by calling \
`map_or_else(<g>, <f>)` instead"
} else {
"called `map(f).unwrap_or_else(g)` on a `Result` value. This can be done more directly by calling \
`.map_or_else(g, f)` instead"
"called `map(<f>).unwrap_or_else(<g>)` on a `Result` value. This can be done more directly by calling \
`.map_or_else(<g>, <f>)` instead"
};
// get snippets for args to map() and unwrap_or_else()
let map_snippet = snippet(cx, map_args[1].span, "..");
Expand All @@ -2753,16 +2752,15 @@ fn lint_map_unwrap_or_else<'tcx>(
let multiline = map_snippet.lines().count() > 1 || unwrap_snippet.lines().count() > 1;
let same_span = map_args[1].span.ctxt() == unwrap_args[1].span.ctxt();
if same_span && !multiline {
span_lint_and_note(
let var_snippet = snippet(cx, map_args[0].span, "..");
span_lint_and_sugg(
cx,
MAP_UNWRAP_OR,
expr.span,
msg,
None,
&format!(
"replace `map({0}).unwrap_or_else({1})` with `map_or_else({1}, {0})`",
map_snippet, unwrap_snippet,
),
"try this",
format!("{}.map_or_else({}, {})", var_snippet, unwrap_snippet, map_snippet),
Applicability::MachineApplicable,
);
return true;
} else if same_span && multiline {
Expand Down Expand Up @@ -2809,8 +2807,8 @@ fn lint_map_or_none<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, map
if is_option {
let self_snippet = snippet(cx, map_or_args[0].span, "..");
let func_snippet = snippet(cx, map_or_args[2].span, "..");
let msg = "called `map_or(None, f)` on an `Option` value. This can be done more directly by calling \
`and_then(f)` instead";
let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \
`and_then(..)` instead";
(
OPTION_MAP_OR_NONE,
msg,
Expand Down Expand Up @@ -2848,18 +2846,20 @@ fn lint_map_or_none<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, map
fn lint_filter_next<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, filter_args: &'tcx [hir::Expr<'_>]) {
// lint if caller of `.filter().next()` is an Iterator
if match_trait_method(cx, expr, &paths::ITERATOR) {
let msg = "called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling \
`.find(p)` instead.";
let msg = "called `filter(..).next()` on an `Iterator`. This is more succinctly expressed by calling \
`.find(..)` instead.";
let filter_snippet = snippet(cx, filter_args[1].span, "..");
if filter_snippet.lines().count() <= 1 {
let iter_snippet = snippet(cx, filter_args[0].span, "..");
// add note if not multi-line
span_lint_and_note(
span_lint_and_sugg(
cx,
FILTER_NEXT,
expr.span,
msg,
None,
&format!("replace `filter({0}).next()` with `find({0})`", filter_snippet),
"try this",
format!("{}.find({})", iter_snippet, filter_snippet),
Applicability::MachineApplicable,
);
} else {
span_lint(cx, FILTER_NEXT, expr.span, msg);
Expand All @@ -2879,9 +2879,9 @@ fn lint_skip_while_next<'tcx>(
cx,
SKIP_WHILE_NEXT,
expr.span,
"called `skip_while(p).next()` on an `Iterator`",
"called `skip_while(<p>).next()` on an `Iterator`",
None,
"this is more succinctly expressed by calling `.find(!p)` instead",
"this is more succinctly expressed by calling `.find(!<p>)` instead",
);
}
}
Expand All @@ -2895,7 +2895,7 @@ fn lint_filter_map<'tcx>(
) {
// lint if caller of `.filter().map()` is an Iterator
if match_trait_method(cx, expr, &paths::ITERATOR) {
let msg = "called `filter(p).map(q)` on an `Iterator`";
let msg = "called `filter(..).map(..)` on an `Iterator`";
let hint = "this is more succinctly expressed by calling `.filter_map(..)` instead";
span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint);
}
Expand All @@ -2904,17 +2904,19 @@ fn lint_filter_map<'tcx>(
/// lint use of `filter_map().next()` for `Iterators`
fn lint_filter_map_next<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, filter_args: &'tcx [hir::Expr<'_>]) {
if match_trait_method(cx, expr, &paths::ITERATOR) {
let msg = "called `filter_map(p).next()` on an `Iterator`. This is more succinctly expressed by calling \
`.find_map(p)` instead.";
let msg = "called `filter_map(..).next()` on an `Iterator`. This is more succinctly expressed by calling \
`.find_map(..)` instead.";
let filter_snippet = snippet(cx, filter_args[1].span, "..");
if filter_snippet.lines().count() <= 1 {
span_lint_and_note(
let iter_snippet = snippet(cx, filter_args[0].span, "..");
span_lint_and_sugg(
cx,
FILTER_MAP_NEXT,
expr.span,
msg,
None,
&format!("replace `filter_map({0}).next()` with `find_map({0})`", filter_snippet),
"try this",
format!("{}.find_map({})", iter_snippet, filter_snippet),
Applicability::MachineApplicable,
);
} else {
span_lint(cx, FILTER_MAP_NEXT, expr.span, msg);
Expand All @@ -2931,7 +2933,7 @@ fn lint_find_map<'tcx>(
) {
// lint if caller of `.filter().map()` is an Iterator
if match_trait_method(cx, &map_args[0], &paths::ITERATOR) {
let msg = "called `find(p).map(q)` on an `Iterator`";
let msg = "called `find(..).map(..)` on an `Iterator`";
ThibsG marked this conversation as resolved.
Show resolved Hide resolved
let hint = "this is more succinctly expressed by calling `.find_map(..)` instead";
span_lint_and_help(cx, FIND_MAP, expr.span, msg, None, hint);
}
Expand All @@ -2946,7 +2948,7 @@ fn lint_filter_map_map<'tcx>(
) {
// lint if caller of `.filter().map()` is an Iterator
if match_trait_method(cx, expr, &paths::ITERATOR) {
let msg = "called `filter_map(p).map(q)` on an `Iterator`";
let msg = "called `filter_map(..).map(..)` on an `Iterator`";
ThibsG marked this conversation as resolved.
Show resolved Hide resolved
let hint = "this is more succinctly expressed by only calling `.filter_map(..)` instead";
span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint);
}
Expand All @@ -2961,7 +2963,7 @@ fn lint_filter_flat_map<'tcx>(
) {
// lint if caller of `.filter().flat_map()` is an Iterator
if match_trait_method(cx, expr, &paths::ITERATOR) {
let msg = "called `filter(p).flat_map(q)` on an `Iterator`";
let msg = "called `filter(..).flat_map(..)` on an `Iterator`";
ThibsG marked this conversation as resolved.
Show resolved Hide resolved
let hint = "this is more succinctly expressed by calling `.flat_map(..)` \
and filtering by returning `iter::empty()`";
span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint);
Expand All @@ -2977,7 +2979,7 @@ fn lint_filter_map_flat_map<'tcx>(
) {
// lint if caller of `.filter_map().flat_map()` is an Iterator
if match_trait_method(cx, expr, &paths::ITERATOR) {
let msg = "called `filter_map(p).flat_map(q)` on an `Iterator`";
let msg = "called `filter_map(..).flat_map(..)` on an `Iterator`";
ThibsG marked this conversation as resolved.
Show resolved Hide resolved
let hint = "this is more succinctly expressed by calling `.flat_map(..)` \
and filtering by returning `iter::empty()`";
span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint);
Expand Down Expand Up @@ -3148,9 +3150,9 @@ fn lint_chars_cmp(
"like this",
format!("{}{}.{}({})",
if info.eq { "" } else { "!" },
snippet_with_applicability(cx, args[0][0].span, "_", &mut applicability),
snippet_with_applicability(cx, args[0][0].span, "..", &mut applicability),
suggest,
snippet_with_applicability(cx, arg_char[0].span, "_", &mut applicability)),
snippet_with_applicability(cx, arg_char[0].span, "..", &mut applicability)),
applicability,
);

Expand Down Expand Up @@ -3197,7 +3199,7 @@ fn lint_chars_cmp_with_unwrap<'tcx>(
"like this",
format!("{}{}.{}('{}')",
if info.eq { "" } else { "!" },
snippet_with_applicability(cx, args[0][0].span, "_", &mut applicability),
snippet_with_applicability(cx, args[0][0].span, "..", &mut applicability),
suggest,
c),
applicability,
Expand Down Expand Up @@ -3272,7 +3274,7 @@ fn lint_single_char_pattern(cx: &LateContext<'_>, _expr: &hir::Expr<'_>, arg: &h
fn lint_single_char_push_string(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
let mut applicability = Applicability::MachineApplicable;
if let Some(extension_string) = get_hint_if_single_char_arg(cx, &args[1], &mut applicability) {
let base_string_snippet = snippet_with_applicability(cx, args[0].span, "_", &mut applicability);
let base_string_snippet = snippet_with_applicability(cx, args[0].span, "..", &mut applicability);
let sugg = format!("{}.push({})", base_string_snippet, extension_string);
span_lint_and_sugg(
cx,
Expand Down Expand Up @@ -3315,7 +3317,7 @@ fn lint_asref(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str, as_re
expr.span,
&format!("this call to `{}` does nothing", call_name),
"try this",
snippet_with_applicability(cx, recvr.span, "_", &mut applicability).to_string(),
snippet_with_applicability(cx, recvr.span, "..", &mut applicability).to_string(),
applicability,
);
}
Expand Down
8 changes: 4 additions & 4 deletions clippy_lints/src/methods/option_map_unwrap_or.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,15 @@ pub(super) fn lint<'tcx>(
// lint message
// comparing the snippet from source to raw text ("None") below is safe
// because we already have checked the type.
let arg = if unwrap_snippet == "None" { "None" } else { "a" };
let arg = if unwrap_snippet == "None" { "None" } else { "<a>" };
let unwrap_snippet_none = unwrap_snippet == "None";
let suggest = if unwrap_snippet_none {
"and_then(f)"
"and_then(<f>)"
} else {
"map_or(a, f)"
"map_or(<a>, <f>)"
};
let msg = &format!(
"called `map(f).unwrap_or({})` on an `Option` value. \
"called `map(<f>).unwrap_or({})` on an `Option` value. \
This can be done more directly by calling `{}` instead",
arg, suggest
);
Expand Down
3 changes: 0 additions & 3 deletions tests/ui/filter_map_next.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@
fn main() {
let a = ["1", "lol", "3", "NaN", "5"];

let element: Option<i32> = a.iter().filter_map(|s| s.parse().ok()).next();
assert_eq!(element, Some(1));

#[rustfmt::skip]
let _: Option<u32> = vec![1, 2, 3, 4, 5, 6]
.into_iter()
Expand Down
17 changes: 5 additions & 12 deletions tests/ui/filter_map_next.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
error: called `filter_map(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find_map(p)` instead.
--> $DIR/filter_map_next.rs:6:32
|
LL | let element: Option<i32> = a.iter().filter_map(|s| s.parse().ok()).next();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::filter-map-next` implied by `-D warnings`
= note: replace `filter_map(|s| s.parse().ok()).next()` with `find_map(|s| s.parse().ok())`

error: called `filter_map(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find_map(p)` instead.
--> $DIR/filter_map_next.rs:10:26
error: called `filter_map(..).next()` on an `Iterator`. This is more succinctly expressed by calling `.find_map(..)` instead.
--> $DIR/filter_map_next.rs:7:26
|
LL | let _: Option<u32> = vec![1, 2, 3, 4, 5, 6]
| __________________________^
Expand All @@ -19,6 +10,8 @@ LL | | if x == 2 {
LL | | })
LL | | .next();
| |_______________^
|
= note: `-D clippy::filter-map-next` implied by `-D warnings`

error: aborting due to 2 previous errors
error: aborting due to previous error

10 changes: 10 additions & 0 deletions tests/ui/filter_map_next_fixable.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// run-rustfix

#![warn(clippy::all, clippy::pedantic)]

fn main() {
let a = ["1", "lol", "3", "NaN", "5"];

let element: Option<i32> = a.iter().find_map(|s| s.parse().ok());
assert_eq!(element, Some(1));
}
10 changes: 10 additions & 0 deletions tests/ui/filter_map_next_fixable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// run-rustfix

#![warn(clippy::all, clippy::pedantic)]

fn main() {
let a = ["1", "lol", "3", "NaN", "5"];

let element: Option<i32> = a.iter().filter_map(|s| s.parse().ok()).next();
assert_eq!(element, Some(1));
}
10 changes: 10 additions & 0 deletions tests/ui/filter_map_next_fixable.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: called `filter_map(..).next()` on an `Iterator`. This is more succinctly expressed by calling `.find_map(..)` instead.
--> $DIR/filter_map_next_fixable.rs:8:32
|
LL | let element: Option<i32> = a.iter().filter_map(|s| s.parse().ok()).next();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `a.iter().find_map(|s| s.parse().ok())`
|
= note: `-D clippy::filter-map-next` implied by `-D warnings`

error: aborting due to previous error

8 changes: 4 additions & 4 deletions tests/ui/filter_methods.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: called `filter(p).map(q)` on an `Iterator`
error: called `filter(..).map(..)` on an `Iterator`
--> $DIR/filter_methods.rs:5:21
|
LL | let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect();
Expand All @@ -7,7 +7,7 @@ LL | let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x *
= note: `-D clippy::filter-map` implied by `-D warnings`
= help: this is more succinctly expressed by calling `.filter_map(..)` instead

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

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

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