Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide MethodCall-like information about constructor calls. #8663

Closed
farmaazon opened this issue Jan 3, 2024 · 11 comments · Fixed by #8743
Closed

Provide MethodCall-like information about constructor calls. #8663

farmaazon opened this issue Jan 3, 2024 · 11 comments · Fixed by #8743
Assignees

Comments

@farmaazon
Copy link
Contributor

To display proper placeholders for constructor's argument everywhere, we need to get similar information as for method calls in execution updates. It could be even exactly the same methodCall field.

@farmaazon
Copy link
Contributor Author

@hubertp @4e6 do you think this is hard/long to implement? If yes, we'd like to do some workarounds for constructors in GUI. If not, then we'll just wait for proper solution.

@4e6
Copy link
Contributor

4e6 commented Jan 3, 2024

I can see that the engine does not return the function schema for the partially applied constructors (while it does for the methods). I need to check what is the reason for that.

@4e6
Copy link
Contributor

4e6 commented Jan 4, 2024

There's not enough runtime information to build a method pointer for partially applied constructors. They are represented as closures in runtime. I need to check the codegen and see if I can supply the required info.

@hubertp hubertp removed the triage label Jan 4, 2024
@hubertp
Copy link
Collaborator

hubertp commented Jan 4, 2024

@hubertp @4e6 do you think this is hard/long to implement? If yes, we'd like to do some workarounds for constructors in GUI. If not, then we'll just wait for proper solution.

To answer your question - we don't know yet. It's hard to estimate this change atm. But Dmitry is looking into it.

@hubertp hubertp moved this from ❓New to 📤 Backlog in Issues Board Jan 8, 2024
@4e6 4e6 moved this from 📤 Backlog to 🔧 Implementation in Issues Board Jan 9, 2024
@4e6
Copy link
Contributor

4e6 commented Jan 9, 2024

In #6957 the FunctionSchema was added to the response to provide information about the returned function.

Below there is a recap of what's working and what doesn't in case of constructors.

Given the type definition.

type T
    A x y
  1. ✖️ T.A - a call to constructor
  • Should return (in pseudo code because the exact JSON value is too verbose)
    ExpressionUpdate(
      type = Function,
      methodCall = MethodCall(MethodPointer(Main, Main.T, A), [0, 1]),
      payload = Value(
        functionSchema = MethodCall(MethodPointer(Main, Main.T, A), [0, 1])
      )
    )
  • Returns
    ExpressionUpdate(
      type = Function,
      methodCall = MethodCall(MethodPointer(Main, Main.T, A), []),
      payload = Value()
    )
  1. ✖️ T.A 1 - a call to a partially applied constructor
  • Should return
    ExpressionUpdate(
      type = Function,
      methodCall = MethodCall(MethodPointer(Main, Main.T, A), [1]),
      payload = Value(
        functionSchema = MethodCall(MethodPointer(Main, Main.T, A), [1])
      )
    )
  • Returns
    ExpressionUpdate(
      type = Function,
      methodCall = MethodCall(MethodPointer(Main, Main.T, A), []),
      payload = Value()
    )
  1. ✔️ T.A 1 2 - a call to a fully applied constructor
  • Should return
    ExpressionUpdate(
      type = Main.T,
      methodCall = MethodCall(MethodPointer(Main, Main.T, A), []),
      payload = Value()
    )
  • Returns
    ExpressionUpdate(
      type = Main.T,
      methodCall = MethodCall(MethodPointer(Main, Main.T, A), []),
      payload = Value()
    )

@enso-bot
Copy link

enso-bot bot commented Jan 9, 2024

Dmitry Bushev reports a new STANDUP for yesterday (2024-01-08):

Progress: Started working on the task. Identified the issue. Started analyzing different examples of the application of the constructor function. Reviewed how the partial application in the ordinary functions is implemented. It should be finished by 2024-01-12.

Next Day: Next day I will be working on the #8663 task. Continue working on the task

@enso-bot
Copy link

enso-bot bot commented Jan 9, 2024

Dmitry Bushev reports a new STANDUP for today (2024-01-09):

Progress: Continue working on the task. Identified the issue. Finally got the idea of what info is missing from the updates about the constructor calls. Got an idea of how to implement the fix without changing the runtime representation. It should be finished by 2024-01-12.

Next Day: Next day I will be working on the #8663 task. Continue working on the task

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Jan 10, 2024

I believe it is good idea to start with a test. If I understand @4e6 correctly, the problem is in FunctionPointer.fromFunction method. Let's see how it behaves for different kinds of functions:

diff --git engine/runtime/src/test/java/org/enso/interpreter/test/instrument/FunctionPointerTest.java engine/runtime/src/test/java/org/enso/interpreter/test/instrument/FunctionPointerTest.java
new file mode 100644
index 0000000000..9dbd4fb997
--- /dev/null
+++ engine/runtime/src/test/java/org/enso/interpreter/test/instrument/FunctionPointerTest.java
@@ -0,0 +1,174 @@
+package org.enso.interpreter.test.instrument;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertNotNull;
+
+import java.nio.file.Paths;
+import java.util.Map;
+import java.util.logging.Level;
+
+import org.enso.interpreter.runtime.callable.atom.AtomConstructor;
+import org.enso.interpreter.runtime.callable.function.Function;
+import org.enso.polyglot.RuntimeOptions;
+import org.graalvm.polyglot.Context;
+import org.graalvm.polyglot.Language;
+import org.graalvm.polyglot.Source;
+import org.graalvm.polyglot.io.IOAccess;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.enso.interpreter.service.ExecutionService.FunctionPointer;
+import org.enso.interpreter.test.TestBase;
+
+public class FunctionPointerTest extends TestBase {
+
+  private Context context;
+
+  @Before
+  public void initContext() {
+    context =
+        Context.newBuilder()
+            .allowExperimentalOptions(true)
+            .option(
+                RuntimeOptions.LANGUAGE_HOME_OVERRIDE,
+                Paths.get("../../distribution/component").toFile().getAbsolutePath())
+            .option(RuntimeOptions.LOG_LEVEL, Level.WARNING.getName())
+            .logHandler(System.err)
+            .allowExperimentalOptions(true)
+            .allowIO(IOAccess.ALL)
+            .allowAllAccess(true)
+            .build();
+
+    var engine = context.getEngine();
+    Map<String, Language> langs = engine.getLanguages();
+    Assert.assertNotNull("Enso found: " + langs, langs.get("enso"));
+  }
+
+  @After
+  public void disposeContext() {
+    context.close();
+  }
+
+  @Test
+  public void moduleFunctionPointer() throws Exception {
+    var rawCode = """
+        from Standard.Base import all
+
+        run a b = a + b
+        """;
+    var src = Source.newBuilder("enso", rawCode, "TestFunctionPointer.enso").build();
+    var module = context.eval(src);
+    var res = module.invokeMember("eval_expression", "run");
+
+    assertTrue("fn: " + res, res.canExecute());
+    var rawRes = TestBase.unwrapValue(context, res);
+    assertTrue("function: " + rawRes, rawRes instanceof Function);
+    var c = FunctionPointer.fromFunction((Function) rawRes);
+    assertNotNull(c);
+    assertEquals("TestFunctionPointer", c.moduleName().toString());
+    assertEquals("TestFunctionPointer", c.typeName().toString());
+    assertEquals("run", c.functionName().toString());
+  }
+
+  @Test
+  public void typeStaticMethodPointer() throws Exception {
+    var rawCode = """
+        from Standard.Base import all
+
+        type X
+            run a b = a + b
+        """;
+    var src = Source.newBuilder("enso", rawCode, "StaticMethodPointer.enso").build();
+    var module = context.eval(src);
+    var res = module.invokeMember("eval_expression", "X.run");
+
+    assertTrue("fn: " + res, res.canExecute());
+    var rawRes = TestBase.unwrapValue(context, res);
+    assertTrue("function: " + rawRes, rawRes instanceof Function);
+    var c = FunctionPointer.fromFunction((Function) rawRes);
+    assertNotNull(c);
+    assertEquals("StaticMethodPointer", c.moduleName().toString());
+    assertEquals("StaticMethodPointer.X", c.typeName().toString());
+    assertEquals("run", c.functionName().toString());
+
+    var apply = res.execute(1);
+    assertTrue("fn: " + apply, apply.canExecute());
+    var rawApply = TestBase.unwrapValue(context, res);
+    assertTrue("function: " + rawApply, rawApply instanceof Function);
+    var a = FunctionPointer.fromFunction((Function) rawApply);
+    assertNotNull(a);
+    assertEquals("StaticMethodPointer", a.moduleName().toString());
+    assertEquals("StaticMethodPointer.X", a.typeName().toString());
+    assertEquals("run", a.functionName().toString());
+  }
+
+  @Test
+  public void typeInstanceMethodPointer() throws Exception {
+    var rawCode = """
+        from Standard.Base import all
+
+        type X
+            run self b c = [self, b, c]
+        """;
+    var src = Source.newBuilder("enso", rawCode, "InstanceMethodPointer.enso").build();
+    var module = context.eval(src);
+    var res = module.invokeMember("eval_expression", "X.run");
+
+    assertTrue("fn: " + res, res.canExecute());
+    var rawRes = TestBase.unwrapValue(context, res);
+    assertTrue("function: " + rawRes, rawRes instanceof Function);
+    var c = FunctionPointer.fromFunction((Function) rawRes);
+    assertNotNull(c);
+    assertEquals("InstanceMethodPointer", c.moduleName().toString());
+    assertEquals("InstanceMethodPointer.X", c.typeName().toString());
+    assertEquals("run", c.functionName().toString());
+
+    var apply = res.execute(1);
+    assertTrue("fn: " + apply, apply.canExecute());
+    var rawApply = TestBase.unwrapValue(context, res);
+    assertTrue("function: " + rawApply, rawApply instanceof Function);
+    var a = FunctionPointer.fromFunction((Function) rawApply);
+    assertNotNull(a);
+    assertEquals("InstanceMethodPointer", a.moduleName().toString());
+    assertEquals("InstanceMethodPointer.X", a.typeName().toString());
+    assertEquals("run", a.functionName().toString());
+  }
+
+  @Test
+  public void typeConstructorPointer() throws Exception {
+    var rawCode = """
+        from Standard.Base import all
+
+        type X
+            Run a b
+        """;
+    var src = Source.newBuilder("enso", rawCode, "ConstructorPointer.enso").build();
+    var module = context.eval(src);
+    var res = module.invokeMember("eval_expression", "X.Run");
+
+    assertTrue("fn: " + res, res.canInstantiate());
+    var rawRes = TestBase.unwrapValue(context, res);
+    assertTrue("function: " + rawRes.getClass(), rawRes instanceof AtomConstructor);
+    var rawFn = ((AtomConstructor) rawRes).getConstructorFunction();
+    var c = FunctionPointer.fromFunction((Function) rawFn);
+    assertNotNull("We should get a pointer for " + rawFn, c);
+    /*
+    assertEquals("ConstructorPointer", c.moduleName().toString());
+    assertEquals("ConstructorPointer.X", c.typeName().toString());
+    assertEquals("run", c.functionName().toString());
+
+    var apply = res.execute(1);
+    assertTrue("fn: " + apply, apply.canExecute());
+    var rawApply = TestBase.unwrapValue(context, res);
+    assertTrue("function: " + rawApply, rawApply instanceof Function);
+    var a = FunctionPointer.fromFunction((Function) rawApply);
+    assertNotNull(a);
+    assertEquals("ConstructorPointer", a.moduleName().toString());
+    assertEquals("ConstructorPointer.X", a.typeName().toString());
+    assertEquals("run", a.functionName().toString());
+    */
+  }
+}

The last line before /* block */ fails. E.g. FunctionPointer.fromFunction really returns null for AtomConstructor.getConstructorFunction().

@enso-bot
Copy link

enso-bot bot commented Jan 11, 2024

Dmitry Bushev reports a new STANDUP for yesterday (2024-01-10):

Progress: Continue working on the task. Updated the update handling logic to use the method pointer detected during the call if it fails to build the function schema for the result function. This is suboptimal because I cannot be sure that it holds for any function. Got another idea to create a separate root node for a constructor function. It should be finished by 2024-01-12.

Next Day: Next day I will be working on the #8663 task. Continue working on the task

@enso-bot
Copy link

enso-bot bot commented Jan 11, 2024

Dmitry Bushev reports a new STANDUP for today (2024-01-11):

Progress: Continue working on the task. End up re-using the method root node for the constructor function instead of creating a separate one. Fixed the NPE in the function pointer building logic. Fixed the runtime tests. Created the PR. It should be finished by 2024-01-12.

Next Day: Next day I will be working on the #8663 task. Continue working on the task

@4e6 4e6 moved this from 🔧 Implementation to 👁️ Code review in Issues Board Jan 12, 2024
@mergify mergify bot closed this as completed in #8743 Jan 12, 2024
mergify bot pushed a commit that referenced this issue Jan 12, 2024
close #8663

Changelog:
- update: use `MethodRootNode` for the atom constructor function to preserve the call info in runtime
- fix: return function schema for atom constructors
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Jan 12, 2024
@enso-bot
Copy link

enso-bot bot commented Jan 13, 2024

Dmitry Bushev reports a new STANDUP for yesterday (2024-01-12):

Progress: Finisnhing working on the task. Was working on review comments. Refactored the function pointer logic. Added the tests for the function pointer. It should be finished by 2024-01-12.

Next Day: Next day I will be working on the #8663 task. Continue working on the task

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants