fix(mut_mutex_lock): FP in projection chains

[ada4a]

fixes #16253

changelog: [mut_mutex_lock]: fix FP in projection chains

[rustbot]

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]

No changes for 37d8ef4

[Jarcho]

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]

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)

[ada4a]

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

[samueltardieu]

r? Jarcho since they reviewed it already and I haven't started yet.

[Jarcho]

Rethinking this, indexing and field accesses should just not lint. Both would take exclusive access to only part of a value which would conflict with any immutable reborrow over the whole value. The code I gave you also didn't make sure the base of the projection was actually a mutable reference. We don't want to suggest on x.lock() when x is an owned value since it has a good chance of already having another active borrow.

[ada4a]

That's a pretty big change to the PR, so I've decided to open a v2 instead of force-pushing over everything: #16435