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

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

Closed
hakanai opened this issue May 31, 2017 · 3 comments · Fixed by #861

Comments

@hakanai
Copy link
Contributor

hakanai commented May 31, 2017

loadLibrary as of v4.4.0 currently looks like this:

{code}
public static T loadLibrary(String name, Class interfaceClass, Map<String, ?> options) {
if (!Library.class.isAssignableFrom(interfaceClass)) {
throw new IllegalArgumentException("Interface (" + interfaceClass.getSimpleName() + ")"
+ " of library=" + name + " does not extend " + Library.class.getSimpleName());
}
{code}

This allows code to compile but then throws an error at runtime, which is trappy. We hit this when we had a library defined as:

{code}
public interface Kernel32Custom extends WinNT
{code}

WinNT did extend Library in v4.0.0 but does not in v4.4.0.

Instead, the signature should just take <T extends Library>.

{code}
public static T loadLibrary(String name, Class interfaceClass, Map<String, ?> options) {
{code}

Then this change would have shown up as a compile-time error, making it possible to catch the issue much sooner.

@Groostav
Copy link
Contributor

Groostav commented Jun 7, 2017

@trejkaz I believe JNA still targets pre 1.5 VM targets, so you'll notice there's a lot of features from newer language revisions (including generics) that JNA simply doesn't use.

@hakanai
Copy link
Contributor Author

hakanai commented Jun 7, 2017

Not really a great excuse, given that the class in question is already using generics, but OK.

@matthiasblaesing
Copy link
Member

@trejkaz nobody is apologizing. Indeed JNA uses generics and could be optimized. Feel free to suggest a fix (via PR). I just want to point out, that in the past (when generics were first introduced), decisions were made that were wrong in hindsight.

So when changes are suggested, backwards compatibility needs to be taken into account (at least on the binary level, but slight less important, on the source level).

hakanai pushed a commit to hakanai/jna that referenced this issue 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 java-native-access#822.
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 a pull request may close this issue.

3 participants