enso-org / enso

Hybrid visual and textual functional programming.
https://ensoanalytics.com
Apache License 2.0
7.36k stars 324 forks source link

Type in Truffle Node breaks GraalVM semantics #6809

Closed JaroslavTulach closed 1 year ago

JaroslavTulach commented 1 year ago

What is the difference between code and data in Truffle? Code is supposed to live longer and be more immutable than data. In short:

In order for Node to live longer, be immutable, be reusable, , it is forbidden to reference data from Node. Rather than that Node shall reference a "general shape of data". See the difference between Function - the data and FunctionSchema - something embedded in Nodes. Global data are stored in EnsoContext - including Type instances.

There is at least few violations of this rule that needs to be fixed:

It is necessary to replace such references with one level of indirection - the actual Type must be obtained from EnsoContext - the Node can only hold something to discover the type effectively.

There is a branch wip/jtulach/NoTypeInNodePlease_6809 which contains a failing test. The test creates one Engine and uses the same Source in two different Contexts. Using the same engine & source means there is just one instance of AST nodes shared between the two contexts. The CatchTypeBranchNode embeds Type from the first context, so the first execution succeeds, but second fails as the Type for Text is different.

JaroslavTulach commented 1 year ago

@kustosz suggests to share Type instances between multiple org.graalvm.polyglot.Contexts - e.g. EnsoContexts. True: We could store them in EnsoLanguage!

JaroslavTulach commented 1 year ago

Surprisingly when the type is not a builtin, but a regular type defined in source, it holds its identity:

diff --git engine/runtime/src/test/java/org/enso/interpreter/test/SharedEngineTest.java engine/runtime/src/test/java/org/enso/interpreter/test/SharedEngineTest.java
index c1815ce6b2..9825872fd1 100644
--- engine/runtime/src/test/java/org/enso/interpreter/test/SharedEngineTest.java
+++ engine/runtime/src/test/java/org/enso/interpreter/test/SharedEngineTest.java
@@ -2,7 +2,6 @@ package org.enso.interpreter.test;

 import java.io.ByteArrayOutputStream;
 import java.nio.file.Paths;
-
 import org.enso.polyglot.RuntimeOptions;
 import org.graalvm.polyglot.Context;
 import org.graalvm.polyglot.Engine;
@@ -40,11 +39,17 @@ public class SharedEngineTest extends TestBase {
   private final Source typeCase = Source.newBuilder("enso", """
     from Standard.Base import Vector, Text, Number

+    type Own
+      Value c
+
+    init x = Own.Value x
+
     check x = case x of
         _ : Vector -> 1
         _ : Text -> 2
         _ : Number -> 3
-        _ -> 4
+        _ : Own -> 4
+        _ -> 5
     """,
     "type_case.enso"
   ).buildLiteral();
@@ -56,8 +61,21 @@ public class SharedEngineTest extends TestBase {
     assertEquals(2, r.asInt());
   }

+  @Test
+  public void ownTypeCaseFirstRun() {
+    var init = this.ctx.eval(typeCase).invokeMember("eval_expression", "init");
+    var ownValue = init.execute("Hi");
+    var fn = this.ctx.eval(typeCase).invokeMember("eval_expression", "check");
+    var r = fn.execute(ownValue);
+    assertEquals(4, r.asInt());
+  }
+
   @Test
   public void typeCaseSecondRun() {
     typeCaseFirstRun();
   }
+  @Test
+  public void ownTypeCaseSecondRun() {
+    ownTypeCaseFirstRun();
+  }
 }

Both ownType.... runs return 4.

enso-bot[bot] commented 1 year ago

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-08):

Progress: - SharedEngineTest & EnsoContext: https://github.com/enso-org/enso/pull/7493

Next Day: Python interop & bugfixes