Throw panic on "no currying for conversions" #6940
Conversation
Previously, a `RuntimeException` would be thrown when an attempt would be made to curry a conversion function. That is problematic for IDE where `executionFailed` means we can't enter functions due to lack of method pointers info. A more generic solution that allows to recover from execution failures will need a follow up. Closes #6897.
hubertp
requested review from
jdunkerley,
radeusgd,
GregoryTravis,
4e6,
JaroslavTulach and
Akirathan
as code owners
| throw new RuntimeException( | ||
| "Conversion currying without `this` or `that` argument is not supported."); | ||
| var ctx = EnsoContext.get(this); | ||
| throw new PanicException(ctx.getBuiltins().error().makeNoConversionCurrying(canApplyThis, canApplyThat, conversion), this); |
There was a problem hiding this comment.
RuntimeException is completely inappropriate for reporting errors in user code. Throwing PanicException is certainly more desirable for such situations.
engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Error.java
Show resolved
Hide resolved
| @@ -112,4 +112,7 @@ spec = | |||
| meta_from.rename "foo" 123 . should_equal "foo called" | |||
| meta_from.rename "foo" . should_equal .foo | |||
|
|
|||
| Test.specify "should not allow currying" <| | |||
| Panic.recover Any (Foo.from) . catch Any .to_display_text . should_equal "Conversion currying without `that` argument is not supported." | |||
There was a problem hiding this comment.
Wouldn't should_fail_with be better suited for the test? Or do you want the test to pass even when no exception happen?
There was a problem hiding this comment.
Wanted to test the actual message. But checking the error is sufficient, I guess.
There was a problem hiding this comment.
Wouldn't should_fail_with be better suited for the test?
Actually that won't work with Panics.
There was a problem hiding this comment.
I would appreciate if we do test the message.
There was a problem hiding this comment.
should_fail_with will work if you first do Panic.recover - recover will convert the Panic into dataflow error and then should_fail_with can handle it.
There was a problem hiding this comment.
And using catch you can then check the error message. Like this:
| Panic.recover Any (Foo.from) . catch Any .to_display_text . should_equal "Conversion currying without `that` argument is not supported." | |
| r = Panic.recover Any (Foo.from) | |
| r.should_fail_with No_Conversion_Currying | |
| r.catch . to_display_text . should_equal "Conversion currying without `that` argument is not supported." |
There was a problem hiding this comment.
Alternatively, you can use Test.expect_panic_with directly - this is a function meant for testing panics. But it won't allow you to check the error message and I think checking it is good - we had lots of places where to_display_text would crash because of typos and that went undetected only until the IDE was unable to display error properly. We want to go in the direction of having to_display_text of every error unit tested to ensure no regressions of error display can get into the IDE.
...e/src/main/java/org/enso/interpreter/node/expression/builtin/error/NoConversionCurrying.java
Show resolved
Hide resolved
hubertp
added
CI: No changelog needed
Pull Request Description
Previously, a
RuntimeExceptionwould be thrown when an attempt would be made to curry a conversion function. That is problematic for IDE whereexecutionFailedmeans we can't enter functions due to lack of method pointers info.Closes #6897.
Important Notes
A more generic solution that allows to recover from execution failures will need a follow up.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build.