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

Zero-sized LCellOwner APIs? #39

Open
SoniEx2 opened this issue Dec 28, 2022 · 20 comments
Open

Zero-sized LCellOwner APIs? #39

SoniEx2 opened this issue Dec 28, 2022 · 20 comments

Comments

@SoniEx2
Copy link
Contributor

SoniEx2 commented Dec 28, 2022

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
Copy link
Contributor Author

SoniEx2 commented Dec 29, 2022

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
Copy link
Owner

uazu commented Dec 29, 2022

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
Copy link
Contributor Author

SoniEx2 commented Dec 29, 2022

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
Copy link
Owner

uazu commented Jan 12, 2023

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
Copy link
Owner

uazu commented Jan 12, 2023

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
Copy link
Contributor Author

SoniEx2 commented Jan 12, 2023

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
Copy link
Contributor Author

SoniEx2 commented Jan 12, 2023

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

@uazu
Copy link
Owner

uazu commented Jan 12, 2023

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
Copy link
Contributor Author

SoniEx2 commented Jan 12, 2023

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
Copy link
Owner

uazu commented Jan 12, 2023

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
Copy link
Contributor Author

SoniEx2 commented Jan 12, 2023

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

@uazu
Copy link
Owner

uazu commented Jan 29, 2023

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
Copy link
Contributor Author

SoniEx2 commented Jan 29, 2023

ah, certainly! no worries. take your time!

@uazu
Copy link
Owner

uazu commented May 2, 2023

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
Copy link
Contributor Author

SoniEx2 commented May 2, 2023

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
Copy link
Owner

uazu commented May 3, 2023

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
Copy link
Contributor Author

SoniEx2 commented May 3, 2023

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

@uazu
Copy link
Owner

uazu commented May 3, 2023

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
Copy link
Contributor Author

SoniEx2 commented May 3, 2023

what are you trying?

@uazu
Copy link
Owner

uazu commented May 3, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants