Inconsistent type checking with single vs. multiple constructors

[feltech]

I'm not really sure how to produce a simple test case, since my experience of this problem hinges on using two separate Lua binding libraries together. I'm also not sure whether this is a bug or feature request.

When only a single sol::constructors is defined in a new_usertype call, the code path goes directly to overload_match_arity_single<Fx, I, .... If multiple are defined, the code path starts at overload_match_arity_single<Fx, Fx1, Fxs.... In the first instance, there are no type checks - as long as the arity matches, the function matches. In the second case there are both arity and type checks.

The case I've come across is whilst using a game library (Urho3D), which uses tolua for C++/Lua bindings. I want to pass tolua-bound type to my sol-bound constructor. This works fine as long as I only have a single constructor, since there are no type checks. As soon as I add a second constructor it fails because of the type checking.

A simple fix I hacked in was to redefine as follows

template <std::size_t... M, typename Match, typename... Args>
inline int overload_match_arity(types<>, std::index_sequence<>, std::index_sequence<M...>, Match&&, lua_State* L, int, int, Args&&...) {
	return std::numeric_limits<lua_Integer>::max();
}

template <typename Fx, typename Fx1, typename... Fxs, std::size_t I, std::size_t I1, std::size_t... In, std::size_t... M, typename Match, typename... Args>
inline int overload_match_arity_single(types<Fx, Fx1, Fxs...>, std::index_sequence<I, I1, In...>, std::index_sequence<M...>, Match&& matchfx, lua_State* L, int fxarity, int start, Args&&... args) {
...
	if (!stack::stack_detail::check_types<true>{}.check(args_list(), L, start, no_panic, tracking)) {
		int matched = overload_match_arity(types<Fx1, Fxs...>(), std::index_sequence<I1, In...>(), std::index_sequence<M...>(), std::forward<Match>(matchfx), L, fxarity, start, std::forward<Args>(args)...);
		if (matched == std::numeric_limits<lua_Integer>::max() && traits::free_arity != fxarity) {
			return luaL_error(L, "sol: no matching function call takes this number of arguments and the specified types");
		}
	}
	return matchfx(types<Fx>(), index_value<I>(), return_types(), args_list(), L, fxarity, start, std::forward<Args>(args)...);
}

That is, not to immediately flag an error when no function fully matches (types + arity), but to return an "error code" of numeric_limits<lua_Integer>::max() . If this is returned, do one last ditch effort to check if the arity matches regardless of whether the types match. Otherwise flag a Lua error as before. This is then consistent with if a single constructor is called.

I know practically nothing of such low level Lua stuff, so I'm sure there's a better way. But it works.

[ThePhD]

And how would someone, at a high level, go disabling type checks for overloading checks?

[ThePhD]

Note that what I'm asking for is: what would you like to see on sol::constructors (or sol::factories, sol::initializers, sol::overload) that will stop the type checks and enable what you want. I am less interested in what the implementation looks like, more like what a high-level way to specify "arity overloading only" or "barely check at all".

Also, as a side note I can imagine incompatibilities rise specifically when going with the "objects bound by toLua" part, since the metatable implementation is wildly incompatible. This might be a good time for me to make an overridable checker function a user can take up to change how userdata specifically (that is, C++-bound types) are checked. That is, before we do any of sol2's checks, simply pass the stack index, handler, etc. to a user-defined "sol::stack::userdata_checker" that can be explicitly specialized for your needs (like, say, returning "yes, this type is totally fine" 100% of the time or using some toLua function to check userdata).

I would imagine that would be the low-level solution you could use, barring any high-level sol::by_arity<sol::overloaded<...>> junk we bikeshed here.

[ThePhD]

I should also probably mention it's not inconsistent: when dealing with multiple overloads, we have to do type-checking / arity-checking in order to find the right function. If you define SOL_CHECK_ARGUMENTS, we check every single function. All the time. Which means your code would probably fail right off the bat, even in the single-constructor case.

[feltech]

Thanks for the quick response! I'm quite new to sol, so am unaware of how such a change interacts with the broader picture.

A way to add a custom checker sounds perfect. It would mean figuring out how tolua verifies types and finding a way to hook into that. But that sounds like a worthwhile exercise for a more clean extensible solution.

However, an "arity checking only" solution sounds much easier to implement, has some performance benefits, and would suit my purposes for now.

I would argue that if SOL_CHECK_ARGUMENTS is not defined, then an arity check should always be sufficient for a match to be found (after all type checks have failed), for consistency as well as my selfish reasons.

[ThePhD]

The checker bit might actually be a bit harder to do: sure, we can override the default check but what about the casting operation? Right now we return void* vpointer_original = lua_touserdata(L, index); then do T** pointer_to_pointer_of_data = static_cast<T**>( vpointer_original );

If toLua stores its data differently, you'll need some custom handling no matter how I implement the checker. If your types are all bound with toLua, you can override the checker, getter AND pusher using customization points:

http://sol2.readthedocs.io/en/latest/tutorial/customization.html - https://github.com/ThePhD/sol2/blob/develop/examples/customization.cpp
http://sol2.readthedocs.io/en/latest/api/usertype_memory.html

[feltech]

Unfortunately for my case, I'm trying to use sol for my own types (I'm really impressed by the simplicity, features and performance), whereas Urho3D uses (the seemingly abandoned) tolua++ for it's types.

There seems to be some compatibility between the two, at least, since as I say, a single constructor (or multiple constructors with my hack) works. I.e. a correctly formed (tolua bound) object is passed to my (sol bound) constructor.

I'll take a look at the customization points, to see what might be possible.

[ThePhD]

I added some new customization points, that are much more flexible than the others. They're documented in various places, but the example probably explains it best. Pull from the latest, then check out this example:

https://github.com/ThePhD/sol2/blob/develop/examples/interop/kaguya.cpp

Replace the kaguya-specific logic with logic for toLua.

Other information:
http://sol2.readthedocs.io/en/latest/api/stack.html#userdata-checker
http://sol2.readthedocs.io/en/latest/safety.html#config

If you figure out how to do toLua, I would strongly encourage you to write an example and submit it back to me. I would love to keep a rolling, updated list of interop examples, with any simple libraries that are applicable.

[ThePhD]

I wanted to make sure this logic was solid, so I went and made 2 more examples. So now we test @satoren's kaguya, @vapourismo's Luwra, and @vinniefalco's LuaBridge, all which can be integrated into sol2:

https://github.com/ThePhD/sol2/tree/develop/examples/interop

It seems like the toLua logic for userdata stuff would be a great example aside from those 3 because it's data-compatible with sol2, meaning you can likely omit the userdata_getter altogether, just making sure the userdata_checker is right. I am definitely interested in seeing how this turns out, so let me know how it all shakes down.

[feltech]

This looks great! It may take me some time to figure out how to add such an interface for tolua.

It looks like I need to use tolua's static int lua_isusertype (lua_State* L, int lo, const char* type), which checks if type is equal to the object's metatable name in the registry(?). So I'll need to get hold of that type name from the object, somehow.

[feltech]

OK, I'm right on the edge of what I can understand of the Lua C API, but it appears as if the type checking in tolua is somewhat simple.

tolua generates binding code with strings embedded for checking against the string representation of the metatables. E.g. some code generated for Urho3D's bindings for the Viewport class's constructor:

static int tolua_GraphicsLuaAPI_Viewport_new01_local(lua_State* tolua_S)
{
 tolua_Error tolua_err;
 if (
 !tolua_isusertable(tolua_S,1,"Viewport",0,&tolua_err) ||
 !tolua_isusertype(tolua_S,2,"Scene",0,&tolua_err) ||
 !tolua_isusertype(tolua_S,3,"Camera",0,&tolua_err) ||
 !tolua_isusertype(tolua_S,4,"RenderPath",1,&tolua_err) ||
 !tolua_isnoobj(tolua_S,5,&tolua_err)
 )
 goto tolua_lerror;
 else
 {
  Scene* scene = ((Scene*)  tolua_tousertype(tolua_S,2,0));
  Camera* camera = ((Camera*)  tolua_tousertype(tolua_S,3,0));
  RenderPath* renderPath = ((RenderPath*)  tolua_tousertype(tolua_S,4,0));
 {
  Viewport* tolua_ret = (Viewport*)  Mtolua_new((Viewport)(GetContext(tolua_S),scene,camera,renderPath));
  tolua_pushusertype(tolua_S,(void*)tolua_ret,"Viewport");
 tolua_register_gc(tolua_S,lua_gettop(tolua_S));
 }
 }
 return 1;
tolua_lerror:
 return tolua_GraphicsLuaAPI_Viewport_new00_local(tolua_S);
}

The tolua_isusertype calls essentially just call the aforementioned lua_isusertype, which checks if the parameter is a metatable and that the metatable's name matches the given string.

So the best I think I can do in your new userdata_checker would be to get the demangled class name from T (I think I saw code in sol to do that) and check if the metatable name is the same as T.

Does that sound about right?

[ThePhD]

Look at sol::detail::demangle<T>(), and sol::detail::short_demangle<T>. You should also check how toLua generates a binding for a name that's nested in a namespace. E.g., what happens with:

namespace foo {
     struct Viewport {};
}
struct Viewport {};

[feltech]

Good point. I need to experiment. The first problem I've come across is that it appears userdata_getter must be specialised if SOL_ENABLE_INTEROP is defined. With no other changes, I get the compilation error:

vendor/include/sol.hpp:8240:16: error: cannot convert ‘sol::stack::extensible<UrFelt::UrSurface>*’ to ‘UrFelt::UrSurface*’ in return
     return ugr.second;

[ThePhD]

Good, catch, I can fix that now.

[feltech]

Yey! I got it working.

It would be nice to not have to also implement the getter, which I essentially just lifted from the sol code.

I also haven't handled the potential case of nested/namespaced classes. In Urho3D, none of the Lua API is namespaced and in the tolua documentation there is no mention of nested classes or namespaces. So I don't know if it's a limitation of tolua or just what the Urho3D devs decided.

I'll try to mock up a test case, but tolua is a bit more involved that the other examples, since it's a command line utility that generates cpp files from bespoke ".pkg" files, which you then embed in your program.

The checker/getter code I've come up with is

namespace sol {
namespace stack {
	template <typename T>
	struct userdata_checker<extensible<T>> {
		template <typename Handler>
		static bool check(
			lua_State* L, int relindex, type index_type, Handler&& handler, record& tracking
		) {
			// just marking unused parameters for no compiler warnings
			(void)index_type;
			(void)handler;
			int index = lua_absindex(L, relindex);
			std::string name = sol::detail::short_demangle<T>();
			tolua_Error tolua_err;
			return tolua_isusertype(L, index, name.c_str(), 0, &tolua_err);
		}
	};

	template <typename T>
	struct userdata_getter<extensible<T>> {
		static std::pair<bool, T*> get(
			lua_State* L, int relindex, void* unadjusted_pointer, record& tracking
		) {
			// you may not need to specialize this method every time:
			// some libraries are compatible with sol2's layout
			int index = lua_absindex(L, relindex);
			if (
				!userdata_checker<extensible<T>>::check(
					L, index, type::userdata, no_panic, tracking
				)
			) {
				return { false, nullptr };
			}
			void** pudata = static_cast<void**>(unadjusted_pointer);
			T* udata = static_cast<T*>(*pudata);
			return { true, udata };
		}
	};
}
} // namespace sol::stack

[ThePhD]

I think you can just use sol::detail::demangle in that case and hope it scales with namespace-nested types. And you don't exactly have to give me a whole example or even a compilable one: just something that has the right #include at the top so someone who DOES have it has a good chance of it working right out of the box.

[feltech]

I did try sol::detail::demangle initially and it gave me back e.g. Urho3D::Vector3, when I needed just Vector3 to get tolua to match it.

[ThePhD]

@feltech Try the latest and remove your userdata_getter, see if it works out for you.

Also, this is the example I ended up writing (but can't really test, mostly because I don't want to build a fresh toLua): https://github.com/ThePhD/sol2/blob/develop/examples/interop/tolua.cpp

[feltech]

Excellent! Works like a charm. Example also looks good to me.

Thanks a lot for all your work, and the speed with which you resolved this!

[ThePhD]

You're welcome.

Best of Luck.