Ensure that warnings are preserved on Nothing values passing back to …

…Enso through polyglot boundary (#5677) Fixes #5672 # Important Notes - Added a subproject `enso-test-java-helpers` which allows the in-Enso tests to add Java helpers for testing.
enso-org · Feb 17, 2023 · 4dcf802 · 4dcf802
1 parent 84eaf2c
commit 4dcf802
Show file tree

Hide file tree

Showing 7 changed files with 159 additions and 63 deletions.
diff --git a/build.sbt b/build.sbt
@@ -302,7 +302,8 @@ lazy val enso = (project in file("."))
     `std-google-api`,
     `std-image`,
     `std-table`,
-    `simple-httpbin`
+    `simple-httpbin`,
+    `enso-test-java-helpers`
   )
   .settings(Global / concurrentRestrictions += Tags.exclusive(Exclusive))
   .settings(
@@ -737,7 +738,11 @@ val generateParserJavaSources = TaskKey[Seq[File]](
 `syntax-rust-definition` / generateParserJavaSources / fileInputs +=
   (`syntax-rust-definition` / baseDirectory).value.toGlob / "src" / ** / "*.rs"
 
-def generateRustParser(base: File, changes: sbt.nio.FileChanges, log: ManagedLogger): Seq[File] = {
+def generateRustParser(
+  base: File,
+  changes: sbt.nio.FileChanges,
+  log: ManagedLogger
+): Seq[File] = {
   import scala.jdk.CollectionConverters._
   import java.nio.file.Paths
 
@@ -1120,18 +1125,19 @@ lazy val `interpreter-dsl` = (project in file("lib/scala/interpreter-dsl"))
 // === Sub-Projects ===========================================================
 // ============================================================================
 
-val truffleRunOptionsNoAssert = if (java.lang.Boolean.getBoolean("bench.compileOnly")) {
-  Seq(
-    "-Dpolyglot.engine.IterativePartialEscape=true",
-    "-Dpolyglot.engine.BackgroundCompilation=false",
-    "-Dbench.compileOnly=true"
-  )
-} else {
-  Seq(
-    "-Dpolyglot.engine.IterativePartialEscape=true",
-    "-Dpolyglot.engine.BackgroundCompilation=false"
-  )
-}
+val truffleRunOptionsNoAssert =
+  if (java.lang.Boolean.getBoolean("bench.compileOnly")) {
+    Seq(
+      "-Dpolyglot.engine.IterativePartialEscape=true",
+      "-Dpolyglot.engine.BackgroundCompilation=false",
+      "-Dbench.compileOnly=true"
+    )
+  } else {
+    Seq(
+      "-Dpolyglot.engine.IterativePartialEscape=true",
+      "-Dpolyglot.engine.BackgroundCompilation=false"
+    )
+  }
 val truffleRunOptions = "-ea" +: truffleRunOptionsNoAssert
 
 val truffleRunOptionsNoAssertSettings = Seq(
@@ -1192,7 +1198,7 @@ lazy val `language-server` = (project in file("engine/language-server"))
       "org.scalatest"              %% "scalatest"            % scalatestVersion  % Test,
       "org.scalacheck"             %% "scalacheck"           % scalacheckVersion % Test,
       "org.graalvm.sdk"             % "polyglot-tck"         % graalVersion      % "provided",
-      "org.eclipse.jgit"            % "org.eclipse.jgit"     % jgitVersion,
+      "org.eclipse.jgit"            % "org.eclipse.jgit"     % jgitVersion
     ),
     Test / testOptions += Tests
       .Argument(TestFrameworks.ScalaCheck, "-minSuccessfulTests", "1000"),
@@ -1323,12 +1329,11 @@ lazy val `runtime-language-epb` =
       instrumentationSettings
     )
 
-/**
- * Runs gu (GraalVM updater) command with given `args`.
- * For example `runGu(Seq("install", "js"))`.
- * @param logger Logger for the `gu` command.
- * @param args Arguments for the `gu` command.
- */
+/** Runs gu (GraalVM updater) command with given `args`.
+  * For example `runGu(Seq("install", "js"))`.
+  * @param logger Logger for the `gu` command.
+  * @param args Arguments for the `gu` command.
+  */
 def runGu(logger: ManagedLogger, args: Seq[String]): String = {
   val javaHome = new File(
     System.getProperty("java.home")
@@ -1343,7 +1348,7 @@ def runGu(logger: ManagedLogger, args: Seq[String]): String = {
     logger,
     os,
     javaHome,
-    args:_*
+    args: _*
   )
 }
 
@@ -1352,8 +1357,7 @@ def installedGuComponents(logger: ManagedLogger): Seq[String] = {
     logger,
     Seq("list")
   )
-  val components = componentList
-    .linesIterator
+  val components = componentList.linesIterator
     .drop(2)
     .map { line =>
       line
@@ -1380,7 +1384,8 @@ ThisBuild / installGraalJs := {
   }
 }
 
-lazy val installNativeImage = taskKey[Unit]("Install native-image GraalVM component")
+lazy val installNativeImage =
+  taskKey[Unit]("Install native-image GraalVM component")
 ThisBuild / installNativeImage := {
   val logger = streams.value.log
   if (!installedGuComponents(logger).contains("native-image")) {
@@ -1474,6 +1479,7 @@ lazy val runtime = (project in file("engine/runtime"))
   .settings(
     (Runtime / compile) := (Runtime / compile)
       .dependsOn(`std-base` / Compile / packageBin)
+      .dependsOn(`enso-test-java-helpers` / Compile / packageBin)
       .dependsOn(`std-image` / Compile / packageBin)
       .dependsOn(`std-database` / Compile / packageBin)
       .dependsOn(`std-google-api` / Compile / packageBin)
@@ -1611,7 +1617,9 @@ lazy val `runtime-with-polyglot` =
         val runtimeClasspath =
           (LocalProject("runtime") / Compile / fullClasspath).value
         val runtimeInstrumentsClasspath =
-          (LocalProject("runtime-with-instruments") / Compile / fullClasspath).value
+          (LocalProject(
+            "runtime-with-instruments"
+          ) / Compile / fullClasspath).value
         val appendClasspath =
           (runtimeClasspath ++ runtimeInstrumentsClasspath)
             .map(_.data)
@@ -1625,8 +1633,8 @@ lazy val `runtime-with-polyglot` =
         "ENSO_TEST_DISABLE_IR_CACHE" -> "false"
       ),
       libraryDependencies ++= Seq(
-        "org.graalvm.sdk" % "graal-sdk" % graalVersion % "provided",
-        "org.scalatest" %% "scalatest" % scalatestVersion % Test
+        "org.graalvm.sdk" % "graal-sdk" % graalVersion     % "provided",
+        "org.scalatest"  %% "scalatest" % scalatestVersion % Test
       )
     )
     .dependsOn(runtime % "compile->compile;test->test;runtime->runtime")
@@ -1693,7 +1701,6 @@ lazy val `engine-runner` = project
     assembly := assembly
       .dependsOn(`runtime-with-instruments` / assembly)
       .value,
-
     rebuildNativeImage := NativeImage
       .buildNativeImage(
         "runner",
@@ -1720,7 +1727,6 @@ lazy val `engine-runner` = project
       .dependsOn(installNativeImage)
       .dependsOn(assembly)
       .value,
-
     buildNativeImage := NativeImage
       .incrementalNativeImageBuild(
         rebuildNativeImage,
@@ -2033,6 +2039,18 @@ lazy val `std-base` = project
     }.value
   )
 
+lazy val `enso-test-java-helpers` = project
+  .in(file("test/Tests/polyglot-sources/enso-test-java-helpers"))
+  .settings(
+    frgaalJavaCompilerSetting,
+    autoScalaLibrary := false,
+    Compile / packageBin / artifactPath :=
+      file("test/Tests/polyglot/java/helpers.jar"),
+    libraryDependencies ++= Seq(
+      "org.graalvm.truffle" % "truffle-api" % graalVersion % "provided"
+    )
+  )
+
 lazy val `std-table` = project
   .in(file("std-bits") / "table")
   .enablePlugins(Antlr4Plugin)
@@ -2267,8 +2285,11 @@ pkgStdLibInternal := Def.inputTask {
       (`std-image` / Compile / packageBin).value
     case "Table" =>
       (`std-table` / Compile / packageBin).value
+    case "TestHelpers" =>
+      (`enso-test-java-helpers` / Compile / packageBin).value
     case "All" =>
       (`std-base` / Compile / packageBin).value
+      (`enso-test-java-helpers` / Compile / packageBin).value
       (`std-table` / Compile / packageBin).value
       (`std-database` / Compile / packageBin).value
       (`std-image` / Compile / packageBin).value

diff --git a/distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso b/distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso
@@ -745,20 +745,9 @@ type Output_Stream
            the `?` character.
         replacement_sequence = Encoding_Utils.INVALID_CHARACTER.bytes encoding on_problems=Problem_Behavior.Ignore
         java_charset = encoding.to_java_charset
-        ## This is a workaround due to the fact that this `action` will be
-           called as a callback from a Java method and we have a bug where a
-           `Nothing` returned from such a method will loose its attached
-           warnings. To avoid that, we return a Text value instead. It is then
-           replaced with a `Nothing` on the other side of the call, but making
-           sure that any warnings are inherited.
-           The related bug is tracked at https://github.com/enso-org/enso/issues/5672
-        wrapped_action encoder =
-            result = action encoder
-            Pair.new result Nothing
-        results = Encoding_Utils.with_stream_encoder java_stream java_charset replacement_sequence.to_array wrapped_action
+        results = Encoding_Utils.with_stream_encoder java_stream java_charset replacement_sequence.to_array action
         problems = Vector.from_polyglot_array results.problems . map Encoding_Error.Error
-        unwrapped_result = results.result . first
-        on_problems.attach_problems_after unwrapped_result problems
+        on_problems.attach_problems_after results.result problems
 
 ## An input stream, allowing for interactive reading of contents from an open
    file.

diff --git a/...java/org/enso/interpreter/node/expression/builtin/interop/syntax/HostValueToEnsoNode.java b/...java/org/enso/interpreter/node/expression/builtin/interop/syntax/HostValueToEnsoNode.java
@@ -4,8 +4,9 @@
 import com.oracle.truffle.api.interop.InteropLibrary;
 import com.oracle.truffle.api.library.CachedLibrary;
 import com.oracle.truffle.api.nodes.Node;
-import org.enso.interpreter.runtime.EnsoContext;
+import org.enso.interpreter.node.expression.foreign.CoerceNothing;
 import org.enso.interpreter.runtime.data.text.Text;
+import org.enso.interpreter.runtime.error.WarningsLibrary;
 
 /**
  * Converts a value returned by a polyglot call back to a value that can be further used within Enso
@@ -21,7 +22,6 @@ public static HostValueToEnsoNode build() {
   public static HostValueToEnsoNode getUncached() {
     return HostValueToEnsoNodeGen.getUncached();
   }
-
   /**
    * Converts an arbitrary value to a value usable within Enso code.
    *
@@ -61,8 +61,12 @@ Text doString(String txt) {
   }
 
   @Specialization(guards = {"o != null", "nulls.isNull(o)"})
-  Object doNull(Object o, @CachedLibrary(limit = "3") InteropLibrary nulls) {
-    return EnsoContext.get(this).getBuiltins().nothing();
+  Object doNull(
+      Object o,
+      @CachedLibrary(limit = "3") InteropLibrary nulls,
+      @CachedLibrary(limit = "3") WarningsLibrary warnings,
+      @Cached CoerceNothing coerceNothing) {
+    return coerceNothing.execute(o);
   }
 
   @Fallback

diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/foreign/CoerceNothing.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/foreign/CoerceNothing.java
@@ -1,19 +1,27 @@
 package org.enso.interpreter.node.expression.foreign;
 
+import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.Fallback;
+import com.oracle.truffle.api.dsl.GenerateUncached;
 import com.oracle.truffle.api.dsl.Specialization;
 import com.oracle.truffle.api.interop.InteropLibrary;
+import com.oracle.truffle.api.interop.UnsupportedMessageException;
 import com.oracle.truffle.api.library.CachedLibrary;
-import org.enso.interpreter.runtime.EnsoContext;
 import com.oracle.truffle.api.nodes.Node;
+import com.oracle.truffle.api.profiles.ConditionProfile;
+import org.enso.interpreter.runtime.EnsoContext;
+import org.enso.interpreter.runtime.error.Warning;
+import org.enso.interpreter.runtime.error.WarningsLibrary;
+import org.enso.interpreter.runtime.error.WithWarnings;
 
+@GenerateUncached
 public abstract class CoerceNothing extends Node {
   public static CoerceNothing build() {
     return CoerceNothingNodeGen.create();
   }
 
   /**
-   * Converts an null polyglot representation into an equivalent Nothing representation in Enso
+   * Converts a null polyglot representation into an equivalent Nothing representation in Enso
    * context.
    *
    * @param value the polyglot value to perform coercion on
@@ -22,8 +30,22 @@ public static CoerceNothing build() {
   public abstract Object execute(Object value);
 
   @Specialization(guards = "interop.isNull(value)")
-  public Object doNothing(Object value, @CachedLibrary(limit = "1") InteropLibrary interop) {
-    return EnsoContext.get(this).getBuiltins().nothing();
+  public Object doNothing(
+      Object value,
+      @CachedLibrary(limit = "1") InteropLibrary interop,
+      @CachedLibrary(limit = "3") WarningsLibrary warningsLibrary,
+      @Cached("createCountingProfile()") ConditionProfile nullWarningProfile) {
+    var nothing = EnsoContext.get(this).getBuiltins().nothing();
+    if (nullWarningProfile.profile(warningsLibrary.hasWarnings(value))) {
+      try {
+        Warning[] attachedWarnings = warningsLibrary.getWarnings(value, null);
+        return new WithWarnings(nothing, attachedWarnings);
+      } catch (UnsupportedMessageException e) {
+        return nothing;
+      }
+    }
+
+    return nothing;
   }
 
   @Fallback

diff --git a/std-bits/base/src/main/java/org/enso/base/Encoding_Utils.java b/std-bits/base/src/main/java/org/enso/base/Encoding_Utils.java
@@ -3,6 +3,7 @@
 import org.enso.base.encoding.ReportingStreamDecoder;
 import org.enso.base.encoding.ReportingStreamEncoder;
 import org.enso.base.text.ResultWithWarnings;
+import org.graalvm.polyglot.Value;
 
 import java.io.IOException;
 import java.io.InputStream;
@@ -167,15 +168,13 @@ private static ReportingStreamDecoder create_stream_decoder(InputStream stream,
    * <p>It returns the result returned from the executed action and any encoding problems that
    * occurred when processing it.
    */
-  public static <R> WithProblems<R, String> with_stream_decoder(
-      InputStream stream, Charset charset, Function<ReportingStreamDecoder, R> action)
+  public static WithProblems<Value, String> with_stream_decoder(
+      InputStream stream, Charset charset, Function<ReportingStreamDecoder, Value> action)
       throws IOException {
-    R result;
+    Value result;
     ReportingStreamDecoder decoder = create_stream_decoder(stream, charset);
-    try {
+    try (decoder) {
       result = action.apply(decoder);
-    } finally {
-      decoder.close();
     }
     return new WithProblems<>(result, decoder.getReportedProblems());
   }
@@ -198,18 +197,16 @@ private static ReportingStreamEncoder create_stream_encoder(
    * <p>It returns the result returned from the executed action and any encoding problems that
    * occurred when processing it.
    */
-  public static <R> WithProblems<R, String> with_stream_encoder(
+  public static WithProblems<Value, String> with_stream_encoder(
       OutputStream stream,
       Charset charset,
       byte[] replacementSequence,
-      Function<ReportingStreamEncoder, R> action)
+      Function<ReportingStreamEncoder, Value> action)
       throws IOException {
-    R result;
+    Value result;
     ReportingStreamEncoder encoder = create_stream_encoder(stream, charset, replacementSequence);
-    try {
+    try (encoder) {
       result = action.apply(encoder);
-    } finally {
-      encoder.close();
     }
     return new WithProblems<>(result, encoder.getReportedProblems());
   }

diff --git a/...urces/enso-test-java-helpers/src/main/java/org/enso/base_test_helpers/CallbackHelper.java b/...urces/enso-test-java-helpers/src/main/java/org/enso/base_test_helpers/CallbackHelper.java
@@ -0,0 +1,11 @@
+package org.enso.base_test_helpers;
+
+import org.graalvm.polyglot.Value;
+
+import java.util.function.Function;
+
+public class CallbackHelper {
+  public static Value runCallbackInt(Function<Integer, Value> callback, int x) {
+    return callback.apply(x);
+  }
+}