rhaiscript / rhai

Rhai - An embedded scripting language for Rust.
https://crates.io/crates/rhai
Apache License 2.0
3.7k stars 175 forks source link

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

Open therealprof opened 1 month ago

therealprof commented 1 month ago

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 commented 1 month ago

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

What other info do you suggest?

therealprof commented 1 month ago

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:

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/rhaiscript/rhai/blob/03730d8a9926bc965411d81ab3b14443c4e10e39/src/types/fn_ptr.rs#L361-L362 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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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: https://github.com/rhaiscript/rhai/blob/03730d8a9926bc965411d81ab3b14443c4e10e39/src/types/fn_ptr.rs#L361-L362

schungx commented 1 month ago

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