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

Extension methods for Java classes #9129

Closed

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Feb 21, 2024

Pull Request Description

Addresses one part of #8821 - allows definition and invocation of extension method on Java classes.

Checklist

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

  • The documentation has been updated, if necessary.
  • All code follows the
    Scala,
    Java,
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.

@@ -150,6 +155,14 @@ public static PolyglotCallType getPolyglotCallType(
UnresolvedSymbol symbol,
InteropLibrary library,
MethodResolverNode methodResolverNode) {
if (symbol.getScope().getMethodForPolyglot(self, symbol.getName(), false)
Copy link
Member

Choose a reason for hiding this comment

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

getMethodForPolyglot already returns Function type. Thus, the instanceof check here is redundant, and null check should be sufficient.

Suggested change
if (symbol.getScope().getMethodForPolyglot(self, symbol.getName(), false)
if (symbol.getScope().getMethodForPolyglot(self, symbol.getName(), false) != null) {

Comment on lines +210 to +215
var m = polyMethods.get(meta);
if (m == null) {
m = new HashMap<>();
polyMethods.put(meta, m);
}
m.put(name, new CachingSupplier<>(fn));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var m = polyMethods.get(meta);
if (m == null) {
m = new HashMap<>();
polyMethods.put(meta, m);
}
m.put(name, new CachingSupplier<>(fn));
var m = polyMethods.computeIfAbsent(meta, _ -> new HashMap<>());

@@ -200,6 +202,44 @@ public void registerPolyglotSymbol(String name, Object sym) {
polyglotSymbols.put(name, sym);
}

private final Map<String, Map<String, Supplier<Function>>> polyMethods = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

minor: Although not enforced, we tend to declare fields before methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, the code I have put into ModuleScope is so ugly that it deserves a total rewrite. With e279793 we seem to have the desirable functionality, but the implementation has to improve significantly.

Pavel, do you think the following would be a fine goal?

Give every Java class an Enso Type

I am still a bit afraid of doing so, but it would solve a lot of problems. If there was a Type wrapper around each Java class used in Enso, we could easily assign the extension methods the regular way - e.g. without duplicating all these internal fields and register methods. We would just assign them to the appropriate Type wrapping the Java class. Moreover Java types would appear in the IDE as types. Something that

wanted to achieve a long time ago.

I would just need to open Type up and a properly "cache" the type and java class mapping - guarantee it is 1:1. We probably still need the eigen type concept (even for Java classes) as that's where Enso stores static methods. The current implementation struggles a lot differentiating between static and non-static methods (see current MethodInvokeNode). And yes, re-implementing #3764 to register each method only once and just dispatch properly would make me happier, but it is another thing I am hesitating/afraid to do...


group_builder.specify "extension method on IntHolder (static syntax)" <|
hold = IntHolder.new (6 * 7)
v = IntHolder.value_plus hold 3
Copy link
Member Author

Choose a reason for hiding this comment

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

With the latest commit af736be all tests but this one seem to be passing:

    - [FAILED] extension method on IntHolder (static syntax) [98ms]
        Reason: An unexpected panic was thrown: (Type_Error.Error Integer org.enso.base_test_helpers.IntHolder@16120270 '`add`')
        at <enso> Any.value_plus(/enso/test/Base_Tests/src/Data/Polyglot_Spec.enso:78:1-54)
        at <enso> <anonymous><arg-1>(enso/test/Base_Tests/src/Data/Polyglot_Spec.enso:52:13-39)

for some reason I have an oversaturated argument there. The static calls were introduced by

With 488ca26 the tests seem to work. The code of 488ca26 is however completely horrible and needs to be polished to run on fast path somehow. PS: I still don't understand why #3764 was implemented on the level of IR and not on Truffle level and ModuleScopes.

@JaroslavTulach JaroslavTulach linked an issue Feb 22, 2024 that may be closed by this pull request
@JaroslavTulach
Copy link
Member Author

The approach of compiling Standard libraries into runner executable seems viable - #9866 (comment). That however means we don't want to expose Java objects in the system as first class types/instances. If a library compiled by NI sends object to library using regular JVM or vice versa they will not see the other object as being from (own) JVM.

As such I am closing this PR as going in the wrong direction.

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

Successfully merging this pull request may close these issues.

Extension methods & conversions for Java host types
3 participants