Skip to content

Fix panicking_unwrap FP on field access with implicit deref #16196

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.

Sign up for GitHub

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
llogiq merged 1 commit into from
Dec 5, 2025
Merged

Fix panicking_unwrap FP on field access with implicit deref #16196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 7 additions & 2 deletions clippy_lints/src/unwrap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,14 +180,19 @@ impl Local {
field_indices,
..
} => {
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)| {
Comment on lines +183 to +195
Copy link
Contributor

@ada4a ada4a Dec 5, 2025

Choose a reason for hiding this comment

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

Why does this work though?... It definitely no longer aligns with what the comment is describing

Copy link
Contributor

@ada4a ada4a Dec 5, 2025

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor Author

@profetia profetia Dec 5, 2025

Choose a reason for hiding this comment

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

I don't really get your point, but for the case of the issue, there is a deref before the field access

Copy link
Contributor

@ada4a ada4a Dec 5, 2025

Choose a reason for hiding this comment

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

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

match proj.kind {
ProjectionKind::Field(f_idx, _) => f_idx == field_idx,
// If this is a projection we don't expect, it _might_ be mutating something
Expand Down
19 changes: 19 additions & 0 deletions tests/ui/checked_unwrap/simple_conditionals.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,3 +472,22 @@ fn issue15321() {
//~^ unnecessary_unwrap
}
}

mod issue16188 {
struct Foo {
value: Option<i32>,
}

impl Foo {
pub fn bar(&mut self) {
let print_value = |v: i32| {
println!("{}", v);
};

if self.value.is_none() {
self.value = Some(10);
print_value(self.value.unwrap());
}
}
}
}