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

Animal Sniffer is missing proper support for @PolymorphicSignature methods (such as MethodHandles.invoke) #67

Open
lukehutch opened this issue Aug 14, 2019 · 4 comments

Comments

@lukehutch
Copy link

lukehutch commented Aug 14, 2019

For the code

AccessibleObject field = /* ... */;

MethodHandle isAccessible =
        MethodHandles.lookup().findVirtual(AccessibleObject.class, "isAccessible",
            MethodType.methodType(boolean.class));

accessible.set((Boolean) isAccessible.invokeExact(field));

I get the following for org.codehaus.mojo.signature:java17:1.0:

[ERROR] Undefined reference: Boolean java.lang.invoke.MethodHandle.invokeExact(java.lang.reflect.AccessibleObject)

However this method is defined in JDK 1.7:

https://docs.oracle.com/javase/7/docs/api/java/lang/invoke/MethodHandle.html#invokeExact(java.lang.Object...)

MethodHandle::invoke is also missing from the java17 signatures (and maybe other things).

@lukehutch
Copy link
Author

Hi, pinging on this again... I can't use MethodHandle in my project until this is fixed, so method lookups are running much slower than they should be.

@lukehutch
Copy link
Author

lukehutch commented Nov 26, 2019

I discovered that if I change

accessible.set((Boolean) isAccessible.invokeExact(field));

to

accessible.set((Boolean) isAccessible.invokeExact(new Object[] {field}));

I get

Covariant return type change detected: Object java.lang.invoke.MethodHandle.invoke(Object[]) has 
been changed to Boolean java.lang.invoke.MethodHandle.invoke(Object[])

This explains part of the problem. This is the same issue as experienced with a change in return type of several ByteBuffer methods:

plasma-umass/doppio#497 (comment)

However, Animal Sniffer was not able to detect the covariant return type problem without the cast to (Object), instead giving an error Unidentified reference: Boolean java.lang.invoke.MethodHandle.invokeExact(java.lang.reflect.AccessibleObject) (which made it very difficult to find out what was wrong). This seems to be a bug or shortcoming in Animal Sniffer itself, not the signature files.

There is a second problem though: in fact this is a false positive. invokeExact (and invoke etc.) still returns Object today in JDK 13. I just happened to cast the result to Boolean because the isAccessible method happens to return boolean.

However there is a third problem, due to Animal Sniffer not being able to recognize that a single parameter of type AccessibleObject matches a varargs parameter of type Object.... If I change the line to the following:

accessible.set((Boolean) isAccessible.invokeExact(new Object[] {field}));

then I still get

Undefined reference: Object java.lang.invoke.MethodHandle.invoke(java.lang.reflect.AccessibleObject)

The only way to fix this error completely is to change the code to:

accessible.set((Boolean) (Object) isAccessible.invokeExact(new Object[] {field}));

which is (I assume) less efficient than calling the varargs method the normal way. Why does Animal Sniffer require this new Object[] {...} in order to match the method?

@lukehutch
Copy link
Author

lukehutch commented Nov 26, 2019

Furthermore when trying to call a static method from a MethodHandle, the wrapping of parameters in new Object[] {...} does not even work. I get:

java.lang.invoke.WrongMethodTypeException: cannot convert MethodHandle(ParamType1,ParamType2)ReturnType to (Object[])Object

I think the real issue here is that Animal Sniffer is missing explicit support for @PolymorphicSignature methods, and they cannot be supported properly just using type inference or type coercion.

@lukehutch lukehutch changed the title Java-1.7 signatures are missing MethodHandle::invokeExact Animal Sniffer is missing proper support for @PolymorphicSignature methods Nov 26, 2019
@lukehutch lukehutch changed the title Animal Sniffer is missing proper support for @PolymorphicSignature methods Animal Sniffer is missing proper support for @PolymorphicSignature methods (such as MethodHandles.invoke) Oct 11, 2021
@lukehutch
Copy link
Author

Hi, I ran into this again. Can somebody from Mojohaus please take a look at this?

copybara-service bot pushed a commit to google/guava that referenced this issue Jun 13, 2023
This is the first use of a Java 9 API in Guava, but we use the API only when it's available, so we maintain compatibility with Java 8. Use of Java 9 APIs is relevant to #6549 and #3990 (and also mojohaus/animal-sniffer#67).

I didn't make the same change for `guava-android`, which [will add `java.util.zip.CRC32C` in API Level 34](https://developer.android.com/reference/java/util/zip/CRC32C). I don't know if Android is providing similar performance improvements, so it might not even matter. But even if I wanted to do it, I can't with my current approach, which relies on `MethodHandle`—unless I want to make even the usage of `MethodHandle` conditional on a reflective check :)

RELNOTES=`hash`: Enhanced `crc32c()` to use Java's hardware-accelerated implementation where available.
PiperOrigin-RevId: 539722059
copybara-service bot pushed a commit to google/guava that referenced this issue Jun 13, 2023
This is the first use of a Java 9 API in Guava, but we use the API only when it's available, so we maintain compatibility with Java 8. Use of Java 9 APIs is relevant to #6549 and #3990 (and also mojohaus/animal-sniffer#67).

I didn't make the same change for `guava-android`, which [will add `java.util.zip.CRC32C` in API Level 34](https://developer.android.com/reference/java/util/zip/CRC32C). I don't know if Android is providing similar performance improvements, so it might not even matter. But even if I wanted to do it, I can't with my current approach, which relies on `MethodHandle`—unless I want to make even the usage of `MethodHandle` conditional on a reflective check :)

RELNOTES=`hash`: Enhanced `crc32c()` to use Java's hardware-accelerated implementation where available.
PiperOrigin-RevId: 539722059
copybara-service bot pushed a commit to google/guava that referenced this issue Jun 13, 2023
This is the first use of a Java 9 API in Guava, but we use the API only when it's available, so we maintain compatibility with Java 8. Use of Java 9 APIs is relevant to #6549 and #3990 (and also mojohaus/animal-sniffer#67).

I didn't make the same change for `guava-android`, which [will add `java.util.zip.CRC32C` in API Level 34](https://developer.android.com/reference/java/util/zip/CRC32C). I don't know if Android is providing similar performance improvements, so it might not even matter. But even if I wanted to do it, I can't with my current approach, which relies on `MethodHandle`—unless I want to make even the usage of `MethodHandle` conditional on a reflective check :)

RELNOTES=`hash`: Enhanced `crc32c()` to use Java's hardware-accelerated implementation where available.
PiperOrigin-RevId: 539722059
copybara-service bot pushed a commit to google/guava that referenced this issue Jun 13, 2023
This is the first use of a Java 9 API in Guava, but we use the API only when it's available, so we maintain compatibility with Java 8. Use of Java 9 APIs is relevant to #6549 and #3990 (and also mojohaus/animal-sniffer#67).

I didn't make the same change for `guava-android`, which [will add `java.util.zip.CRC32C` in API Level 34](https://developer.android.com/reference/java/util/zip/CRC32C). I don't know if Android is providing similar performance improvements, so it might not even matter. But even if I wanted to do it, I can't with my current approach, which relies on `MethodHandle`—unless I want to make even the usage of `MethodHandle` conditional on a reflective check :)

RELNOTES=`hash`: Enhanced `crc32c()` to use Java's hardware-accelerated implementation where available.
PiperOrigin-RevId: 539983316
chrisvest added a commit to chrisvest/netty that referenced this issue May 3, 2024
Animal Sniffer inspects bytecode call-sites to look for calls to methods not defined by Java 8.
However, it doesn't know about signature polymorphic methods, like MethodHandle.invokeExact(), so we will always get warnings about those calls.

Since we can now natively build on Java 8 (this was previously not possible with Java 6), we can instead rely on the test suite to make sure we don't make calls to methods not available in Java 8.

The associated animal sniffer issue is mojohaus/animal-sniffer#67
normanmaurer pushed a commit to netty/netty that referenced this issue May 7, 2024
Motivation:
We've been using reflection as a way to abstract over Java API versions.
Now that we're based on Java 8, some of this reflection usage is no
longer necessary. In some cases, the APIs we were reflecting on are now
directly available. In other cases, we'd like a little more performance
and can call through method handles instead.

Modification:
Remove some reflection usage that was necessary when running on Java 6
or 7, and replace it with direct calls if the code is still needed.
Replace the more performance sensitive reflection usage with
MethodHandles.

Also remove the animal sniffer maven plug-in, because animal sniffer
does not support signature polymorphic method calls.
See mojohaus/animal-sniffer#67

Result:
Cleaner, and perhaps slightly faster, code.

Fixes #14009
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

No branches or pull requests

1 participant