Fix Dataflow Error propagation for Builtins accepting primitives (#3400)

[ci no changelog needed]

Fixes https://www.pivotaltracker.com/story/show/181652841

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/special/JoinThreadNode.java

@@ -5,9 +5,9 @@
 
 @BuiltinMethod(type = "Special", name = "<join_thread>")
 public class JoinThreadNode extends Node {
-  public Object execute(Object thread) {
+  public Object execute(Object _this) {
     try {
-      ((Thread) thread).join();
+      ((Thread) _this).join();
     } catch (InterruptedException e) {
     }
     return null;

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/special/ReadRefNode.java

@@ -6,7 +6,7 @@
 
 @BuiltinMethod(type = "Special", name = "<read_ref>")
 public class ReadRefNode extends Node {
-  public Object execute(Ref ref) {
-    return ref.getValue();
+  public Object execute(Ref _this) {
+    return _this.getValue();
   }
 }

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/special/RunThreadNode.java

@@ -16,11 +16,11 @@ static RunThreadNode build() {
     return RunThreadNodeGen.create();
   }
 
-  abstract Thread execute(@MonadicState Object state, @Suspend Object th);
+  abstract Thread execute(@MonadicState Object state, @Suspend Object _this);
 
   @CompilerDirectives.TruffleBoundary
   @Specialization
-  Thread doExecute(Object state, Object th) {
+  Thread doExecute(Object state, Object _this) {
     Context ctx = Context.get(this);
     Thread thread =
         ctx.getEnvironment()
@@ -29,7 +29,7 @@ Thread doExecute(Object state, Object th) {
                   Object p = ctx.getThreadManager().enter();
                   try {
                     ThunkExecutorNodeGen.getUncached()
-                        .executeThunk(th, state, BaseNode.TailStatus.NOT_TAIL);
+                        .executeThunk(_this, state, BaseNode.TailStatus.NOT_TAIL);
                   } finally {
                     ctx.getThreadManager().leave(p);
                   }

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/special/WriteRefNode.java

@@ -6,8 +6,8 @@
 
 @BuiltinMethod(type = "Special", name = "<write_ref>")
 public class WriteRefNode extends Node {
-  public Object execute(Ref ref, Object value) {
-    ref.setValue(value);
+  public Object execute(Ref _this, Object value) {
+    _this.setValue(value);
     return null;
   }
 }

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/MethodProcessor.java

@@ -1,18 +1,30 @@
 package org.enso.interpreter.dsl;
 
-import org.apache.commons.lang3.StringUtils;
-import org.enso.interpreter.dsl.model.MethodDefinition;
-
-import javax.annotation.processing.*;
-import javax.lang.model.SourceVersion;
-import javax.lang.model.element.*;
-import javax.tools.Diagnostic;
-import javax.tools.JavaFileObject;
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.io.Writer;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
 import java.util.stream.Collectors;
+import javax.annotation.processing.Filer;
+import javax.annotation.processing.Processor;
+import javax.annotation.processing.RoundEnvironment;
+import javax.annotation.processing.SupportedAnnotationTypes;
+import javax.lang.model.SourceVersion;
+import javax.lang.model.element.Element;
+import javax.lang.model.element.ElementKind;
+import javax.lang.model.element.ExecutableElement;
+import javax.lang.model.element.Name;
+import javax.lang.model.element.TypeElement;
+import javax.tools.Diagnostic;
+import javax.tools.JavaFileObject;
+import org.enso.interpreter.dsl.model.MethodDefinition;
+import org.enso.interpreter.dsl.model.MethodDefinition.ArgumentDefinition;
 import org.openide.util.lookup.ServiceProvider;
 
 /**
@@ -149,19 +161,23 @@ private void generateCode(MethodDefinition methodDefinition) throws IOException
       out.println();
 
       for (MethodDefinition.ArgumentDefinition arg : methodDefinition.getArguments()) {
-        if (!arg.isState() && !arg.isFrame() && !arg.isCallerInfo()) {
-          String condName = mkArgumentInternalVarName(arg) + "ConditionProfile";
-          String branchName = mkArgumentInternalVarName(arg) + "BranchProfile";
+        if (arg.shouldCheckErrors()) {
+          String condName = mkArgumentInternalVarName(arg) + DATAFLOW_ERROR_PROFILE;
           out.println(
               "  private final ConditionProfile "
                   + condName
                   + " = ConditionProfile.createCountingProfile();");
+        }
+
+        if (arg.isPositional() && !arg.isThis()) {
+          String branchName = mkArgumentInternalVarName(arg) + PANIC_SENTINEL_PROFILE;
           out.println("  private final BranchProfile " + branchName + " = BranchProfile.create();");
-          if (!arg.isThis() && !arg.acceptsWarning()) {
-            String warningName = mkArgumentInternalVarName(arg) + "WarningProfile";
-            out.println(
-                "  private final BranchProfile " + warningName + " = BranchProfile.create();");
-          }
+        }
+
+        if (arg.shouldCheckWarnings()) {
+          String warningName = mkArgumentInternalVarName(arg) + WARNING_PROFILE;
+          out.println(
+              "  private final BranchProfile " + warningName + " = BranchProfile.create();");
         }
       }
       out.println("  private final BranchProfile anyWarningsProfile = BranchProfile.create();");
@@ -295,42 +311,41 @@ private void generateCode(MethodDefinition methodDefinition) throws IOException
 
   private void generateArgumentRead(
       PrintWriter out, MethodDefinition.ArgumentDefinition arg, String argsArray) {
-    if (!arg.requiresCast()) {
-      generateUncastedArgumentRead(out, arg, argsArray);
-    } else if (arg.getName().equals("this") && arg.getPosition() == 0) {
-      generateUncheckedArgumentRead(out, arg, argsArray);
-    } else {
-      generateCheckedArgumentRead(out, arg, argsArray);
-    }
-
-    if (!arg.acceptsError()) {
-
-      String varName = mkArgumentInternalVarName(arg);
-      String condProfile = mkArgumentInternalVarName(arg) + "ConditionProfile";
+    String argReference = argsArray + "[" + arg.getPosition() + "]";
+    if (arg.shouldCheckErrors()) {
+      String condProfile = mkArgumentInternalVarName(arg) + DATAFLOW_ERROR_PROFILE;
       out.println(
           "    if ("
               + condProfile
               + ".profile(TypesGen.isDataflowError("
-              + varName
+              + argReference
               + "))) {\n"
               + "      return new Stateful(state, "
-              + varName
+              + argReference
+              + ");\n"
+              + "    }");
+    }
+    if (!arg.isThis()) {
+      String branchProfile = mkArgumentInternalVarName(arg) + PANIC_SENTINEL_PROFILE;
+      out.println(
+          "    if (TypesGen.isPanicSentinel("
+              + argReference
+              + ")) {\n"
+              + "      "
+              + branchProfile
+              + ".enter();\n"
+              + "      throw TypesGen.asPanicSentinel("
+              + argReference
               + ");\n"
               + "    }");
-      if (!(arg.getName().equals("this") && arg.getPosition() == 0)) {
-        String branchProfile = mkArgumentInternalVarName(arg) + "BranchProfile";
-        out.println(
-            "    else if (TypesGen.isPanicSentinel("
-                + varName
-                + ")) {\n"
-                + "      "
-                + branchProfile
-                + ".enter();\n"
-                + "      throw TypesGen.asPanicSentinel("
-                + varName
-                + ");\n"
-                + "    }");
-      }
+    }
+
+    if (!arg.requiresCast()) {
+      generateUncastedArgumentRead(out, arg, argsArray);
+    } else if (arg.isThis()) {
+      generateUncheckedArgumentRead(out, arg, argsArray);
+    } else {
+      generateCheckedArgumentRead(out, arg, argsArray);
     }
   }
 
@@ -395,7 +410,7 @@ private boolean generateWarningsCheck(
       PrintWriter out, List<MethodDefinition.ArgumentDefinition> arguments, String argumentsArray) {
     List<MethodDefinition.ArgumentDefinition> argsToCheck =
         arguments.stream()
-            .filter(arg -> !arg.acceptsWarning() && !arg.isThis())
+            .filter(ArgumentDefinition::shouldCheckWarnings)
             .collect(Collectors.toList());
     if (argsToCheck.isEmpty()) {
       return false;
@@ -407,7 +422,7 @@ private boolean generateWarningsCheck(
             "    if ("
                 + arrayRead(argumentsArray, arg.getPosition())
                 + " instanceof WithWarnings) {");
-        out.println("      " + mkArgumentInternalVarName(arg) + "WarningProfile.enter();");
+        out.println("      " + mkArgumentInternalVarName(arg) + WARNING_PROFILE + ".enter();");
         out.println("      anyWarnings = true;");
         out.println(
             "      WithWarnings withWarnings = (WithWarnings) "
@@ -506,4 +521,8 @@ protected MethodMetadataEntry toMetadataEntry(String line) {
     if (elements.length != 2) throw new RuntimeException("invalid builtin metadata entry: " + line);
     return new MethodMetadataEntry(elements[0], elements[1]);
   }
+
+  private static final String DATAFLOW_ERROR_PROFILE = "IsDataflowErrorConditionProfile";
+  private static final String PANIC_SENTINEL_PROFILE = "PanicSentinelBranchProfile";
+  private static final String WARNING_PROFILE = "WarningProfile";
 }

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/model/MethodDefinition.java

@@ -198,6 +198,7 @@ public static class ArgumentDefinition {
     private static final String THUNK = "org.enso.interpreter.runtime.callable.argument.Thunk";
     private static final String CALLER_INFO = "org.enso.interpreter.runtime.callable.CallerInfo";
     private static final String DATAFLOW_ERROR = "org.enso.interpreter.runtime.error.DataflowError";
+    private static final String THIS = "this";
     private final String typeName;
     private final TypeMirror type;
     private final String name;
@@ -222,7 +223,7 @@ public ArgumentDefinition(VariableElement element, int position) {
       String[] typeNameSegments = type.toString().split("\\.");
       typeName = typeNameSegments[typeNameSegments.length - 1];
       String originalName = element.getSimpleName().toString();
-      name = originalName.equals("_this") ? "this" : originalName;
+      name = originalName.equals("_this") ? THIS : originalName;
       isState = element.getAnnotation(MonadicState.class) != null && type.toString().equals(OBJECT);
       isSuspended = element.getAnnotation(Suspend.class) != null;
       acceptsError =
@@ -244,6 +245,27 @@ public boolean validate(ProcessingEnvironment processingEnvironment) {
                 element);
         return false;
       }
+
+      if (isThis() && position != 0) {
+        processingEnvironment
+            .getMessager()
+            .printMessage(
+                Diagnostic.Kind.ERROR,
+                "Argument `_this` must be the first positional argument.",
+                element);
+        return false;
+      }
+
+      if (isPositional() && position == 0 && !isThis()) {
+        processingEnvironment
+            .getMessager()
+            .printMessage(
+                Diagnostic.Kind.ERROR,
+                "The first positional argument should be called `_this`.",
+                element);
+        return false;
+      }
+
       return true;
     }
 
@@ -317,7 +339,15 @@ public boolean acceptsWarning() {
     }
 
     public boolean isThis() {
-      return name.equals("this") || position == 0;
+      return name.equals(THIS);
+    }
+
+    public boolean shouldCheckErrors() {
+      return isPositional() && !isThis() && !acceptsError();
+    }
+
+    public boolean shouldCheckWarnings() {
+      return isPositional() && !isThis() && !acceptsWarning();
     }
   }
 }

test/Tests/src/Data/Array_Spec.enso

@@ -12,18 +12,18 @@ spec = Test.group "Arrays" <|
         as_vec = json.into (Vector.Vector Number)
         as_vec.should_equal <| Vector.fill 100 0
 
-    Test.specify "should allow accessing elements"
+    Test.specify "should allow accessing elements" <|
         arr = [1, 2, 3] . to_array
         arr.at 0 . should_equal 1
         arr.at 2 . should_equal 3
 
-    Test.specify "should allow setting elements"
+    Test.specify "should allow setting elements" <|
         arr = [1, 2, 3] . to_array
         arr.set_at 1 10
         arr.at 1 . should_equal 10
         Vector.from_array arr . should_equal [1, 10, 3]
 
-    Test.specify "should panic on out of bounds access"
+    Test.specify "should panic on out of bounds access" <|
         arr = [1, 2, 3] . to_array
         Test.expect_panic_with (arr.at -1) Invalid_Array_Index_Error
         Test.expect_panic_with (arr.at 3) Invalid_Array_Index_Error
@@ -33,4 +33,9 @@ spec = Test.group "Arrays" <|
         arr = [1, 2, 3] . to_array
         arr.method . should_equal 0
 
+    Test.specify "should propagate dataflow errors" <|
+        err = Error.throw (Illegal_State_Error "Foo")
+        res = Array.new err
+        res . should_fail_with Illegal_State_Error
+
 main = Test.Suite.run_main here.spec