Fix matching JS strings

[radeusgd]

Pull Request Description

• Fixes case of fails to match a constant string against a string coming from JS #9202

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

• The documentation has been updated, if necessary.
• Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
• All code follows the
  Scala,
  Java,
  and
  Rust
  style guides. In case you are using a language not listed above, follow the Rust style guide.
• All code has been tested:
  • Unit tests have been written where possible.
  • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

[radeusgd]

I'm not exactly sure if this specialization is needed.

I added it as I imagine it would cover a 'fast-path' for Java String. The doInteropString should also cover that, but I imagine that asString may be more expensive.

OTOH, even without the fix - I could not find a failure scenario with a Java String (do we convert them to Enso Text more eagerly than JS??), so it feels like it may have been already handled by doText.

So I'm not sure if this specialization is useful at all. @JaroslavTulach do you think I should remove or keep it?

[hubertp]

HostValueToEnsoNode?

[hubertp]

I think this one is spurious/redundant but wait on @JaroslavTulach 's opinion

[JaroslavTulach]

Updated:

Yes, all string like objects should be wrapped into Text in Enso. E.g. the bug/fix should really be in HostValueToEnsoNode

Looks like HostValueToEnsoNode does a conversion of String to Text, but there is no such conversion for TruffleObject. Moreover there probably shouldn't be any such conversion - TruffleObject instances are supposed to flow thru the interpreters unchanged (as the Truffle TCK checks)...

[radeusgd]

So I should remove the doJavaString branch, but after all I should keep the doInteropString branch, as the HostValueToEnsoNode is already doing everything it can do - and we just have to keep in mind that we must not only accept Text but also isString interop objects.

Is my understanding correct @JaroslavTulach ?

[JaroslavTulach]

HostValueToEnsoNode is already doing everything it can do

It does, when it gets called. Which it apparently didn't until these changes.

we must not only accept Text but also isString interop objects.

Probably we should rewrite ToJavaStringNode to accept not only Text, but also isString interop objects.

should keep the doInteropString branch,

Yes, very likely we should keep it.

[radeusgd]

Probably we should rewrite ToJavaStringNode to accept not only Text, but also isString interop objects.

But ToJavaStringNode is described as "Converts Enso Text to a Java String.".

We probably will still keep separate execution paths for Text type and isString, so I'm not sure if we should be modifying ToJavaStringNode. At least it does not seem needed for this PR, if I understand things correctly.

[radeusgd]

Before adding the fix, the JS test would fail:

Afterwards, it is succeeding.

[hubertp]

HostValueToEnsoNode?

[hubertp]

I think this one is spurious/redundant but wait on @JaroslavTulach 's opinion

[JaroslavTulach]

I believe this is the correct fix. We should remove CoerceNothing and use :

diff --git engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/interop/syntax/HostValueToEnsoNode.java engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/interop/syntax/HostValueToEnsoNode.java
index 76fdc55715..c59b2e90e8 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/interop/syntax/HostValueToEnsoNode.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/interop/syntax/HostValueToEnsoNode.java
@@ -7,10 +7,14 @@ import com.oracle.truffle.api.dsl.NeverDefault;
 import com.oracle.truffle.api.dsl.ReportPolymorphism;
 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 com.oracle.truffle.api.nodes.Node;
-import org.enso.interpreter.node.expression.foreign.CoerceNothing;
+import com.oracle.truffle.api.profiles.CountingConditionProfile;
+import org.enso.interpreter.runtime.EnsoContext;
 import org.enso.interpreter.runtime.data.text.Text;
+import org.enso.interpreter.runtime.error.WarningsLibrary;
+import org.enso.interpreter.runtime.error.WithWarnings;
 
 /**
  * Converts a value returned by a polyglot call back to a value that can be further used within Enso
@@ -66,12 +70,23 @@ public abstract class HostValueToEnsoNode extends Node {
     return Text.create(txt);
   }
 
-  @Specialization(guards = {"o != null", "iop.isNull(o)"})
+  @Specialization(guards = {"value != null", "iop.isNull(value)"})
   Object doNull(
-      Object o,
+      Object value,
       @CachedLibrary(limit = "3") InteropLibrary iop,
-      @Cached CoerceNothing coerceNothing) {
-    return coerceNothing.execute(o);
+      @CachedLibrary(limit = "3") WarningsLibrary warningsLibrary,
+      @Cached CountingConditionProfile nullWarningProfile) {
+    var ctx = EnsoContext.get(this);
+    var nothing = ctx.getBuiltins().nothing();
+    if (nothing != value && nullWarningProfile.profile(warningsLibrary.hasWarnings(value))) {
+      try {
+        var attachedWarnings = warningsLibrary.getWarnings(value, null, false);
+        return WithWarnings.wrap(ctx, nothing, attachedWarnings);
+      } catch (UnsupportedMessageException e) {
+        return nothing;
+      }
+    }
+    return nothing;
   }
 
   @Fallback
diff --git engine/runtime/src/main/java/org/enso/interpreter/node/expression/foreign/ForeignMethodCallNode.java engine/runtime/src/main/java/org/enso/interpreter/node/expression/foreign/ForeignMethodCallNode.java
index 048c145206..c1a34913ad 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/expression/foreign/ForeignMethodCallNode.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/expression/foreign/ForeignMethodCallNode.java
@@ -6,19 +6,20 @@ import com.oracle.truffle.api.nodes.DirectCallNode;
 import com.oracle.truffle.api.nodes.ExplodeLoop;
 import com.oracle.truffle.api.profiles.BranchProfile;
 import org.enso.interpreter.node.ExpressionNode;
+import org.enso.interpreter.node.expression.builtin.interop.syntax.HostValueToEnsoNode;
 import org.enso.interpreter.runtime.error.DataflowError;
 
 /** Performs a call into a given foreign call target. */
 public class ForeignMethodCallNode extends ExpressionNode {
   private @Children ExpressionNode[] arguments;
   private @Child DirectCallNode callNode;
-  private @Child CoerceNothing coerceNothingNode;
+  private @Child HostValueToEnsoNode coerceNode;
   private final BranchProfile[] errorProfiles;
 
   ForeignMethodCallNode(ExpressionNode[] arguments, CallTarget foreignCt) {
     this.arguments = arguments;
     this.callNode = DirectCallNode.create(foreignCt);
-    this.coerceNothingNode = CoerceNothing.build();
+    this.coerceNode = HostValueToEnsoNode.build();
 
     this.errorProfiles = new BranchProfile[arguments.length];
     for (int i = 0; i < arguments.length; i++) {
@@ -48,6 +49,6 @@ public class ForeignMethodCallNode extends ExpressionNode {
         return args[i];
       }
     }
-    return coerceNothingNode.execute(callNode.call(args));
+    return coerceNode.execute(callNode.call(args));
   }
 }

for some reason ForeignMethodCallNode wasn't doing HostValueToEnsoNode conversions. It is surprising Enso even worked!

[JaroslavTulach]

Please remove CoerceNothing node and apply this diff.