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

Implement hasLanguage interop message for all enso objects #11538

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

Akirathan
Copy link
Member

Blocks #11468

Pull Request Description

This PR implements the Truffle contract that all the Enso objects must export hasLanguage and getLanguage messages. By refactoring EnsoObject from an interface to an abstract class.

Important Notes

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 12, 2024
@Akirathan Akirathan self-assigned this Nov 12, 2024
@JaroslavTulach
Copy link
Member

Quick note: I believe there are other ways to enforce the contract while keeping EnsoObject an interface.

@Akirathan Akirathan changed the title Convert EnsoObject to abstract class Implement hasLanguage interop message for all enso objects Nov 13, 2024
@Akirathan Akirathan changed the title Implement hasLanguage interop message for all enso objects Implement hasLanguage interop message for all enso objects Nov 13, 2024

/** All non-primitive Enso types extends from {@code EnsoObject}. */
public interface EnsoObject extends TruffleObject {}
@ExportLibrary(InteropLibrary.class)
Copy link
Member

Choose a reason for hiding this comment

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

I assume the goal is to provide default implementation of hasLanguage and getLanguage message for all Enso interop objects - e.g. those who implement EnsoObject interface.

  • I would hope there is a better pattern to achieve this than converting to abstract class
  • I don't like the toDisplayString to be defined, but unimplemented

Let's see if Truffle API offers some better alternative to achieve such a default common implementation of some ExportLibrary messages?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's see if Truffle API offers some better alternative to achieve such a default common implementation of some ExportLibrary messages?

I tried to leave EnsoObject as an interface, annotate it with @ExportLibrary(InteropLibrary.class), and implement hasLanguage and getLanguage as default methods. But @ExportMessage on default method on interface does not work - the Truffle DSL processor fails with an error. Any ideas are welcome.

As for toDisplayString implementation - that is required by Truffle DSL, because if you export one of getLanguage or hasLanguage messages, then, toDisplayString is abstract and has to be implemented. However, I am still not sure what is the proper way to define it here, in EnsoObject.

@Akirathan
Copy link
Member Author

An approach with leaving EnsoObject as interfce, and defining a separate class LanguageForEnsoObject with @ExportLibrary(value = InteropLibrary.class, receiverType = EnsoObject.class) does not work:

diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoObject.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoObject.java
index ce60158116..eb65b641b2 100644
--- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoObject.java
+++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoObject.java
@@ -1,6 +1,15 @@
 package org.enso.interpreter.runtime.data;

 import com.oracle.truffle.api.interop.TruffleObject;
+import com.oracle.truffle.api.library.DynamicDispatchLibrary;
+import com.oracle.truffle.api.library.ExportLibrary;
+import com.oracle.truffle.api.library.ExportMessage;

 /** All non-primitive Enso types extends from {@code EnsoObject}. */
-public interface EnsoObject extends TruffleObject {}
+@ExportLibrary(DynamicDispatchLibrary.class)
+public interface EnsoObject extends TruffleObject {
+  @ExportMessage
+  Class<?> dispatch() {
+    return LanguageForEnsoObject.class;
+  }
+}
diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/LanguageForEnsoObject.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/LanguageForEnsoObject.java
index cfd3eda4c0..6114f01e0e 100644
--- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/LanguageForEnsoObject.java
+++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/LanguageForEnsoObject.java
@@ -1,8 +1,10 @@
 package org.enso.interpreter.runtime.data;

+import com.oracle.truffle.api.interop.InteropLibrary;
+import com.oracle.truffle.api.interop.TruffleObject;
 import com.oracle.truffle.api.library.ExportLibrary;

-@ExportLibrary()
-public final class LanguageForEnsoObject {
-
+@ExportLibrary(value = InteropLibrary.class, receiverType = EnsoObject.class)
+public final class LanguageForEnsoObject implements TruffleObject {
+  // TODO: Export hasLanguage and getLanguage methods
 }

This patch fails to compile because Truffle DSL processor fails with errror: "@ExportLibrary is not supported for interfaces". And the receiver type must export DynamicDispatchLibrary.

TL;DR; EnsoObject cannot stay as an interface.

With the exception of toDisplayString message.
It is not automatically delegated because it is implemented in the super type.
@Akirathan Akirathan marked this pull request as ready for review November 14, 2024 17:57
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.

2 participants