uazu / qcell

Statically-checked alternatives to RefCell and RwLock
Apache License 2.0
356 stars 22 forks source link

Zero-sized LCellOwner APIs? #39

Open SoniEx2 opened 1 year ago

SoniEx2 commented 1 year ago

Currently, LCellOwner is zero-sized, but &LCellOwner and &mut LCellOwner are not. We wonder if it's possible to have a sound API which borrows the LCellOwner but is zero-sized?

This is a bit of a micro-optimization but anyway.

SoniEx2 commented 1 year ago

so like what if we had LCellOwnerRef<'a, 'id> and LCellOwnerMut<'a, 'id> which are just wrappers around PhantomData<&'a LCellOwner<'id>> and PhantomData<&'a mut LCellOwner<'id>> respectively, and then had LCellOwner.borrow() and LCellOwner.borrow_mut()?

uazu commented 1 year ago

Those API calls are #[inline] so the &LCellOwner should optimise away completely, since it is unused within the function. Have you tried checking the assembly output to see whether it's actually taking and passing an address? It shouldn't need to.

Or is it a problem with passing the address down the stack to where it is used, through un-inlined calls?

SoniEx2 commented 1 year ago

Or is it a problem with passing the address down the stack to where it is used, through un-inlined calls?

yeah, it is a bit inconvenient for wrapping LCells in more complex ways.

uazu commented 1 year ago

Have you tried coding this? I've started coding it up, and there is the problem that when you pass an LCellOwnerRefMut down the stack, we want it to have the normal borrow-handling, i.e. to be "lent" to the method being called, and then to revert ownership to the caller again when the call finishes. So I don't see how I can implement that when the borrow is within the PhantomData. I need to pass it as self (not &self or &mut self which would pass an address, which you want to avoid) so I need some kind of a "lending-clone" for Self that doesn't exist.

Let me know if you have any idea how to solve this.

uazu commented 1 year ago

Or I suppose it could be "single-use", i.e. it gets passed down the stack and then consumed at the leaf. I'll try that.

SoniEx2 commented 1 year ago

so the idea is to have a .borrow_mut() on the LCellOwner that creates an LCellOwnerRefMut, well why not also have a .borrow_mut() the LCellOwnerRefMut?

(also .rw/.ro/etc on the LCellOwnerRefMut itself, which also uses autoreborrow by taking &mut/&/etc)

SoniEx2 commented 1 year ago

(fun fact: impl'ing on &mut Foo, e.g. impl<'a, 'de> Deserializer<'de> for &'a mut Foo<'de>, also turns off autoreborrow)

uazu commented 1 year ago

I'm having trouble with this test. owner.ref_mut() is what you're calling borrow_mut(), i.e. it generates an LCellOwnerRefMut. This doesn't generate a compilation error, but it should. So c1ref isn't maintaining the borrow on the owner as it should.

//! LCellOwner::scope(|mut owner| {
//!     let c1 = Rc::new(LCell::new(100u32));
//!     fn test(o: &mut LCellOwner) {}
//!
//!     let o = owner.ref_mut();
//!     let c1ref = o.ro(&c1);
//!     test(&mut owner);    // Compile error
//!     println!("{}", *c1ref);
//! });

My implementation must be faulty:

pub struct LCellOwnerRefMut<'b, 'id> {
    phantomdata: PhantomData<&'b mut LCellOwner<'id>>,
}

impl<'b, 'id> LCellOwnerRefMut<'b, 'id> {
    pub fn new(_: &'b mut LCellOwner<'id>) -> Self {
        Self {
            phantomdata: PhantomData,
        }
    }
    #[inline]
    pub fn ro<'a, T>(&self, lc: &'a LCell<'id, T>) -> &'a T {
        unsafe { &*lc.value.get() }
    }
    #[inline]
    pub fn rw<'a, T>(&mut self, lc: &'a LCell<'id, T>) -> &'a mut T {
        unsafe { &mut *lc.value.get() }
    }
}

Getting a & on the PhantomData isn't the same as getting a & on the LCellOwner. How to get a borrow with the 'b lifetime out of the PhantomData?

(The nested .borrow_mut() stuff can wait until this one is solved.)

SoniEx2 commented 1 year ago

in LCellOwner there is:

    pub fn rw<'a, T: ?Sized>(&'a mut self, lc: &'a LCell<'id, T>) -> &'a mut T {

this "expands" to:

    pub fn rw<'a, T: ?Sized>(self: &'a mut LCellOwner<'id>, lc: &'a LCell<'id, T>) -> &'a mut T {

in the case of the PhantomData, what you have is just self: &'_ mut PhantomData<&'b mut LCellOwner<'id>>. yeah?

we believe it should be self: &'a mut PhantomData<&'b mut LCellOwner<'id>> - the 'a should borrow the borrow of the owner, not the borrow of the owner itself. it is, after all, effectively a reborrow.

uazu commented 1 year ago

Okay, that seems to be working. I'll do some more testing in free moments in the coming days to see if I can break it. Also, I would prefer to get my head clear enough on this to understand exactly how this is working. I guess we're entangling the 'a and 'b lifetimes somehow.

Also, for LCellOwnerRefMut::borrow_mut, hopefully we can do the same trick, borrow to create a new 'b lifetime entangled with the previous 'b lifetime.

SoniEx2 commented 1 year ago

variance is an important concept to keep in mind here. https://doc.rust-lang.org/reference/subtyping.html

uazu commented 1 year ago

I think it's just lifetime nesting that makes it work, i.e. the existence of &'a &'b _ means that 'b has to outlive 'a.

The status on this is that I haven't had a lot of time. (My time right now is limited due to some Long Covid symptoms coming and going unfortunately.)

Also it adds a lot more API surface, so I have to be careful as I add it to not make a slip. I could implement the existing functionality in terms of these refs internally, but I'd prefer to keep the existing code easy to review without too much abstraction. So the rough plan is to add this with a cargo feature to enable it. I just have to find some clear time to go through all the detail.

SoniEx2 commented 1 year ago

ah, certainly! no worries. take your time!

uazu commented 1 year ago

I've been looking at implementing reborrowing for a custom type in another situation (for work), and it seems problematic to say the least. Obvious solutions don't work. For future reference: https://haibane-tenshi.github.io/rust-reborrowing/

SoniEx2 commented 1 year ago

yes, reborrowing is not supported for custom types.

the solution is to not use reborrowing for custom types. after all, implicit reborrowing can be made explicit with a simple syntactic choice (&* or &mut *). maybe implicit reborrowing should become a clippy lint.

uazu commented 1 year ago

It seems that there is one error in that article. He states that reborrowing by making the embedded reference public and then using a macro to reborrow the reference from inside the struct and create a new struct around that reborrowed reference won't work away from the original borrowing site. But I've tried it and it works. So it appears that there is a way around it. So a borrow_mut method won't be possible, but a borrow_mut_...! macro might work. (Assuming the PhantomData doesn't complicate things.)

SoniEx2 commented 1 year ago

No, a borrow_mut method should be fine, as long as it takes &mut itself.

uazu commented 1 year ago

This is exactly what I've been failing to achieve. I cannot get a borrow_mut to work in practice, no matter how I annotate with lifetimes or whatever. That blog post seems to agree that it is impossible (although he may be wrong).

SoniEx2 commented 1 year ago

what are you trying?

uazu commented 1 year ago

Okay, that is so weird. I'd deleted the non-working reborrow method code I had because I got the macro working instead. So I coded the method again and now I can't get it to go wrong. It works fine. I must have got some incorrect lifetime relation somewhere which just broke everything. So maybe this is all going to work fine. Sorry for the confusion.