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

Painless: Fix bug for static method calls on interfaces #31348

Merged
merged 2 commits into from Jun 15, 2018

Conversation

Projects
None yet
4 participants
@jdconrad
Copy link
Contributor

commented Jun 14, 2018

We were not calling static interface methods correctly leading to errors. Java 8 didn't care, but Java 9+ is stricter. Example --

java.lang.IncompatibleClassChangeError: Method java.util.Comparator.comparing(Ljava/util/function/Function;)Ljava/util/Comparator; must be InterfaceMethodref constant

StackOverflow explanation -- (https://stackoverflow.com/questions/34360718/invokestatic-on-static-method-in-interface?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa)

@elasticmachine

This comment has been minimized.

Copy link

commented Jun 14, 2018

type = owner.type;
}

if (java.lang.reflect.Modifier.isStatic(modifiers)) {
writer.invokeStatic(type, method);
} else if (java.lang.reflect.Modifier.isInterface(owner.clazz.getModifiers())) {
// special case for interfaces where the interface function needs to be set to true

This comment has been minimized.

Copy link
@nik9000

nik9000 Jun 14, 2018

Contributor

I think this'd be more clear if you said something like "invokeStatic assumes that it is not invoking a method on an interface."

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 14, 2018

Author Contributor

Agreed. Cleaned up a bit.

@@ -66,6 +66,9 @@
/** delegate method type method as type */
public final Type delegateType;

/** whether a call is made on a delegate interface */
public final int isDelegateInterface;

This comment has been minimized.

Copy link
@nik9000

nik9000 Jun 14, 2018

Contributor

I wonder if it'd be worth making this a boolean for readability. I'm fine with it staying an integer if you like but I think it is worth a comment why it is an integer and that it can only be 0 or 1.

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 14, 2018

Author Contributor

Fixed and commented.

} else if (delegateMethod.augmentation != null) {
delegateClassName = delegateMethod.augmentation.getName();
isDelegateInterface = delegateMethod.augmentation.isInterface() ? 1 : 0;

This comment has been minimized.

Copy link
@nik9000

nik9000 Jun 14, 2018

Contributor

This'd only come up if the target augmentation method is defined on an interface, right? Maybe we should not allow that at all.

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 14, 2018

Author Contributor

Agreed, but I'd like to do that in a follow up PR as we discussed.

@nik9000
Copy link
Contributor

left a comment

I think we should forbid augmentation methods on interfaces. That just feels so weird. And we can't make a test case for it anyway. But maybe that should be in a followup?

I think you should add a test for method references to static methods on an interface. Once you do that, merge away!

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2018

@nik9000 Thanks for the fast turn around on this review! Added a test for method references triggering an invokeStatic through lambdaBootstrap and fixed up a couple things based on your comments.

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2018

Will commit this as soon as it passes CI

@jdconrad jdconrad merged commit 0324103 into elastic:master Jun 15, 2018

3 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

jdconrad added a commit that referenced this pull request Jun 15, 2018

Painless: Fix bug for static method calls on interfaces (#31348)
Static method calls on interfaces were not being called correctly
which was causing JVM crashes.  This change fixes the issue.

jdconrad added a commit that referenced this pull request Jun 15, 2018

Painless: Fix bug for static method calls on interfaces (#31348)
Static method calls on interfaces were not being called correctly
which was causing JVM crashes.  This change fixes the issue.

tlrx added a commit that referenced this pull request Jun 15, 2018

Merge branch 'master' into ccr
* master:
  992c788 Uncouple persistent task state and status (#31031)
  8c6ee7d Describe how to add a plugin in Dockerfile (#31340)
  1c5cec0 Remove http status code maps (#31350)
  87a676e Do not set vm.max_map_count when unnecessary (#31285)
  e5b7137 TEST: getCapturedRequestsAndClear should be atomic (#31312)
  0324103 Painless: Fix bug for static method calls on interfaces (#31348)
  d6d0727 QA: Fix resolution of default distribution (#31351)
  fcf1e41 Extract common http logic to server (#31311)
  6dd81ea Build: Fix the license in the pom zip and tar (#31336)
  8f886cd Treat ack timeout more like a publish timeout (#31303)
  9b29327 [ML] Add description to ML filters (#31330)
  f7a0caf SQL: Fix build on Java 10
  375d09c [TEST] Fix RemoteClusterClientTests#testEnsureWeReconnect
  4877cec More detailed tracing when writing metadata (#31319)
  bbfe1ec [Tests] Mutualize fixtures code in BaseHttpFixture (#31210)

tlrx added a commit that referenced this pull request Jun 15, 2018

Merge branch '6.x' into ccr-6.x
* 6.x:
  6d711fa (origin/6.x, 6.x) Uncouple persistent task state and status (#31031)
  f0f16b7 [TEST] Make SSL restrictions update atomic (#31050)
  652193f Describe how to add a plugin in Dockerfile (#31340)
  bb568ab TEST: getCapturedRequestsAndClear should be atomic (#31312)
  6f94914 Do not set vm.max_map_count when unnecessary (#31285)
  c12f3c7 Painless: Fix bug for static method calls on interfaces (#31348)
  21128e2 QA: Fix resolution of default distribution (#31351)
  df17a83 Build: Fix the license in the pom zip and tar (#31336)
  3e76b15 Treat ack timeout more like a publish timeout (#31303)

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.