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

Enso builtins don't expose Enso methods #11589

Open
JaroslavTulach opened this issue Nov 19, 2024 · 12 comments · May be fixed by #11687
Open

Enso builtins don't expose Enso methods #11589

JaroslavTulach opened this issue Nov 19, 2024 · 12 comments · May be fixed by #11687
Assignees

Comments

@JaroslavTulach
Copy link
Member

Quite surprisingly Enso builtins don't expose Enso methods to org.graalvm.polyglot.Value. That results in quite surprising situations:

Looks like Enso builtin types do not expose their Enso methods to org.graalvm.polyglot.Value. Let's write ValuesGenerator based test and verify all Enso builtins expose their (public) Enso methods properly.

@Akirathan
Copy link
Member

The following test obviously fails:

@Test
public void booleanExportsMethods() {
var src = """
import Standard.Base.Runtime.Ref.Ref
main =
Ref.new 0
""";
var ref = ContextUtils.evalModule(ctx, src);
assertThat("'get' member is invocable",
ref.canInvokeMember("get"), is(true));
}

ref.canInvokeMember("get") sends isMemberInvocable("get") message to the interop library implementation of Ref. Ref does not expose such message and, therefore, fails.

Interop protocol for Atom changed recently in #11217 and its getMembers message implementation says in the docs:

/**
* Returns list of fields of the Atom. If {@code includeInternal} is true, all methods, including
* project-private, are included. Fields are returned as filed getters, i.e., methods. Only fields
* for the constructor that was used to construct this atom are returned.
*/

The obvious solution for this issue is to make the interop protocol for every builtin consistent with the protocol of Atom. More specifically, make these interop messages consistent:

  • getMembers
  • hasMembers
  • isMemberReadable
  • isMemberInvocable
  • isMemberInternal
  • getMetaObject
  • hasMetaObject

@Akirathan Akirathan moved this from ⚙️ Design to 🔧 Implementation in Issues Board Nov 27, 2024
@enso-bot
Copy link

enso-bot bot commented Nov 29, 2024

Pavel Marek reports a new STANDUP for today (2024-11-29):

Progress: - Add a test that collects all the builtin objects, and checks that they expose Enso methods (coverage almost 100%)

@Akirathan Akirathan linked a pull request Dec 4, 2024 that will close this issue
5 tasks
@enso-bot
Copy link

enso-bot bot commented Dec 4, 2024

Pavel Marek reports a new STANDUP for today (2024-12-04):

Progress: - Increased test coverage - also try to invoke some methods on the builtin types.

  • Every builtin type extends BuiltinObject.
    • Enforced during annotation processing.
  • PR ready for review. It should be finished by 2024-12-04.

@enso-bot
Copy link

enso-bot bot commented Dec 11, 2024

Pavel Marek reports a new STANDUP for the provided date (2024-12-09):

Progress: - Encountered a regression in not functional benchmarks - #11808

  • Helping Dmitry with Scala, and other dependencies, version bumps
  • Creating my so far biggest Enso workflow for analysing our benchmarks. What a great experience! It should be finished by 2024-12-11.

@enso-bot
Copy link

enso-bot bot commented Dec 11, 2024

Pavel Marek reports a new STANDUP for today (2024-12-11):

Progress: - Found the reason why tests are failing and created a minimal reproducer

GitHub
https://enso-org.github.io/engine-benchmark-results/ - enso-org/engine-benchmark-results

@enso-bot
Copy link

enso-bot bot commented Dec 12, 2024

Pavel Marek reports a new STANDUP for today (2024-12-12):

Progress: - Struggling with the blocker #11835

  • Seems like this would require non-trivial changes in Java methods in stdlibs that can be called from Enso
  • Alternative: builtin objects do not have members, but their types have
    • Type exports hasMembers method and invokeMember.
  • Rewriting the PR to the new approach - Builtins expose Enso methods #11687 (comment) It should be finished by 2024-12-17.

@enso-bot
Copy link

enso-bot bot commented Dec 13, 2024

Pavel Marek reports a new STANDUP for today (2024-12-13):

Progress: - Review of #11850

@enso-bot
Copy link

enso-bot bot commented Dec 16, 2024

Pavel Marek reports a new STANDUP for today (2024-12-16):

Progress: - Found out the bug that causes Invalid polyglot layer sharing assertion errors - #11861 (comment)

  • Tests are green, but still need to investigate the invalid sharing error.
  • Started investigation on where is the InvalidSharingAssertionError coming from - Every builtin type has a common super class #11861 (review)
    • Should every EnsoContext has a separate instance of Builtins, or can they be shared?
    • How does this relate to the static field Builtins.loadedBuiltinConstructors? It should be finished by 2024-12-17.

@enso-bot
Copy link

enso-bot bot commented Dec 17, 2024

Pavel Marek reports a new STANDUP for today (2024-12-17):

Progress: - Adding debugging info when current context is different than the one in the cached value.

  • Also store stack traces. It should be finished by 2024-12-17.

@JaroslavTulach
Copy link
Member Author

  • Should every EnsoContext has a separate instance of Builtins, or can they be shared?

No, they cannot be shared. Each EnsoContext should currently have its own instance of Builtins.

  • How does this relate to the static field Builtins.loadedBuiltinConstructors?

Right, how? I'd like to know the answer too. Preferably without me using the debugger.

@Akirathan
Copy link
Member

  • Should every EnsoContext has a separate instance of Builtins, or can they be shared?

No, they cannot be shared. Each EnsoContext should currently have its own instance of Builtins.

  • How does this relate to the static field Builtins.loadedBuiltinConstructors?

Right, how? I'd like to know the answer too. Preferably without me using the debugger.

@JaroslavTulach Answer provided in #11861 (comment)

@enso-bot
Copy link

enso-bot bot commented Dec 23, 2024

Pavel Marek reports a new STANDUP for today (2024-12-23):

Progress: - Integrate some review feedback.

mergify bot pushed a commit that referenced this issue Dec 24, 2024
This PR is separated from #11589. It only introduces [BuiltinObject](https://github.com/enso-org/enso/blob/8feab15290c45a485815619972a93ab69f34e78a/engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/BuiltinObject.java), a common supertype for all the builtin types. It does not change any behavior.

`BuiltinObject` defines `hasMetaObject`, `getMetaObject`, `hasType` and `getType` messages, so they no longer have to be implemented in subclasses.

# Important Notes
- Introduce also test [BuiltinsJavaInteropTest](https://github.com/enso-org/enso/blob/0d92891b8eecd3071f0f4f1d8c55524b637d14a8/engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/builtins/BuiltinsJavaInteropTest.java)
- Builtin annotation processor [enforces](1fe2f3e) that every builtin class extend `BuiltinObject` class.
Frizi pushed a commit that referenced this issue Dec 30, 2024
This PR is separated from #11589. It only introduces [BuiltinObject](https://github.com/enso-org/enso/blob/8feab15290c45a485815619972a93ab69f34e78a/engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/BuiltinObject.java), a common supertype for all the builtin types. It does not change any behavior.

`BuiltinObject` defines `hasMetaObject`, `getMetaObject`, `hasType` and `getType` messages, so they no longer have to be implemented in subclasses.

# Important Notes
- Introduce also test [BuiltinsJavaInteropTest](https://github.com/enso-org/enso/blob/0d92891b8eecd3071f0f4f1d8c55524b637d14a8/engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/builtins/BuiltinsJavaInteropTest.java)
- Builtin annotation processor [enforces](1fe2f3e) that every builtin class extend `BuiltinObject` class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🔧 Implementation
Development

Successfully merging a pull request may close this issue.

2 participants