Skip to content

Commit

Permalink
Auto merge of rust-lang#12094 - yuxqiu:search_is_some, r=xFrednet,ARa…
Browse files Browse the repository at this point in the history 
…ndomDev99

fix: incorrect suggestions when `.then` and `.then_some` is used

fixes rust-lang#11910

In the current implementation of `search_is_some`, if a `.is_none` call is followed by a `.then` or `.then_some` call, the generated `!` will incorrectly negate the values returned by the `then` and `.then_some` calls. To fix this, we need to add parentheses to the generated suggestions when appropriate.

changelog: [`search_is_some`]: add parenthesis to suggestions when appropriate
  • Loading branch information
@bors
bors committed Apr 14, 2024
2 parents 832fdb6 + b89fa53 commit 7063e34
Show file tree
Hide file tree
Showing 4 changed files with 204 additions and 8 deletions.
38 changes: 31 additions & 7 deletions clippy_lints/src/methods/search_is_some.rs
View file
Expand Up @@ -2,7 +2,8 @@ use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
use clippy_utils::source::{snippet, snippet_with_applicability};
use clippy_utils::sugg::deref_closure_args;
use clippy_utils::ty::is_type_lang_item;
use clippy_utils::{is_trait_method, strip_pat_refs};
use clippy_utils::{get_parent_expr, is_trait_method, strip_pat_refs};
use hir::ExprKind;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::PatKind;
Expand Down Expand Up @@ -35,7 +36,7 @@ pub(super) fn check<'tcx>(
// suggest `any(|..| *..)` instead of `any(|..| **..)` for `find(|..| **..).is_some()`
let mut applicability = Applicability::MachineApplicable;
let any_search_snippet = if search_method == "find"
&& let hir::ExprKind::Closure(&hir::Closure { body, .. }) = search_arg.kind
&& let ExprKind::Closure(&hir::Closure { body, .. }) = search_arg.kind
&& let closure_body = cx.tcx.hir().body(body)
&& let Some(closure_arg) = closure_body.params.first()
{
Expand Down Expand Up @@ -72,16 +73,24 @@ pub(super) fn check<'tcx>(
);
} else {
let iter = snippet(cx, search_recv.span, "..");
let sugg = if is_receiver_of_method_call(cx, expr) {
format!(
"(!{iter}.any({}))",
any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
)
} else {
format!(
"!{iter}.any({})",
any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
)
};
span_lint_and_sugg(
cx,
SEARCH_IS_SOME,
expr.span,
msg,
"consider using",
format!(
"!{iter}.any({})",
any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
),
sugg,
applicability,
);
}
Expand Down Expand Up @@ -127,13 +136,18 @@ pub(super) fn check<'tcx>(
let string = snippet(cx, search_recv.span, "..");
let mut applicability = Applicability::MachineApplicable;
let find_arg = snippet_with_applicability(cx, search_arg.span, "..", &mut applicability);
let sugg = if is_receiver_of_method_call(cx, expr) {
format!("(!{string}.contains({find_arg}))")
} else {
format!("!{string}.contains({find_arg})")
};
span_lint_and_sugg(
cx,
SEARCH_IS_SOME,
expr.span,
msg,
"consider using",
format!("!{string}.contains({find_arg})"),
sugg,
applicability,
);
},
Expand All @@ -142,3 +156,13 @@ pub(super) fn check<'tcx>(
}
}
}

fn is_receiver_of_method_call(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool {
if let Some(parent_expr) = get_parent_expr(cx, expr)
&& let ExprKind::MethodCall(_, receiver, ..) = parent_expr.kind
&& receiver.hir_id == expr.hir_id
{
return true;
}
false
}
50 changes: 50 additions & 0 deletions tests/ui/search_is_some_fixable_none.fixed
View file
Expand Up @@ -213,3 +213,53 @@ mod issue7392 {
let _ = !v.iter().any(|fp| test_u32_2(*fp.field));
}
}

mod issue_11910 {
fn computations() -> u32 {
0
}

struct Foo;
impl Foo {
fn bar(&self, _: bool) {}
}

fn test_normal_for_iter() {
let v = vec![3, 2, 1, 0, -1, -2, -3];
let _ = !v.iter().any(|x| *x == 42);
Foo.bar(!v.iter().any(|x| *x == 42));
}

fn test_then_for_iter() {
let v = vec![3, 2, 1, 0, -1, -2, -3];
(!v.iter().any(|x| *x == 42)).then(computations);
}

fn test_then_some_for_iter() {
let v = vec![3, 2, 1, 0, -1, -2, -3];
(!v.iter().any(|x| *x == 42)).then_some(0);
}

fn test_normal_for_str() {
let s = "hello";
let _ = !s.contains("world");
Foo.bar(!s.contains("world"));
let s = String::from("hello");
let _ = !s.contains("world");
Foo.bar(!s.contains("world"));
}

fn test_then_for_str() {
let s = "hello";
let _ = (!s.contains("world")).then(computations);
let s = String::from("hello");
let _ = (!s.contains("world")).then(computations);
}

fn test_then_some_for_str() {
let s = "hello";
let _ = (!s.contains("world")).then_some(0);
let s = String::from("hello");
let _ = (!s.contains("world")).then_some(0);
}
}
50 changes: 50 additions & 0 deletions tests/ui/search_is_some_fixable_none.rs
View file
Expand Up @@ -219,3 +219,53 @@ mod issue7392 {
let _ = v.iter().find(|fp| test_u32_2(*fp.field)).is_none();
}
}

mod issue_11910 {
fn computations() -> u32 {
0
}

struct Foo;
impl Foo {
fn bar(&self, _: bool) {}
}

fn test_normal_for_iter() {
let v = vec![3, 2, 1, 0, -1, -2, -3];
let _ = v.iter().find(|x| **x == 42).is_none();
Foo.bar(v.iter().find(|x| **x == 42).is_none());
}

fn test_then_for_iter() {
let v = vec![3, 2, 1, 0, -1, -2, -3];
v.iter().find(|x| **x == 42).is_none().then(computations);
}

fn test_then_some_for_iter() {
let v = vec![3, 2, 1, 0, -1, -2, -3];
v.iter().find(|x| **x == 42).is_none().then_some(0);
}

fn test_normal_for_str() {
let s = "hello";
let _ = s.find("world").is_none();
Foo.bar(s.find("world").is_none());
let s = String::from("hello");
let _ = s.find("world").is_none();
Foo.bar(s.find("world").is_none());
}

fn test_then_for_str() {
let s = "hello";
let _ = s.find("world").is_none().then(computations);
let s = String::from("hello");
let _ = s.find("world").is_none().then(computations);
}

fn test_then_some_for_str() {
let s = "hello";
let _ = s.find("world").is_none().then_some(0);
let s = String::from("hello");
let _ = s.find("world").is_none().then_some(0);
}
}
74 changes: 73 additions & 1 deletion tests/ui/search_is_some_fixable_none.stderr
View file
Expand Up @@ -282,5 +282,77 @@ error: called `is_none()` after searching an `Iterator` with `find`
LL | let _ = v.iter().find(|fp| test_u32_2(*fp.field)).is_none();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `!v.iter().any(|fp| test_u32_2(*fp.field))`

error: aborting due to 43 previous errors
error: called `is_none()` after searching an `Iterator` with `find`
--> tests/ui/search_is_some_fixable_none.rs:235:17
|
LL | let _ = v.iter().find(|x| **x == 42).is_none();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `!v.iter().any(|x| *x == 42)`

error: called `is_none()` after searching an `Iterator` with `find`
--> tests/ui/search_is_some_fixable_none.rs:236:17
|
LL | Foo.bar(v.iter().find(|x| **x == 42).is_none());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `!v.iter().any(|x| *x == 42)`

error: called `is_none()` after searching an `Iterator` with `find`
--> tests/ui/search_is_some_fixable_none.rs:241:9
|
LL | v.iter().find(|x| **x == 42).is_none().then(computations);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `(!v.iter().any(|x| *x == 42))`

error: called `is_none()` after searching an `Iterator` with `find`
--> tests/ui/search_is_some_fixable_none.rs:246:9
|
LL | v.iter().find(|x| **x == 42).is_none().then_some(0);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `(!v.iter().any(|x| *x == 42))`

error: called `is_none()` after calling `find()` on a string
--> tests/ui/search_is_some_fixable_none.rs:251:17
|
LL | let _ = s.find("world").is_none();
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `!s.contains("world")`

error: called `is_none()` after calling `find()` on a string
--> tests/ui/search_is_some_fixable_none.rs:252:17
|
LL | Foo.bar(s.find("world").is_none());
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `!s.contains("world")`

error: called `is_none()` after calling `find()` on a string
--> tests/ui/search_is_some_fixable_none.rs:254:17
|
LL | let _ = s.find("world").is_none();
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `!s.contains("world")`

error: called `is_none()` after calling `find()` on a string
--> tests/ui/search_is_some_fixable_none.rs:255:17
|
LL | Foo.bar(s.find("world").is_none());
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `!s.contains("world")`

error: called `is_none()` after calling `find()` on a string
--> tests/ui/search_is_some_fixable_none.rs:260:17
|
LL | let _ = s.find("world").is_none().then(computations);
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `(!s.contains("world"))`

error: called `is_none()` after calling `find()` on a string
--> tests/ui/search_is_some_fixable_none.rs:262:17
|
LL | let _ = s.find("world").is_none().then(computations);
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `(!s.contains("world"))`

error: called `is_none()` after calling `find()` on a string
--> tests/ui/search_is_some_fixable_none.rs:267:17
|
LL | let _ = s.find("world").is_none().then_some(0);
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `(!s.contains("world"))`

error: called `is_none()` after calling `find()` on a string
--> tests/ui/search_is_some_fixable_none.rs:269:17
|
LL | let _ = s.find("world").is_none().then_some(0);
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `(!s.contains("world"))`

error: aborting due to 55 previous errors

0 comments on commit 7063e34

Please sign in to comment.