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

Remove incorrect handling of covariant return types #83

Merged
merged 2 commits into from
Jul 25, 2021
Merged

Remove incorrect handling of covariant return types #83

merged 2 commits into from
Jul 25, 2021

Conversation

ogolberg
Copy link
Contributor

@ogolberg ogolberg commented Apr 22, 2020

This removes incorrect covariant return type handling introduced in MANIMALSNIFFER-49. The ticket is no longer publicly accessible, so I lack the context of the original change.

The block of code removed in this PR prevented animalsniffer from catching covariant changes to return types which were compile-time compatible but not binary-compatible.

For example, if we take the code below

ByteBuffer buffer = ...
buffer.rewind();

and compile it on Java 8, we'll get bytecode which invokes

java.nio.ByteBuffer.rewind()Ljava/nio/Bufffer

If we compile it on Java 9+, while still targeting Java 8, we will get bytecode which invokes

java.nio.ByteBuffer.rewind()Ljava/nio/ByteBuffer

That's because this method with the more specific ByteBuffer return type is available in JDK9+.

Now, if we run the latter binary on Java 8, we will get a NoSuchMethodError.

A recent example of this in the wild: grpc/grpc-java#6839 (note that grpc-java uses animalnsniffer).

@ogolberg
Copy link
Contributor Author

I also propose cutting 1.16.1 with this fix because many projects out there are using 1.16

@lukehutch
Copy link

I have been bitten by this several times (#77).

Does removing the check allow animal-sniffer to catch the problem?

@ogolberg
Copy link
Contributor Author

ogolberg commented Jul 6, 2020

Does removing the check allow animal-sniffer to catch the problem?

Correct. Covariant return types are purely a compile-time construct, so this check is extraneous.

@lukehutch
Copy link

@ogolberg No it's not just a compiletime construct -- your code will compile fine with a covariant change in return type (in particular if you ignore the return value), but then can fail at runtime with an exception.

@ogolberg
Copy link
Contributor Author

ogolberg commented Jul 6, 2020

@lukehutch yes, I think we're on the same page here.

What I mean is that the compiler does all the work to handle covariant return types (https://docs.oracle.com/javase/tutorial/java/generics/bridgeMethods.html#bridgeMethods), there's nothing special in the bytecode/vm to make them work; therefore, the special casing that I am removing in this PR is unnecessary.

@lukehutch
Copy link

But Animal Sniffer should fail and stop compilation if a covariant return type is detected. The current code detects the change but doesn't stop the build with an error. How does removing the check altogether address this problem?

@ogolberg
Copy link
Contributor Author

ogolberg commented Jul 6, 2020

the find method will fall through and return false, which eventually will bubble up as an error

@lukehutch
Copy link

lukehutch commented Jul 6, 2020

But then there will be no descriptive error message given. See my PR #87 for an alternative. (I'm not sure if the logic is correct though.)

@ogolberg
Copy link
Contributor Author

ogolberg commented Jul 6, 2020

sure, additional logging may be worth the cost of running that loop code - I was just going after correctness

@ogolberg
Copy link
Contributor Author

any chance this can be merged?

@olamy
Copy link
Member

olamy commented Jul 19, 2021

why removing an integration test?
If it was wrong it should be updated to reflect what the code change here because this change doesn't have any test to demonstrate the issue.

@ogolberg
Copy link
Contributor Author

restored/updated integration tests

@olamy olamy merged commit ba0d71e into mojohaus:master Jul 25, 2021
ogolberg added a commit to open-toast/protokt that referenced this pull request Feb 2, 2022
ogolberg added a commit to open-toast/protokt that referenced this pull request Feb 2, 2022
Addresses incorrect handling of covariant return types (i.e. the infamous ByteBuffer API issue) when validating android compatibility (see mojohaus/animal-sniffer#83).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants