Suspended application of default arguments playing badly with warnings and causing weird behaviour

[radeusgd]

Consider the following repro suspending-defaults.enso:

from Standard.Base import all

type My_Table
    Value x

    union self (tables : My_Table | Vector) arg1=1 arg2=2 arg3=3 =
        IO.println "Called ("+self.to_text+").union "+tables.to_text+" with "+arg1.to_text+" "+arg2.to_text+" "+arg3.to_text
        My_Table.Value [self.x, tables, arg1+arg2+arg3]

    columns self =
        "{Columns = "+self.x.to_text+"}"

call_union tables =
    first = tables.first
    rest = tables.drop 1
    first.union rest ...

main =
    check table =
        IO.println table.columns
    check <| call_union [My_Table.Value 1]
    tw = Warning.attach "foo" (My_Table.Value 2)
    check <| call_union [tw]
	check <| call_union [tw] arg2=1000

Actual behaviour

Running the above script yields:

Called ((My_Table.Value 1)).union [] with 1 2 3
{Columns = [1, [], 6]}
Called ((My_Table.Value 2)).union [] with 1 2 3
Called ((My_Table.Value 2)).union [] with 1 2 3
Called ((My_Table.Value 2)).union [] with 1 2 3
Execution finished with an error: Method `columns` of type My_Table could not be found.
        at <enso> suspending-defaults.main.check<arg-1>(suspending-defaults.enso:20:20-32)
        at <enso> suspending-defaults.main.check(suspending-defaults.enso:20:9-32)
        at <enso> suspending-defaults.main(suspending-defaults.enso:23:5-28)

We can see 2 anomalies here:

1. The second call to union is executed 3 times (!) (as indicated by the prints) even though it should only happen once. We have a problem with re-running a side-effecting computation - that should never happen!
2. Afterwards, the return value is somehow broken and we cannot call the columns method on it. Even though in the exactly same scenario above such call has worked all fine.

The difference between first and second calls is only that the second one contains warnings.

Expected behaviour

The program should print

Called ((My_Table.Value 1)).union [] with 1 2 3
{Columns = [1, [], 6]}
Called ((My_Table.Value 2)).union [] with 1 2 3
{Columns = [2, [], 6]}
Called ((My_Table.Value 2)).union [] with 1 1000 3
{Columns = [2, [], 1004]}

[radeusgd]

Of course if I remove the Warning.attach, we do get the expected behaviour. But it should also work the same with warnings attached.

Interestingly also, if I remove the ... then the first 2 calls now work fine. The third one will fail however because we no longer suspend the default arguments, so I could no longer override arg2:

Called ((My_Table.Value 1)).union [] with 1 2 3
{Columns = [1, [], 6]}
Called ((My_Table.Value 2)).union [] with 1 2 3
{Columns = [2, [], 6]}
Called ((My_Table.Value 2)).union [] with 1 2 3
Execution finished with an error: Type error: expected a function, but got My_Table.Value.
        at <enso> suspending-defaults.main<arg-1>(suspending-defaults.enso:24:14-38)
        at <enso> suspending-defaults.main(suspending-defaults.enso:24:5-38)

But this 'proves' that the problematic behaviour is caused by some interaction between both the suspending of default arguments using ... operator and warnings.

[JaroslavTulach]

The columns method is being searched for on (org.enso.interpreter.runtime.callable.function.Function) My_Table.union[e.enso:6-8] self=(My_Table.Value 2) tables=[]

[JaroslavTulach]

Simpler reproducer:

from Standard.Base import all

type Numb
    Value x

    mul self (other: Numb) repeat=1 =
        if repeat <= 0 then self else
            next = Numb.Value self.x*other.x
            @Tail_Call next.mul other repeat-1 

    neg self =
        Numb.Value -self.x

double n = n.mul n ...

main =
    tw = Warning.attach "foo" (Numb.Value 2)
    double tw . neg

[JaroslavTulach]

Change that fixes the new reproducer as well as the original one:

diff --git engine/runtime/src/main/java/org/enso/interpreter/node/callable/thunk/ThunkExecutorNode.java engine/runtime/src/main/java/org/enso/interpreter/node/callable/thunk/ThunkExecutorNode.java
index d0e271455b..dacae60ce3 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/callable/thunk/ThunkExecutorNode.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/callable/thunk/ThunkExecutorNode.java
@@ -1,9 +1,16 @@
 package org.enso.interpreter.node.callable.thunk;
 
 import com.oracle.truffle.api.CompilerAsserts;
-import com.oracle.truffle.api.dsl.*;
+import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.Cached.Exclusive;
+import com.oracle.truffle.api.dsl.Fallback;
+import com.oracle.truffle.api.dsl.GenerateUncached;
+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.frame.VirtualFrame;
+import com.oracle.truffle.api.interop.UnsupportedMessageException;
+import com.oracle.truffle.api.library.CachedLibrary;
 import com.oracle.truffle.api.nodes.DirectCallNode;
 import com.oracle.truffle.api.nodes.IndirectCallNode;
 import com.oracle.truffle.api.nodes.Node;
@@ -17,6 +24,8 @@ import org.enso.interpreter.runtime.callable.argument.CallArgumentInfo;
 import org.enso.interpreter.runtime.callable.function.Function;
 import org.enso.interpreter.runtime.control.TailCallException;
 import org.enso.interpreter.runtime.state.State;
+import org.enso.interpreter.runtime.warning.AppendWarningNode;
+import org.enso.interpreter.runtime.warning.WarningsLibrary;
 
 /** Node responsible for executing (forcing) thunks passed to it as runtime values. */
 @GenerateUncached
@@ -146,6 +155,26 @@ public abstract class ThunkExecutorNode extends Node {
         isTail);
   }
 
+  @Specialization(
+      guards = {"warnings.hasWarnings(fn)"},
+      limit = "3")
+  Object doWarnings(
+      VirtualFrame frame,
+      Object fn,
+      State state,
+      BaseNode.TailStatus isTail,
+      @CachedLibrary("fn") WarningsLibrary warnings,
+      @Cached ThunkExecutorNode delegate,
+      @Cached AppendWarningNode append) {
+    try {
+      var raw = warnings.removeWarnings(fn);
+      var res = delegate.executeThunk(frame, raw, state, isTail);
+      return res;
+    } catch (UnsupportedMessageException e) {
+      return fn;
+    }
+  }
+
   @Fallback
   Object doOther(Object thunk, State state, BaseNode.TailStatus isTail) {
     return thunk;
diff --git test/Base_Tests/src/Semantic/Warnings_Spec.enso test/Base_Tests/src/Semantic/Warnings_Spec.enso
index f16f62bb2c..a78bf02b97 100644
--- test/Base_Tests/src/Semantic/Warnings_Spec.enso
+++ test/Base_Tests/src/Semantic/Warnings_Spec.enso
@@ -22,6 +22,20 @@ type Wrap
 
     materialize v -> Wrap = v
 
+type Numb
+    Value x
+
+    mul self (other: Numb) repeat=1 =
+        if repeat <= 0 then self else
+            next = Numb.Value self.x*other.x
+            @Tail_Call next.mul other repeat-1 
+
+    neg self =
+        Numb.Value -self.x
+
+    double n = n.mul n ...
+
+
 f x _ = Warning.attach "Baz!" x
 
 type My_Fancy_Collection
@@ -472,6 +486,13 @@ add_specs suite_builder = suite_builder.group "Dataflow Warnings" group_builder-
         wrap.foo . should_equal 3
         Warning.get_all wrap.foo . should_equal [ "warn!!!" ]
 
+    group_builder.specify "warning & suspended return" <|
+        tw = Warning.attach "pozor!" (Numb.Value 2)
+        res = Numb.double tw . neg
+
+        Warning.get_all res . should_equal [ "pozor!" ]
+        res.x . should_equal 4
+
 main filter=Nothing =
     suite = Test.build suite_builder->
         add_specs suite_builder

alas, the change breaks Warnings_Spec.enso.