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

Add function name information for "attempt to call a nil value" #255

Closed
JDaance opened this issue Nov 30, 2021 · 10 comments · Fixed by #625
Closed

Add function name information for "attempt to call a nil value" #255

JDaance opened this issue Nov 30, 2021 · 10 comments · Fixed by #625
Assignees
Labels
enhancement New feature or request

Comments

@JDaance
Copy link

JDaance commented Nov 30, 2021

Loving everything luau so far, but this feature missing from lua-5.1/luajit is big enough for me to create an issue. Everything is so well done in luau (and its very fast) that I suspect there may be good reasons for this (performance), but here goes.

Error report in luau:
image

Same error on luajit/lua-5.1 (identical error message on both):
image

Its very helpful to have the original error message!

@JDaance JDaance added the enhancement New feature or request label Nov 30, 2021
@zeux
Copy link
Collaborator

zeux commented Nov 30, 2021

I guess at the minimum the line information here is wrong. Just to make sure I understand what happens here at runtime - does withChunkDiff return a valid table that doesn't have withTags key, so withTags returns nil and attempting to call that function fails?

Lua (and probably luajit) use bytecode tracing to do symbolic execution of bytecode in an event an error happens to try to recover the name of the expression. We don't have this mechanism. That said, in this specific case it's probably possible for us to generate a better error, although fixing the line might be a good first step.

@zeux
Copy link
Collaborator

zeux commented Nov 30, 2021

The line info will be fixed later this week. I'm unsure about the actual error message - looking at the code I think it should be feasible to emit something like attempt to call non-existent method 'withTags' without affecting the fast path, but it's rather specific to method calls. Still, given the prevalence of method calls it might make sense to make this change.

@JDaance
Copy link
Author

JDaance commented Nov 30, 2021

Just to make sure I understand what happens here at runtime - does withChunkDiff return a valid table that doesn't have withTags key, so withTags returns nil and attempting to call that function fails?

Yes, thats exactly right, I should have explained that.

The line info will be fixed later this week. I'm unsure about the actual error message - looking at the code I think it should be feasible to emit something like attempt to call non-existent method 'withTags' without affecting the fast path, but it's rather specific to method calls. Still, given the prevalence of method calls it might make sense to make this change.

Sounds glorious! I run into this error all the time, I have a habit of just calling the methods I need without declaring them, thinking out load, outside in approach. Also chaining method calls like the above.

zeux added a commit that referenced this issue Dec 3, 2021
- Fix some cases where type checking would overflow the native stack
- Improve autocomplete behavior when assigning a partially written function call (not currently exposed through command line tools)
- Improve autocomplete type inference feedback for some expressions where previously the type would not be known
- Improve quantification performance during type checking for large types
- Improve type checking for table literals when the expected type of the table is known because of a type annotation
- Fix type checking errors in cases where required module has errors in the resulting type
- Fix debug line information for multi-line chained call sequences (Add function name information for "attempt to call a nil value" #255)
- lua_newuserdata now takes 2 arguments to match Lua/LuaJIT APIs better; lua_newuserdatatagged should be used if the third argument was non-0.
- lua_ref can no longer be used with LUA_REGISTRYINDEX to prevent mistakes when migrating Lua FFI (Inconsistency with lua_ref #247)
- Fix assertions and possible crashes when executing script code indirectly via metatable dispatch from lua_equal/lua_lessthan/lua_getfield/etc. (Hitting a crash in an assert after lua_equal is called. #259)
- Fix flamegraph scripts to run under Python 2
@LoganDark
Copy link
Contributor

How would this work in production environments? Would the runtime behavior of which error message is emitted change to the old version without a name? Or will variable names of method calls be preserved in production for the error message? Neither one sounds particularly exciting.

@JDaance
Copy link
Author

JDaance commented Jan 28, 2022

How would this work in production environments? Would the runtime behavior of which error message is emitted change to the old version without a name? Or will variable names of method calls be preserved in production for the error message? Neither one sounds particularly exciting.

Are you saying the method names can be optimized away so they dont need to be included in the bytecode? For globals known at compile time or something like that?

I still run into this multiple times a day, having a chain of method calls and not knowing which one is nil, and I also spend time looking up the exact source location to look at the call to see the name, instead of just seeing the stacktrace and know whats up. So anyway you could solve it so I see the method name would be great for me.

And I appreciate the line numbers being correct now, it helps!

@LoganDark
Copy link
Contributor

Non-local variables get assigned to the script's environment rather than the current scope, so those keep their names because they have to at runtime.

However, local variables don't keep their names at runtime, they're just stack slots.

@JDaance
Copy link
Author

JDaance commented Jan 28, 2022

Non-local variables get assigned to the script's environment rather than the current scope, so those keep their names because they have to at runtime.

However, local variables don't keep their names at runtime, they're just stack slots.

Are local variable names involved in method calls somehow? You could call a method on a local variable, like

local t = {}
t:myMethod()

But the name t is not what my issue is about, its the myMethod part that is so nicely output by lua and luajit.

@LoganDark
Copy link
Contributor

Non-local variables get assigned to the script's environment rather than the current scope, so those keep their names because they have to at runtime.
However, local variables don't keep their names at runtime, they're just stack slots.

Are local variable names involved in method calls somehow? You could call a method on a local variable, like

local t = {}
t:myMethod()

But the name t is not what my issue is about, its the myMethod part that is so nicely output by lua and luajit.

Yes, myMethod exists at runtime. Namecall knows the name of the method it's calling. If you did this:

local x = ...
x()

Luau wouldn't know the nil value was named x.

However upon re-reading your issue I realize you are talking about namecall specifically so this isn't an issue. My apologies.

@JDaance
Copy link
Author

JDaance commented Jan 28, 2022

No problem!

@zeux zeux mentioned this issue Jun 1, 2022
@zeux zeux self-assigned this Jul 14, 2022
@zeux zeux closed this as completed in #625 Aug 4, 2022
@JDaance
Copy link
Author

JDaance commented Aug 5, 2022

🤩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging a pull request may close this issue.

3 participants