Closed finnbear closed 2 years ago
The documentation of Weak::upgrade()
currently says:
Returns None if the inner value has since been dropped.
But with this change, it could also return None while the value hasn't been dropped yet, but is just borrowed out mutably through RefMut
. I'm not sure if that's a big problem, but I can imagine code that might make assumptions about Weak::upgrade()
returning None meaning that the original object is fully gone.
If this proposal is implemented, a guard is only required during mutation. Immutable accesses can take place both by de-referencing (e.g. the synchronous business logic part of my app) and, in the same program, sharing Weak
's that get upgraded to Rc before the access takes place (e.g. the asynchronous Yew component hierarchy part of my app).
Using that pattern, everything that only needs immutable access then needs to use a Weak<T>
, such that requesting access (like RefCell::borrow()
) would go through Weak::upgrade()
. But that also means that if the only users left are those who need only immutable access, there are only weak pointers left, and the T
gets dropped and deallocated. That seems wrong?
I agree that using a Rc<RefCell<T>>
can be a bit verbose with .borrow()
everywhere, but using a Weak<T>
with Weak::upgrade()
everywhere doesn't seem much better.
I implemented this here.
Note that your implementation breaks and can result in a use-after-free when leaking a RefMut:
use std::rc::Rc;
use rc_borrow_mut::RcBorrowMut;
fn main() {
let mut rc = Rc::new("asdf".to_string());
std::mem::forget(Rc::borrow_mut(&mut rc));
drop(rc.clone());
println!("use after free: {rc:?}");
}
$ cargo run
use after free: "@��q"
I agree that using a Rc<RefCell
> can be a bit verbose with .borrow() everywhere, but using a Weak with Weak::upgrade() everywhere doesn't seem much better.
Minor correction: Rc<RefCell<T>>
does force you to use .borrow()
everywhere but, under this proposal, immutable accesses can take place via impl Deref for Rc
wherever &rc
is within reach (i.e. by the owner). Weak
is required for parts of the code that do not have a &rc
.
But that also means that if the only users left are those who need only immutable access, there are only weak pointers left, and the T gets dropped and deallocated. That seems wrong?
Thanks for pointing this out! That's not ideal.
Note that your implementation breaks and can result in a use-after-free when leaking a RefMut:
Thanks for pointing this out! :fearful:
While I knew not to trust destructors, I was under the (false) impression that code like this wouldn't compile:
let mut vec = vec![1, 2, 3];
std::mem::forget(vec.iter_mut());
println!("{vec:?}");
Fixing this would require taking ownership of the Rc
(weird and not sure what gets put in its place if called on a struct field) or adding some 'sentinel' value to cause Rc::clone
to fail somehow, I no longer believe this feature is worth including in std
. I'll also archive the repository on GitHub and add a warning comment.
Proposal
Problem statement
It is impossible to get a mutable reference into an
Rc<T>
when the weak reference count is non-zero, even when the strong reference count is 1.Motivation, use-cases
Right now, if I want to share and mutate data in my single-threaded Yew/Webassembly application, I have to use
Rc<RefCell<T>>
. Unfortunately, theRefCell
means that all immutable borrows require aRef
guard. As I originally designed my application around simplyClone
ing the state to share it, my hundreds of immutable accesses are via de-referencing and would all need to be adapted to use aRef
guard.If this proposal is implemented, a guard is only required during mutation. Immutable accesses can take place both by de-referencing (e.g. the synchronous business logic part of my app) and, in the same program, sharing
Weak<T>
's that get upgraded toRc<T>
before the access takes place (e.g. the asynchronous Yew component hierarchy part of my app).Solution sketches
Add
std::rc::RefMut
.Then, either:
Rc<T>::borrow_mut(&mut Self) -> Option<std::rc::RefMut<'_, T>>
(consistent withRc::get_mut
returning anOption
)Rc<T>::borrow_mut(&mut Self) -> std::rc::RefMut<'_, T>
andRc<T>::try_borrow_mut(&mut Self) -> Option<std::rc::RefMut<'_, T>>
(consistent withRefCell::borrow_mut
andRefCell::try_borrow_mut
)The above APIs can be implemented by:
1) Taking a
&mut Self
so that it can't becloned
during the process 2) Checking if the strong reference count is 1. If not, there are otherRc
's, so panic or returnNone
. 3) Setting the strong reference count to zero (so existingWeak
's cannot upgrade) 4) Returning a guard thatimpl Deref<Target = T> + DerefMut
5) The guardimpl Drop
to restore the strong reference count to 1Links and related work
I implemented this here. It uses unsafe and relies on unspecified implementation details of
Rc
(meaning it would benefit from inclusion instd
), but passes tests under Miri and works in my application.What happens now?
This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.