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

Scoped borrow of UserData #84

Closed
luteberget opened this issue Aug 30, 2018 · 6 comments
Closed

Scoped borrow of UserData #84

luteberget opened this issue Aug 30, 2018 · 6 comments

Comments

@luteberget
Copy link

It seems like several people have struggled with putting Rust values into Lua as UserData and then later extracting them from Lua again, hoping for everything to be nice and safe when the value comes back out.

Wrapping a value in something like Rc<RefCell<Option<_>>> works, but is slightly cumbersome to work with. It also requires you to use an owned value. If you only have a mutable reference on hand, it seems like you cannot use it as a UserData. Scopes can create functions which are not 'static so you can use borrowed values there, but this is also somewhat cumbersome.

I have tried to come up with a workaround for this, allowing Scope to create UserData from mut-borrowed values. Hoping to get opinions on whether this is implementation will work. Apologies if I have missed something obvious or something that has already been discussed. I have looked at #20, especially #20 (comment) .

The idea is to give a &'scope mut T reference to the Scope and store it as a raw pointer *mut T, which is plain old data so it can have 'static lifetime. Because we are in a Scope, we know that the pointer will be deleted when the Scope is destructed. The UserData trait can be implemented for BorrowedUserData<T> by running T::add_methods with a modified UserDataMethods implementation which extracts BorrowedUserData<T> and dereferences the pointer whenever the T is needed. Checking for multiple borrows is handled with in the same way as owned values, with RefCell.

Code diff here: luteberget@8d32cca

I am certainly no expert in unsafe Rust, so if I've misunderstood any of the following then this might not work:

  1. Giving out a &'scope mut T reference ensures that the corresponding *mut T points to the T for the duration of the borrow.
  2. The pointer is only accessed and dereferenced when borrowed through the RefCell, so the dereferenced pointer can be used normally as a &T or &mut T in Rust code for the lifetime of the Ref/RefMut.
  3. I'm not sure a raw pointer is actually needed, maybe it would work with mem::transmute into static lifetime instead, or something else.

Also, the BorrowedUserData<T> struct is internal to the crate, so users of the library cannot use AnyUserData::borrow or similar. I think this could be fixed by checking for both T and BorrowedUserData<T> when borrowing from AnyUserData.

Using this feature looks like this:

extern crate rlua;
use rlua::prelude::*;

#[derive(Debug)]
struct MyData {
    val: u64,
}

impl LuaUserData for MyData {
    fn add_methods(methods: &mut LuaUserDataMethods<Self>) {
        methods.add_method_mut("increment", |_,this,()| {
            this.val += 1;
            Ok(())
        });
    }
}

fn main() -> LuaResult<()> {
    let lua = Lua::new();
    let mut x = MyData { val: 0 };
    lua.scope(|s| {
        lua.globals().set("x", s.borrow_userdata(&mut x)?)?;
        // mutate x from Lua
        lua.eval::<()>("x:increment()", None)?;
        Ok(())
    })?;
    x.val += 1;
    // x.val is now 2
    Ok(())
}
@kyren
Copy link
Contributor

kyren commented Aug 31, 2018

Hey, thanks for taking the time to understand this issue and take a stab at it! I think what you wrote is safe or at the very least could be made safe. Pretty cool!

I hadn't thought about this approach before, and I do believe it will work, but it still has a pretty big limitation, namely that the T type you pass to borrow_userdata must still itself be 'static. This change does allow you to mutate a UserData type inside a call to scope, but crucially it still won't allow you to do things like let Lua interact with non-static handles, such as Mutex locks or RefCell borrows.

This is interesting though, I had been approaching the problem as though allowing non-'static UserData types was the key important feature, rather than mutable references to 'static UserData. Maybe I'm wrong about this?

Unfortunately (as you probably already know) if you use this approach to try and lift the 'static restriction, you will still run into the issues described in the comment you mentioned: #20 (comment)

I've been trying to think of the best way of allowing fully non-'static userdata and I think the best way is simply a simpler, parallel API that is just a more convenient way of doing what I described in that comment, using non-'static functions to forward to ScopedUserData methods (or something that is equivalent but maybe slightly faster). You need a parallel API because like I explained in the comment, AnyUserData becomes completely unusable with non-'static userdata.

The ScopedUserData API I'm proposing would be a simpler version of UserData without methods that didn't accept &mut self as the first parameter, and a simple implementation of it would simply use a single scoped function as the __index metamethod, forwarding arguments to appropriate ScopedUserData methods, exactly as you would do if you were doing the "encode everything as a function" trick yourself. The downsides are that this would be potentially slow, but maybe not as slow as I'm thinking?

With your proposal though, now perhaps there's more to think about. Which of these APIs do you consider MORE useful, one that allows for non-'static userdata but is potentially slower and more complex with a separate API, or one that only allows for mutable references to 'static userdata but uses the same API? I personally ran into this problem dealing with Mutex, RwLock, RefCell etc so obviously I would choose the former, but maybe other people have different needs?

@luteberget
Copy link
Author

I guess I was a bit confused about the distinction between Lua borrowing values for a lifetime (vs. owning) and implementing UserData for non-'static types. It is a bit clearer to me now.

There might be different styles of organizing Rust code which require more or less of storing references inside structs (which is the main cause for non-'static lifetimes?). I have tried to organize my data so that everything is owned, using array indices, string keys, slot maps, or similar to let also references be "owned" in the Rust sense, so that everything is plain old hierarchically owned data.

Thinking about the Rust / Lua layer separation in an application, my approach is that Lua is a kind of user interface, which means that all the tricky resources stuff (graphics, networking, etc.) is handled by Rust code which only confers with Lua to incorporate the user's wishes. This could also mean, in some cases, to reify the Lua method calls into command objects. For example, if the Lua code calls load_image("i.jpg"), then this doesn't necessarily run code to load the image, it might instead add some enum value LoadImage("i.jpg") to a queue of things to be processed later.

In this style, I think non-'static types can often be kept apart from Lua. If your relationship between Rust and Lua is more intimate, then this style might be limiting.

For mutex (and similar) specifically, if it contains a T which is 'static, then Lua can borrow the value itself instead of the lock, using a Scope which the lock outlives.

fn update(lua :&Lua, value :Arc<Mutex<MyData>>) -> LuaResult<()> {
    let mut value = value.lock().unwrap();
    lua.scope(|scope| {
        lua.globals().set("x", scope.borrow_userdata(value.deref_mut())?)?;
        lua.eval::<()>("x:increment()", None)?;
        Ok(())
    })?;
    Ok(())
}

@kyren
Copy link
Contributor

kyren commented Aug 31, 2018

In general I agree with you, but I think forbidding non-'static UserData is just really limiting and weird, especially when the API sits right next to a similar feature (scoped callbacks) that doesn't have this limitation.

You're right that doing the borrowing outside of Lua and passing in direct references is a simpler pattern that could work, but there are lots of ways that pattern can become impossible. For example, in thread you linked to, I was responding to somebody trying to use rlua to make an API to specs, and this is (naively) impossible without non-'static UserData. This will be impossible in any API that either hides the fact that it contains a MutexLock / RwLockWriteGuard etc or doesn't give you some way of deref-ing to a 'static T you can implement UserData for. Even if you can get a &T for some 'static T, you often can't implement UserData for T directly due to coherence rules, and wrapper types are impossible because you can't have them be non-'static.

I mean, again you CAN actually do this because you can just make your entire API using nothing but functions, which I've done before to wrap a complex specs-like API, but it's really gnarly and involves basically passing everything through a "dispatch" function.

If I made a parallel ScopedUserData API that was similar to UserData, but ScopedUserDataMethods only had the add_xxx_method variants and not the add_xxx_function variants, would that be acceptable for your use? After thinking about it for a bit, I believe I can make an API that's not awful and is not terribly slow at least. Having a parallel API is an unwelcome complication, but the more I think about it the more I realize that the entire way that the UserData API is designed (to be fast and re-use userdata metatables) is fundamentally incompatible with non-'static UserData, so it DOES at some level make sense to have two APIs for the two "techniques".

The major downside that I can think of of the ScopedUserData approach is of course that you can't get your type back out of AnyUserData, so you won't be able to, for example, implement a RHS metamethod.

I'm interested in your opinion. If you had such an API would you find the inability to get the type out of AnyUserData too limiting? Would you want a borrow_userdata like API in addition to this?

@luteberget
Copy link
Author

A more general solution for non-'static types would certainly make more sense and solve the whole problem (consider my proposal a workaround for the specific case of &mut T where T : 'static).

ScopedUserDataMethods only had the add_xxx_method variants and not the add_xxx_function variants, would that be acceptable for your use?

So far I have only had use for add_method_mut.

two APIs for the two "techniques".

This makes some sense, because the use cases for Lua owning Rust data (GC'ed by Lua) and Lua borrowing Rust data (or owning for a specific lifetime, so no Lua GC in a sense) seem quite different.

would you find the inability to get the type out of AnyUserData too limiting?

I don't immediately see this as a problem.

Would you want a borrow_userdata like API in addition to this?

As long as it is possible to implement the example in my first post without much more code, that's good. I don't think one would often need to use a type as both UserData and ScopedUserData, so having seperate traits and impls for the two would probably work out fine.

@kyren
Copy link
Contributor

kyren commented Sep 4, 2018

Cool, that's all good to hear, it doesn't sound like there's too compelling of a use case for both APIs assuming we can get the more general one working acceptably well.

Which is good, because I've been working on this for the past 2 days or so, and I think I may have actually found a pretty good general solution.

I haven't tested it yet, but I have a new branch where I've managed to make a non-'static userdata API that actually doesn't require a new separate trait. One big downside is that it's a somewhat annoyingly large API change, but it's not too ad. Also, you still of course can't get your non-'static types out of an AnyUserData, but I think at least that all of the userdata methods will still be called in the same way as regular userdata, and it will otherwise have exactly the same behavior. The implementation is downright terrible, but the API at least is simpler than I was fearing it might be.

As soon as I add tests, I think I'm going to open a PR and we can discuss it more, but before that you can check it out here.

@kyren
Copy link
Contributor

kyren commented Sep 5, 2018

There's now PR #86 for you to take a look at. Making a PR so I can get some feedback.

@kyren kyren closed this as completed Oct 25, 2018
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