-
Notifications
You must be signed in to change notification settings - Fork 323
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
Conversation
@@ -150,6 +155,14 @@ public static PolyglotCallType getPolyglotCallType( | |||
UnresolvedSymbol symbol, | |||
InteropLibrary library, | |||
MethodResolverNode methodResolverNode) { | |||
if (symbol.getScope().getMethodForPolyglot(self, symbol.getName(), false) |
There was a problem hiding this comment.
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.
if (symbol.getScope().getMethodForPolyglot(self, symbol.getName(), false) | |
if (symbol.getScope().getMethodForPolyglot(self, symbol.getName(), false) != null) { |
var m = polyMethods.get(meta); | ||
if (m == null) { | ||
m = new HashMap<>(); | ||
polyMethods.put(meta, m); | ||
} | ||
m.put(name, new CachingSupplier<>(fn)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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<>(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
engine/runtime/src/main/java/org/enso/interpreter/runtime/IrToTruffleUtils.java
Show resolved
Hide resolved
|
||
group_builder.specify "extension method on IntHolder (static syntax)" <| | ||
hold = IntHolder.new (6 * 7) | ||
v = IntHolder.value_plus hold 3 |
There was a problem hiding this comment.
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 ModuleScope
s.
The approach of compiling As such I am closing this PR as going in the wrong direction. |
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:
Scala,
Java,
style guides.