Consider nested lifetimes in mut_from_ref
#14471
Conversation
rustbot
added
the
S-waiting-on-review
|
I find it very fitting that both the issue reported and the issue bugfixer have cat profile pictures. |
| hir::intravisit::walk_generic_arg(self, generic_arg); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This function name is not clear enough and it doesn't Have documentation, what about get_ref_lifetime_mutability?
There was a problem hiding this comment.
While writing some docs for it I convinced myself that get_lifetimes was a better name, what do you think?
clippy_lints/src/ptr.rs
Outdated
| for (out, mutability, out_span) in get_ref_lm(ty) { | ||
| if mutability != Some(Mutability::Mut) { | ||
| continue; | ||
| } | ||
| let out_region = cx.tcx.named_bound_var(out.hir_id); | ||
| let args: Option<Vec<_>> = sig | ||
| .decl | ||
| .inputs | ||
| .iter() | ||
| .filter_map(get_ref_lm) | ||
| .flat_map(get_ref_lm) | ||
| .filter(|&(lt, _, _)| cx.tcx.named_bound_var(lt.hir_id) == out_region) | ||
| .map(|(_, mutability, span)| (mutability == Mutability::Not).then_some(span)) | ||
| .map(|(_, mutability, span)| (mutability == Some(Mutability::Not)).then_some(span)) |
There was a problem hiding this comment.
If we only care about Some(Mutability::<Mut, Not>), we can just not add to results when the case is None? Is there anything impeding that?
There was a problem hiding this comment.
The difference is between Some(Not) and Some(Mut)/None actually! The first is collected inside the vector, while the other two short-circuit and make collect return None.
I added a comment to clarify this.
| .filter(|&(lt, _, _)| cx.tcx.named_bound_var(lt.hir_id) == out_region) | ||
| .map(|(_, mutability, span)| (mutability == Mutability::Not).then_some(span)) | ||
| .map(|(_, mutability, span)| (mutability == Some(Mutability::Not)).then_some(span)) |
There was a problem hiding this comment.
What about this change? filter_map is more concise and gives us a possibility for further optimization (AFAIK)
| .map(|(_, mutability, span)| (mutability == Some(Mutability::Not)).then_some(span)) | |
| .filter_map(|(lt, mutability, span)| (mutability == Some(Mutability::Not)).then_some(span)) |
There was a problem hiding this comment.
I tried that initially! But it does not have the same semantics: we really want to return a None to short-circuit the iterator.
arnaudgolfouse
force-pushed
the
mut_from_ref-false-positive
branch
from
15560d1 to
fb8e574
Compare
fixes #7749.
This issue proposes searching for
DerefMutimpls, which is not done here: every lifetime parameter (aka<'a>) is the input types is considered to be potentially mutable, and thus deactivates the lint.changelog: [
mut_from_ref]: Fixes false positive, where lifetimes nested in the type (e.g.Box<&'a mut T>) were not considered.