New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Downgrading of `RefMut` to `Ref` #57401

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Palladinium
Copy link

Palladinium commented Jan 7, 2019

Add RefMut::downgrade(), which consumes a RefMut and returns a Ref to the same RefCell.
This allows to avoid dropping and reborrowing, which is ergonomic in general and particularly useful after a RefMut::map with a non-trivial operation (e.g. hash map update/lookup) as otherwise the mapped ref would be lost in the process.

In addition, add impl From<RefMut> for Ref, which just calls RefMut::downgrade() for when .into() would be more ergonomic than RefMut::downgrade(...).
Can in theory conflict with into()/from() implementations of the Derefable contents, but I'm not sure if that would actually be an issue.

Example:

use std::cell::{RefCell, Ref, RefMut};

let mut hash: RefCell<HashMap<i32, i32>> = RefCell::new(HashMap::new());

let r1 = RefMut::downgrade(RefMut::map(hash.borrow_mut(), |h| h.entry(12).get_or_insert(1)).into());
let r2 = Ref::map(hash.borrow(), |h| h.get(12).unwrap());

assert_eq!(*r1, *r2);

WIP: No tracking issue yet, waiting to see if this is ok as a PR or if it should go through an RFC first.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 7, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @KodrAus (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.


Ref {
value: orig.value,
borrow: BorrowRef::new(borrow).expect("inconsistent RefCell state"),

This comment has been minimized.

@goffrie

goffrie Jan 7, 2019

Contributor

This isn't compatible with map_split, is it?

This comment has been minimized.

@Palladinium

Palladinium Jan 7, 2019

Right, it'll panic if you try to downgrade after a RefMut::map_split() with both references still alive.

Ideally you'd be able to mutably borrow one half and immutably borrow the other like with regular references, but I don't think that's possible with runtime-checked borrows as we can't know in advance how many borrow counters we need to allocate in the RefCell.

Still, I can think of a few ways to ameliorate the issue:

  • Add new APIs for downgrading multiple references at once
  • Add a downgrading version of map_split (and map for consistency)
  • Simply state it as a limitation of the API in the docs

I think the second option is the one I find the most attractive, since it also fixes the related problem of being unable to map with a (&mut T) -> &U function.

This comment has been minimized.

@goffrie

goffrie Jan 9, 2019

Contributor

unable to map with a (&mut T) -> &U function

Hm, I think this is unsound (assuming that your downgrading map returns a shared Ref, not some special kind of ref) in the presence of functions like Cell::from_mut: you could .borrow_mut() to get a RefMut<T>, map_downgrade(Cell::from_mut) to get a Ref<Cell<T>>, then .borrow() again to get a Ref<T> - but then you could write via the Ref<Cell<T>>.

In general RefMut -> Ref conversion, in the presence of map, seems like it could interact badly with other unsafe abstractions...

This comment has been minimized.

@Palladinium

Palladinium Jan 10, 2019

That's a good point, this is trickier than I thought. Reading through the PRs and issues for Cell::from_mut it looks like its soundness was proven pretty conclusively, so I'm wondering if map_downgrade is unsound in general or if all kinds of downgrading would be.

This comment has been minimized.

@RalfJung

RalfJung Jan 10, 2019

Member

It's not just Cell::from_mut. I think Mutex is also incompatible with map_downgrade, at least if it allows calling borrow again after the downgrade: Starting with RefCell<Mutex<T>>, you do .borrow_mut().map_downgrade(|m| &*m.get_mut()), so you end up with a Ref<T> but the mutex is not marked as locked. Then you do .borrow().lock().unwrap(), and you can write into the same memory. (This is a variant of this counterexample for another proposal.)

@Centril Centril requested a review from RalfJung Jan 9, 2019

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Jan 10, 2019

In fact, I think this example breaks the PR as proposed: Starting with RefCell<Mutex<T>>, I can do .borrow_mut().map(Mutex::get_mut).downgrade() to get a Ref<T> (from which I can get an &T) without acquiring the lock. Then I can do .borrow().lock().unwrap() to get an aliasing &mut.

You could fix this (idea by @jhjourdan) by not allowing calls to .borrow() after the downgrade. Leave the RefCell count in the "mutably borrowed" state. So the downgrade would give a Ref that you could clone, enabling several references into the RefCell, but you couldn't create more shared references to the "outside". That would also fix the problem with map_downgrade, and the bad interaction with split.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment