Why does create_function specify 'static?

[dotellie]

When playing around trying to make a prototype for a system I'm developing, I found that I bumped into a wall when using Lua.create_function. It has a lifetime bound specified as 'static which caused trouble, but when I tried changing it to 'lua it worked and it doesn't seem to have caused trouble elsewhere. So my question is why the boundary is 'static instead of 'lua. I'm pretty new to Rust so I may have missed something very obvious here, but I thought I'd ask anyway.

Thanks!

[kyren]

We’ve been through this before:

#33
#20

Anything that is passed to lua morally gets put into an Any, and it’s very very difficult to safely deal with non-'static TypeId identified values. More concretely, if you capture a reference to Lua inside a callback, and place that callback inside Lua, you can then move the Lua and invalidate the reference. Worse yet, inside callbacks you get a different ephemeral Lua instance which has state pointers that are later invalidated, and actually needs to live LESS long than the callback.

If there was some way of saying that a type should STRICTLY outlive a lifetime, their MIGHT be a way of making it safe, but I would still be cautious.

Also, if the lifetimes allowed you to place things containing LuaRefs inside callbacks or userdata, that’s actually a problem in itself:

#41

Sorry for the bad news, but it’s just very complicated to make sound. I recommend Rc, use of Lua tables to store Lua handles, and the rental crate.

[dotellie]

Yeah, now I feel pretty bad. Those issues completely went over my radar. Sorry for bringing it up again, but thank you for the explanation anyway! Maybe this should be added to some kind of FAQ or something?

[jonas-schievink]

We could just add a note to the create_function docs

[jonas-schievink]

FWIW, we could add a lifetime parameter to Lua that specifies how long the longest-lived externally borrowed data lives to make this sound. Given that this has come up a few times already, maybe we should try that? It wouldn't magically solve anything, but maybe it solves a few use cases.

(and this time, I'm very certain that it'll be safe, unlike my last attempt)

[kyren]

Yeah, now I feel pretty bad. Those issues completely went over my radar. Sorry for bringing it up again, but thank you for the explanation anyway! Maybe this should be added to some kind of FAQ or something?

Don't feel bad! It's actually a really good suggestion, which is why we already tried it :D These issues are not exactly obvious, either.

[kyren]

FWIW, we could add a lifetime parameter to Lua that specifies how long the longest-lived externally borrowed data lives to make this sound. Given that this has come up a few times already, maybe we should try that? It wouldn't magically solve anything, but maybe it solves a few use cases.

(and this time, I'm very certain that it'll be safe, unlike my last attempt)

That's really interesting, can you flesh out this plan a bit?

[jonas-schievink]

@kyren Sure, I'll try to describe it.

Basically, we make Lua take an inferred lifetime parameter (Lua needs to be invariant over it) that denotes the shortest-lived external variable that is borrowed (not the longest-lived as I wrote earlier), similar to what the dynamic-arena crate offers (I'm sure there's other more well-known examples, though). Both are, conceptually, heterogeneous collections.

Take this gist:

use std::cell::RefCell;

#[derive(Default)]
struct Container<'a> {
    // Need to be invariant over `'a`. rlua can use a PhantomData.
    // and we should *really*, *really* start writing compile-fail tests.
    elems: RefCell<Vec<Box<Tr + 'a>>>,
}

// This trait isn't necessary for this example.
trait Tr {}
impl<T> Tr for T {}

impl<'a> Container<'a> {
    pub fn new() -> Self {
        Self::default()
    }
    
    // Mimic Lua method taking an immutable Lua
    pub fn add<T: Tr + 'a>(&self, item: T) {
        self.elems.borrow_mut().push(Box::new(item))
    }
}

fn main() {
    {
        // 'static data can be added as usual
        let cont = Container::new();
        cont.add(123);    // 'static
        // We can hopefully move `Container`s soundly now
        let moved = cont;
    }
    
    {
        // Adding non-'static data works, too.
        // `cont` must not be able to leave this scope, and indeed it isn't.
        let lives_longer_than_cont = 123;
        let cont = Container::new();
        cont.add(&lives_longer_than_cont);
        // We can still move it
        let moved = cont;
    }
    
    {
        // Swapping the order makes it fail, as the bottom value is destroyed
        // first:
        /*
        let cont = Container::new();
        let too_short = 123;
        cont.add(&too_short);
        */
    }
    
    {
        // Self-borrows are prevented correctly. Note that we're still able to
        // move `cont` freely. As the current `Lua` can also be moved freely,
        // and self-borrows are prevented, this is still safe.
        /*
        let cont = Container::new();
        cont.add(&cont);
        */
    }
}

We might have to watch out for destructors running, but I'm not sure how and if that can affect rlua.

Inferring the lifetime parameter of Container is left to the compiler. I believe rustc will never infer a lifetime that is smaller than the scope of cont (due to well-formedness constraints on the type).

From the outside, Lua becomes Lua<'a> and most methods currently requiring 'static data now just require 'a. Of course, the added lifetime parameter is a relatively big downside, and you won't be able to do things the compiler can't prove correct, but maybe this is good enough for some/most use cases?

And now, let's lean back and watch as this turns out to be horribly broken :)

[kyren]

Okay, that actually makes a lot of sense.

The main thing I'm worried about is interactions between this and the ephemeral Lua that callbacks get handed.. I guess the lifetime there would be just like the 'lua lifetime right, a lifetime that is just the scope of the callback function?

[jonas-schievink]

Yes, probably. Letting the compiler infer the lifetime should be the best option. I haven't ironed out all the details yet, and I had this idea quite a while back so I don't recall all the details very well.

[dotellie]

@jonas-schievink Do you have any updates on this?

[jonas-schievink]

@magnonellie I'm currently busy with university, so I haven't had the time to work on this. I also think we should land something like #46 to ensure the commented code does indeed not compile when this is implemented. It is also not clear if my proposed solution solves enough practical problems to be worthwhile (someone could implement it and test it out, of course).

You can likely work around this limitation for now by using Rc and related tricks.

[kyren]

So, PR #68 actually addresses this! You can now, with limitations, pass functions to lua that are not 'static.

Like so:

    let mut i = 0;

    let lua = Lua::new();
    lua.scope(|scope| {
        scope
            .create_function(|_, ()| {
                i = 42;
                Ok(())
            })
            .unwrap()
            .call::<_, ()>(())
            .unwrap();
    });
    assert_eq!(i, 42);

When the scope ends, the Lua function is 100% guaranteed (afaict!) to be "invalidated". This means that calling the function will cause an immediate Lua error with a message like "error, call of invalidated function".

Edit: Okay, PR #68 by itself was not safe because I screwed up the scope lifetimes pretty bad.

It was touch and go for a while, but after some education by @jonas-schievink about how variant / invariant lifetimes actually work, and a good night's sleep, the scope functionality should actually be safe now. This functionality should make it into the next rlua release.

This is separate however, from @jonas-schievink's suggestion of having an invariant lifetime on Lua itself, and allowing all Lua userdata types to reference things that outlive it. It might still be useful, but hopefully this change solves at least a large number of use cases.

As an aside, this change allowed Chucklefish internally to remove all of our usage of the rental crate! We used it to capture locks inside Lua, and it was very VERY complicated to do so, but now we simply use Lua::scope to reference the locks in the outer scope, and it's vastly simpler.

[kyren]

I think that the addition of Lua::scope is somewhere between a 90% and a 99% solution to this problem, but I'm going to leave this bug open for the future possibility of using an invariant lifetime on Lua to allow generally for functions / userdata that outlive it. If that strategy doesn't work for userdata due to the Any / TypeId usage of it not being safe, I'm pretty sure it's not worth the extra headache, but if that can be made safe it might be worth it.