Initial implementation of mut_refcell_borrow

[lukaslueg]

Fixes #9044

This is an implementation of #9044: If we have an exclusive reference to a RefCell, the methods that perform an implicit (panic) or explicit (Result) check can be replaced by RefCell::get_mut(&mut self) -> &mut T and performing the operation on the &mut T directly. This situation may come up when the RefCell is created and did not yet disappear behind the immutable reference that is the reason for its existence. This is akin to mut_mutex_lock.

At least initially, I came up light with suggesting MachineApplicable due to the myriad ways RefCell can be used and the fact that its methods return a guard (instead of a &mut), which may not typecheck when changed.

cargo lintcheck comes up empty with this new lint.

Notice that this is a style-lint because this is where mut_mutex_lock lives; perf might actually be better suited?!

I noticed that the language around get_mut quite strongly discourages its use, which may be a misunderstanding with respect to when get_mut can be used, as &mut RefCell does not come up often. This is a separate issue.

---

changelog: new lint: [mut_refcell_borrow]
#9423

[rust-highfive]

r? @Jarcho

(rust-highfive has picked a reviewer for you, use r? to override)

[lukaslueg]

Pulled to draft, because this is very unsatisfactory as it is. For instance, we should lint if the RefCell is owned (instead of behind a &mut), which is probably where most useless-use-of-borrow-mut occur. If someone has comments or suggestions, feel free

[lukaslueg]

There are essentially three cases for this lint, as far as I can see:

• If access to the RefCell is through a mutable borrow (and not behind deref, e.g. &mut Rc<RefCell<_>>), we can always lint to replace access via .borrow(), .borrow_mut(), .take() etc. with .get_mut(), skipping the check inherent to RefCell.
• If the RefCell is currently owned (e.g. let c = RefCell::new(...); c.borrow_mut()) we need to check if is currently already borrowed. There are three cases then:
  • If there is an outstanding RefMut, any access to the RefCell will either panic or fail. We might want to have a different lint for this. No access via .get_mut() is possible, because this would fail the borrow-checker.
  • If there is an outstanding Ref, .get_mut() is not possible, and only trying to get a RefMut would panic/error (.borrow_mut() won't work, but .borrow() would).
  • If there are no Ref or RefMut currently borrowed, we can always lint to .get_mut(), because the RefCell is currently owned and no borrows have been taken out.

I can't see how one would prove if an Expr can be mutably borrowed in its current position. Is there a way to reach into MIR to check this hypothetical?

[Jarcho]

redundant_clone does the same thing, but id doesn't distinguish mutable and immutable borrowers.

[Jarcho]

You're point isn't true. borrow can be called multiple times through &mut RefCell<_>.

[lukaslueg]

You mean the first point? In that case, each .borrow() could be replaced individually. One can .borrow() a &mut RefCell<_> multiple times, but we can't get a &mut RefCell once it has been .borrow()ed.

[Jarcho]

My point was you can do something like:

fn f(a: &mut RefCell<u32>) -> u32 {
    let b = a.borrow();
    let c = a.borrow();
    let d = &* a;
    let e = d.borrow();
    *b + *c + *e
}

You can't replace borrow just because you have a mutable reference.

[bors]

☔ The latest upstream changes (presumably #9541) made this pull request unmergeable. Please resolve the merge conflicts.

[RalfJung]

I just realize we have a needless borrow_mut in Miri that this would have caught. :)

@Jarcho hm, good point. However one could write

fn f(a: &mut RefCell<u32>) -> u32 {
    let a = a.get_mut();
    let b = &*a;
    let c = &*a;
    let e = &*a;
    *b + *c + *e
}

So, it is a slightly larger refactor, but still an improvement overall.

Though there are probably odd corner cases where even this is not possible. But clippy's lint policy is anyway to cover the 95%, not the 100%, isn't it?

[blyxyas]

Closing this for inactivity (296 days), @lukaslueg if you're still interested in working on this lint, feel free to re-open it! ❤️
ヽ(=^･ω･^=)丿