Skip to content

Commit

Permalink
Auto merge of #6942 - mgacek8:issue_6815_search_is_none, r=llogiq
Browse files Browse the repository at this point in the history 
search_is_some: add checking for `is_none()`

fixes: #6815
changelog: search_is_some: add checking for `is_none()`.

To be honest I don't know what is the process of renaming the lints. Appreciate any feedback if that needs to be handled differently. Thanks!
  • Loading branch information
@bors
bors committed Mar 22, 2021
2 parents f3de78e + 2ffee89 commit aca95aa
Show file tree
Hide file tree
Showing 7 changed files with 359 additions and 48 deletions.
57 changes: 46 additions & 11 deletions clippy_lints/src/methods/mod.rs
View file
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
View file
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
View file
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
View file
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
View file
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

0 comments on commit aca95aa

Please sign in to comment.