ThePhD / sol2

Sol3 (sol2 v3.0) - a C++ <-> Lua API wrapper with advanced features and top notch performance - is here, and it's great! Documentation:
http://sol2.rtfd.io/
MIT License
4.2k stars 516 forks source link

Getting an exception when calling lua functions from c++ makes the error completely useless. #1386

Open ghost opened 2 years ago

ghost commented 2 years ago

These are extremely difficult to track down in a large codebase with ~100 custom lua functions

This error isn't helpful, it would be nice if there was a way to print out which function threw an exception or on which line (although I doubt the latter would be possible since its called from c++).

VsDebugConsole_ifpUKDZ9mE

Example code ```cpp #include #include #include "external/sol/sol.hpp" // forward decl class enemy; inline std::vector> g_examples{}; using timer_func = std::function; class timer { public: int64_t m_interval{}; std::chrono::steady_clock::time_point m_lasttick{}; timer_func m_callback{}; }; class enemy { std::map m_timers; public: [[nodiscard]] std::string test(const std::string& example) const { const std::string func = "just a test function "; return func + example; } void add_timer(const std::string& identifier, const int64_t interval, const timer_func& callback) { m_timers[identifier] = { interval, std::chrono::steady_clock::now(), callback }; } void timer_tick() { for (auto& i : m_timers) { const auto duration = std::chrono::duration_cast(std::chrono::steady_clock::now() - i.second.m_lasttick); if (duration.count() < i.second.m_interval) continue; i.second.m_callback(this); i.second.m_lasttick = std::chrono::steady_clock::now(); } } }; int main() { sol::state lua; lua.open_libraries(); lua.new_usertype( "enemy", sol::call_constructor, sol::factories([] { g_examples.emplace_back(std::make_unique()); return std::cref(g_examples.back()); }), "add_timer", &enemy::add_timer, "test", &enemy::test ); lua.script(R"( local ex = enemy() ex:add_timer("test", 100, function(e) local a_variable = e:test(123) -- not a string, crash happens here on timer tick and output is not useful print(a_variable) -- not gonna reach here end) )"); while (!g_examples.empty()) { for (const auto& ex : g_examples) ex->timer_tick(); std::this_thread::sleep_for(std::chrono::milliseconds(1)); } } ```
MJCollett commented 2 years ago

Yes, Sol's error reporting seems to have got worse somewhere along the line.

I've just spent some time trying to hunt down the source of an error message similar to the ones you quote:
An error occurred and panic has been invoked: stack index 1, expected string, received no value: bad get from protected_function_result (is not an error) Eventually, beginning to suspect that the error might have been in the bowels of Sol itself, I switched back to an earlier version of Sol (from 3.3.0 back to 3.0.3, which I had been using until recently), and immediately received an informative message that pinpointed exactly where (file and line) the problem was in the script: An error occurred and panic has been invoked: my/file/name.lua:203: attempt to index a nil value Not only was the 3.3.0 error message unhelpful about location, it was actually wrong in detail, saying that a string was expected, when in fact no strings were involved at all.

jasongibson commented 2 years ago

The change in error messages appears to have originated in ce32549bc6f2690b0993ab0c3e4c66aa37c8c86d. The use of lua_error() vs luaL_error() is the key in type_panic_string() and type_panic_c_str(). Issue 906 also references a similar change in error output.

jasongibson commented 2 years ago

Here is a diff (off of v3.3.0) that fixes it for my use-cases. I suspect that anywhere that push_type_panic_string() and lua_error() are used together might need changing to luaL_error(), though.

> //#include <sol/config.hpp>
40,42c40,42
< #define SOL_VERSION_MINOR 2
< #define SOL_VERSION_PATCH 3
< #define SOL_VERSION_STRING "3.2.3"
---
> #define SOL_VERSION_MINOR 3
> #define SOL_VERSION_PATCH 0
> #define SOL_VERSION_STRING "3.3.0"
9558c9558,9561
<                       lua_pushfstring(L, err, index, type_name, actual_name.c_str(), message.data(), aux_message.data());
---
>                       if (message.size())
>                            lua_pushfstring(L, err, index, type_name, actual_name.c_str(), message.data(), aux_message.data());
>                       else
>                            lua_pushfstring(L, err, index, type_name, actual_name.c_str(), aux_message.data());
9565c9568,9569
<               return lua_error(L);
---
>               size_t len = 0;
>               return luaL_error(L, lua_tolstring(L, -1, &len));
9570c9574,9575
<               return lua_error(L);
---
>               size_t len = 0;
>               return luaL_error(L, lua_tolstring(L, -1, &len));
9611c9616,9618
<                       return lua_error(L);
---
>
>               size_t len = 0;
>               return luaL_error(L, lua_tolstring(L, -1, &len));
shockdot commented 1 year ago

Has there been any official progress with providing better error reporting? I keep running into panic errors as the original poster did, and the report is absolutely zero help.

ghost commented 1 year ago

Has there been any official progress with providing better error reporting? I keep running into panic errors as the original poster did, and the report is absolutely zero help.

I never fixed these issues, although I added lua annotations into my project so these errors became less and less frequent (you need a lua language server though, which is a bit disappointing)

jasongibson commented 1 year ago

@shockdot Did you try the patch? Does it improve the error messages in your tests?

Perhaps if you wanted to repackage the raw diffs as a pull request, it would make it easier to get into the codebase.

shockdot commented 1 year ago

@shockdot Did you try the patch? Does it improve the error messages in your tests?

Perhaps if you wanted to repackage the raw diffs as a pull request, it would make it easier to get into the codebase.

@jasongibson

I did see it, but I wasn't entirely certain what file and section it was pertaining to. I think it's "error_handler.hpp"? When looking at this file I do see areas that match your diff (before your changes), but the issue I ran into is that I find 5 "return lua_error(L);" and you only listed 3 that were changed... so I'm not sure which I should be changing or if I should change all 5 in the same way you did.

Furthermore, I noticed that your version string was changed to 3.3.0... but mine and the latest version in the releases of this git is 3.2.3 (is 3.3.0 uploaded somewhere else)?

Unfortunately, these errors appear to be only happening on a live environment for me... or rather I'm not sure what is being done on the live environment that is causing them so I'm not sure where to start looking in my development environment (I have hundreds of individual lua scripts, so it's hard to track these down without better error reporting).

jasongibson commented 1 year ago

@shockdot Ah, knowing what file to apply the patch to might help :) The diff was against the single-header version of the 3.3.0 release - I don't know which originating file the deltas correspond to. The version string was changed as a hint that it was set wrong in the source vs what the release was called (in case this ticket got picked up and fixed). The version discrepancy got noticed later, in issue 1463

The diffs made the error handling look like the v2 branch's output for my programs test suite (with some minor changes), and matched the Sol2 code prior to the v3 change in behavior.

roman-orekhov commented 1 year ago

Replacing each of the 5 occurrences of return lua_error(L); with return luaL_error(L, lua_tolstring(L, -1, nullptr)); results in adding line location:

[sol2] An error occurred and panic has been invoked: [string "..."]:5: stack index 2, expected string, received number:
[sol2] An exception occurred: lua: error: [string "..."]:5: stack index 2, expected string, received number:
[sol2] An error occurred and panic has been invoked: [string "..."]:5: lua: error: [string "..."]:5: stack index 2, expected string, received number:

And the error is really at the line №5.

The other part of the patch replaces this line https://github.com/ThePhD/sol2/blob/develop/include/sol/error_handler.hpp#L92 but only adds clutter to this example, adding signature of the receiving constructor:

[sol2] An error occurred and panic has been invoked: [string "..."]:5: stack index 2, expected string, received number: (bad argument into 'std::basic_string<char,std::char_traits<char>,std::allocator<char> >(const std::basic_string<char,std::char_traits<char>,std::allocator<char> >&)')
[sol2] An exception occurred: lua: error: [string "..."]:5: stack index 2, expected string, received number: (bad argument into 'std::basic_string<char,std::char_traits<char>,std::allocator<char> >(const std::basic_string<char,std::char_traits<char>,std::allocator<char> >&)')
[sol2] An error occurred and panic has been invoked: lua: error: [string "..."]:5: stack index 2, expected string, received number: (bad argument into 'std::basic_string<char,std::char_traits<char>,std::allocator<char> >(const std::basic_string<char,std::char_traits<char>,std::allocator<char> >&)')