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

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

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.

Important Notes

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 Akirathan force-pushed the wip/akirathan/11589-builtins-expose-methods branch from 73633b3 to e441fab Compare December 10, 2024 13:40
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.

  • OK, let's integrate.

@JaroslavTulach
Copy link
Member

There is a lot of Table_Tests failures like this one:

org.enso.table.error.UnexpectedTypeException: Unexpected value type. Expected a java.time.LocalTime but got com.oracle.truffle.polyglot.PolyglotMap.

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Dec 11, 2024

There is a lot of invalid sharing layer:

 Test org.enso.compiler.test.ExecCompilerTest.chainedSyntax failed: org.graalvm.polyglot.PolyglotException: java.lang.AssertionError: Invalid sharing of AST nodes detected. The current context uses a different sharing layer than the executed node. A common cause of this are CallTargets that are reused across different contexts in an invalid way.Stack trace: 

I'll try to reproduce the failures locally. When running runtime-integration-tests/test I am getting:

[info] Test run org.enso.interpreter.test.interop.TypesExposeConstructorsTest finished: 0 failed, 0 ignored, 8 total, 0.005s
[error] Test org.enso.interpreter.test.hash.HashCodeTest failed: java.lang.AssertionError: It is a time like value (Error: (Compile_Error.Error 'No polyglot symbol for org.enso.base.time.EnsoDateTimeFormatter')), took 0.0 sec
[error]     at org.enso.interpreter.test.ValuesGenerator.timesAndDates(ValuesGenerator.java:531)
[error]     at org.enso.interpreter.test.hash.HashCodeTest.fetchAllUnwrappedValues(HashCodeTest.java:80)
[error]     at org.enso.interpreter.test.hash.HashCodeTest.initContextAndData(HashCodeTest.java:52)
[error]     at jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
[error]     at java.lang.reflect.Method.invoke(Method.java:580)

and then also the sharing layer error latter. I'd start by fixing the first error and hoping the subsequent ones will go away.

@Akirathan
Copy link
Member Author

Trying to first integrate #11835 seems like a dead end. Let's rather try a different approach - builtin objects do not expose members, but their types do. 752ac90 starts to implement hasMember and getMembers on a Type so that it returns methods as invocable members.

@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

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
2 participants