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

Method reference support #18748

Merged
merged 13 commits into from Jun 7, 2016
Merged

Conversation

rmuir
Copy link
Contributor

@rmuir rmuir commented Jun 6, 2016

This adds support for method references (Class::virtualMethod, Class::staticMethod, Class::new).

The syntax for this was added in #18578, as a stub that throws UnsupportedOperationException. This adds the logic. It is useful because these are the simplest lambda types (non-capturing, no desugaring needed), but can make the functional apis usable to users in many ways, based on functions that already exist. For example plugging in different functions for comparators, using streams apis, etc.

There are two cases: static and dynamic. In the static case we call LambdaMetaFactory from bytecode, since we know everything we need at compile time. This is similar to javac's case, just infer interface from the method's argument type.

In the dynamic case, we don't yet know enough to do anything interesting, so we just push the reference on the stack as a string, and mark the argument as a function in a bitset passed as a static bootstrap param. Once types are known at runtime, then we do the same stuff as the static case, just with java code, and replace the "placeholders" using MethodHandles.filterArguments.

Array constructor references (foo[]::new) are not yet supported. We should do that as a followup if we want to support them, as they are a little more complicated (require us to write bridge methods). For cases like this we will need to "expand" our recipe param to be something better than a bitset, so that pointers to our own-written functions can work too.

There are also some random cleanups, and additional strictness to the whitelist: enforcing that we always whitelist covariant and generic overrides. Some of this is kinda unrelated, but happens to be here.

The PR is not yet ready. In particular it is super-messy, and there needs to be a lot of cleanup and refactoring before we should push it. For example, FunctionRef class should maybe work with MethodType rather than ASM types, and Definition should do the functional interface computation stuff (no reflection api needs to be used). The two different apis for the two ways we call this thing (Handle/MethodHandle and Type/MethodType) make things a mess at the moment. Needs help :)

@uschindler i know you are at buzzwords, but if you have some time to look, would be great to get your thoughts on some of this.

@uschindler
Copy link
Contributor

I will check and commit changes when I come back from Berlin buzzwords. 😉

Am 6. Juni 2016 15:49:33 MESZ, schrieb Robert Muir notifications@github.com:

This adds support for method references (Class::virtualMethod,
Class::staticMethod, Class::new).

The syntax for this was added in #18578, as a stub that throws
UnsupportedOperationException. This adds the logic. It is useful
because these are the simplest lambda types (non-capturing, no
desugaring needed), but can make the functional apis usable to users in
many ways, based on functions that already exist. For example plugging
in different functions for comparators, using streams apis, etc.

There are two cases: static and dynamic. In the static case we call
LambdaMetaFactory from bytecode, since we know everything we need at
compile time. This is similar to javac's case, just infer interface
from the method's argument type.

In the dynamic case, we don't yet know enough to do anything
interesting, so we just push the reference on the stack as a string,
and mark the argument as a function in a bitset passed as a static
bootstrap param. Once types are known at runtime, then we do the same
stuff as the static case, just with java code, and replace the
"placeholders" using MethodHandles.filterArguments.

Array constructor references (foo[]::new) are not yet supported. We
should do that as a followup if we want to support them, as they are a
little more complicated (require us to write bridge methods). For cases
like this we will need to "expand" our recipe param to be something
better than a bitset, so that pointers to our own-written functions can
work too.

There are also some random cleanups, and additional strictness to the
whitelist: enforcing that we always whitelist covariant and generic
overrides. Some of this is kinda unrelated, but happens to be here.

The PR is not yet ready. In particular it is super-messy, and there
needs to be a lot of cleanup and refactoring before we should push it.
For example, FunctionRef class should maybe work with MethodType rather
than ASM types, and Definition should do the functional interface
computation stuff (no reflection api needs to be used). The two
different apis for the two ways we call this thing (Handle/MethodHandle
and Type/MethodType) make things a mess at the moment. Needs help :)

@uschindler i know you are at buzzwords, but if you have some time to
look, would be great to get your thoughts on some of this.
You can view, comment on, or merge this pull request online at:

#18748

-- Commit Summary --

  • initial messy impl of painless method references
  • don't do a no-op filter, that was just for testing

-- File Changes --

M modules/lang-painless/src/main/antlr/PainlessParser.g4 (2)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/Def.java
(137)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/DefBootstrap.java
(16)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/Definition.java
(79)
A
modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java
(126)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngineService.java
(1)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java
(2)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/WriterConstants.java
(14)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/PainlessParser.java
(221)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java
(10)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EFunctionRef.java
(41)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LCall.java
(2)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefArray.java
(4)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefCall.java
(14)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefField.java
(4)
M
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LNewObj.java
(6)
M
modules/lang-painless/src/main/resources/org/elasticsearch/painless/java.lang.txt
(14)
M
modules/lang-painless/src/main/resources/org/elasticsearch/painless/java.math.txt
(2)
M
modules/lang-painless/src/main/resources/org/elasticsearch/painless/java.text.txt
(1)
M
modules/lang-painless/src/main/resources/org/elasticsearch/painless/java.time.chrono.txt
(53)
M
modules/lang-painless/src/main/resources/org/elasticsearch/painless/java.time.txt
(32)
M
modules/lang-painless/src/main/resources/org/elasticsearch/painless/java.time.zone.txt
(1)
M
modules/lang-painless/src/main/resources/org/elasticsearch/painless/java.util.txt
(43)
M
modules/lang-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.txt
(4)
M
modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicAPITests.java
(11)
M
modules/lang-painless/src/test/java/org/elasticsearch/painless/DefBootstrapTests.java
(6)
M
modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionRefTests.java
(59)
M
modules/lang-painless/src/test/java/org/elasticsearch/painless/OverloadTests.java
(2)

-- Patch Links --

https://github.com/elastic/elasticsearch/pull/18748.patch
https://github.com/elastic/elasticsearch/pull/18748.diff


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#18748

Uwe Schindler
H.-H.-Meier-Allee 63, 28213 Bremen
http://www.thetaphi.de

@uschindler
Copy link
Contributor

Hi Robert: I just fixed the methodhandles lookup object passed to LambdaMetaFactory. For real lambdas we need to pass the lookup object of the original invokedyanmic, otherwise it wont find the lambda method impl. This fixes one todo

…led code when a conversion fails. This works anyways, because fallback is allowed to throw any Throwable
@uschindler
Copy link
Contributor

Hi Robert,
to mee this looks like a good start. We should only fix the issues with the fromMethodDescriptorString() because I have the feeling this is using wrong class loader. In my opinion, we should (as you say in your XXX comment), fix the whole lookup stuff in the FunctionRef / Def class.

@uschindler
Copy link
Contributor

BTW: filterArguments should be fine!

One question: How do we handle method calls using functional references that are statically compiled (e.g., calls on non-Def methods that pass a functional ref)

@rmuir
Copy link
Contributor Author

rmuir commented Jun 7, 2016

As i mentioned above: we dont need to be doing any lookuping or classloader passing. This is just because of shitty ASM apis. We can fix that.

@rmuir
Copy link
Contributor Author

rmuir commented Jun 7, 2016

One question: How do we handle method calls using functional references that are statically compiled (e.g., calls on non-Def methods that pass a functional ref)

This is the easy case: https://github.com/rmuir/elasticsearch/blob/3238868cc43346c34fe4068c0d7e60eff4e4c387/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EFunctionRef.java#L67-L79

We just invoke LambdaMetaFactory right there, and push the result (which is e.g. a Comparator or whatever the functional interface is) on the stack.

@uschindler
Copy link
Contributor

Ah, missed that one!

Uwe Schindler
H.-H.-Meier-Allee 63, 28213 Bremen
http://www.thetaphi.de

@rmuir
Copy link
Contributor Author

rmuir commented Jun 7, 2016

Yeah, the thing done there is the same as javac, you know the type of the expected interface, so you know everything :)

@rmuir
Copy link
Contributor Author

rmuir commented Jun 7, 2016

I cleaned this up, it is not so horrible anymore. I want to iterate with further stuff as a followup.

@jdconrad
Copy link
Contributor

jdconrad commented Jun 7, 2016

LGTM. Thanks for getting method refs working.

@rmuir
Copy link
Contributor Author

rmuir commented Jun 7, 2016

Thanks for the help here @uschindler @jdconrad . The ugly parts are fixed, we can try to extend it in followup issues with some of the deferred stuff, e.g. type[]::new or object::virtual. I am working on changes for some of those already.

@rmuir rmuir merged commit 90f2aab into elastic:master Jun 7, 2016
@jdconrad jdconrad mentioned this pull request Jun 7, 2016
18 tasks
@clintongormley clintongormley changed the title painless: method reference support Method reference support Jun 13, 2016
@clintongormley clintongormley added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache and removed :Plugin Lang Painless labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement v5.0.0-alpha4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants