Add inherent `impl` block for `Mutex<RefCell<T>>`

[bradleyharden]

Add inherent functions to Mutex<RefCell<T>> to reduce verbosity when using the type. Add a note to the Mutex documentation explaining why Mutex::borrow does not return &mut T and point out the newly created methods on Mutex<RefCell<T>>. The design of Mutex::borrow is a frequently asked question, so it helps to explicitly document the decisions.

Closes #43.

[bradleyharden]

@adamgreig, thanks for taking a look. Did you read all of the added design documentation? I just want to make sure there are no errors.

[adamgreig]

I did, thanks for adding that! I believe it's accurate.

[bradleyharden]

Looks like replace_with came in Rust 1.35 and take in 1.50. Are you interested in upgrading the MSRV? Or should I remove them?

[adamgreig]

Please bump MSRV to 1.50, thanks!

[bradleyharden]

Pinging @adamgreig for workflow approval

[bradleyharden]

Follow up question. If everything in my added documentation is correct, then why is Mutex::inner an UnsafeCell<T> instead of just T? Mutex doesn't provide any interior mutability, so it doesn't seem necessary.

[bradleyharden]

@thalesfragoso, any thoughts on my last comment in this thread?

[thalesfragoso]

@thalesfragoso, any thoughts on my last comment in this thread?

I guess it makes sense, I have to think a bit more about the unsafe impl Sync part though.

[bradleyharden]

I think it works. We defer any interior mutability to T. If T needs it, then it can provide the UnsafeCell itself. And since you have to prove you're in a critical section, you're essentially in single-threaded code, so T: !Sync types are safe to work with. They need to be Send, though, because you could be accessing them from a different context than originally created.

[therealprof]

Nice!

[Dirbaio]

All the added methods can panic if the RefCell is already borrowed, it should be documented.

[bradleyharden]

@Dirbaio, I wanted to avoid repeating the documentation for each RefCell function, to prevent it from becoming stale. Are the links not sufficient? Would it be acceptable to mention the panic issue in the new documentation on the Mutex type? Or do you want to see a # Panics section in each new function?

[therealprof]

Maybe adding #[track_caller] would make sense, both as an indication that the function might panic and also to yield useful information about the problematic caller site.

[bradleyharden]

@therealprof, could that add formatting bloat?

[therealprof]

I don't think it will add any noticeable bloat on top of what's already there. Potentially panicking code is always bloaty...

[bradleyharden]

@therealprof, I added #[track_caller].

@Dirbaio, are the links to the RefCell documentation not enough? Do you want to see a dedicated # Panics section in each method?

@thalesfragoso, have you reviewed the latest version of the documentation and the updates to #[inline]?

[Dirbaio]

IMO always documenting panics explicitly is a nice convention. No strong opinion though, so if maintainers are OK with not documenting it, fine by me :)

[bradleyharden]

Added it

[bradleyharden]

Pinging @therealprof and @thalesfragoso again. Is this ready to merge?

[bors[bot]]

Build succeeded:

• ci (1.50.0, ubuntu-latest)
• ci (stable, macOS-latest)
• ci (stable, ubuntu-latest)
• ci (stable, windows-latest)
• clippy
• rustfmt

[adamgreig]

I'm not concerned about soundness, because we don't add anything here that couldn't be done by a user themselves. I worry we might have missed some other subtlety but I can't think of any...

@cr1901 pointed out we might include try_borrow{,_mut} too. I wonder if we cut a 1.1.0-alpha.1 release with these changes, give people a chance to check them out, potentially add a few more methods from RefCell, and then release 1.1?

[therealprof]

I'm not concerned about soundness, because we don't add anything here that couldn't be done by a user themselves. I worry we might have missed some other subtlety but I can't think of any...

Just because someone could implement thee same unsoundness themselves doesn't mean we shouldn't take extra care, IMHO. 😅 But I get your point.

An alpha release sounds like a splendid idea to me.