Wrong error recovery: bind all things on the right side of unclosed (

[radeusgd]

TL;DR

In the expression foo.rename column (Map.from_vector [["Value1", "Attack Damage"]] the rename_column method gets actually two arguments:

• (Map.from_vector
• [["Value1", "Attack Damage"]]

Usages of the first argument inside rename_column propagate the panic properly; but before it's returned, we try to call if_then_else on the second (which is Vector, not Boolean) what results in a new Panic, overriding the old one.

So, what we can do?

• Make parser bind all things on the right side of unclosed (, so the (Map.from_vector [["Value1", "Attack Damage"]] will be one argument, not two.

Original Report

One example repro is to create a table by loading a CSV or Table.new [["X", [1,2,3]]] and then calling:

table.rename_columns (Map.from_vector [["X", "Y"]]

(note the missing ) here).

What I get in the REPL is a slightly cryptic, but helpful syntax error:

<interactive_source>[1:18-1:18]: Unexpected expression.

But somehow, in the IDE I get results like:

in the preview:

or after submitting with Ctrl+Enter:

[radeusgd]

Maybe related?

A node with an unclosed string literal is happily accepted as a string value. I guess resiliency is nice, but in this case it should just be a syntax error.

[Akirathan]

Attaching logs from LS. I can see there is a message from the LS:

{
    "jsonrpc": "2.0",
    "method": "executionContext/executionStatus",
    "params": {
        "contextId": "c3efe72c-adec-4d65-a814-8161b983f3f2",
        "diagnostics": [
            {
                "kind": "Error",
                "message": "Unexpected expression.",
                "path": {
                    "rootId": "f80686e2-27f0-4417-8104-2c89df908fd6",
                    "segments": [
                        "src",
                        "Main.enso"
                    ]
                },
                "location": {
                    "start": {
                        "line": 10,
                        "character": 29
                    },
                    "end": {
                        "line": 10,
                        "character": 30
                    }
                },
                "expressionId": "27cba2e4-3e06-4970-8cfa-d3155a037c4c",
                "stack": []
            }
        ]
    }
}

So LS sends correct diagnostics.
20230519-193135-801-enso-project-manager.log.zip

I am not certain how this message is handled by the IDE? cc @farmaazon

[farmaazon]

We don't read execution Status (yet). The error on nodes are taken from the payload field of expression update. We don't do any processing, so likely just LS sends them to us in such shape.

(the first error in probably due to previewing the selected suggestion).

[farmaazon]

Indeed, we receive it in the Payload. Moreover, I checked another cases. When I create a node with just a map constructor, I got the proper result:

Connecting it to rename_columns propagated the error:

Only when I copied the expression, I got the error as @radeusgd did:

Now, I have a good theory, and I'm almost sure I am right:

In the expression foo.rename column (Map.from_vector [["Value1", "Attack Damage"]] the rename_column method gets actually two arguments:

1. (Map.from_vector
2. [["Value1", "Attack Damage"]]
   Usages of the first argument inside rename_column propagate the panic properly; but before it's returned, we try to call if_then_else on the second (which is vector, not Bool) what results in a new Panic, overriding the old one.

So, what we can do?

1. Treat is at WAD and close the issue
2. Make parser bind all things on the right side of unclosed (, so the (Map.from_vector [["Value1", "Attack Damage"]] will be one argument, not two.
3. Make the interpreter more eagerly propagate panics, or prefer panics got in the input over panics raised during evaluation.
4. rewrite rename_column so it somehow will have better error messages.

Whatever the answer is, GUI team cannot provide it. Putting back to triage.

[kazcw]

Make parser bind all things on the right side of unclosed (

I think we should do this. It is better to assume the expression is incomplete than incorrect.

[JaroslavTulach]

We'd like following test:

enso$ git diff engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java
diff --git engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java
index 08f424e355..97711770eb 100644
--- engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java
+++ engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java
@@ -2,13 +2,16 @@ package org.enso.compiler;
 
 import java.io.OutputStream;
 import java.nio.file.Paths;
+
 import org.enso.polyglot.RuntimeOptions;
 import org.graalvm.polyglot.Context;
 import org.graalvm.polyglot.PolyglotException;
 import org.junit.AfterClass;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
 import org.junit.BeforeClass;
 import org.junit.Test;
-import static org.junit.Assert.*;
 
 public class ExecCompilerTest {
   private static Context ctx;
@@ -82,4 +85,26 @@ public class ExecCompilerTest {
     var err = run.execute(0);
     assertEquals("Error: Module is not a part of a package.", err.asString());
   }
+
+  @Test
+  public void testExecuteWithError() throws Exception {
+    var module =
+        ctx.eval(
+            "enso",
+            """
+            polyglot java import java.lang.System
+
+            rename_column ~x =
     var err = run.execute(0);
     assertEquals("Error: Module is not a part of a package.", err.asString());
   }
+
+  @Test
+  public void testExecuteWithError() throws Exception {
+    var module =
+        ctx.eval(
+            "enso",
+            """
+            polyglot java import java.lang.System
+
+            rename_column ~x =
+              System.out.println "in rename_column"
+              x
+            from_vector ~v =
+              System.out.println "in from_vector"
+              v
+
+            a x y = rename_column (from_vector [[x, y]]
+        """);
+    var run = module.invokeMember("eval_expression", "a");
+    var err = run.execute("Hi", "There");
+    assertEquals("Got two values", 2, err.getArraySize());
+  }
 }

to print in from_vector as well as in rename_column. Right now it prints just the latter text.

[JaroslavTulach]

When I expand the ErrorCompilerTest with

enso$ git diff engine/runtime/src/test/java/org/enso/compiler/ErrorCompilerTest.java
diff --git engine/runtime/src/test/java/org/enso/compiler/ErrorCompilerTest.java engine/runtime/src/test/java/org/enso/compiler/ErrorCompilerTest.java
index d4c0fc2300..b98a49407a 100644
--- engine/runtime/src/test/java/org/enso/compiler/ErrorCompilerTest.java
+++ engine/runtime/src/test/java/org/enso/compiler/ErrorCompilerTest.java
@@ -358,6 +357,14 @@ public class ErrorCompilerTest extends CompilerTest {
     assertSingleSyntaxError(ir, IR$Error$Syntax$UnexpectedExpression$.MODULE$, "Unexpected expression", 60, 62);
   }
 
+  @Test
+  public void testUnclosedBracket() throws Exception {
+    var ir = parse("""
+    a x y = rename_column (from_vector [[x, y]]
+    """);
+    assertSingleSyntaxError(ir, IR$Error$Syntax$UnexpectedExpression$.MODULE$, "Unexpected expression", 60, 62);
+  }
+
   @Test
   public void testNPE183863754() throws Exception {
     var ir = parse("""

I can see that I get following Tree:

"App[" ", "(from_vector", 
Invalid["", "(", Invalid macro invocation., 
Invalid["", "(", Unmatched delimiter, 
Group["", "(", OpenSymbol["", "(", ], null, null]]], Ident["", "from_vector", 
Ident["", "from_vector", false, 0, false, false]]]"

I don't think TreeToIr.java shall try to understand this kind of Tree in a special way. The Tree shall contain App[Group[App[Ident["from_vector", ..., Invalid["", "(", Unmatched delimiter..., right @kazcw?