Skip to content

[filter_next]: suggest making binding mutable if it needs to be #11016

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

Merged
merged 1 commit into from
Jul 10, 2023
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
51 changes: 41 additions & 10 deletions clippy_lints/src/methods/filter_next.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,29 @@
use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg};
use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
use clippy_utils::source::snippet;
use clippy_utils::ty::implements_trait;
use rustc_ast::{BindingAnnotation, Mutability};
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_lint::LateContext;
use rustc_span::sym;

use super::FILTER_NEXT;

fn path_to_local(expr: &hir::Expr<'_>) -> Option<hir::HirId> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a path_to_local helper in clippy_utils. Can we use that here?

Copy link
Member Author

@y21 y21 Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, the existing path_to_local in utils doesn't "work" if the local is reached through field accesses, so for example it would return false for a.b[0] if given the id of the local a, but we need it to return true, because we use this logic to figure out if a binding needs to be made mutable, and a.b[0].find(..) requires a be mutable. Maybe this could be better named or have a comment explaining it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clippy_utils::path_to_local is too primitive in my opinion. It only handles the case where the expression is a Path, and its Res is a Res::Local. Maybe we should modify it and make it handle this new cases.

(path_to_local's source)

match expr.kind {
hir::ExprKind::Field(f, _) => path_to_local(f),
hir::ExprKind::Index(recv, _) => path_to_local(recv),
hir::ExprKind::Path(hir::QPath::Resolved(
_,
hir::Path {
res: rustc_hir::def::Res::Local(local),
..
},
)) => Some(*local),
_ => None,
}
}

/// lint use of `filter().next()` for `Iterators`
pub(super) fn check<'tcx>(
cx: &LateContext<'tcx>,
Expand All @@ -26,15 +42,30 @@ pub(super) fn check<'tcx>(
if filter_snippet.lines().count() <= 1 {
let iter_snippet = snippet(cx, recv.span, "..");
// add note if not multi-line
span_lint_and_sugg(
cx,
FILTER_NEXT,
expr.span,
msg,
"try",
format!("{iter_snippet}.find({filter_snippet})"),
Applicability::MachineApplicable,
);
span_lint_and_then(cx, FILTER_NEXT, expr.span, msg, |diag| {
let (applicability, pat) = if let Some(id) = path_to_local(recv)
&& let Some(hir::Node::Pat(pat)) = cx.tcx.hir().find(id)
&& let hir::PatKind::Binding(BindingAnnotation(_, Mutability::Not), _, ident, _) = pat.kind
{
(Applicability::Unspecified, Some((pat.span, ident)))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can no longer be a machine applicable suggestion if it needs to be made mutable because of cases like these:

let (Ok(f) | Err(f)) = ...;
f.filter(...).next();

There are multiple bindings that would need to be made mutable, but cx.tcx.hir().find(id) only returns one pattern.

iter_skip_next is doing this as well, so I think having it be unspecified is fine.

if_chain! {
if let Some(id) = path_to_local(recv);
if let Node::Pat(pat) = cx.tcx.hir().get(id);
if let PatKind::Binding(ann, _, _, _) = pat.kind;
if ann != BindingAnnotation::MUT;
then {
application = Applicability::Unspecified;
diag.span_help(
pat.span,
format!("for this change `{}` has to be mutable", snippet(cx, pat.span, "..")),
);
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gues we can walk the parent to find all the bindings, but that's also not simple. This is good enough for now

} else {
(Applicability::MachineApplicable, None)
};

diag.span_suggestion(
expr.span,
"try",
format!("{iter_snippet}.find({filter_snippet})"),
applicability,
);

if let Some((pat_span, ident)) = pat {
diag.span_help(
pat_span,
format!("you will also need to make `{ident}` mutable, because `find` takes `&mut self`"),
);
}
});
} else {
span_lint(cx, FILTER_NEXT, expr.span, msg);
}
Expand Down
10 changes: 10 additions & 0 deletions tests/ui/methods_unfixable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#![warn(clippy::filter_next)]

fn main() {
issue10029();
}

pub fn issue10029() {
let iter = (0..10);
let _ = iter.filter(|_| true).next();
}
15 changes: 15 additions & 0 deletions tests/ui/methods_unfixable.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error: called `filter(..).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(..)` instead
--> $DIR/methods_unfixable.rs:9:13
|
LL | let _ = iter.filter(|_| true).next();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `iter.find(|_| true)`
|
help: you will also need to make `iter` mutable, because `find` takes `&mut self`
--> $DIR/methods_unfixable.rs:8:9
|
LL | let iter = (0..10);
| ^^^^
= note: `-D clippy::filter-next` implied by `-D warnings`

error: aborting due to previous error