Atom constructor is using two function calls #6903
Assignees
Labels
Comments
|
Do we have a test case? Or a test case which is close in behavior to the requested one? |
|
When I save type T
A x y
main = T.A 1 2and save an insight.on('enter', (ctx, frame) => {
print(`${ctx.name} at ${ctx.source.name}:${ctx.line} variables: ${Object.keys(frame)}`);
}, {
roots : true
});then running e.g. there are two function/method invocations. One is insight.on('enter', (ctx, frame) => {
print(`${ctx.name} at ${ctx.source.name}:${ctx.line} variables: ${Object.keys(frame)}`);
}, {
expressions : true
});then it observes two expressions inside of a single constructor: The bug says "two function calls" to constructor - I don't see that. |
|
I put in the description two functions that are created in order to build an instance. The instrument only sees the first one |
|
I am supposed to run this test to reproduce the problem. |
|
I see the failure: sbt:runtime-with-instruments> testOnly *RuntimeServerTest -- -z constructors
[info] - should send method pointer updates of partially applied constructors *** FAILED ***
[info] List(Response(Some(41a6e6de-4439-4623-84c3-9fcf290fd170),PushContextResponse(963843f9-e196-4968-b3bb-b0a5e85e7b43)), Response(None,ExpressionUpdates(963843f9-e196-4968-b3bb-b0a5e85e7b43,Set(ExpressionUpdate(00000000-0000-00aa-449f-8119429d4528,Some(Standard.Builtins.Main.Function),Some(MethodCall(MethodPointer(Enso_Test.Test.Main,Enso_Test.Test.Main.T,A),Vector())),Vector(ExecutionTime(0)),false,true,Value(None))))), Response(None,ExpressionUpdates(963843f9-e196-4968-b3bb-b0a5e85e7b43,Set(ExpressionUpdate(00000000-0000-00ab-6949-6fba1ffb4eac,Some(Enso_Test.Test.Main.T),None,Vector(ExecutionTime(0)),false,true,Value(None))))), Response(None,ExecutionComplete(963843f9-e196-4968-b3bb-b0a5e85e7b43)), Response(None,BackgroundJobsStartedNotification())) did not contain the same elements as List(Response(None,BackgroundJobsStartedNotification()), Response(Some(41a6e6de-4439-4623-84c3-9fcf290fd170),PushContextResponse(963843f9-e196-4968-b3bb-b0a5e85e7b43)), Response(None,ExpressionUpdates(963843f9-e196-4968-b3bb-b0a5e85e7b43,Set(ExpressionUpdate(00000000-0000-00aa-449f-8119429d4528,Some(Standard.Builtins.Main.Function),Some(MethodCall(MethodPointer(Enso_Test.Test.Main,Enso_Test.Test.Main.T,A),Vector(1))),Vector(ExecutionTime(0)),false,true,Value(None))))), Response(None,ExpressionUpdates(963843f9-e196-4968-b3bb-b0a5e85e7b43,Set(ExpressionUpdate(00000000-0000-00ab-6949-6fba1ffb4eac,Some(Enso_Test.Test.Main.T),Some(MethodCall(MethodPointer(Enso_Test.Test.Main,Enso_Test.Test.Main.T,A),Vector())),Vector(ExecutionTime(0)),false,true,Value(None))))), Response(None,ExecutionComplete(963843f9-e196-4968-b3bb-b0a5e85e7b43))) (RuntimeServerTest.scala:855)time to debug it. |
2 tasks
|
I see the failure: sbt:runtime-with-instruments> testOnly *RuntimeServerTest -- -z constructors
[info] - should send method pointer updates of partially applied constructors *** FAILED ***
[info] List(Response(Some(41a6e6de-4439-4623-84c3-9fcf290fd170),PushContextResponse(963843f9-e196-4968-b3bb-b0a5e85e7b43)), Response(None,ExpressionUpdates(963843f9-e196-4968-b3bb-b0a5e85e7b43,Set(ExpressionUpdate(00000000-0000-00aa-449f-8119429d4528,Some(Standard.Builtins.Main.Function),Some(MethodCall(MethodPointer(Enso_Test.Test.Main,Enso_Test.Test.Main.T,A),Vector())),Vector(ExecutionTime(0)),false,true,Value(None))))), Response(None,ExpressionUpdates(963843f9-e196-4968-b3bb-b0a5e85e7b43,Set(ExpressionUpdate(00000000-0000-00ab-6949-6fba1ffb4eac,Some(Enso_Test.Test.Main.T),None,Vector(ExecutionTime(0)),false,true,Value(None))))), Response(None,ExecutionComplete(963843f9-e196-4968-b3bb-b0a5e85e7b43)), Response(None,BackgroundJobsStartedNotification())) did not contain the same elements as List(Response(None,BackgroundJobsStartedNotification()), Response(Some(41a6e6de-4439-4623-84c3-9fcf290fd170),PushContextResponse(963843f9-e196-4968-b3bb-b0a5e85e7b43)), Response(None,ExpressionUpdates(963843f9-e196-4968-b3bb-b0a5e85e7b43,Set(ExpressionUpdate(00000000-0000-00aa-449f-8119429d4528,Some(Standard.Builtins.Main.Function),Some(MethodCall(MethodPointer(Enso_Test.Test.Main,Enso_Test.Test.Main.T,A),Vector(1))),Vector(ExecutionTime(0)),false,true,Value(None))))), Response(None,ExpressionUpdates(963843f9-e196-4968-b3bb-b0a5e85e7b43,Set(ExpressionUpdate(00000000-0000-00ab-6949-6fba1ffb4eac,Some(Enso_Test.Test.Main.T),Some(MethodCall(MethodPointer(Enso_Test.Test.Main,Enso_Test.Test.Main.T,A),Vector())),Vector(ExecutionTime(0)),false,true,Value(None))))), Response(None,ExecutionComplete(963843f9-e196-4968-b3bb-b0a5e85e7b43))) (RuntimeServerTest.scala:855)time to debug it. Looks like the problem is in applying instrumentation. The instances of |
|
Another problem is that The node in question is created by I believe that curried invocations were never instrumentable and they are not ready for be instrumentable. CCing @kustosz. |
|
This change "convinces" the instrumentation to deliver sendExpressionUpdate for the curried function: diff --git engine/runtime-with-instruments/src/test/scala/org/enso/interpreter/test/instrument/RuntimeServerTest.scala engine/runtime-with-instruments/src/test/scala/org/enso/interpreter/test/instrument/RuntimeServerTest.scala
index c69df9a7c3..721b50ae61 100644
--- engine/runtime-with-instruments/src/test/scala/org/enso/interpreter/test/instrument/RuntimeServerTest.scala
+++ engine/runtime-with-instruments/src/test/scala/org/enso/interpreter/test/instrument/RuntimeServerTest.scala
@@ -32,7 +32,7 @@ class RuntimeServerTest
var context: TestContext = _
class TestContext(packageName: String) extends InstrumentTestContext {
-
+ System.setProperty("truffle.instrumentation.trace", "true");
val tmpDir: Path = Files.createTempDirectory("enso-test-packages")
sys.addShutdownHook(FileSystem.removeDirectoryIfExists(tmpDir))
val lockManager = new ThreadSafeFileLockManager(tmpDir.resolve("locks"))
diff --git engine/runtime/src/main/java/org/enso/interpreter/instrument/IdExecutionService.java engine/runtime/src/main/java/org/enso/interpreter/instrument/IdExecutionService.java
index 8c6c2be623..c2b31208df 100644
--- engine/runtime/src/main/java/org/enso/interpreter/instrument/IdExecutionService.java
+++ engine/runtime/src/main/java/org/enso/interpreter/instrument/IdExecutionService.java
@@ -4,7 +4,6 @@ import com.oracle.truffle.api.CallTarget;
import com.oracle.truffle.api.instrumentation.EventBinding;
import com.oracle.truffle.api.instrumentation.ExecutionEventNodeFactory;
import com.oracle.truffle.api.nodes.RootNode;
-import java.util.ArrayList;
import java.util.Arrays;
import java.util.Objects;
import java.util.UUID;
@@ -16,9 +15,7 @@ import org.enso.interpreter.node.MethodRootNode;
import org.enso.interpreter.node.callable.FunctionCallInstrumentationNode;
import org.enso.interpreter.node.expression.atom.QualifiedAccessorNode;
import org.enso.interpreter.runtime.Module;
-import org.enso.interpreter.runtime.callable.argument.ArgumentDefinition;
import org.enso.interpreter.runtime.callable.atom.AtomConstructor;
-import org.enso.interpreter.runtime.callable.function.FunctionSchema;
import org.enso.interpreter.runtime.data.Type;
import org.enso.logger.masking.MaskedString;
import org.enso.pkg.QualifiedName;
@@ -233,7 +230,7 @@ public interface IdExecutionService {
typeName = null;
functionName = rootNode.getName();
}
- case default -> {
+ default -> {
moduleName = null;
typeName = null;
functionName = rootNode.getName();
diff --git engine/runtime/src/main/java/org/enso/interpreter/node/callable/FunctionCallInstrumentationNode.java engine/runtime/src/main/java/org/enso/interpreter/node/callable/FunctionCallInstrumentationNode.java
index 4716cd1cec..32119b8074 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/callable/FunctionCallInstrumentationNode.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/callable/FunctionCallInstrumentationNode.java
@@ -16,13 +16,12 @@ import com.oracle.truffle.api.library.ExportMessage;
import com.oracle.truffle.api.nodes.Node;
import com.oracle.truffle.api.nodes.NodeInfo;
import com.oracle.truffle.api.source.SourceSection;
-import org.enso.interpreter.runtime.callable.function.Function;
-import org.enso.interpreter.runtime.tag.IdentifiedTag;
-
import java.util.Arrays;
import java.util.UUID;
import org.enso.interpreter.node.ClosureRootNode;
+import org.enso.interpreter.runtime.callable.function.Function;
import org.enso.interpreter.runtime.tag.AvoidIdInstrumentationTag;
+import org.enso.interpreter.runtime.tag.IdentifiedTag;
/**
* A node used for instrumenting function calls. It does nothing useful from the language
@@ -165,7 +164,7 @@ public class FunctionCallInstrumentationNode extends Node implements Instrumenta
@Override
public SourceSection getSourceSection() {
Node parent = getParent();
- return parent == null ? null : parent.getSourceSection();
+ return parent == null ? null : parent.getEncapsulatingSourceSection();
}
/** @return the expression ID of this node. */
@@ -181,4 +180,9 @@ public class FunctionCallInstrumentationNode extends Node implements Instrumenta
public void setId(UUID expressionId) {
this.id = expressionId;
}
+
+ @Override
+ public String toString() {
+ return super.toString() + "[id=" + id + "]";
+ }
}
diff --git engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/CurryNode.java engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/CurryNode.java
index 8803831db1..02b2a3b0fa 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/CurryNode.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/CurryNode.java
@@ -4,6 +4,7 @@ import com.oracle.truffle.api.CompilerDirectives;
import com.oracle.truffle.api.frame.VirtualFrame;
import com.oracle.truffle.api.nodes.NodeInfo;
import com.oracle.truffle.api.profiles.BranchProfile;
+import java.util.concurrent.locks.Lock;
import org.enso.interpreter.node.BaseNode;
import org.enso.interpreter.node.callable.ExecuteCallNode;
import org.enso.interpreter.node.callable.InvokeCallableNode;
@@ -16,8 +17,6 @@ import org.enso.interpreter.runtime.callable.function.FunctionSchema;
import org.enso.interpreter.runtime.control.TailCallException;
import org.enso.interpreter.runtime.state.State;
-import java.util.concurrent.locks.Lock;
-
/** Handles runtime function currying and oversaturated (eta-expanded) calls. */
@NodeInfo(description = "Handles runtime currying and eta-expansion")
public class CurryNode extends BaseNode {
diff --git engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java
index cd5acd0f17..b2c912332a 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java
@@ -1,13 +1,17 @@
package org.enso.interpreter.node.callable.dispatch;
-import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
-import com.oracle.truffle.api.dsl.*;
+import com.oracle.truffle.api.dsl.Cached;
+import com.oracle.truffle.api.dsl.ImportStatic;
+import com.oracle.truffle.api.dsl.Specialization;
import com.oracle.truffle.api.frame.VirtualFrame;
+import com.oracle.truffle.api.instrumentation.InstrumentableNode.WrapperNode;
import com.oracle.truffle.api.nodes.Node;
import com.oracle.truffle.api.nodes.NodeInfo;
import com.oracle.truffle.api.source.SourceSection;
+import java.util.UUID;
import org.enso.interpreter.Constants;
import org.enso.interpreter.node.BaseNode;
+import org.enso.interpreter.node.ExpressionNode;
import org.enso.interpreter.node.callable.CaptureCallerInfoNode;
import org.enso.interpreter.node.callable.FunctionCallInstrumentationNode;
import org.enso.interpreter.node.callable.InvokeCallableNode;
@@ -20,8 +24,6 @@ import org.enso.interpreter.runtime.callable.function.Function;
import org.enso.interpreter.runtime.callable.function.FunctionSchema;
import org.enso.interpreter.runtime.state.State;
-import java.util.UUID;
-
/**
* This class represents the protocol for remapping the arguments provided at a call site into the
* positional order expected by the definition of the {@link Function}.
@@ -87,6 +89,21 @@ public abstract class InvokeFunctionNode extends BaseNode {
if (cachedSchema.getCallerFrameAccess().shouldFrameBePassed()) {
callerInfo = captureCallerInfoNode.execute(callerFrame.materialize());
}
+ if (!(functionCallInstrumentationNode instanceof WrapperNode)) {
+ if (functionCallInstrumentationNode.getSourceSection() != null) {
+ if (functionCallInstrumentationNode.getId() == null) {
+ Node p = getParent();
+ while (p != null) {
+ if (p instanceof ExpressionNode expr && expr.getId() != null) {
+ functionCallInstrumentationNode.setId(expr.getId());
+ break;
+ }
+ p = p.getParent();
+ }
+ }
+ notifyInserted(functionCallInstrumentationNode);
+ }
+ }
functionCallInstrumentationNode.execute(
callerFrame, function, state, mappedArguments.getSortedArguments());
return curryNode.execute(
@@ -135,6 +152,11 @@ public abstract class InvokeFunctionNode extends BaseNode {
callerInfo = captureCallerInfoNode.execute(callerFrame.materialize());
}
+ if (!(functionCallInstrumentationNode instanceof WrapperNode)) {
+ if (functionCallInstrumentationNode.getSourceSection() != null) {
+ notifyInserted(functionCallInstrumentationNode);
+ }
+ }
functionCallInstrumentationNode.execute(
callerFrame, function, state, mappedArguments.getSortedArguments());construction of |
|
Thinking about the issue more, I believe we are trying to tackle it from a wrong angle. What's @Frizi's request? When there is a partially applied function, then we want to know what arguments have been applied. However that is not information about "a call" that has happened, but about a "future call" - we have function value and the IDE needs to know what to do with it. Right?
That means (in my opinion) that ProgramExecutionSupport.scala needs to do following when composing
|
|
Jaroslav Tulach reports a new STANDUP for yesterday (2023-06-01): Progress: - Whole day debugging & investigation of atom constructors
Next Day: Imports & Runtime Type Checks |
3 tasks
|
Dmitry Bushev reports a new STANDUP for today (2023-06-02): Progress: Started working on the issue. It seems that I was able to find the workaround for the partially applied construction calls in the instrument. Without changing the runtime execution scheme. Updated the tests. Created the PR. It should be finished by 2023-06-07. Next Day: Next day I will be working on the #6903 task. Continue working on the task |
|
Superseded by #6957 |
github-project-automation
bot
moved this from 🔴 Changes requested
to 🟢 Accepted
in Issues Board
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Consider the example
The
T.A 1 2expression is executed in two steps:Ais resolved to a methodT.A(methodAwith a self parameterT)enso/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/atom/AtomConstructor.java
Lines 176 to 185 in ab15923
T.Athen returns a function with two argumentsxandythat constructs theAinstanceenso/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/atom/AtomConstructor.java
Lines 162 to 173 in ab15923
Issue
When instrumented, the
T.A 1 2expression returns the underlying method callA self. Instead, we want to report a methodA x yas it looks from the user (and the suggestion database) perspective.The text was updated successfully, but these errors were encountered: