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

Builtins expose Enso methods #11687

Merged
merged 58 commits into from
Jan 13, 2025

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Nov 27, 2024

Closes #11589

Pull Request Description

Builtin types expose methods via the interop protocol. In fact, all types expose methods via interop protocol.

To get and invoke a method on a builtin object via interop protocol:

var meta = interop.getMetaObject(text);
meta.invokeMember(meta, "+", ...);

Example in InvokeBuiltinMethodViaInteropTest

To get and invoke a method on a builtin object via org.graalvm.polyglot.Value (this can be done, e.g., in std libs in Java code):

Value object;
Value meta = object.getMetaObject();
meta.invokeMember(...);

Example in BuiltinTypesExposeMethodsTest.

Important Notes

Method members are reported in Type.getMembers as internal members.
This has a side effect that in the debugger, types have method properties (but not atoms):
image

The first attempt to implement hasMember and getMembers on BuiltinObject abandoned - #11687 (comment)

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@Akirathan Akirathan added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Nov 27, 2024
@Akirathan Akirathan self-assigned this Nov 27, 2024
@enso-org enso-org deleted a comment from enso-bot bot Dec 2, 2024
@enso-org enso-org deleted a comment from enso-bot bot Dec 2, 2024
@@ -543,6 +543,12 @@ boolean isObjectWithMembers(Object object, InteropLibrary interop) {
if (interop.isTime(object)) {
return false;
}
if (interop.isDuration(object)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both TimeZone and Duration now have members, so we need to add these guards here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole EqualsXyzNode set of @Specialize annotation is fragile, unreadable, not fully testable and deserves a rewrite. At least that's the feeling I get every time I am changing the code.

Such a gentle change is OK, unless total rewrite is attempted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my answer in #11687 (comment) - yes it is a mess.

@Akirathan
Copy link
Member Author

the
#11589 issue started with three examples of missing builtin methods
shall we not fix some of these cases in this PR?

@JaroslavTulach Ref.get, Vector.to_text, and File.path mentioned in #11589 (comment) tested in InvokeBuiltinMethodViaInteropTest

@Akirathan
Copy link
Member Author

BuiltinObject super class split into a separate PR - #11861

@JaroslavTulach JaroslavTulach self-requested a review January 7, 2025 05:30
@JaroslavTulach
Copy link
Member

@Akirathan can you merge in the most recent develop branch? Then we can check and see what can be done with the rest...

# Conflicts:
#	engine/runtime-integration-tests/src/test/java/org/enso/example/PolyglotTestClass.java
#	engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/builtins/BuiltinsJavaInteropTest.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/BuiltinObject.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoDate.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoDateTime.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoDuration.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoFile.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoTimeOfDay.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoTimeZone.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/data/ManagedResource.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Ref.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/data/hash/EnsoHashMap.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/Array.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrayLikeHelpers.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/Vector.java
#	engine/runtime/src/main/java/org/enso/interpreter/runtime/warning/Warning.java
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of using static methods on Type seems to be a great solution to all the problems we had so far. The PR is simple and unlocks a functionality that was clearly missing.

Can you simplify these three known workarounds in the production code as well? Then this PR will immediately be useful for Enso users!

ctx,
() -> {
var interop = InteropLibrary.getUncached();
var refUnwrapped = ContextUtils.unwrapValue(ctx, ref);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so this is result of Ref.new 42.

assertThat(
"Ref builtin object should not have any members",
interop.hasMembers(refUnwrapped),
is(false));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Ref object has no members.

"Ref should have a meta-object (Ref type)",
interop.hasMetaObject(refUnwrapped),
is(true));
var refMeta = interop.getMetaObject(refUnwrapped);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One has to ask for Ref object's meta-object - e.g. Type

assertThat(
"Ref meta-object should have a 'get' method",
interop.isMemberInvocable(refMeta, "get"),
is(true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and there is invocable method get on the Ref type.

"Ref meta-object should have a 'get' method",
interop.isMemberInvocable(refMeta, "get"),
is(true));
var res = interop.invokeMember(refMeta, "get", new Object[] {refUnwrapped});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method can be invoked as static method on the Ref type with first argument being the Ref object.

@Akirathan
Copy link
Member Author

The approach to export methods as internal members of types and not on atoms/builtin objects was chosen because of #12043 - at the moment, it is not possible to implement hasMembers interop message on builtin objects.

@Akirathan
Copy link
Member Author

Akirathan commented Jan 13, 2025

Can you simplify these three known workarounds in the production code as well? Then this PR will immediately be useful for Enso users!

@JaroslavTulach Usages of Ref.get and Path.root simplified in 0c97057.

I tried to simplify Warning.to_text as well in WithWarnings like so:

diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/warning/WithWarnings.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/warning/WithWarnings.java
index 7b8dce1d6e..8acf02eedd 100644
--- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/warning/WithWarnings.java
+++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/warning/WithWarnings.java
@@ -8,7 +8,9 @@ import com.oracle.truffle.api.dsl.Cached.Shared;
 import com.oracle.truffle.api.interop.ArityException;
 import com.oracle.truffle.api.interop.InteropLibrary;
 import com.oracle.truffle.api.interop.TruffleObject;
+import com.oracle.truffle.api.interop.UnknownIdentifierException;
 import com.oracle.truffle.api.interop.UnsupportedMessageException;
+import com.oracle.truffle.api.interop.UnsupportedTypeException;
 import com.oracle.truffle.api.library.CachedLibrary;
 import com.oracle.truffle.api.library.ExportLibrary;
 import com.oracle.truffle.api.library.ExportMessage;
@@ -201,6 +203,7 @@ public final class WithWarnings extends EnsoObject {
     for (var w : warns) {
       try {
         var wText = node.execute(toText, state, new Object[] {w.getValue()});
+        var myText = invokeToText(w, where);
         if (wText instanceof Text t) {
           if (prefix != null) {
             text = text.add(prefix);
@@ -214,6 +217,19 @@ public final class WithWarnings extends EnsoObject {
     return text;
   }
 
+  private static Object invokeToText(Warning warn, Node where) {
+    System.out.println("[mylog] invokeToText");
+    var interop = InteropLibrary.getUncached();
+    try {
+      var meta = interop.getMetaObject(warn);
+      var toTextRes = interop.invokeMember(meta, "to_text", warn);
+      return toTextRes;
+    } catch (UnsupportedMessageException | ArityException | UnknownIdentifierException |
+             UnsupportedTypeException e) {
+      throw EnsoContext.get(where).raiseAssertionPanic(where, e.getMessage(), e);
+    }
+  }
+
   @ExportMessage
   Object send(Message message, Object[] args, @CachedLibrary(limit = "3") ReflectionLibrary lib)
       throws Exception {

But that does not work, because to_text is defined on Any, and the Type.methods method discovery does not include methods from Any, neither extension methods. I have created issue for it to track it down - #12045

Let's merge this PR with the current changes.

# Conflicts:
#	distribution/lib/Standard/Base/0.0.0-dev/src/Meta/Enso_Project.enso
@Akirathan Akirathan merged commit c2f0925 into develop Jan 13, 2025
48 checks passed
@Akirathan Akirathan deleted the wip/akirathan/11589-builtins-expose-methods branch January 13, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enso builtins don't expose Enso methods
4 participants