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

Can't AnyUserData::borrow() userdata created by Scope::create_userdata() in mlua 0.10.0 #476

Open
Tracked by #1842
sxyazi opened this issue Oct 27, 2024 · 14 comments

Comments

@sxyazi
Copy link

sxyazi commented Oct 27, 2024

I have the following code that worked in 0.9.9:

fn main() -> mlua::Result<()> {
  struct Foo;
  impl UserData for Foo {}

  let lua = Lua::new();
  lua.scope(|scope| {
    lua.globals().set("foo", scope.create_userdata(Foo)?)?;
    lua.globals().set(
      "test",
      lua.create_function(|_, ud: AnyUserData| {
        ud.borrow::<Foo>()?;
        Ok("passed")
      })?,
    )?;
    lua.load("print(test(foo))").exec()
  })
}

which prints:

passed

But with mlua 0.10.0, I got this error:

Error: CallbackError { traceback: "stack traceback:\n\t[C]: in function 'test'\n\t[string \"src/main.rs:80:13\"]:1: in main chunk", cause: UserDataTypeMismatch }

Change borrow() to borrow_scoped() doesn't make any difference either:

- ud.borrow::<Foo>()?;
+ ud.borrow_scoped(|_: &Foo| {})?;
Error: CallbackError { traceback: "stack traceback:\n\t[C]: in function 'test'\n\t[string \"src/main.rs:80:13\"]:1: in main chunk", cause: UserDataTypeMismatch }
@alerque
Copy link
Contributor

alerque commented Oct 27, 2024

This is probably the same issue discussed in #475.

@sxyazi
Copy link
Author

sxyazi commented Oct 27, 2024

This issue stems from #475, since UserDataRef is no longer allowed in scope context, so I need to use AnyUserData::borrow_scoped(), but I found that borrow_scoped() (and even borrow()) doesn't work either.

I need this because I'm trying to build an iterator, and here's the full code (mlua 0.9.9): https://github.com/sxyazi/yazi/blob/main/yazi-fm/src/lives/yanked.rs#L32

@khvzak
Copy link
Member

khvzak commented Oct 27, 2024

Scope::create_userdata makes non-static userdata (no T: 'static requirement).
It cannot be borrowed in any mlua version.
Is Foo non-static? Otherwise can you use Lua::create_userdata instead? Or any ref-based method?

@sxyazi
Copy link
Author

sxyazi commented Oct 27, 2024

Is Foo non-static? Otherwise can you use Lua::create_userdata instead? Or any ref-based method?

Yes, Foo is a non-static owned type that is only valid within the scope

In my real scenario, Foo is used to hold a pointer to data that is only valid within the scope, so I need to ensure that Foo is also only valid within that scope, so I have to use Scope::create_userdata()

It cannot be borrowed in any mlua version.

This code can print "hello from foo" normally in 0.9.9, but not in 0.10.0:

fn main() -> mlua::Result<()> {
  struct Foo(String);
  impl UserData for Foo {}

  let lua = Lua::new();
  lua.scope(|scope| {
    let foo = Foo("hello from foo".to_string());

    lua.globals().set("foo", scope.create_userdata(foo)?)?;
    lua.globals().set(
      "test",
      lua.create_function(|_, ud: AnyUserData| {
        let foo = ud.borrow::<Foo>()?;
        Ok(foo.0.clone())
      })?,
    )?;
    lua.load("print(test(foo))").exec()
  })
}

Scope::create_userdata makes non-static userdata (no T: 'static requirement).

I noticed that Scope::create_userdata() in 0.9.9 requires 'static, while in 0.10.0 doesn't, maybe related?

Function signature for 0.9.9:

pub fn create_userdata<T>(&self, data: T) -> Result<AnyUserData<'lua>>
    where
        T: UserData + 'static,

Function signature for 0.10.0:

pub fn create_userdata<T>(&'scope self, data: T) -> Result<AnyUserData>
    where
        T: UserData + 'env,

@khvzak
Copy link
Member

khvzak commented Oct 27, 2024

If you used Scope::create_userata in v0.9, then your data was 'static?
Could you try Lua::create_userdata instead?

@sxyazi
Copy link
Author

sxyazi commented Oct 27, 2024

Sorry for the confusion, let me explain it again.

I'm fixing an issue where my iterator doesn't work in 0.10.0, but it works in 0.9.9, and here, Foo (or rather Iter) itself is static, but Iter holds a pointer to non-static data, lets say *const Vec<File> - I manually ensure that *const Vec<File> is always valid as long as within the scope, see #300 (comment).

So Iter also needs to be treated as non-static and disconnected when the scope ends, to prevent it from pointing to invalid non-static data, so Scope::create_userdata() is needed, but since Iter itself is an owned static type, so the ref-based method can't be used.

I'm finding it hard to describe my case as it also involves some unsafe code, but I've written a new example where you can just think of each usize element of the iterator as a *const File pointer that always valid within the scope:

fn main() -> mlua::Result<()> {
  struct Iter;

  impl Iterator for Iter {
    type Item = &'static usize;

    fn next(&mut self) -> Option<Self::Item> { Some(&123) }
  }

  impl UserData for Iter {}

  let lua = Lua::new();
  lua.scope(|scope| {
    lua.globals().set("iter", scope.create_userdata(Iter)?)?;
    lua.globals().set(
      "test",
      // mlua 0.9.9 - works
      lua.create_function(|_, mut iter: UserDataRefMut<Iter>| Ok(iter.next().copied()))?,
      // mlua 0.10.0 - doesn't work
      // lua.create_function(|_, iter: AnyUserData| {
      // 	iter.borrow_mut_scoped(|iter: &mut Iter| iter.next().copied())
      // })?,
    )?;
    lua.load("print(test(iter))").exec()
  })
}

Hope this helps with understanding!

@khvzak
Copy link
Member

khvzak commented Oct 27, 2024

Thanks for the information, I'll try to sort it out soon.

@khvzak
Copy link
Member

khvzak commented Oct 28, 2024

@sxyazi Would making Scope::seal_userdata public (and safe), solve your problem?
This method allow to shorten lifetime of AnyUserData object to the scope lifetime.

@sxyazi
Copy link
Author

sxyazi commented Oct 29, 2024

If I understood correctly, you mean using the combination of Lua::create_userdata() + Scope::seal_userdata() instead of the existing Scope::create_userdata() right?

I like this idea, as it allows mlua to avoid dealing with edge cases and leaves the responsibility to the user. I quickly read through the code for Lua::create_userdata() and Scope::create_userdata(), and I noticed that the latter is much more complex. Are their behaviors consistent, and is there anything I need to be aware of when using them?

Also, I noticed that the implementation of seal_userdata in Scope::create_userdata() includes a deregistration of the metatable, while Scope::seal_userdata() does not:

mlua/src/scope.rs

Lines 204 to 206 in 4f56575

// Deregister metatable
let mt_ptr = get_metatable_ptr(state, -1);
rawlua.deregister_userdata_metatable(mt_ptr);

Should I implement this deregistration myself when manually calling Scope::create_userdata(), or is it not important at all?

@khvzak
Copy link
Member

khvzak commented Oct 29, 2024

Scope::create_userdata in v0.10 is renamed (and rewritten) Scope::create_nonstatic_userdata

It allow to create non-static userdata objects (that does not implement Any trait) in exchange of having unique metatable attached to each object. This is the reason why it's complex (and was much more complex in v0.9) and does not support borrowing underlying data.

I removed the original Scope::create_userdata and Scope::create_any_userdata in favour of using references instead (Scope::create_userdata_ref and Scope::create_any_userdata_ref).

Your case is a bit unusual as you use unsafe and moved Scope object to a global static variable.

@khvzak
Copy link
Member

khvzak commented Oct 29, 2024

If you used previously Scope::create_userdata and Scope::create_any_userdata you can continue to use Lua::create_userdata and Lua::create_any_userdata instead. Both implementations had almost identical requirements T: 'static. The only difference is shorter lifetime in scoped version.

So, Lua::create_userdata() (or Lua::create_any_userdata) + Scope::seal_userdata() should be the way to replicate old behaviour. I'll make the changes shortly.

@sxyazi
Copy link
Author

sxyazi commented Oct 30, 2024

Thanks for the clarification! I think I understand what's happening with the Scope now.

It would be really helpful to add these behavioral changes to the release notes for anyone migrating from 0.9 to 0.10 as it's not just a hard compilation error - these changes happen at runtime, which can be much harder to catch :) Ah never mind, it's already included in d27d136, nice work! 👍

@khvzak
Copy link
Member

khvzak commented Oct 31, 2024

@sxyazi I added Scope::add_destructor that would allow to cleanup any resources at the scope end. Would this work for your case?

Eg:

let ud = lua.create_any_userdata(String::from("foo"))?;
lua.scope(|scope| {
    scope.add_destructor(|| {
        _ = ud.take::<String>();
    });
    Ok(())
})?;
// ud is destroyed now

Unfortunately, exposing Scope::seal_userdata is not safe as would allow in some edge cases cleanup already borrowed resources, eg:

let ud = lua.create_any_userdata(String::from("hello"))?;
ud.borrow_scoped::<StdString, _>(|s| {
    lua.scope(|scope| {
        scope.seal_userdata(&ud)?;
        Ok(())
    })
    .unwrap();
    assert_eq!(s, "foo"); // Use after free!
})?;

@sxyazi
Copy link
Author

sxyazi commented Oct 31, 2024

Is there a way to drop AnyUserData without knowing its exact type, or can I implement it through Lua::exec_raw() myself?

I want to store all my AnyUserData that need to be destructed in a Vec<AnyUserData> and destruct them in bulk in an add_destructor(), instead of adding a destructor for each one, since I have a lot of AnyUserData — every field access creates a new AnyUserData, and a single render could make ~1000 that need to be consumed.

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

3 participants