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

Update from 2.4.4 to 2.4.7 broke Java11 compatibility (Could not find sun.misc.Unsafe) #197

Closed
don-vip opened this Issue Jul 3, 2018 · 21 comments

Comments

Projects
None yet
3 participants
@don-vip
Copy link
Contributor

don-vip commented Jul 3, 2018

What steps will reproduce the problem?

We updated equalsverifier from 2.4.4 to 2.4.7.

What is the code that triggers this problem?

All our 54 unit tests calling equalsverifier.

Provide an example of a complete class (equals method, hashCode method, relevant fields) and a call to EqualsVerifier.

    @Test
    public void testEqualsContract() {
        TestUtils.assumeWorkingEqualsVerifier();
        EqualsVerifier.forClass(NodePair.class).usingGetClass()
            .suppress(Warning.ANNOTATION) // FIXME: remove it after https://github.com/jqno/equalsverifier/issues/152 is fixed
            .withPrefabValues(Node.class, new Node(1), new Node(2))
            .verify();
    }

What error message or stack trace does EqualsVerifier give?

java.lang.UnsupportedOperationException: Cannot define class using reflection: Could not find sun.misc.Unsafe
	at nl.jqno.equalsverifier.internal.lib.bytebuddy.dynamic.loading.ClassInjector$UsingReflection$Dispatcher$Initializable$Unavailable.defineClass(ClassInjector.java:334)
	at nl.jqno.equalsverifier.internal.lib.bytebuddy.dynamic.loading.ClassInjector$UsingReflection.inject(ClassInjector.java:187)
	at nl.jqno.equalsverifier.internal.lib.bytebuddy.dynamic.loading.ClassLoadingStrategy$Default$InjectionDispatcher.load(ClassLoadingStrategy.java:205)
	at nl.jqno.equalsverifier.internal.lib.bytebuddy.dynamic.TypeResolutionStrategy$Passive.initialize(TypeResolutionStrategy.java:79)
	at nl.jqno.equalsverifier.internal.lib.bytebuddy.dynamic.DynamicType$Default$Unloaded.load(DynamicType.java:4247)
	at nl.jqno.equalsverifier.internal.reflection.Instantiator.giveDynamicSubclass(Instantiator.java:91)
	at nl.jqno.equalsverifier.internal.reflection.Instantiator.instantiateAnonymousSubclass(Instantiator.java:70)
	at nl.jqno.equalsverifier.internal.reflection.ObjectAccessor.copyIntoAnonymousSubclass(ObjectAccessor.java:109)
	at nl.jqno.equalsverifier.internal.checkers.HierarchyChecker.checkSubclass(HierarchyChecker.java:122)
	at nl.jqno.equalsverifier.internal.checkers.HierarchyChecker.check(HierarchyChecker.java:46)
	at nl.jqno.equalsverifier.EqualsVerifier.verifyWithExamples(EqualsVerifier.java:425)
	at nl.jqno.equalsverifier.EqualsVerifier.performVerification(EqualsVerifier.java:387)
	at nl.jqno.equalsverifier.EqualsVerifier.verify(EqualsVerifier.java:358)

What did you expect?

No error (as for 2.4.4)

Which version of EqualsVerifier are you using?

2.4.7

Please provide any additional information below.

Java 11 EA build 19. No problem with Java 8, 9, 10.

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Jul 3, 2018

Thanks for reporting this. In fact, I was already working on support for Java 11. (I hope to be able to finish and release that before I go on vacation: 🤞)

I'm surprised, though, that 2.4.4 works on Java 11. I would have expected 2.4.4 to fail as well.

@jqno jqno added the accepted label Jul 3, 2018

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Jul 3, 2018

Also, I guess you can now remove that suppress warning line 😉

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Jul 4, 2018

I'm down to only 3 failing tests, which I can fix by relying on experimental features from ASM (specifically, Opcodes.ASM7_EXPERIMENTAL). I'm not sure if I'm comfortable releasing it that way, though. Are you in any kind of rush with this?

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Jul 4, 2018

(Also: did I see in your jenkins log that you're running JaCoCo on JDK11? If so, how? This is also still a loose end for me.)

@don-vip

This comment has been minimized.

Copy link
Contributor Author

don-vip commented Jul 4, 2018

No rush, it can wait after your holidays if needed :) Technically we use jacoco but we have disabled coverage with JDK11 as it does not work yet, we're waiting for the new jacoco release to re-enable it.

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Jul 4, 2018

I've released what I have so far. As long as you don't use annotations, you should be good on Java 11. If you do, you might run into some issues, which you can suppress with Warning.ANNOTATIONS. So I guess you should put that suppress warning line back again 😉.

I'll leave this issue open until I have a complete solution for Java 11 -- which is probably after a new version of ASM is released.

simon04 pushed a commit to openstreetmap/josm that referenced this issue Jul 5, 2018

floscher pushed a commit to floscher/josm that referenced this issue Jul 5, 2018

simon04 pushed a commit to openstreetmap/josm that referenced this issue Jul 5, 2018

add workaround to jqno/equalsverifier#197
git-svn-id: https://josm.openstreetmap.de/svn/trunk@14004 0c6e7542-c601-0410-84e7-c038aed88b3b

floscher pushed a commit to floscher/josm that referenced this issue Jul 6, 2018

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Sep 26, 2018

I decided to release with ASM's experimental Java 11 support for now after all, since Java 11 is now officially out. I'll do a new release some time after ASM comes with an update.

Check out version 3.0 :)

@jqno jqno closed this Sep 26, 2018

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Sep 26, 2018

PS I found a way to make the ANNOTATION warning obsolete, so I removed it from the API entirely. So, now you'll have to remove that suppression ;).

@raphw

This comment has been minimized.

Copy link

raphw commented Sep 26, 2018

What is it that the annotation cache is for? I wonder if you could do that easier with Byte Buddy's TypePool.

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Sep 26, 2018

I use it to store annotations on the class that EqualsVerifier tests, and its fields, and also the class and fields of its fields, recursively. At least, the annotations that are relevant to how EV works. I used to just read them on demand, but reading the same class for the same annotations over and over using ASM got pretty slow.

What does TypePool do? If I can use that, I could cut the dependency on ASM, which would be nice. :)

@raphw

This comment has been minimized.

Copy link

raphw commented Sep 26, 2018

What stops you from using the reflection API for that?

TypePool is basically a class file parser that exposes the same API as the Java reflection API but can extract compile-time visible annotations.

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Sep 26, 2018

Sometimes these annotations have a RetentionPolicy.CLASS, for example FindBugs's @NonNull.
From what you say, it looks like TypePool can do exactly what I need. Do you have an example of how to use it somewhere? I'd love to look into this further.

@raphw

This comment has been minimized.

Copy link

raphw commented Sep 26, 2018

As simple as:

TypeDescription desc = TypePool.Default.of(someClass.getClassLoader())
  .describe(someClass.getName())
  .resolve();

If several classes have the same class loader, it helps to reuse the type pool as it caches classes internally.

You can probably use TypePool.Default.WithLazyResolution if you filter many annotations by name. This avoid I/O until you are using a property. You can also load annotations once you looked them up, eg:

AnnotationDescription.Loadable<NonNull> annonDesc = desc.getDeclaredAnnotations()
  .ofType(NonNull.class);
if (annonDesc != null) {
  NonNull nn = annotationDesc.loadSilent();
}

Byte Buddy also detects if the current VM supports type annotations and makes them available on demand. (Important difference: calling getDeclaredAnnotations() on a TypeDescription.Generic returns type annotations, on a non-generic TypeDescription, it returns regular annotations.

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Sep 26, 2018

That's pretty cool! I'll open an issue to look into this later. Thanks for pointing this out to me, I had no idea :).

@raphw

This comment has been minimized.

Copy link

raphw commented Sep 26, 2018

There is nothing wrong with how you are doing it, I just thought that this might make it easier. I discovered hundrets of class file corner cases, especially with type annotations, I guess you might be able to benefit from that. Glad I could help!

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Sep 26, 2018

The ability to mooch off of your hard work is of course a very nice benefit ;) , but mainly I'm happy about the fact that adopting this will enable me to drop the dependency on ASM (this is the only thing I use it for directly) and hopefully simplify my code in the process. So thank you!
I think I'm taking a small break from EqualsVerifier now, but I'm excited to take a look at this when I go back to it.

@don-vip

This comment has been minimized.

Copy link
Contributor Author

don-vip commented Sep 26, 2018

Thanks a lot Rafael! It's very nice to see upstream developers helping downstream projects 👍

@raphw

This comment has been minimized.

Copy link

raphw commented Sep 26, 2018

Plan for world domination:

  1. Make everybody use Byte Buddy
  2. ???
@raphw

This comment has been minimized.

Copy link

raphw commented Oct 11, 2018

Did you update? What Java version are you using?

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Oct 11, 2018

Indeed, as @raphw says, updating to 3.0 should fix the problem. Check out the migration guide here: http://jqno.nl/equalsverifier/migration2to3/

@jqno

This comment has been minimized.

Copy link
Owner

jqno commented Dec 4, 2018

@raphw Thanks again for the suggestion: I was able to completely remove the (direct) dependency on ASM, the code is now easier to comprehend, and it's also about 60 lines shorter. A nice improvement!

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.