Convert no such method Panic to UnknownIdentifierException #7487
Conversation
JaroslavTulach
added
the
CI: No changelog needed
|
Is IIRC we have some code relying on I.e. will this work fine? |
engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Error.java
Outdated
Show resolved
Hide resolved
|
Also - thanks for such a quick fix! Please don't take my comments as fussing for the sake of it - I'm just trying to understand what are the implications and if this changes how we should handle missing method errors in the libraries (in general handling such errors is most often not the best pattern, but due to lack of interfaces, it sometimes provides us useful workarounds, so we rely on this in a few places - I want to make sure we are safe in doing so). |
No, |
Perfect! ❤️ |
There was a problem hiding this comment.
Looks good to me.
Can we extend the tests with one last check reproducing the scenario below?
from Standard.Base import all
polyglot java import foo.Foo
type Fooable_Class
Instance x
foo : Integer
foo self = self.x+100
type Not_A_Number
Instance
main =
i = Fooable_Class.Instance (Not_A_Number.Instance)
method_name caught_panic = Meta.meta caught_panic.payload.symbol . name
IO.println <| Panic.catch No_Such_Method (i.foo) method_name
IO.println <| Panic.catch No_Such_Method (Foo.callFoo i) method_name
I'm curious what will happen if we get an Enso No_Such_Method in Enso code which itself is called from within Java. Will the No_Such_Method be converted to the Java exception when crossing the boundary? I expect not, but I'd like to check it.
I'd expect both pure-Enso and polyglot lines to print the same result. Will that be the case?
JaroslavTulach
added
the
CI: Ready to merge
I have taken code from the original bugreport #7437 and converted it into JavaInteropTest. I cannot easily integrate your code, I'd would have to rewrite it. I don't see a reason to do so as I don't believe anything is broken. Feel free to provide/integrate an applicable diff, if you want new test that just tests something that already works. |
Ok, I will send a diff in a moment, or if you allow me I can push it onto the branch. I'm working on making this a JUnit test. For now, I've got an Enso script: which yields As we can see there are several issues here.
|
|
Patch for adding a test I'm interested in: Subject: [PATCH] Add a test checking propagation of Panics through the polyglot call.
---
Index: engine/runtime/src/test/java/org/enso/interpreter/test/JavaInteropTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/engine/runtime/src/test/java/org/enso/interpreter/test/JavaInteropTest.java b/engine/runtime/src/test/java/org/enso/interpreter/test/JavaInteropTest.java
--- a/engine/runtime/src/test/java/org/enso/interpreter/test/JavaInteropTest.java (revision 7381176053df7dc35e38d67a2eb86240621042d0)
+++ b/engine/runtime/src/test/java/org/enso/interpreter/test/JavaInteropTest.java (revision 93ba3c602dbc6afacc86d9ca1546ed5cde543253)
@@ -2,6 +2,7 @@
import java.io.ByteArrayOutputStream;
import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
import java.util.List;
import org.graalvm.polyglot.Context;
@@ -35,14 +36,17 @@
out.reset();
}
- private void checkPrint(String code, List<String> expected) {
- Value result = evalModule(ctx, code);
- assertTrue("should return Nothing", result.isNull());
- String[] logLines = out
+ private String[] getStdOutLines() {
+ return out
.toString(StandardCharsets.UTF_8)
.trim()
.split(System.lineSeparator());
- assertArrayEquals(expected.toArray(), logLines);
+ }
+
+ private void checkPrint(String code, List<String> expected) {
+ Value result = evalModule(ctx, code);
+ assertTrue("should return Nothing", result.isNull());
+ assertArrayEquals(expected.toArray(), getStdOutLines());
}
@Test
@@ -203,4 +207,55 @@
assertEquals("obj.toString() = (Instance 23)", res.getArrayElement(3).asString());
assertEquals("{(Instance 23)}.foo() = 123", res.getArrayElement(4).asString());
}
+
+ @Test
+ public void testInterfaceProxyFailures() {
+ var code = """
+ from Standard.Base import all
+ import Standard.Base.Errors.Common.No_Such_Method
+
+ polyglot java import org.enso.example.ToString as Foo
+
+ type My_Exc
+ Error
+
+ type Fooable_Panic
+ Value
+
+ foo : Integer
+ foo self =
+ IO.println "Executing Fooable_Panic.foo"
+ Panic.throw My_Exc.Error
+
+ type Fooable_Unresolved
+ Value
+
+ foo : Integer
+ foo self =
+ IO.println "Executing Fooable_Unresolved.foo"
+ "".nonexistent_text_method
+
+ main =
+ a = Panic.catch My_Exc (Foo.callFoo Fooable_Panic.Value) (.payload)
+ b = Panic.catch No_Such_Method (Foo.callFoo Fooable_Unresolved.Value) (caught-> caught.payload.method_name)
+ [a, b]
+ """;
+
+ Value res;
+ try {
+ res = evalModule(ctx, code);
+ } catch (Exception e) {
+ System.err.println("The test code failed to execute with exception: " + e);
+ System.err.println("It has produced the following stdout so far:");
+ Arrays.stream(getStdOutLines()).forEach(System.err::println);
+ throw e;
+ }
+ assertTrue("It is an array", res.hasArrayElements());
+ assertEquals("Array with 2 elements", 2, res.getArraySize());
+ assertEquals("my panic payload message 1", res.getArrayElement(0).asString());
+ assertEquals("nonexistent_text_method", res.getArrayElement(1).asString());
+ var stdout = getStdOutLines();
+ var expectedLines = List.of("Executing Fooable_Panic.foo", "Executing Fooable_Unresolved.foo");
+ assertArrayEquals(expectedLines.toArray(), stdout);
+ }
}And as I have worried, it indeed fails: As we can see, a regular panic is propagated all fine. But once we have a |
JaroslavTulach
removed
the
CI: Ready to merge
There was a problem hiding this comment.
I think we need to verify before converting No_Such_Method that it is actually relying to the 'currently being resolved' method, on the current object, and not convert all such errors.
The fact that the error saying that nonexistent_text_method does not exist gets converted into an error that a completely different object does not support method foo is quite confusing.
But the fact that the method is actually executed twice because of this - I think we should revisit that before merging, as executing a method multiple times because of a bug in resolution seems very problematic in presence of side effects (as the test suggests).
engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Error.java
Outdated
Show resolved
Hide resolved
|
Btw. interestingly the results of the JavaInteropTest seem to be not appearing in the test report uploaded on CI. I wonder if we can do sth to fix this (possibly cc @mwu-tow)? |
…anicException again
Co-authored-by: Pavel Marek <pavel.marek@enso.org>
…so-org/enso into wip/jtulach/UnknownIdentifierException_7437
...ne/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/HostMethodCallNode.java
Outdated
Show resolved
Hide resolved
engine/runtime/src/test/java/org/enso/interpreter/test/JavaInteropTest.java
Outdated
Show resolved
Hide resolved
|
Thank you for the test, Radek.
Yes, that's what bd77383 does. It marks some All the tests are passing (feel free to commit new ones) and the code looks way better then some of the attempts tried previously. Please review. |
| @@ -60,7 +59,8 @@ InvokeMethodNode buildSorter(int length) { | |||
| args, | |||
| InvokeCallableNode.DefaultsExecutionMode.EXECUTE, | |||
| InvokeCallableNode.ArgumentsExecutionMode.PRE_EXECUTED, | |||
| 0); | |||
| 0, | |||
| true); | |||
There was a problem hiding this comment.
InteropMethodCallNode does interop call onBoundary. Thus passing true in.
engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/atom/Atom.java
Show resolved
Hide resolved
Pull Request Description
There is a special handling of toString() and other methods in GraalVM SDK. However we need to throw standard
InteropExceptionto activate it. ConvertingPanicExceptionthat represents no such method error on the boundary toUnknownIdentifierException. With such change one can call.toString()on proxies wrapping Enso objects.Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Java,