From 4dcf802831bea716c26b7870f056b43d19744a12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Wa=C5=9Bko?= Date: Fri, 17 Feb 2023 14:38:26 +0100 Subject: [PATCH] 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. --- build.sbt | 81 ++++++++++++------- .../Base/0.0.0-dev/src/System/File.enso | 15 +--- .../interop/syntax/HostValueToEnsoNode.java | 12 ++- .../expression/foreign/CoerceNothing.java | 30 ++++++- .../java/org/enso/base/Encoding_Utils.java | 21 +++-- .../base_test_helpers/CallbackHelper.java | 11 +++ test/Tests/src/Semantic/Warnings_Spec.enso | 52 ++++++++++++ 7 files changed, 159 insertions(+), 63 deletions(-) create mode 100644 test/Tests/polyglot-sources/enso-test-java-helpers/src/main/java/org/enso/base_test_helpers/CallbackHelper.java diff --git a/build.sbt b/build.sbt index f39fc7ec0f96..c07fda75a817 100644 --- 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 index 48a01204c333..1ab164dc6113 100644 --- 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/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/interop/syntax/HostValueToEnsoNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/interop/syntax/HostValueToEnsoNode.java index a4141f086160..5aedc038af4f 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/interop/syntax/HostValueToEnsoNode.java +++ b/engine/runtime/src/main/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 index 4c1df034898b..7eb40fc06dd7 100644 --- 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 index 88096de2b10d..5a9d69293ee5 100644 --- 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, *

It returns the result returned from the executed action and any encoding problems that * occurred when processing it. */ - public static WithProblems with_stream_decoder( - InputStream stream, Charset charset, Function action) + public static WithProblems with_stream_decoder( + InputStream stream, Charset charset, Function 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( *

It returns the result returned from the executed action and any encoding problems that * occurred when processing it. */ - public static WithProblems with_stream_encoder( + public static WithProblems with_stream_encoder( OutputStream stream, Charset charset, byte[] replacementSequence, - Function action) + Function 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/test/Tests/polyglot-sources/enso-test-java-helpers/src/main/java/org/enso/base_test_helpers/CallbackHelper.java b/test/Tests/polyglot-sources/enso-test-java-helpers/src/main/java/org/enso/base_test_helpers/CallbackHelper.java new file mode 100644 index 000000000000..50b9cb48e338 --- /dev/null +++ b/test/Tests/polyglot-sources/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 callback, int x) { + return callback.apply(x); + } +} diff --git a/test/Tests/src/Semantic/Warnings_Spec.enso b/test/Tests/src/Semantic/Warnings_Spec.enso index 1332e4d59eaa..3d5cdb28b17a 100644 --- a/test/Tests/src/Semantic/Warnings_Spec.enso +++ b/test/Tests/src/Semantic/Warnings_Spec.enso @@ -1,6 +1,8 @@ from Standard.Base import all polyglot java import java.lang.Long +polyglot java import java.util.function.Function as Java_Function +polyglot java import org.enso.base_test_helpers.CallbackHelper from Standard.Test import Test, Test_Suite import Standard.Test.Extensions @@ -246,4 +248,54 @@ spec = Test.group "Dataflow Warnings" <| Warning.get_all a . map .value . should_contain_the_same_elements_as ["a"] Warning.get_all b . map .value . should_contain_the_same_elements_as ["b"] + Test.specify "should be preserved around polyglot calls" <| + x = Warning.attach "x" 1 + + java_id = Java_Function.identity + f x = Warning.attach "f("+x.to_text+")" <| Pair.new "A" x+10 + ## We compose our Enso functions with Java identity, forcing our methods + to be lifted into the Java polyglot world and being used as callbacks + from within Java. + javaized_f = Java_Function.identity.andThen f + + r1 = java_id.apply x + r1.should_equal 1 + Warning.get_all r1 . map .value . should_contain_the_same_elements_as ["x"] + + r2 = javaized_f.apply x + r2.should_equal (Pair.new "A" 11) + Warning.get_all r2 . map .value . should_contain_the_same_elements_as ["f(1)", "x"] + + ## The following will not work, as if the polyglot method expects an + `Object` it will get converted to a Java 'primitive' losing the + attached warnings. The only way to preserve warnings in that case is + to explicitly expect a `Value` return type, as in the test below. + #g x = Warning.attach "g("+x.to_text+")" x+10 + #h x = Warning.attach "h("+x.to_text+")" "{x="+x.to_text+"}" + #i x = Warning.attach "i("+x.to_text+")" Nothing + + Test.specify "should be better preserved around polyglot calls expecting a Value" <| + x = Warning.attach "x" 1 + + f x = Warning.attach "f("+x.to_text+")" <| Pair.new "A" x+10 + g x = Warning.attach "g("+x.to_text+")" x+10 + h x = Warning.attach "h("+x.to_text+")" "{x="+x.to_text+"}" + i x = Warning.attach "i("+x.to_text+")" Nothing + + r1 = CallbackHelper.runCallbackInt f x + r1.should_equal (Pair.new "A" 11) + Warning.get_all r1 . map .value . should_contain_the_same_elements_as ["f(1)", "x"] + + r2 = CallbackHelper.runCallbackInt g x + r2.should_equal 11 + Warning.get_all r2 . map .value . should_contain_the_same_elements_as ["g(1)", "x"] + + r3 = CallbackHelper.runCallbackInt h x + r3.should_equal "{x=1}" + Warning.get_all r3 . map .value . should_contain_the_same_elements_as ["h(1)", "x"] + + r4 = CallbackHelper.runCallbackInt i x + r4.should_equal Nothing + Warning.get_all r4 . map .value . should_contain_the_same_elements_as ["i(1)", "x"] + main = Test_Suite.run_main spec