[filter_next]: suggest making binding mutable if it needs to be
#11016
Conversation
|
r? @dswij (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
| && 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))) |
There was a problem hiding this comment.
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.
rust-clippy/clippy_lints/src/methods/iter_skip_next.rs
Lines 23 to 35 in ce0a48a
There was a problem hiding this comment.
I gues we can walk the parent to find all the bindings, but that's also not simple. This is good enough for now
|
Can you help to review this? @blyxyas |
|
Yep, will review it! 🤠 |
| 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> { |
There was a problem hiding this comment.
We have a path_to_local helper in clippy_utils. Can we use that here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| && 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))) |
There was a problem hiding this comment.
I gues we can walk the parent to find all the bindings, but that's also not simple. This is good enough for now
|
☔ The latest upstream changes (presumably #11055) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Thank you! @bors r=blyxyas,dswij |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fixes #10029
changelog: [
filter_next]: suggest making binding mutable if it needs to be and adjust applicability