Skip to content
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

Mapping Ref to a value containing a reference #54776

Open
d-e-s-o opened this issue Oct 3, 2018 · 7 comments
Open

Mapping Ref to a value containing a reference #54776

d-e-s-o opened this issue Oct 3, 2018 · 7 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@d-e-s-o
Copy link
Contributor

d-e-s-o commented Oct 3, 2018

Consider the following specific use case explaining a more general
issue: We have a struct, RefStrings, that keeps a RefCell containing
some form of collection, Vec<String>. Part of RefStrings's API
surface is a function that aims to expose an iterator over the
underlying collection. The only possibility I see for exposing the
iterator (conceptually) is via a Ref and using RefCell's map
function:

struct RefStrings(RefCell<Vec<String>>);

impl RefStrings {
    fn iter<'t, 's: 't>(&'s self) -> Ref<'t, Iter<String>> {
        Ref::map(self.0.borrow(), |x| x.iter())
    }
}

(playground)

Unfortunately, that does not work. The reason being that Ref is
defined to hold a reference to a member of the borrowed object, but an
iterator is effectively an actual object in itself that just happens to
have such a reference.

I have not found a way to get this working using the existing
functionality. Am I just missing something?

A possible solution that I worked with now is the introduction of a new
associated function Ref::map_val that returns an object that
effectively contains another object (i.e., something that is Sized)
that may hold a reference to the borrowed data (e.g., a concrete
iterator type). That solves the problem reasonably nicely, in my
opinion.

struct RefStrings(RefCell<Vec<String>>);

impl RefStrings {
    fn iter<'t, 's: 't>(&'s self) -> RefVal<'t, Iter<String>> {
        Ref::map_val(self.0.borrow(), |x| x.iter())
    }
}

// ...
// RefVal is defined as:
pub struct RefVal<'b, T> {
    value: T,
    borrow: BorrowRef<'b>,
}

(see the cell crate for the full functionality; note that the API provided is still limited, i.e., I mostly implemented what I needed right away)

Unfortunately I have not found a way to provide said functionality as
anything else than a replacement of RefCell itself, with all the code
duplication that accompanies.

So, I am filing this issue to discuss

  1. whether I am just missing something and there is a trivial way to
    accomplish what I hopefully explained well enough
  2. if that is not the case, whether this functionality is desired to be
    included in the Rust standard library (I believe this is a general
    problem with a general solution; despite probably not being a very
    common one)
  3. if such a desire exists, the steps to be taken to include this
    functionality (RFC process?)

EDIT: Proposed solution turned out to be unsafe with no known workaround. So really this issue is only to discuss other solutions.

@jonas-schievink jonas-schievink added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Jan 27, 2019
@elrnv
Copy link

elrnv commented Jun 20, 2019

I am also very bothered by the awkwardness of dealing with RefCell<Vec<_>>s.

With your solution, however, I found that I can invalidate the iterator:

use cell::*;
use std::slice::Iter;

struct MyRef(RefCell<Vec<usize>>);

impl MyRef {
    fn iter<'t, 's: 't>(&'s self) -> RefVal<'t, Iter<usize>> {
        Ref::map_val(self.0.borrow(), |x| x.iter())
    }
}

fn main() {
    let v = MyRef(RefCell::new(vec![1,2,3,4,5]));
    println!("capacity before: {:?}", v.0.borrow().capacity());
    
    let it = v.iter().clone();

    v.0.borrow_mut().push(0);

    println!("capacity after: {:?}", v.0.borrow().capacity());

    for &a in it {
        print!("{} ", a);
    }
}

This will compile and run. If the capacity before and after don't match, it means the vector has been reallocated and the output will read something other than "1 2 3 4 5 ". But just seeing that I can push to the original Vec after building the iterator is a red flag.

@d-e-s-o
Copy link
Contributor Author

d-e-s-o commented Jun 20, 2019

Yeah, that looks like an instance of a known problem to me. It has been discussed in quite some length on Github and the Rust user forum, but I am not sure of a good solution.

@elrnv
Copy link

elrnv commented Jun 20, 2019

Ah thank you. I agree that it looks like the same problem.
Thanks for pushing this through!

@d-e-s-o
Copy link
Contributor Author

d-e-s-o commented Aug 13, 2019

Just to get everybody on the same page: the proposed solution is unsafe in a very subtle way, so I am no longer suggesting to upstream it. I am keeping this issue open to discuss alternative solutions as the motivating problem seems to be something that others have stumbled over as well.

@hiddenhare
Copy link

hiddenhare commented Aug 13, 2019

When it comes to alternative solutions, as I understand it, the use-case is that "I have a struct which uses a RefCell<T> internally, and I'd like one of the struct's methods to return a value U that borrows from T, without requiring the caller to create an intermediate Ref<T> on the stack".

The main motivating example for me was a RefCell nested within another RefCell. For others, it was returning an iterator over a value stored in a RefCell. For simple borrowing of a value within a value within a RefCell, we already have Ref::map.

One solution proposed on the internals thread was to have a struct similar to your current RefVal<U>, but require U to implement Deref, and have the RefVal deref to U::Target. Unfortunately this would have the same problem I described in d-e-s-o/cell#5. I don't think it's possible to make this solution safe.

For a RefCell nested within another RefCell, we could imagine a type MetaRef<'a, T> where T is always Ref<'a, U>, RefMut<'a, U>, MetaRef<'a, U> or MetaRefMut<'a, U>. It's essentially a chain of Refs which upcasts the lifetimes of all of the inner Refs to 'a. I haven't thought about this in too much detail, but I think it would be possible to make it safe.

For iterators, we could imagine a type IterRefs<'a, T, U> where T: Iterator<Item = &'a U>. So for example, when iterating over a RefCell<Vec<i32>>, our return type would be an IterRefs<'a, slice::Iter<'a, i32>, i32>. The trick is that IterRefs would implement Iterator { Item = Ref<'a, U>; }, so all of its returned references would borrow-check appropriately. This would carry a performance cost: incrementing and decrementing the borrow-count once for each iteration step.

We could imagine similar types IterRefsMut<'a, T, U> where T: Iterator<Item = &'a mut U>, and IterVals<'a, T, U> where U: 'static. Alternatively, it might be possible to replace the U in IterRefs<'a, T, U> with an output type, e.g. (Ref<'a, K>, Ref<'a, V>) so that IterRefs can iterate over HashMaps and so on.

I believe it would be safe to construct an IterRefs<'a, <&'a V>::Iterator, U> from a Ref<'a, V> where for<'a> &'a V: IntoIterator<Item = &'a U>, no closure required. Again, I'd need to give this a little more thought. It may also be possible to permit arbitrary iterators like String::chars or HashMap::keys if we require the user to provide a non-capturing closure fn(&'a T) -> Iter... I'm not certain.

I believe it would be possible to implement both IterRefs and MetaRef as libraries, rather than intrusively replacing RefCell. If I find the time, I'll probably take a stab at it within the next few weeks.

@hiddenhare
Copy link

Some research has uncovered the mature owning_ref crate, which has already solved my own use-case. It provides an OwningHandle type, which bundles a pointer to a RefCell along with a Ref which borrows from that pointer.

There's a pull request which would add a restricted OwningIter, fairly similar to my proposed API above. I'm not sure whether that API would actually be sound as it's been presented; I intend to look into it if I find the time.

cc @Kimundi

@Ixrec
Copy link
Contributor

Ixrec commented Aug 13, 2019

Probably worth mentioning that it's not entirely clear whether crates like owning_ref are sound: rust-lang/unsafe-code-guidelines#194

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants