enso-org / enso

Hybrid visual and textual functional programming.
https://ensoanalytics.com
Apache License 2.0
7.36k stars 324 forks source link

Instrumentor allows executing expressions in local context #8293

Closed 4e6 closed 11 months ago

4e6 commented 11 months ago

Extracted from the discussion comment https://github.com/enso-org/enso/pull/8148#discussion_r1390337139

The onReturn method of the Instrumentor should contain an extra expression argument

    on_return self (fn : Text -> Any -> Nothing) (expression : Text | Nothing = Nothing) 

that will be evaluated in the context of the intercepted node.

JaroslavTulach commented 11 months ago

Direct & naive implementation is ready:

diff --git distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso
index b8cdf46d29..feca506f80 100644
--- distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso
+++ distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso
@@ -622,8 +622,10 @@ type Instrumentor

        Arguments:
        - fn: The callback function accepting UUID and computed value
-    on_return self (fn : Text -> Any -> Nothing) =
-        new = instrumentor_builtin "onReturn" [ self.impl, fn ]
+       - expression: Expression to evaluate on_return - by default Nothing -
+         e.g. to provide the return value of the function
+    on_return self (fn : Text -> Any -> Nothing) (expression : Text|Nothing)=Nothing =
+        new = instrumentor_builtin "onReturn" [ self.impl, fn, expression ]
         Instrumentor.Value new

     ## PRIVATE
diff --git engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/Instrumentor.java engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/Instrumentor.java
index 007a9e1d75..6a00c4f100 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/Instrumentor.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/Instrumentor.java
@@ -21,6 +21,7 @@ final class Instrumentor implements EnsoObject, IdExecutionService.Callbacks {
   private final Module module;
   private final Object onEnter;
   private final Object onReturn;
+  private final Object onReturnExpr;
   private final Object onCall;
   private final EventBinding<?> handle;

@@ -30,16 +31,18 @@ final class Instrumentor implements EnsoObject, IdExecutionService.Callbacks {
     this.target = target;
     this.onEnter = null;
     this.onReturn = null;
+    this.onReturnExpr = null;
     this.onCall = null;
     this.handle = null;
   }

-  Instrumentor(Instrumentor orig, Object onEnter, Object onReturn, Object onCall, boolean activate) {
+  Instrumentor(Instrumentor orig, Object onEnter, Object onReturn, Object onReturnExpr, Object onCall, boolean activate) {
     this.module = orig.module;
     this.service = orig.service;
     this.target = orig.target;
     this.onEnter = onEnter != null ? onEnter : orig.onEnter;
     this.onReturn = onReturn != null ? onReturn : orig.onReturn;
+    this.onReturnExpr = onReturnExpr != null ? onReturnExpr : orig.onReturnExpr;
     this.onCall = onCall != null ? onCall : orig.onCall;
     this.handle = !activate ? null : service.bind(
       module, target, this, new Timer.Disabled()
@@ -73,7 +76,9 @@ final class Instrumentor implements EnsoObject, IdExecutionService.Callbacks {
   public void updateCachedResult(IdExecutionService.Info info) {
     try {
       if (onReturn != null) {
-        InteropLibrary.getUncached().execute(onReturn, info.getId().toString(), info.getResult());
+        var iop = InteropLibrary.getUncached();
+        var result = onReturnExpr == null || iop.isNull(onReturnExpr) ? info.getResult() : info.eval(iop.asString(onReturnExpr));
+        iop.execute(onReturn, info.getId().toString(), result);
       }
     } catch (InteropException ignored) {
     }
diff --git engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/InstrumentorBuiltin.java engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/InstrumentorBuiltin.java
index a9681276da..fe45e841d5 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/InstrumentorBuiltin.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/InstrumentorBuiltin.java
@@ -1,20 +1,18 @@
 package org.enso.interpreter.node.expression.builtin.meta;

-import com.oracle.truffle.api.nodes.Node;
-
 import org.enso.interpreter.dsl.BuiltinMethod;
 import org.enso.interpreter.runtime.EnsoContext;
 import org.enso.interpreter.runtime.callable.UnresolvedSymbol;
 import org.enso.interpreter.runtime.callable.function.Function;
 import org.enso.interpreter.runtime.data.text.Text;
 import org.enso.interpreter.runtime.data.vector.ArrayLikeAtNode;
-import org.enso.interpreter.runtime.data.vector.ArrayLikeCoerceToArrayNode;
 import org.enso.interpreter.runtime.data.vector.ArrayLikeLengthNode;
 import org.enso.interpreter.runtime.error.PanicException;

 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.interop.InvalidArrayIndexException;
+import com.oracle.truffle.api.nodes.Node;

 @BuiltinMethod(
     type = "Meta",
@@ -33,7 +31,7 @@ public class InstrumentorBuiltin extends Node {
       if (atNode.executeAt(args, 0) instanceof Instrumentor b) {
         ret = switch (op) {
           case "onEnter" -> onEnter(b, atNode.executeAt(args, 1));
-          case "onReturn" -> onReturn(b, atNode.executeAt(args, 1));
+          case "onReturn" -> onReturn(b, atNode.executeAt(args, 1), atNode.executeAt(args, 2));
           case "onCall" -> onCall(b, atNode.executeAt(args, 1));
           case "activate" -> activate(b, atNode.executeAt(args, 1));
           case "deactivate" -> b.deactivate();
@@ -70,16 +68,16 @@ public class InstrumentorBuiltin extends Node {
   @CompilerDirectives.TruffleBoundary
   private Object onEnter(Instrumentor b, Object arg) {
     if (arg instanceof Function fn) {
-      return new Instrumentor(b, fn, null, null, false);
+      return new Instrumentor(b, fn, null, null, null, false);
     } else {
       return null;
     }
   }

   @CompilerDirectives.TruffleBoundary
-  private Object onReturn(Instrumentor b, Object arg) {
+  private Object onReturn(Instrumentor b, Object arg, Object expr) {
     if (arg instanceof Function fn) {
-      return new Instrumentor(b, null, fn, null, false);
+      return new Instrumentor(b, null, fn, expr, null, false);
     } else {
       return null;
     }
@@ -88,7 +86,7 @@ public class InstrumentorBuiltin extends Node {
   @CompilerDirectives.TruffleBoundary
   private Object onCall(Instrumentor b, Object arg) {
     if (arg instanceof Function fn) {
-      return new Instrumentor(b, null, null, fn, false);
+      return new Instrumentor(b, null, null, null, fn, false);
     } else {
       return null;
     }
@@ -97,7 +95,7 @@ public class InstrumentorBuiltin extends Node {
   @CompilerDirectives.TruffleBoundary
   private Object activate(Instrumentor b, Object arg) {
     if (arg instanceof Function fn) {
-      var builder = new Instrumentor(b, null, null, null, true);
+      var builder = new Instrumentor(b, null, null, null, null, true);
       var ctx = EnsoContext.get(this);
       return ctx.getResourceManager().register(builder, fn);
     } else {
diff --git test/Tests/src/Semantic/Instrumentor_Spec.enso test/Tests/src/Semantic/Instrumentor_Spec.enso
index 4e130f4fad..1dcdeff9fd 100644
--- test/Tests/src/Semantic/Instrumentor_Spec.enso
+++ test/Tests/src/Semantic/Instrumentor_Spec.enso
@@ -44,6 +44,36 @@ spec =
             # no more instrumenting after finalize
             b.to_vector.length . should_equal 1

+        Test.specify "access local variables " <|
+            b = Vector.new_builder
+
+            collect uuid:Text result =
+                a_plus_b_uuid = "00000000-aaaa-bbbb-0000-000000000000" # UUID for a+b
+                if uuid == a_plus_b_uuid then
+                    b.append result
+                Nothing
+
+            instrumenter = Meta.meta .fib . instrument . on_return collect expression="b-a" . activate
+
+            instrumenter . with _->
+                result = fib 10
+
+                v = b.to_vector
+
+                v.length . should_equal 1
+                v.at 0 . should_equal 89
+                result . should_equal 89
+
+            instrumenter.finalize
+            # no op:
+            instrumenter.finalize
+
+            result = fib 10
+            result . should_equal 89
+
+            # no more instrumenting after finalize
+            b.to_vector.length . should_equal 1
+
         Test.specify "replay with caches and specify different result" <|
             replay uuid:Text = case uuid of
                 "00000000-ffff-bbbb-0000-000000000000" -> 42
JaroslavTulach commented 11 months ago

However the previous implementation has a problem! It tries to evaluate expression at all locations in the fib function and not all of them have access to a and b variable. Ideally we'd evaluate the expression on at "00000000-aaaa-bbbb-0000-000000000000" # UUID for a+b and nowhere else.

enso-bot[bot] commented 11 months ago

Jaroslav Tulach reports a new STANDUP for yesterday (2023-11-19):

Progress: - https://github.com/enso-org/enso/issues/8293#issuecomment-1817797389

Next Day: Bigfixing & stabilization

enso-bot[bot] commented 11 months ago

Jaroslav Tulach reports a new STANDUP for yesterday (2023-11-20):

Progress: - suspended thunk execution & merge of https://github.com/enso-org/enso/pull/8331

Next Day: Bugfixing whatever gets planned