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.

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

search_is_some: add checking for is_none() #6942

Merged
merged 1 commit into from
Mar 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
57 changes: 46 additions & 11 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -588,26 +588,31 @@ declare_clippy_lint! {

declare_clippy_lint! {
/// **What it does:** Checks for an iterator or string search (such as `find()`,
/// `position()`, or `rposition()`) followed by a call to `is_some()`.
/// `position()`, or `rposition()`) followed by a call to `is_some()` or `is_none()`.
///
/// **Why is this bad?** Readability, this can be written more concisely as
/// `_.any(_)` or `_.contains(_)`.
/// **Why is this bad?** Readability, this can be written more concisely as:
/// * `_.any(_)`, or `_.contains(_)` for `is_some()`,
/// * `!_.any(_)`, or `!_.contains(_)` for `is_none()`.
///
/// **Known problems:** None.
///
/// **Example:**
/// ```rust
/// # let vec = vec![1];
/// let vec = vec![1];
/// vec.iter().find(|x| **x == 0).is_some();
///
/// let _ = "hello world".find("world").is_none();
/// ```
/// Could be written as
/// ```rust
/// # let vec = vec![1];
/// let vec = vec![1];
/// vec.iter().any(|x| *x == 0);
///
/// let _ = !"hello world".contains("world");
/// ```
pub SEARCH_IS_SOME,
complexity,
"using an iterator or string search followed by `is_some()`, which is more succinctly expressed as a call to `any()` or `contains()`"
"using an iterator or string search followed by `is_some()` or `is_none()`, which is more succinctly expressed as a call to `any()` or `contains()` (with negation in case of `is_none()`)"
}

declare_clippy_lint! {
Expand Down Expand Up @@ -1720,12 +1725,42 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
["flat_map", "filter_map"] => filter_map_flat_map::check(cx, expr, arg_lists[1], arg_lists[0]),
["flat_map", ..] => flat_map_identity::check(cx, expr, arg_lists[0], method_spans[0]),
["flatten", "map"] => map_flatten::check(cx, expr, arg_lists[1]),
["is_some", "find"] => search_is_some::check(cx, expr, "find", arg_lists[1], arg_lists[0], method_spans[1]),
["is_some", "position"] => {
search_is_some::check(cx, expr, "position", arg_lists[1], arg_lists[0], method_spans[1])
[option_check_method, "find"] if "is_some" == *option_check_method || "is_none" == *option_check_method => {
search_is_some::check(
cx,
expr,
"find",
option_check_method,
arg_lists[1],
arg_lists[0],
method_spans[1],
)
},
["is_some", "rposition"] => {
search_is_some::check(cx, expr, "rposition", arg_lists[1], arg_lists[0], method_spans[1])
[option_check_method, "position"]
if "is_some" == *option_check_method || "is_none" == *option_check_method =>
{
search_is_some::check(
cx,
expr,
"position",
option_check_method,
arg_lists[1],
arg_lists[0],
method_spans[1],
)
},
[option_check_method, "rposition"]
if "is_some" == *option_check_method || "is_none" == *option_check_method =>
{
search_is_some::check(
cx,
expr,
"rposition",
option_check_method,
arg_lists[1],
arg_lists[0],
method_spans[1],
)
},
["extend", ..] => string_extend_chars::check(cx, expr, arg_lists[0]),
["count", "into_iter"] => iter_count::check(cx, expr, &arg_lists[1], "into_iter"),
Expand Down
107 changes: 78 additions & 29 deletions clippy_lints/src/methods/search_is_some.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,23 @@ use rustc_span::symbol::sym;
use super::SEARCH_IS_SOME;

/// lint searching an Iterator followed by `is_some()`
/// or calling `find()` on a string followed by `is_some()`
/// or calling `find()` on a string followed by `is_some()` or `is_none()`
#[allow(clippy::too_many_lines)]
pub(super) fn check<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx hir::Expr<'_>,
search_method: &str,
option_check_method: &str,
search_args: &'tcx [hir::Expr<'_>],
is_some_args: &'tcx [hir::Expr<'_>],
method_span: Span,
) {
// lint if caller of search is an Iterator
if is_trait_method(cx, &is_some_args[0], sym::Iterator) {
let msg = format!(
"called `is_some()` after searching an `Iterator` with `{}`",
search_method
"called `{}()` after searching an `Iterator` with `{}`",
option_check_method, search_method
);
let hint = "this is more succinctly expressed by calling `any()`";
let search_snippet = snippet(cx, search_args[1].span, "..");
if search_snippet.lines().count() <= 1 {
// suggest `any(|x| ..)` instead of `any(|&x| ..)` for `find(|&x| ..).is_some()`
Expand All @@ -53,20 +54,49 @@ pub(super) fn check<'tcx>(
}
};
// add note if not multi-line
span_lint_and_sugg(
cx,
SEARCH_IS_SOME,
method_span.with_hi(expr.span.hi()),
&msg,
"use `any()` instead",
format!(
"any({})",
any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
),
Applicability::MachineApplicable,
);
match option_check_method {
"is_some" => {
span_lint_and_sugg(
cx,
SEARCH_IS_SOME,
method_span.with_hi(expr.span.hi()),
&msg,
"use `any()` instead",
format!(
"any({})",
any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
),
Applicability::MachineApplicable,
);
},
"is_none" => {
let iter = snippet(cx, search_args[0].span, "..");
span_lint_and_sugg(
cx,
SEARCH_IS_SOME,
expr.span,
&msg,
"use `!_.any()` instead",
format!(
"!{}.any({})",
iter,
any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
),
Applicability::MachineApplicable,
);
},
_ => (),
}
} else {
span_lint_and_help(cx, SEARCH_IS_SOME, expr.span, &msg, None, hint);
let hint = format!(
"this is more succinctly expressed by calling `any()`{}",
if option_check_method == "is_none" {
" with negation"
} else {
""
}
);
span_lint_and_help(cx, SEARCH_IS_SOME, expr.span, &msg, None, &hint);
}
}
// lint if `find()` is called by `String` or `&str`
Expand All @@ -83,18 +113,37 @@ pub(super) fn check<'tcx>(
if is_string_or_str_slice(&search_args[0]);
if is_string_or_str_slice(&search_args[1]);
then {
let msg = "called `is_some()` after calling `find()` on a string";
let mut applicability = Applicability::MachineApplicable;
let find_arg = snippet_with_applicability(cx, search_args[1].span, "..", &mut applicability);
span_lint_and_sugg(
cx,
SEARCH_IS_SOME,
method_span.with_hi(expr.span.hi()),
msg,
"use `contains()` instead",
format!("contains({})", find_arg),
applicability,
);
let msg = format!("called `{}()` after calling `find()` on a string", option_check_method);
match option_check_method {
"is_some" => {
let mut applicability = Applicability::MachineApplicable;
let find_arg = snippet_with_applicability(cx, search_args[1].span, "..", &mut applicability);
span_lint_and_sugg(
cx,
SEARCH_IS_SOME,
method_span.with_hi(expr.span.hi()),
&msg,
"use `contains()` instead",
format!("contains({})", find_arg),
applicability,
);
},
"is_none" => {
let string = snippet(cx, search_args[0].span, "..");
let mut applicability = Applicability::MachineApplicable;
let find_arg = snippet_with_applicability(cx, search_args[1].span, "..", &mut applicability);
span_lint_and_sugg(
cx,
SEARCH_IS_SOME,
expr.span,
&msg,
"use `!_.contains()` instead",
format!("!{}.contains({})", string, find_arg),
applicability,
);
},
_ => (),
}
}
}
}
Expand Down
37 changes: 36 additions & 1 deletion tests/ui/search_is_some.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
// aux-build:option_helpers.rs
#![warn(clippy::search_is_some)]
#![allow(dead_code)]
extern crate option_helpers;
use option_helpers::IteratorFalsePositives;

#[warn(clippy::search_is_some)]
#[rustfmt::skip]
fn main() {
let v = vec![3, 2, 1, 0, -1, -2, -3];
Expand Down Expand Up @@ -36,3 +37,37 @@ fn main() {
// `Pattern` that is not a string
let _ = "hello world".find(|c: char| c == 'o' || c == 'l').is_some();
}

#[rustfmt::skip]
fn is_none() {
let v = vec![3, 2, 1, 0, -1, -2, -3];
let y = &&42;


// Check `find().is_none()`, multi-line case.
let _ = v.iter().find(|&x| {
*x < 0
}
).is_none();

// Check `position().is_none()`, multi-line case.
let _ = v.iter().position(|&x| {
x < 0
}
).is_none();

// Check `rposition().is_none()`, multi-line case.
let _ = v.iter().rposition(|&x| {
x < 0
}
).is_none();

// Check that we don't lint if the caller is not an `Iterator` or string
let falsepos = IteratorFalsePositives { foo: 0 };
let _ = falsepos.find().is_none();
let _ = falsepos.position().is_none();
let _ = falsepos.rposition().is_none();
// check that we don't lint if `find()` is called with
// `Pattern` that is not a string
let _ = "hello world".find(|c: char| c == 'o' || c == 'l').is_none();
}
44 changes: 40 additions & 4 deletions tests/ui/search_is_some.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: called `is_some()` after searching an `Iterator` with `find`
--> $DIR/search_is_some.rs:13:13
--> $DIR/search_is_some.rs:14:13
|
LL | let _ = v.iter().find(|&x| {
| _____________^
Expand All @@ -12,7 +12,7 @@ LL | | ).is_some();
= help: this is more succinctly expressed by calling `any()`

error: called `is_some()` after searching an `Iterator` with `position`
--> $DIR/search_is_some.rs:19:13
--> $DIR/search_is_some.rs:20:13
|
LL | let _ = v.iter().position(|&x| {
| _____________^
Expand All @@ -24,7 +24,7 @@ LL | | ).is_some();
= help: this is more succinctly expressed by calling `any()`

error: called `is_some()` after searching an `Iterator` with `rposition`
--> $DIR/search_is_some.rs:25:13
--> $DIR/search_is_some.rs:26:13
|
LL | let _ = v.iter().rposition(|&x| {
| _____________^
Expand All @@ -35,5 +35,41 @@ LL | | ).is_some();
|
= help: this is more succinctly expressed by calling `any()`

error: aborting due to 3 previous errors
error: called `is_none()` after searching an `Iterator` with `find`
--> $DIR/search_is_some.rs:48:13
|
LL | let _ = v.iter().find(|&x| {
| _____________^
LL | | *x < 0
LL | | }
LL | | ).is_none();
| |______________________________^
|
= help: this is more succinctly expressed by calling `any()` with negation

error: called `is_none()` after searching an `Iterator` with `position`
--> $DIR/search_is_some.rs:54:13
|
LL | let _ = v.iter().position(|&x| {
| _____________^
LL | | x < 0
LL | | }
LL | | ).is_none();
| |______________________________^
|
= help: this is more succinctly expressed by calling `any()` with negation

error: called `is_none()` after searching an `Iterator` with `rposition`
--> $DIR/search_is_some.rs:60:13
|
LL | let _ = v.iter().rposition(|&x| {
| _____________^
LL | | x < 0
LL | | }
LL | | ).is_none();
| |______________________________^
|
= help: this is more succinctly expressed by calling `any()` with negation

error: aborting due to 6 previous errors

35 changes: 34 additions & 1 deletion tests/ui/search_is_some_fixable.fixed
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// run-rustfix

#![allow(dead_code)]
#![warn(clippy::search_is_some)]

fn main() {
Expand Down Expand Up @@ -33,3 +33,36 @@ fn main() {
let _ = s1[2..].contains(&s2);
let _ = s1[2..].contains(&s2[2..]);
}

fn is_none() {
let v = vec![3, 2, 1, 0, -1, -2, -3];
let y = &&42;

// Check `find().is_none()`, single-line case.
let _ = !v.iter().any(|x| *x < 0);
let _ = !(0..1).any(|x| **y == x); // one dereference less
let _ = !(0..1).any(|x| x == 0);
let _ = !v.iter().any(|x| *x == 0);

// Check `position().is_none()`, single-line case.
let _ = !v.iter().any(|&x| x < 0);

// Check `rposition().is_none()`, single-line case.
let _ = !v.iter().any(|&x| x < 0);

let s1 = String::from("hello world");
let s2 = String::from("world");

// caller of `find()` is a `&`static str`
let _ = !"hello world".contains("world");
let _ = !"hello world".contains(&s2);
let _ = !"hello world".contains(&s2[2..]);
// caller of `find()` is a `String`
let _ = !s1.contains("world");
let _ = !s1.contains(&s2);
let _ = !s1.contains(&s2[2..]);
// caller of `find()` is slice of `String`
let _ = !s1[2..].contains("world");
let _ = !s1[2..].contains(&s2);
let _ = !s1[2..].contains(&s2[2..]);
}
Loading