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

Changing Class<T> to Class<T extends Library> for Native.loadLibrary #861

Merged
merged 1 commit into from
Oct 2, 2017

Conversation

hakanai
Copy link
Contributor

@hakanai hakanai commented Sep 22, 2017

It was possible to pass a non-library class into this method and get a runtime exception, when the same check can be done at compile-time, so this fix changes it to be done at compile-time.

After erasure, with this change the return type changes from Object to Library. So as far as backwards compatibility:

  1. If you were assigning the result to Object, that will still work
  2. If you were passing something that wasn't a Class<? extends Library>, your code will now not compile, instead of failing at runtime.
  3. I can't remember the rules for binary compatibility.

Fixes #822.

It was possible to pass a non-library class into this method and get a runtime exception, when the same check can be done at compile-time, so this fix changes it to be done at compile-time.

After erasure, with this change the return type changes from `Object` to `Library`. So as far as backwards compatibility:

1. If you were assigning the result to `Object`, that will still work
2. If you were passing something that wasn't a `Class<? extends Library>`, your code will now not compile, instead of failing at runtime.
3. I can't remember the rules for binary compatibility.

Fixes java-native-access#822.
@matthiasblaesing
Copy link
Member

matthiasblaesing commented Sep 23, 2017

This indeed breaks binary compatibility. The problem is, that the return type is part of the method signature. And the constraint on the generic changes the return type from java.lang.Object to com.sun.jna.Library. So while on the fundamental level the methods are compatible this change will require a rebuild.

I'm inclined to merge this, as your argument is sound. But please give me some time to think this through.

@matthiasblaesing
Copy link
Member

Ok - I don't see a painless solution. Having both variants in the source code (one marked deprecated) would be contrary to the recent cleanup of deprecated methods and also does not comply with the java language (javac comes to the conclusion, that the generic erasure is identical). I tested ASM and was able to readd bridge methods via byte code injection, but that also does help a bit, as from the tool perspective, the old methods are still there and can be called (maybe with a deprecation warning) and the bytecode manipulation has also be done somewhere, so maintanance cost rises.

Being at the point where backwards compatibility is already broken and major number bumped, I'll merge this in the next few days, if noone raises objections or comes up with a better plan.

@matthiasblaesing matthiasblaesing merged commit 424d979 into java-native-access:master Oct 2, 2017
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

Successfully merging this pull request may close these issues.

loadLibrary method signature should check that the class extends Library at compile-time, not runtime
3 participants