From 76409b285de04886a9bfdf02a5fe7f0cc232463e Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Mon, 27 Mar 2023 19:49:20 +0200 Subject: [PATCH] Ensure new and wrapper nodes inherit UUID (#6067) Instrumentation of calls involving warning values never really worked because: 1) newly created nodes didn't set the UUID of their children 2) the instrumentable wrappers always had an empty (i.e. null) UUID and they never referred `get`/`setId` calls to their delegates On the surface, everything worked fine. Except when one actually relied on the instrumentation of values with warnings for proper setup. Then no instrumentation (replacement of nodes) was performed due to empty UUID (as required by `hasTag` of `FunctionCallInstrumentationNode`). Closes #6045. Discovered in #5893. --- CHANGELOG.md | 2 + .../instrument/IdExecutionInstrument.java | 11 +- .../test/NodeCountingTestInstrument.java | 108 +++++++++++++++++- .../instrument/IncrementalUpdatesTest.java | 4 +- .../WarningInstrumentationTest.java | 92 +++++++++++++++ .../FunctionCallInstrumentationNode.java | 8 +- .../node/callable/InvokeCallableNode.java | 4 + .../node/callable/InvokeConversionNode.java | 1 + .../node/callable/InvokeMethodNode.java | 4 + .../callable/dispatch/InvokeFunctionNode.java | 5 + 10 files changed, 225 insertions(+), 14 deletions(-) create mode 100644 engine/runtime-with-instruments/src/test/java/org/enso/interpreter/test/instrument/WarningInstrumentationTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d79c1dab6..66aa4553a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -651,6 +651,7 @@ - [Use SHA-1 for calculating hashes of modules' IR and bindings][5791] - [Don't install Python component on Windows][5900] - [Detect potential name conflicts between exported types and FQNs][5966] +- [Ensure calls involving warnings remain instrumented][6067] [3227]: https://github.com/enso-org/enso/pull/3227 [3248]: https://github.com/enso-org/enso/pull/3248 @@ -753,6 +754,7 @@ [5791]: https://github.com/enso-org/enso/pull/5791 [5900]: https://github.com/enso-org/enso/pull/5900 [5966]: https://github.com/enso-org/enso/pull/5966 +[6067]: https://github.com/enso-org/enso/pull/6067 # Enso 2.0.0-alpha.18 (2021-10-12) diff --git a/engine/runtime-instrument-id-execution/src/main/java/org/enso/interpreter/instrument/IdExecutionInstrument.java b/engine/runtime-instrument-id-execution/src/main/java/org/enso/interpreter/instrument/IdExecutionInstrument.java index 7d6bfd8c85..0b14a5f0c2 100644 --- a/engine/runtime-instrument-id-execution/src/main/java/org/enso/interpreter/instrument/IdExecutionInstrument.java +++ b/engine/runtime-instrument-id-execution/src/main/java/org/enso/interpreter/instrument/IdExecutionInstrument.java @@ -230,9 +230,9 @@ public class IdExecutionInstrument extends TruffleInstrument implements IdExecut Node node = context.getInstrumentedNode(); if (node instanceof FunctionCallInstrumentationNode - && result instanceof FunctionCallInstrumentationNode.FunctionCall) { + && result instanceof FunctionCallInstrumentationNode.FunctionCall functionCall) { UUID nodeId = ((FunctionCallInstrumentationNode) node).getId(); - onFunctionReturn(nodeId, result, context); + onFunctionReturn(nodeId, functionCall, context); } else if (node instanceof ExpressionNode) { onExpressionReturn(result, node, context); } @@ -307,11 +307,10 @@ public class IdExecutionInstrument extends TruffleInstrument implements IdExecut } @CompilerDirectives.TruffleBoundary - private void onFunctionReturn(UUID nodeId, Object result, EventContext context) throws ThreadDeath { + private void onFunctionReturn(UUID nodeId, FunctionCallInstrumentationNode.FunctionCall result, EventContext context) throws ThreadDeath { calls.put( - nodeId, new FunctionCallInfo((FunctionCallInstrumentationNode.FunctionCall) result)); - functionCallCallback.accept( - new ExpressionCall(nodeId, (FunctionCallInstrumentationNode.FunctionCall) result)); + nodeId, new FunctionCallInfo(result)); + functionCallCallback.accept(new ExpressionCall(nodeId, result)); // Return cached value after capturing the enterable function call in `functionCallCallback` Object cachedResult = cache.get(nodeId); if (cachedResult != null) { diff --git a/engine/runtime-with-instruments/src/test/java/org/enso/interpreter/test/NodeCountingTestInstrument.java b/engine/runtime-with-instruments/src/test/java/org/enso/interpreter/test/NodeCountingTestInstrument.java index b52097dbf9..cd33a79446 100644 --- a/engine/runtime-with-instruments/src/test/java/org/enso/interpreter/test/NodeCountingTestInstrument.java +++ b/engine/runtime-with-instruments/src/test/java/org/enso/interpreter/test/NodeCountingTestInstrument.java @@ -1,14 +1,22 @@ package org.enso.interpreter.test; +import com.oracle.truffle.api.frame.VirtualFrame; import com.oracle.truffle.api.instrumentation.EventContext; import com.oracle.truffle.api.instrumentation.ExecutionEventNode; import com.oracle.truffle.api.instrumentation.ExecutionEventNodeFactory; import com.oracle.truffle.api.instrumentation.SourceSectionFilter; import com.oracle.truffle.api.instrumentation.TruffleInstrument; import com.oracle.truffle.api.nodes.Node; +import com.oracle.truffle.api.nodes.RootNode; import com.oracle.truffle.api.source.SourceSection; +import org.enso.interpreter.node.MethodRootNode; +import org.enso.interpreter.node.callable.FunctionCallInstrumentationNode; +import org.enso.pkg.QualifiedName; + import java.util.List; import java.util.Map; +import java.util.Objects; +import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.function.Function; @@ -22,6 +30,8 @@ public class NodeCountingTestInstrument extends TruffleInstrument { public static final String INSTRUMENT_ID = "node-count-test"; private Map all = new ConcurrentHashMap<>(); private Map> counter = new ConcurrentHashMap<>(); + + private Map calls = new ConcurrentHashMap<>(); private Env env; @Override @@ -33,7 +43,17 @@ public class NodeCountingTestInstrument extends TruffleInstrument { public void enable() { this.env .getInstrumenter() - .attachExecutionEventFactory(SourceSectionFilter.ANY, new CountingFactory()); + .attachExecutionEventFactory(SourceSectionFilter.ANY, new CountingAndFunctionCallFactory()); + } + + public void enable(SourceSectionFilter filter) { + this.env + .getInstrumenter() + .attachExecutionEventFactory(filter, new CountingAndFunctionCallFactory()); + } + + public Map registeredCalls() { + return calls; } public Map> assertNewNodes(String msg, int min, int max) { @@ -73,7 +93,7 @@ public class NodeCountingTestInstrument extends TruffleInstrument { } } - private final class CountingFactory implements ExecutionEventNodeFactory { + private final class CountingAndFunctionCallFactory implements ExecutionEventNodeFactory { @Override public ExecutionEventNode create(EventContext context) { final Node node = context.getInstrumentedNode(); @@ -81,8 +101,92 @@ public class NodeCountingTestInstrument extends TruffleInstrument { if (all.put(node, node) == null) { counter.computeIfAbsent(node.getClass(), (__) -> new CopyOnWriteArrayList<>()).add(node); } + return new NodeWrapper(context, calls); } return null; } } + + private class NodeWrapper extends ExecutionEventNode { + + private final EventContext context; + + private final Map calls; + + public NodeWrapper(EventContext context, Map calls) { + this.context = context; + this.calls = calls; + } + + public void onReturnValue(VirtualFrame frame, Object result) { + Node node = context.getInstrumentedNode(); + if (node instanceof FunctionCallInstrumentationNode instrumentableNode + && result instanceof FunctionCallInstrumentationNode.FunctionCall functionCall) { + onFunctionReturn(instrumentableNode, functionCall); + } + } + + private void onFunctionReturn(FunctionCallInstrumentationNode node, FunctionCallInstrumentationNode.FunctionCall result) { + if (node.getId() != null) { + calls.put(node.getId(), new FunctionCallInfo(result)); + } + } + + } + + public class FunctionCallInfo { + + private final QualifiedName moduleName; + private final QualifiedName typeName; + private final String functionName; + + public FunctionCallInfo(FunctionCallInstrumentationNode.FunctionCall call) { + RootNode rootNode = call.getFunction().getCallTarget().getRootNode(); + if (rootNode instanceof MethodRootNode methodNode) { + moduleName = methodNode.getModuleScope().getModule().getName(); + typeName = methodNode.getType().getQualifiedName(); + functionName = methodNode.getMethodName(); + } else { + moduleName = null; + typeName = null; + functionName = rootNode.getName(); + } + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + FunctionCallInfo that = (FunctionCallInfo) o; + return Objects.equals(moduleName, that.moduleName) + && Objects.equals(typeName, that.typeName) + && Objects.equals(functionName, that.functionName); + } + + @Override + public int hashCode() { + return Objects.hash(moduleName, typeName, functionName); + } + + @Override + public String toString() { + return moduleName + "::" + typeName + "::" + functionName; + } + + public QualifiedName getModuleName() { + return moduleName; + } + + public QualifiedName getTypeName() { + return typeName; + } + + public String getFunctionName() { + return functionName; + } + } } diff --git a/engine/runtime-with-instruments/src/test/java/org/enso/interpreter/test/instrument/IncrementalUpdatesTest.java b/engine/runtime-with-instruments/src/test/java/org/enso/interpreter/test/instrument/IncrementalUpdatesTest.java index 31c267090f..e921a994fd 100644 --- a/engine/runtime-with-instruments/src/test/java/org/enso/interpreter/test/instrument/IncrementalUpdatesTest.java +++ b/engine/runtime-with-instruments/src/test/java/org/enso/interpreter/test/instrument/IncrementalUpdatesTest.java @@ -65,9 +65,7 @@ public class IncrementalUpdatesTest { sendUpdatesWhenFunctionBodyIsChangedBySettingValue("4", ConstantsGen.INTEGER, "4", "5", "5", LiteralNode.class); var m = context.languageContext().findModule(MODULE_NAME).orElse(null); assertNotNull("Module found", m); - var numbers = m.getIr().preorder().filter((v1) -> { - return v1 instanceof IR$Literal$Number; - }); + var numbers = m.getIr().preorder().filter((v1) -> v1 instanceof IR$Literal$Number); assertEquals("One number found: " + numbers, 1, numbers.size()); if (numbers.head() instanceof IR$Literal$Number n) { assertEquals("updated to 5", "5", n.value()); diff --git a/engine/runtime-with-instruments/src/test/java/org/enso/interpreter/test/instrument/WarningInstrumentationTest.java b/engine/runtime-with-instruments/src/test/java/org/enso/interpreter/test/instrument/WarningInstrumentationTest.java new file mode 100644 index 0000000000..cf0003e35f --- /dev/null +++ b/engine/runtime-with-instruments/src/test/java/org/enso/interpreter/test/instrument/WarningInstrumentationTest.java @@ -0,0 +1,92 @@ +package org.enso.interpreter.test.instrument; + +import com.oracle.truffle.api.instrumentation.SourceSectionFilter; +import com.oracle.truffle.api.instrumentation.StandardTags; +import org.enso.interpreter.runtime.tag.AvoidIdInstrumentationTag; +import org.enso.interpreter.runtime.tag.IdentifiedTag; +import org.enso.interpreter.test.Metadata; +import org.enso.interpreter.test.NodeCountingTestInstrument; +import org.enso.polyglot.RuntimeOptions; +import org.graalvm.polyglot.Context; +import org.graalvm.polyglot.Language; +import org.graalvm.polyglot.Source; +import static org.junit.Assert.assertEquals; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.io.OutputStream; +import java.nio.file.Paths; +import java.util.Map; + +public class WarningInstrumentationTest { + + private Context context; + private NodeCountingTestInstrument instrument; + + @Before + public void initContext() { + context = Context.newBuilder() + .allowExperimentalOptions(true) + .option( + RuntimeOptions.LANGUAGE_HOME_OVERRIDE, + Paths.get("../../distribution/component").toFile().getAbsolutePath() + ) + .logHandler(OutputStream.nullOutputStream()) + .allowExperimentalOptions(true) + .allowIO(true) + .allowAllAccess(true) + .build(); + + var engine = context.getEngine(); + Map langs = engine.getLanguages(); + Assert.assertNotNull("Enso found: " + langs, langs.get("enso")); + + instrument = engine.getInstruments().get(NodeCountingTestInstrument.INSTRUMENT_ID).lookup(NodeCountingTestInstrument.class); + SourceSectionFilter builder = SourceSectionFilter.newBuilder() + .tagIs(StandardTags.ExpressionTag.class, StandardTags.CallTag.class) + .tagIs(IdentifiedTag.class) + .tagIsNot(AvoidIdInstrumentationTag.class) + .build(); + instrument.enable(builder); + } + + @After + public void disposeContext() { + context.close(); + } + + @Test + public void instrumentValueWithWarnings() throws Exception { + var metadata = new Metadata(); + + var idOp1 = metadata.addItem(151, 34, null); + var idOp2 = metadata.addItem(202, 31, null); + var idOp3 = metadata.addItem(250, 13, null); + var rawCode = """ + from Standard.Base import all + from Standard.Base.Warning import Warning + from Standard.Table.Data.Table import Table + + run column_name = + operator1 = Table.new [[column_name, [1,2,3]]] + operator2 = Warning.attach "Text" operator1 + operator3 = operator2.get + operator3 + """; + var code = metadata.appendToCode(rawCode); + var src = Source.newBuilder("enso", code, "TestWarning.enso").build(); + var module = context.eval(src); + var res = module.invokeMember("eval_expression", "run"); + res.execute("A"); + + var calls = instrument.registeredCalls(); + + assertEquals(calls.keySet().size(), 3); + assertEquals(calls.get(idOp1).getFunctionName(), "new"); + assertEquals(calls.get(idOp2).getFunctionName(), "attach"); + assertEquals(calls.get(idOp3).getTypeName().item(), "Table"); + assertEquals(calls.get(idOp3).getFunctionName(), "get"); + } +} diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/FunctionCallInstrumentationNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/FunctionCallInstrumentationNode.java index b7aaf3e5f5..4716cd1cec 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/FunctionCallInstrumentationNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/FunctionCallInstrumentationNode.java @@ -142,13 +142,15 @@ public class FunctionCallInstrumentationNode extends Node implements Instrumenta */ @Override public WrapperNode createWrapper(ProbeNode probeNode) { - return new FunctionCallInstrumentationNodeWrapper(this, probeNode); + var wrapper = new FunctionCallInstrumentationNodeWrapper(this, probeNode); + wrapper.setId(this.getId()); + return wrapper; } /** - * Makrs this node with relevant runtime tags. + * Marks this node with relevant runtime tags. * - * @param tag the tag to check agains. + * @param tag the tag to check against. * @return true if the node carries the {@code tag}, false otherwise. */ @Override diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeCallableNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeCallableNode.java index 1f0f88d1ef..3720b29fc1 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeCallableNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeCallableNode.java @@ -279,6 +279,7 @@ public abstract class InvokeCallableNode extends BaseNode { invokeFunctionNode.getDefaultsExecutionMode(), invokeFunctionNode.getArgumentsExecutionMode())); childDispatch.setTailStatus(getTailStatus()); + childDispatch.setId(invokeFunctionNode.getId()); notifyInserted(childDispatch); } } finally { @@ -356,5 +357,8 @@ public abstract class InvokeCallableNode extends BaseNode { invokeFunctionNode.setId(id); invokeMethodNode.setId(id); invokeConversionNode.setId(id); + if (childDispatch != null) { + childDispatch.setId(id); + } } } diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeConversionNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeConversionNode.java index 03757b1555..870dad08a7 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeConversionNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeConversionNode.java @@ -164,6 +164,7 @@ public abstract class InvokeConversionNode extends BaseNode { invokeFunctionNode.getArgumentsExecutionMode(), thatArgumentPosition)); childDispatch.setTailStatus(getTailStatus()); + childDispatch.setId(invokeFunctionNode.getId()); notifyInserted(childDispatch); } } finally { diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java index 1b4a81301b..79fddcd65b 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java @@ -156,6 +156,7 @@ public abstract class InvokeMethodNode extends BaseNode { invokeFunctionNode.getArgumentsExecutionMode(), thisArgumentPosition)); childDispatch.setTailStatus(getTailStatus()); + childDispatch.setId(invokeFunctionNode.getId()); notifyInserted(childDispatch); } } finally { @@ -525,5 +526,8 @@ public abstract class InvokeMethodNode extends BaseNode { */ public void setId(UUID id) { invokeFunctionNode.setId(id); + if (childDispatch != null) { + childDispatch.setId(id); + } } } diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java index 1c7dacd86b..f9f5fd5bec 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java @@ -189,4 +189,9 @@ public abstract class InvokeFunctionNode extends BaseNode { public void setId(UUID id) { functionCallInstrumentationNode.setId(id); } + + /** Returns expression ID of this node. */ + public UUID getId() { + return functionCallInstrumentationNode.getId(); + } }