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

Type in Truffle Node breaks GraalVM semantics #6809

Closed
JaroslavTulach opened this issue May 23, 2023 · 3 comments · Fixed by #7493
Closed

Type in Truffle Node breaks GraalVM semantics #6809

JaroslavTulach opened this issue May 23, 2023 · 3 comments · Fixed by #7493
Assignees
Labels
--breaking Important: a change that will break a public API or user-facing behaviour --bug Type: bug --data corruption or loss Important: data corruption or loss -compiler

Comments

@JaroslavTulach
Copy link
Member

JaroslavTulach commented May 23, 2023

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

  • code is represented by com.oracle.truffle.api.nodes.Node
  • data are either primitive values or TruffleObject subclasses

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 JaroslavTulach added --breaking Important: a change that will break a public API or user-facing behaviour --bug Type: bug -compiler --data corruption or loss Important: data corruption or loss labels May 23, 2023
@jdunkerley jdunkerley added this to the Beta Release milestone May 30, 2023
@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board May 30, 2023
@JaroslavTulach
Copy link
Member Author

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

@JaroslavTulach
Copy link
Member Author

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.

@jdunkerley jdunkerley removed this from the Beta Release milestone Jul 11, 2023
@JaroslavTulach JaroslavTulach moved this from 📤 Backlog to 🔧 Implementation in Issues Board Aug 3, 2023
@JaroslavTulach JaroslavTulach moved this from 🔧 Implementation to 👁️ Code review in Issues Board Aug 8, 2023
@mergify mergify bot closed this as completed in #7493 Aug 8, 2023
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Aug 8, 2023
mergify bot pushed a commit that referenced this issue Aug 8, 2023
…constant (#7493)

Fixes #6809 by giving up on any attempt to share `Node` among multiple `Context` via sharing a single `Engine`.
@enso-bot
Copy link

enso-bot bot commented Aug 9, 2023

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

Progress: - SharedEngineTest & EnsoContext: #7493

Next Day: Python interop & bugfixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--breaking Important: a change that will break a public API or user-facing behaviour --bug Type: bug --data corruption or loss Important: data corruption or loss -compiler
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants