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

Unify conversions of polyglot values entering Enso engine into a single Node implementation #5178

Open
wdanilo opened this issue Feb 5, 2023 · 2 comments
Labels
-compiler p-low Low priority x-chore Type: chore

Comments

@wdanilo
Copy link
Member

wdanilo commented Feb 5, 2023

This task is automatically imported from the old Task Issue Board and it was originally created by jaroslavtulach.
Original issue is here.


While working on #183000876 it come to my attention that there are two similar, yet different nodes performing a very important functionality:

At various places we are using different combinations of these nodes. That results in inconsistencies in treating foreign values. Some may be converted when passed as parameters, but not converted when obtained from a method or read from an array. We have to unify all these boundary conversions into a single Node and use it everywhere.

This is closely related to runtime type system (not the one @ekmett is working on, internal to Enso engine). We have to guarantee the conversion node accepts all interop valid values and makes sure all of them are consistently converted to on of Enso supported types.

Btw. rule of Truffle Interop is: delay conversions of TruffleObject instances as long as possible. CoercePrimitiveNode does convert - e.g. it is probably not the right approach and HostValueToEnsoNode is better.

Comments:

Just to clarify - CoerceNothing was introduced separately because CoercePrimitiveNode is in epb and the former needs Nothing. (Hubert Plociniczak - Sep 13, 2022)


Can the dependency of epb be reverted? E.g. move all the code into `runtime` and only leave the language registration in the `runtime-language-epb`? Then we wouldn't have to have two conversion nodes and could unify around a single node to convert it all. EPB isn't the only place that requires conversions. Array, Vector builtins, Java inteorp need that as well. (jaroslavtulach - Sep 15, 2022)
@JaroslavTulach
Copy link
Member

JaroslavTulach commented Dec 5, 2024

Both of these nodes still seem to be present in our code base. The fact that these nodes aren't unified has already caused a problem:

@JaroslavTulach
Copy link
Member

Regardless of unification, this is an interesting assert we should add:

diff --git engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/CurryNode.java engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/CurryNode.java
@@ -105,6 +109,7 @@ public class CurryNode extends BaseNode {
     if (appliesFully) {
       if (!postApplicationSchema.hasOversaturatedArgs()) {
         var value = doCall(frame, function, callerInfo, state, arguments);
+        assert isValidEnsoValue(value) : "Unexpected value " + value;
         if (defaultsExecutionMode.isExecute()
             && (value instanceof Function
                 || (value instanceof AtomConstructor cons
@@ -161,4 +166,24 @@ public class CurryNode extends BaseNode {
       default -> loopingCall.executeDispatch(frame, function, callerInfo, state, arguments, null);
     };
   }
+
+  private boolean isValidEnsoValue(Object value) {
+    if (value instanceof TruffleObject) {
+      return true;
+    }
+    if (value instanceof Boolean) {
+      return true;
+    }
+    if (value instanceof Long) {
+      return true;
+    }
+    if (value instanceof Double) {
+      return true;
+    }
+    var ctx = EnsoContext.get(this);
+    CompilerDirectives.transferToInterpreter();
+    var type = value != null ? value.getClass().getName() : "null";
+    var msg = "Unexpected value " + value + " type " + type;
+    throw ctx.raiseAssertionPanic(this, msg, null);
+  }
 }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-compiler p-low Low priority x-chore Type: chore
Projects
None yet
Development

No branches or pull requests

3 participants