Fix panicking_unwrap FP on field access with implicit deref
#16196
Conversation
rustbot
added
the
S-waiting-on-review
|
Lintcheck changes for c905503
This comment will be updated if you push new changes |
rustbot
removed
the
S-waiting-on-review
| let field_projections = place | ||
| .projections | ||
| .iter() | ||
| .filter(|proj| matches!(proj.kind, ProjectionKind::Field(_, _))) | ||
| .collect::<Vec<_>>(); | ||
| is_potentially_local_place(*local_id, place) | ||
| // If there were projections other than field projections, err on the side of caution and say that they | ||
| // _might_ be mutating something. | ||
| // | ||
| // The reason we use `<=` and not `==` is that a mutation of `struct` or `struct.field1` should count as | ||
| // mutation of the child fields such as `struct.field1.field2` | ||
| && place.projections.len() <= field_indices.len() | ||
| && iter::zip(&place.projections, field_indices.iter().copied().rev()).all(|(proj, field_idx)| { | ||
| && field_projections.len() <= field_indices.len() | ||
| && iter::zip(&field_projections, field_indices.iter().copied().rev()).all(|(proj, field_idx)| { |
There was a problem hiding this comment.
Why does this work though?... It definitely no longer aligns with what the comment is describing
There was a problem hiding this comment.
I think what happened here is that I got my conditions wrong: when an non-field projections appears, we're supposed to return true, as it might be modifying something somehow -- but what the code was actually doing is returning false inside the .all(), meaning that the whole function would return false as well.
I came up with the following patch, which goes through each case a bit more thouroughly -- what do you think?
{
if !is_potentially_local_place(*local_id, place) {
// This can't be a local place -- no point checking any further
return false;
}
let mut field_indices = field_indices.iter().rev().copied();
for proj in &place.projections {
let ProjectionKind::Field(f_idx, _) = proj.kind else {
// This is a projection we didn't expect -- it _might_ be mutating something
return true;
};
let Some(field_idx) = field_indices.next() else {
// The field projections are going deeper than our `Local` -- a mutation in there
// can't change whether the `Local` is the success or error variant
return false;
};
if f_idx != field_idx {
// The projection is going into an unrelated field -- it won't mutate our `Local`
return false;
}
}
// There are no more projections, and we haven't strayed off the field access chain that our `Local`
// is located on, so a mutation of this place might mutate our `Local`
true
}There was a problem hiding this comment.
I don't really get your point, but for the case of the issue, there is a deref before the field access
There was a problem hiding this comment.
What I'm trying to say is that just filtering out non-field projections might be dangerous, as they might end up modifying the Local... somehow. So I think we should instead early-return when encountering them. The patch above achieves that using the first let-else
4 participants
Closes #16188
changelog: [
panicking_unwrap] fix FP on field access with implicit deref