Improved error messages from Bound::borrow/borrow_mut

[Divy1211]

Suggestion

My suggestion is to improve the error message generated by Bound<_', T> when the runtime borrowing rules are violated to look something like this:

If a shared reference exists and a borrow_mut was attempted:

Bound::borrow_mut - Cannot create a mutable reference when a shared reference exists.

Perhaps an additional help text could also be included?:

Help: If a shared reference is no longer needed, consider calling `drop` on the `PyRef` instance. A mutable borrow is only allowed when all shared references have been dropped

If a mutable borrow exists and borrow is attempted:

Bound::borrow - Cannot create a shared reference when a mutable reference exists.

Help: If a mutable reference is no longer needed, consider calling `drop` on the `PyRefMut` instance.

Motivation

TL;DR: Just providing a better indicator of where the error comes from, to provide better debugging direction.

Story Time:

While trying to write some code, I had a struct containing Arc<PyObject>s and Arc<RwLock<<T>>s in Rust and when I tried to call bound_obj.extract::<Struct>(), I got an error that simply said Already mutably borrowed. Since there's no backtrace I was confused as to where the error came from. Not being super familiar with the semantics of Arcs and RwLocks I thought the error was coming from trying to clone them, instead of the fact that extract immutably borrows the Bound<'_, T>. It was only after I implemented Clone manually (and other unsuccessful debugging) I found that the failure occurs before clone is even called.

After hand tracing my value back to its source, I realised that a name (the mutable reference I made earlier) not being live does not trigger an implicit drop() like I thought, and that I needed to call it manually (that's where the suggestion for the extra help comes from, because I was initially creating nested scopes until I recalled drop exists.)

Implementation

I'd be happy to try and implement this! I think this is simple enough and I'd love to be able to gain some familiarity with PyO3's code itself =)

[mejrs]

Since there's no backtrace I was confused as to where the error came from.

We can, in principle, use Location to track where borrows happen, like how std's debug_refcell feature does it. But our borrow tracking is already fairly complicated, and tracking borrows would also be complicated. I'm reluctant to add more complexity to it.

[davidhewitt]

I guess we wouldn't need to store any state where the borrows happen at least? We could just slap #[track_caller] on a bunch of methods and then use the location to make errors a little bit easier to understand?

[mejrs]

I guess we wouldn't need to store any state where the borrows happen at least?

We would need to store it inside the borrow checker implementation. See main...mejrs:pyo3:debug_pyref for example. then we'd also need to be clever about how to track and untrack where shared borrows start and end.