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

Allow non-'static UserData created from Scope #86

Merged
merged 5 commits into from
Sep 16, 2018
Merged

Allow non-'static UserData created from Scope #86

merged 5 commits into from
Sep 16, 2018

Conversation

kyren
Copy link
Contributor

@kyren kyren commented Sep 5, 2018

This PR allows types that implement the UserData trait but are not 'static to be created inside calls to Lua::scope.

Conceptually, it is pretty simple, it does the exact same thing as the previous call to Scope::create_userdata, except it doesn't require that the UserData type be 'static.

There are three API changes here:

  1. When using Scope::create_userdata, since the type it takes no longer requires 'static, you can no longer borrow the value out of the AnyUserData handle you get back. This applies whether or not the type you pass in is actually 'static (there's no specialization going on), so this means that purely from an API standpoint, there also needs to be a Scope::create_static_userdata to do what the previous create_userdata method did, which only lifts the Send requirement and still allows TypeId based borrowing via AnyUserData.

  2. Performance wise, Scope::create_static_userdata is going to be much much faster than Scope::create_userdata, so you might want to replace calls to Scope::create_userdata with Scope::create_static_userdata.

  3. UserDataMethods now must be a trait, because of complicated lifetime issues around having a callback type with a user specified 'lua lifetime.

The implementation is.. pretty gnarly, and also probably pretty slow. I originally tried a simpler implementation that can actually be implemented on top of the old rlua completely in safe code, but I ran into a single enormously irritating problem, which is that PUC-Rio Lua has different behavior with binary metamethods like __add depending on whether or not the key '__add' exists in the metatable of one or both arguments. Since the mere existence of a metamethod changes behavior, I could not implement a safe "dispatch" UserData type which simply implemented all possible metamethods and forwarded to a dispatcher. Instead, we must create a fresh metatable only with the metamethods requested from the implementation of UserData, to perfectly match the behavior of Scope::create_static_userdata / Lua::create_userdata.

I'm open for ideas on how to make it faster, but since you are more or less forced to create a unique metatable on each creation, so I don't really know how fast you can make it.

The change opens up a possible footgun by naming the new Scope::create_userdata the same as the old one, and renaming the old one to Scope::create_static_userdata, but I can't think of a better name for them. Is Scope::create_nonstatic_userdata better?

Uses the same UserData trait, and should at least in theory support everything
that 'static UserData does, except that any functions added that rely on
AnyUserData are pretty much useless.

Probably pretty slow and I'm not sure how to make it dramatically faster, which
is a shame because generally when you need non'-static userdata you might be
creating it kind of a lot (if it was long-lived, it would probably be 'static).

Haven't added tests yet, will do that next.
Tried to explain the rationale for safety around callbacks in Lua and Scope a
bit better, because every time I don't look at this for a while I forget my
reasoning.  I'm not always so great at using the right terminology, so to
whoever reads this, if I got this wrong please tell me.
@kyren
Copy link
Contributor Author

kyren commented Sep 5, 2018

Btw, this PR REALLY makes me want to go through rlua and change the callback type to use for<'lua>.

The problem is, that without ATCs, you would have to massively, massively change the API of rlua and it would involve writing function wrappers as macros instead of functions, because you end up not being able to write out the type of the function required to produce a for<'lua> type callback.

This PR though and the soundness bug before this involving scoped callbacks make it really obvious just how wrong the callback type really is though.

I'm working on a radical redesign of a scripting API that doesn't have these safety footguns, but that's going to be tested in luster first.

@luteberget
Copy link

Cool, I will try this out on my application. I'm not familiar enough with the Lua FFI to do a thorough review though.

Just a quick thought: what if TypeId worked for these UserData types, how much better would it be? Could it be worth it to allow implementers to specify some type identifier manually? The trait could give a type that is 'static, like this:

trait UserData {
  type IdentifiableType;
  (...)
}

impl<'a> UserData for MyData<'a>  {
  type IdentifiableType = for<'b> Fn(MyData<'b>);
  (...)
}

... or even adding a fn type_id() -> usize to the UserData trait. As far as I understand, lifetime parameters cannot cause different code to be generated, so ignoring them for TypeId should work. It's not pretty, though.

@kyren
Copy link
Contributor Author

kyren commented Sep 16, 2018

Just a quick thought: what if TypeId worked for these UserData types, how much better would it be? Could it be worth it to allow implementers to specify some type identifier manually? The trait could give a type that is 'static, like this:

Unfortunately, as I understand it, it just can't be safe. I think if you use unstable rust right now you can even get a TypeId for non-'static types that's afaik equal to any other TypeId no matter the lifetimes substituted, but it's really really really dangerous to use.

You're right that differing lifetimes can't generate different code, but that's actually kind of the problem. Because you can't tell at runtime which lifetime was used to create a given object, you end up always being able to transmute any type MyType<'a> to MyType<'static> by just storing it inside something equivalent to Box. Since this is trivially turned into basically any sort of lifetime transmute, it's obviously wildly unsafe. This is why std::any::Any requires 'static.

@kyren
Copy link
Contributor Author

kyren commented Sep 16, 2018

I'm going to clean up a couple of these outstanding PRs and stuff starting with this one. I've sat and thought about this for 2 ish weeks, and I can't come up with any better way of doing this other than maybe some very slight perf improvements to the same basic technique. Any improvement would either have the same API or be a radical API change for not just this, so I'm going to go ahead and merge this.

Edit: well, I still haven't thought about the create_userdata vs create_static_userdata naming problem... I will actually probably rename create_userdata to create_nonstatic_userdata for the time being just so there are not extra version update refactoring footguns.

@kyren kyren merged commit 58b991b into master Sep 16, 2018
@kyren kyren deleted the unstatic branch October 1, 2018 09:16
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

Successfully merging this pull request may close these issues.

2 participants