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

fix: incorrect suggestions when .then and .then_some is used #12094

Merged
merged 6 commits into from
Apr 14, 2024
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 31 additions & 7 deletions clippy_lints/src/methods/search_is_some.rs
Original file line number Diff line number Diff line change
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})")
};
yuxqiu marked this conversation as resolved.
Show resolved Hide resolved
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 {
xFrednet marked this conversation as resolved.
Show resolved Hide resolved
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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