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

Unhelpful error when trying to call a non-existing function or with wrong signature #899

Open
therealprof opened this issue Jul 15, 2024 · 16 comments

Comments

@therealprof
Copy link

The returned error when trying to call a function implemented in Rust with either incorrect name or incorrect signature is not helpful. E.g.

Function not found: add_param_setting (rusp::usp_builder::add::CreateObjectBuilder, &str | ImmutableString | String, &str | ImmutableString | String, i64) (line 5, position 10)

It's basically throwing back at you what you're trying to do without any hints about what might be wrong with it.

I guess in the interest of space we don't want to do some guesswork to find the correct fix and make a suggestion. But it would be nice to at least detect whether the function doesn't exist at all (i.e. wrong function name) or the function does exist but has a non-matching signature.

@schungx
Copy link
Collaborator

schungx commented Jul 16, 2024

Well, it does print out the parameter types so you can find the function that is missing...

What other info do you suggest?

@therealprof
Copy link
Author

Well, it does print out the parameter types so you can find the function that is missing...

It takes quite a bit of work to look into the Rust code to figure out what's wrong. People who want to write Rhai scripts don't necessarily know how to navigate the the Rust implementation to figure out the source of an error. There're some additional pitfalls, things like numbers always being as i64 which trip even me frequently.

I was thinking of changing all arguments into Dynamic and returning Results so I can create more useful error messages, but that seems very heavy handed...

What other info do you suggest?

At the very least I would split the one error message into two:

  • Unknown function
  • Call arguments don't match signature

Ideally the signature mismatch would result in a message pointing out the signature of the function that was found with that name, but I don't know whether that's easily possible or too complex.

@schungx
Copy link
Collaborator

schungx commented Jul 17, 2024

Well the point is... Function not found is the same as wrong parameter types.

Rhai functions can be overloaded. There may be multiple functions with the same name and even the same number of parameters.

I don't mind if we can find a way to convey this information more easily such as show a list of similarly named functions... However some rye uses already depend on the fact that the function signature follows the error message

@therealprof
Copy link
Author

For me adding suggestions would be a nice bonus but the real important bit is: As a user I want to know whether a function of that name doesn't exist at all or there is at least one such function but the combination of passed-in arguments in didn't match any of the function signatures.

If you write a shell script and you get a command not found error, you'll check for a completely different issue than if you had gotten an invalid arguments error.

The same is true for Rhai, especially with complex signatures. It took me quite some time to figure out that my script stopped working in the middle of nowhere just because of a typo in the function name while I was meticulously checking the correctness of the passed in arguments against the signature of the function I would have wanted to call, if there hadn't been the typo.

I dug a little deeper and checked out the places generating ErrorFunctionNotFound and handling them and it seems that there are some places already checking on whether there is a function by that name

rhai/src/types/fn_ptr.rs

Lines 361 to 362 in 03730d8

ERR::ErrorFunctionNotFound(sig, ..)
if MOVE_PTR && this_ptr.is_some() && sig.starts_with(self.fn_name()) =>

but not making use of the information by passing it on to the error handler.

Unfortunately the code is a bit too convoluted for me to figure out how to split this into two distinct errors at the moment due to lack of time.

@schungx
Copy link
Collaborator

schungx commented Jul 17, 2024

I think I get your point, but exhaustively searching for similarly-named functions would incur a performance hit...

It is OK if the failure is uncaught and so taking time to do it is not on the happy path, but if the user catch the exception, then it can potentially be a performance hit.

@therealprof
Copy link
Author

therealprof commented Jul 17, 2024

Regarding adding a suggestion: I'd be more worried about the size impact than the performance impact on the error path but I'm not actually suggesting that we do this.

For now I'm only recommending that ErrorFunctionNotFound is split into ErrorFunctionNotFound and something like ErrorFunctionNameNotFound, where the ErrorFunctionNotFound can still indicate that there's no function with a compatible signature and and the new one indicating that there is no function by that name at all.

@schungx
Copy link
Collaborator

schungx commented Jul 17, 2024

That's the point... there is no way to detect whether there are functions of that name registered, short of scanning the entire registered functions base as well as all loaded scripts plus their embedded modules.

In other words an exhaustive scanning just to find out.

@therealprof
Copy link
Author

That's the point... there is no way to detect whether there are functions of that name registered, short of scanning the entire registered functions base as well as all loaded scripts plus their embedded modules.

I don't understand. When we're calling the function we have to notice that it doesn't exist (or has an incompatible signature), at least that is my understanding of https://github.com/rhaiscript/rhai/blob/03730d8a9926bc965411d81ab3b14443c4e10e39/src/types/fn_ptr.rs#L359C14-L359C22

Why would we need to scan the registered functions to figure out that a function exists when we already know that it doesn't? In my naive view it's only a matter of figuring out why ErrorFunctionNotFound was returned and it does seem like that code is already checking whether the signature contains the called name here:

rhai/src/types/fn_ptr.rs

Lines 361 to 362 in 03730d8

ERR::ErrorFunctionNotFound(sig, ..)
if MOVE_PTR && this_ptr.is_some() && sig.starts_with(self.fn_name()) =>

@schungx
Copy link
Collaborator

schungx commented Jul 18, 2024

Well, Rhai doesnt search by function name. That would be too slow.

It hashes up the function name, arity and parameter types into a single number then lookup that number in a sequence of hash tables.

To search by name requires scanning through all those hash tables to access the function name, which currently is not stored together with the function.

See https://rhai.rs/book/rust/dynamic-args.html#tldr for more details

@elkowar
Copy link

elkowar commented Dec 26, 2024

I see your argument here, @schungx, but I'd really love for you to reconsider:
In an error case, especially one like "a function that doesn't exist was called", the performance should, in most cases, not be mission critical. Therefore, I'd argue at least providing the option to have a "A function with this other signature exists" message in the error would be worth it.

Alternatively, it would be nice if we could access all of the existing functions ourselves in some structured manner, without having to resort to the JSON objects, so we could implement something like this on top of the existing errors. For my project, I'd be more than happy to go through all available functions in case of a "this function does not exist" error, match the signatures, and maybe even go through a levenshtein-distance thing to add further help output including a "did you mean this similarly named function?" to the error.

The current structure here is severely limiting the quality and helpfulness of error messages I can provide, and I feel as though the tradeoff made here is not worth it -- especially given that this could relatively easily be an optional feature one would explicitly have to enable on the engine.

@schungx
Copy link
Collaborator

schungx commented Dec 26, 2024

Maybe a separate lookup function that can feed in a function signature and return matches?

@elkowar
Copy link

elkowar commented Dec 27, 2024

I'm not sure if that would help, given that the main goal would be finding near matches, i.e. matches where the function name is correct, but the argument types are wrong, or cases where the function name is nearly correct, etc.

Just having some way to access a set of all functions and their signatures in a namespace, in a strongly typed, structured manner, would allow the user to implement any kind of error handling and error message on top of the existing errors.

I do think it'd be worth it to integrate something like this into rhai itself eventually, but as a first solution just allowing the user to implement this themselves in a clean way would already be a huge step!

@schungx
Copy link
Collaborator

schungx commented Dec 28, 2024

In this case, we can already do that by turning on metadata and then generating the function lists...

There is already such an implementation: https://github.com/rhaiscript/rhai/blob/main/src/packages/lang_core.rs#L224

However it is not marked pub. Maybe we should make it public (or make a public function calling into it)?

@elkowar
Copy link

elkowar commented Dec 28, 2024

If that, and optimally the corresponding type definitions, where public, that'd already be a huge help, yes!

@schungx
Copy link
Collaborator

schungx commented Dec 29, 2024

Yes, it contains everything to know about a function including all the parameter types and names.

@schungx
Copy link
Collaborator

schungx commented Dec 31, 2024

Try collect_fn_metadata from #945

You'd need the internals feature.

The functions in the list are ordered in such a way that it follows the functions resolution order of Rhai.

BTW, if you do cook up a nice fuzzy function-match error message, consider contributing it back here so I can include it into the standard package!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants