Skip to content

Conversation

@ada4a
Copy link
Contributor

@ada4a ada4a commented Dec 18, 2025

fixes #16253

changelog: [mut_mutex_lock]: fix FP in projection chains

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 18, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 18, 2025

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@github-actions
Copy link

github-actions bot commented Dec 18, 2025

No changes for 37d8ef4

@Jarcho
Copy link
Contributor

Jarcho commented Dec 18, 2025

So this is still wrong. The thing this needs to check for is that each adjustment to the place could have been a mutable adjustment rather than an immutable one. Would be something like (not quite real code and please write it better):

let mut e = recv;
'outer: loop {
  if !expr_adjustments(e)
    .take_while(|a| match a.kind {
        Adjust::Deref(x) => Some((a.target, x)),
        _ => None,
    }).try_fold(expr_ty(e), |ty, (target, deref)| match deref {
      None => (!ty.is_imm_ref()).then(target),
      Some(_) => impls_deref_mut(ty).then(target),
    }).is_some()
  {
    return;
  }
  loop {
      match e.kind {
        ExprKind::Field(base, _) => e = base,
        ExprKind::Index(base, idx) if impls_idx_mut(expr_ty_adjusted(base), expr_ty_adjusted(idx)) => e = base,
        // `base` here can't actually have adjustments
        ExprKind::UnOp(UnOp::Deref, base) if impls_deref_mut(expr_ty_adjusted(base)) => {
            e = base;
            continue;
        },
        _ => break 'outer,
      }
      continue 'outer;
  }
}

@ada4a
Copy link
Contributor Author

ada4a commented Dec 19, 2025

Okay so I took your code, fixed some small bugs/typos, but importantly also removed the inner loop, because, as you implied yourself, it made the code a lot more confusing for imo (?) little benefit. I did keep each step as a separate commit though, so I can easily remove some of the changes if you prefer.

Looking at the first couple of dogfood results, they do seem to be legit -- apart from the one in entry_unfixable.rs where we lint on a Mutex stored in a static, which I think is an FP? I'll go take care of the other cases for now. (EDIT: hah, Lintcheck is all just this static mutex case now)

since there won't be any adjustments after an explicit deref, the
adjustment checking step should finish pretty fast
@ada4a
Copy link
Contributor Author

ada4a commented Dec 19, 2025

I implemented a pretty basic check for statics, but I think there ought to be a more robust approach -- especially given that this statics thing didn't happen before

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mut_mutex_lock: FP on struct.mutex where struct is behind &

4 participants