sol2 icon indicating copy to clipboard operation
sol2 copied to clipboard

sol::property does not support sol::yielding

Open eterchun opened this issue 1 year ago • 3 comments

Lua: 5,4 Sol: trunk

I am trying to implement an interface that requires at least one of the sol::property functions to yield. I've run into the issue that sol::property does not seem to accept sol::yielding wrapped functions. I've created the test code below showing essentially what I am trying to do and it fails to compile on either gcc or clang. Based on the errors (reproduced below), I cannot see a specialization for sol::yielding_t in the relevant call path for sol::usertype_proxy<Table, Key>&& sol::usertype_proxy<Table, Key>::operator= when the type is a property_wrapper.

Is there another way I should be going about this that I am missing? Otherwise, my initial thoughts on a potential fix are below.

TIA.

#define SOL_ALL_SAFETIES_ON 1
#include <limits>
#include <sol/sol.hpp>

#define TEST_YIELDING 1

struct test_struct {
    int i;
};


int main()
{
	sol::state lua;
	lua.open_libraries(sol::lib::base, sol::lib::package, sol::lib::coroutine, sol::lib::string, sol::lib::table);

    int return_value;

    auto test_struct_t = lua.new_usertype<test_struct>("test_struct", sol::no_constructor);
    test_struct_t["i"] = sol::property(
#ifdef TEST_YIELDING
        sol::yielding([&](test_struct* ptr, sol::this_state ts) {
            sol::state_view L = ts;
            return_value = (*ptr).i;
        }),
#else
        [&](test_struct* ptr, sol::this_state ts) {
            sol::state_view L = ts;
            return (*ptr).i;
        },
#endif
        [](test_struct *ptr, int val) {
            (*ptr).i = val;
        }
    );
    
	sol::thread thread1 = sol::thread::create(lua);

    test_struct obj{1};

    lua["object"] = &obj;
    sol::coroutine co1 = thread1.state().load(R"(
        print(tostring(object.i))
    )");

    auto res = co1();
#ifdef TEST_YIELDING
    auto res2 = co1(return_value);
#endif
    return 1;
}

godbolt

My initial guess is that the specialization of lua_call_wrapper for property_wrapper needs to add a check for meta::is_specialization_of<U, yielding_t to the is_specialized bool. It looks like that call path already constructs and utilizes the lua_call_wrapper for the value, similar to what is done elsewhere to use the yielding_t specialization of lua_call_wrapper, e.g., in a different specialization below.

Error:

In file included from /opt/compiler-explorer/libs/sol2/trunk/include/sol/function_types_core.hpp:28,
                 from /opt/compiler-explorer/libs/sol2/trunk/include/sol/function_types.hpp:27,
                 from /opt/compiler-explorer/libs/sol2/trunk/include/sol/unsafe_function.hpp:31,
                 from /opt/compiler-explorer/libs/sol2/trunk/include/sol/function.hpp:28,
                 from /opt/compiler-explorer/libs/sol2/trunk/include/sol/sol.hpp:54,
                 from <source>:3:
/opt/compiler-explorer/libs/sol2/trunk/include/sol/wrapper.hpp: In instantiation of 'static decltype(auto) sol::wrapper<F, <template-parameter-1-2> >::call(F&, Args&& ...) [with Args = {}; F = sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >; <template-parameter-1-2> = void]':
/opt/compiler-explorer/libs/sol2/trunk/include/sol/wrapper.hpp:51:16:   required from 'decltype(auto) sol::wrapper<F, <template-parameter-1-2> >::caller::operator()(F&, Args&& ...) const [with Args = {}; F = sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >; <template-parameter-1-2> = void]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/stack.hpp:147:32:   required from 'decltype(auto) sol::stack::stack_detail::eval(sol::types<>, std::index_sequence<>, lua_State*, int, Handler&&, sol::stack::record&, Fx&&, Args&& ...) [with bool checked = true; Handler = sol::argument_handler<sol::types<void> >&; Fx = sol::wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, void>::caller; Args = {sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >&}; std::index_sequence<> = std::integer_sequence<long unsigned int>; lua_State = lua_State]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/stack.hpp:196:21:   required from 'decltype(auto) sol::stack::stack_detail::call(sol::types<R>, sol::types<Args ...>, std::index_sequence<__indices ...>, lua_State*, int, Fx&&, FxArgs&& ...) [with bool checkargs = true; long unsigned int ...I = {}; R = void; Args = {}; Fx = sol::wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, void>::caller; FxArgs = {sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >&}; std::index_sequence<__indices ...> = std::integer_sequence<long unsigned int>; lua_State = lua_State]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/stack.hpp:226:35:   required from 'decltype(auto) sol::stack::call(sol::types<R>, sol::types<Args ...>, lua_State*, int, Fx&&, FxArgs&& ...) [with bool check_args = true; R = void; Args = {}; Fx = sol::wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, void>::caller; FxArgs = {sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >&}; lua_State = lua_State]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/stack.hpp:268:21:   required from 'int sol::stack::call_into_lua(sol::types<R, Args ...>, sol::types<Args ...>, lua_State*, int, Fx&&, FxArgs&& ...) [with bool check_args = true; bool clean_stack = true; Ret0 = void; Ret = {}; Args = {}; Fx = sol::wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, void>::caller; FxArgs = {sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >&}; lua_State = lua_State]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/call.hpp:801:57:   [ skipping 4 instantiation contexts, use -ftemplate-backtrace-limit=0 to disable ]
/opt/compiler-explorer/libs/sol2/trunk/include/sol/usertype_storage.hpp:804:52:   required from 'void sol::u_detail::usertype_storage<T>::set(lua_State*, Key&&, Value&&) [with Key = const char*; Value = sol::property_wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, main()::<lambda(test_struct*, int)> >; T = test_struct; lua_State = lua_State]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/usertype.hpp:90:12:   required from 'sol::basic_usertype<T, base_type>& sol::basic_usertype<T, base_type>::set(Key&&, Value&&) [with Key = const char*; Value = sol::property_wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, main()::<lambda(test_struct*, int)> >; T = test_struct; base_type = sol::basic_reference<false>]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/usertype_proxy.hpp:64:12:   required from 'void sol::usertype_proxy<Table, Key>::tuple_set(std::index_sequence<__var_indices ...>, T&&) && [with long unsigned int ...I = {0}; T = sol::property_wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, main()::<lambda(test_struct*, int)> >; Table = sol::basic_usertype<test_struct, sol::basic_reference<false> >&; Key = const char*; std::index_sequence<__var_indices ...> = std::integer_sequence<long unsigned int, 0>]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/usertype_proxy.hpp:86:30:   required from 'sol::usertype_proxy<Table, Key>&& sol::usertype_proxy<Table, Key>::set(T&&) && [with T = sol::property_wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, main()::<lambda(test_struct*, int)> >; Table = sol::basic_usertype<test_struct, sol::basic_reference<false> >&; Key = const char*]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/usertype_proxy.hpp:97:31:   required from 'sol::usertype_proxy<Table, Key>&& sol::usertype_proxy<Table, Key>::operator=(T&&) && [with T = sol::property_wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, main()::<lambda(test_struct*, int)> >; Table = sol::basic_usertype<test_struct, sol::basic_reference<false> >&; Key = const char*]'
<source>:35:5:   required from here
/opt/compiler-explorer/libs/sol2/trunk/include/sol/wrapper.hpp:45:33: error: no match for call to '(sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >) ()'
   45 |                         return f(std::forward<Args>(args)...);
      |                                ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ASM generation compiler returned: 1
In file included from /opt/compiler-explorer/libs/sol2/trunk/include/sol/function_types_core.hpp:28,
                 from /opt/compiler-explorer/libs/sol2/trunk/include/sol/function_types.hpp:27,
                 from /opt/compiler-explorer/libs/sol2/trunk/include/sol/unsafe_function.hpp:31,
                 from /opt/compiler-explorer/libs/sol2/trunk/include/sol/function.hpp:28,
                 from /opt/compiler-explorer/libs/sol2/trunk/include/sol/sol.hpp:54,
                 from <source>:3:
/opt/compiler-explorer/libs/sol2/trunk/include/sol/wrapper.hpp: In instantiation of 'static decltype(auto) sol::wrapper<F, <template-parameter-1-2> >::call(F&, Args&& ...) [with Args = {}; F = sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >; <template-parameter-1-2> = void]':
/opt/compiler-explorer/libs/sol2/trunk/include/sol/wrapper.hpp:51:16:   required from 'decltype(auto) sol::wrapper<F, <template-parameter-1-2> >::caller::operator()(F&, Args&& ...) const [with Args = {}; F = sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >; <template-parameter-1-2> = void]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/stack.hpp:147:32:   required from 'decltype(auto) sol::stack::stack_detail::eval(sol::types<>, std::index_sequence<>, lua_State*, int, Handler&&, sol::stack::record&, Fx&&, Args&& ...) [with bool checked = true; Handler = sol::argument_handler<sol::types<void> >&; Fx = sol::wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, void>::caller; Args = {sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >&}; std::index_sequence<> = std::integer_sequence<long unsigned int>; lua_State = lua_State]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/stack.hpp:196:21:   required from 'decltype(auto) sol::stack::stack_detail::call(sol::types<R>, sol::types<Args ...>, std::index_sequence<__indices ...>, lua_State*, int, Fx&&, FxArgs&& ...) [with bool checkargs = true; long unsigned int ...I = {}; R = void; Args = {}; Fx = sol::wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, void>::caller; FxArgs = {sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >&}; std::index_sequence<__indices ...> = std::integer_sequence<long unsigned int>; lua_State = lua_State]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/stack.hpp:226:35:   required from 'decltype(auto) sol::stack::call(sol::types<R>, sol::types<Args ...>, lua_State*, int, Fx&&, FxArgs&& ...) [with bool check_args = true; R = void; Args = {}; Fx = sol::wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, void>::caller; FxArgs = {sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >&}; lua_State = lua_State]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/stack.hpp:268:21:   required from 'int sol::stack::call_into_lua(sol::types<R, Args ...>, sol::types<Args ...>, lua_State*, int, Fx&&, FxArgs&& ...) [with bool check_args = true; bool clean_stack = true; Ret0 = void; Ret = {}; Args = {}; Fx = sol::wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, void>::caller; FxArgs = {sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >&}; lua_State = lua_State]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/call.hpp:801:57:   [ skipping 4 instantiation contexts, use -ftemplate-backtrace-limit=0 to disable ]
/opt/compiler-explorer/libs/sol2/trunk/include/sol/usertype_storage.hpp:804:52:   required from 'void sol::u_detail::usertype_storage<T>::set(lua_State*, Key&&, Value&&) [with Key = const char*; Value = sol::property_wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, main()::<lambda(test_struct*, int)> >; T = test_struct; lua_State = lua_State]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/usertype.hpp:90:12:   required from 'sol::basic_usertype<T, base_type>& sol::basic_usertype<T, base_type>::set(Key&&, Value&&) [with Key = const char*; Value = sol::property_wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, main()::<lambda(test_struct*, int)> >; T = test_struct; base_type = sol::basic_reference<false>]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/usertype_proxy.hpp:64:12:   required from 'void sol::usertype_proxy<Table, Key>::tuple_set(std::index_sequence<__var_indices ...>, T&&) && [with long unsigned int ...I = {0}; T = sol::property_wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, main()::<lambda(test_struct*, int)> >; Table = sol::basic_usertype<test_struct, sol::basic_reference<false> >&; Key = const char*; std::index_sequence<__var_indices ...> = std::integer_sequence<long unsigned int, 0>]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/usertype_proxy.hpp:86:30:   required from 'sol::usertype_proxy<Table, Key>&& sol::usertype_proxy<Table, Key>::set(T&&) && [with T = sol::property_wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, main()::<lambda(test_struct*, int)> >; Table = sol::basic_usertype<test_struct, sol::basic_reference<false> >&; Key = const char*]'
/opt/compiler-explorer/libs/sol2/trunk/include/sol/usertype_proxy.hpp:97:31:   required from 'sol::usertype_proxy<Table, Key>&& sol::usertype_proxy<Table, Key>::operator=(T&&) && [with T = sol::property_wrapper<sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >, main()::<lambda(test_struct*, int)> >; Table = sol::basic_usertype<test_struct, sol::basic_reference<false> >&; Key = const char*]'
<source>:35:5:   required from here
/opt/compiler-explorer/libs/sol2/trunk/include/sol/wrapper.hpp:45:33: error: no match for call to '(sol::yielding_t<main()::<lambda(test_struct*, sol::this_state)> >) ()'
   45 |                         return f(std::forward<Args>(args)...);
      |                                ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Execution build compiler returned: 1

eterchun avatar May 06 '23 13:05 eterchun

Following up, I tried the potential change and everything compiled, but it seemed to cause issues with execution. For now I am working around this by using a table with index and new_index set to the functions I had been passing into sol::property.

eterchun avatar May 07 '23 14:05 eterchun

It seems that meta_functions on usertypes may not respect sol::yielding either. I created a quick test on godbolt showing the behavior.

The functions get called, but processing continues in sol rather than yielding at the end of each call.

eterchun avatar May 08 '23 14:05 eterchun

I have something working for my current uses and wanted to leave some breadcrumbs for anyone needing to do the same thing (assuming yielding support does not get expanded).

I ended up creating raw functions to override index and new_index that directly call lua_yield. One of the examples shows most of the pieces of this. To get yielding behavior you can return lua_yield(L, 0) (or some other number of returns potentially) rather than the result of sol::stack::push. I am not yet sure whether this will ultimately cause me issues elsewhere.

As for yielding support in sol, I ultimately gave up modifying it because the code path I initially identified only controls part of the behavior and, while the code would compile, during runtime the wrong property function would get called. I expect this is because the pusher path also does not support nested yielding_t. However, I did not look into that aspect of the codebase.

For the call path, more than my initial potential change is required. The main issue is that the decision on whether to yield or return directly is made in the top level call function and is currently based solely on whether the function passed in is wrapped with sol::yielding_t. One potential fix that covers sol::property_wrapper but may not work for other wrapper types is to create a test of the called function type that detects whether the call should yield. E.g.,


namespace detail {
	template <typename T, bool is_index, typename = void>
	struct is_yielding : meta::is_specialization_of<T, yielding_t> { };
	template <typename R, typename W>
	struct is_yielding<property_wrapper<R, W>, true> : is_yielding<R, true> { };
	template <typename R, typename W>
	struct is_yielding<property_wrapper<R, W>, false> : is_yielding<W, true> { };
} // namespace detail

template <typename T, bool is_index>
using is_yielding = detail::is_yielding<std::remove_cv_t<T>, is_index>;

template <typename T, bool is_index>
inline constexpr bool is_yielding_v = is_yielding<std::remove_cv_t<T>, is_index>::value;

If that approach doesn't work with some wrapper types, you could instead change the return type of call_wrapped to a tuple containing a bool indicating whether to yield as well as an int, or just return lua_yield(L, nr) in the yielding_t specialization and directly return the result of call_wrapped rather than only returning the number of results from call_wrapped and then deciding what to do about yielding after.

I do not know what to do about the push path, but it may have to do with the unqualified_pusher specializations calling stack::push rather than the unqualified_pusher implementation of the wrapper type.

eterchun avatar May 10 '23 12:05 eterchun