Groovy with Jigsaw

[Uwe Schindler]

Hi,

 

>  Caused by: java.lang.VerifyError: class com.google.common.reflect.Element overrides final method java.lang.reflect.AccessibleObject.setAccessible(Z)V

 

This is caused by the fact that the new JDK makes setAccessible final in the base class AccessibleObject. The Google Guava class is a wrapper class around any AccessibleObject and delegates all calls to the object behind, while adding some convenience methods.

 

This is a clear backwards compatibility break. Maybe remove the final here! Otherwise Guava has to fix the code and completely remove the reflect.Element wrapper class (to me it looks useless, it just spares you a few modifier stuff because you can call isPublic()).

 

Uwe

 

-----

Uwe Schindler

uschindler at apache.org 

ASF Member, Apache Lucene PMC / Committer

Bremen, Germany

http://lucene.apache.org/

 

From: Cédric Champeau [mailto:cedric.champeau at gmail.com] 
Sent: Friday, September 11, 2015 9:47 AM
To: Alan Bateman
Cc: Uwe Schindler; Jochen Theodorou; jigsaw-dev at openjdk.java.net
Subject: Re: Groovy with Jigsaw

 

For what it's worth, the issue that triggered this conversation is the one reported by Uwe. For Groovy we have a chicken and egg problem for testing, because this change breaks Groovy, and Groovy uses Gradle to build. Since Gradle itself uses Groovy, we have no compatible build tool to test the fix... So it's very problematic. Also the build that we set up failed with:

 

[23:53:08]W:            [Gradle failure report] Caused by: java.lang.VerifyError: class com.google.common.reflect.Element overrides final method java.lang.reflect.AccessibleObject.setAccessible(Z)V

[23:53:08]W:            [Gradle failure report]           at java.lang.ClassLoader.defineClass1(java.base at 9.0/Native Method)

[23:53:08]W:            [Gradle failure report]           at java.lang.ClassLoader.defineClass(java.base at 9.0/ClassLoader.java:820)

[23:53:08]W:            [Gradle failure report]           at java.security.SecureClassLoader.defineClass(java.base at 9.0/SecureClassLoader.java:152)

 

Which indicates the change to setAccessible also broke Guava, which is used by Gradle. So it's going to be very complicated to even try to fix the issue in those conditions. Anyway, it doesn't seem a good idea to introduce a new exception type. Even if it is semantically a bit problematic, wouldn't make ReflectiveOperationException a subclass of SecurityException an option?

 

 

2015-09-11 9:39 GMT+02:00 Alan Bateman <Alan.Bateman at oracle.com>:

On 10/09/2015 23:57, Uwe Schindler wrote:

:

I also wanted to ask the question: setAccessible() already throws some checked exceptions, why invent a new one that’s no longer checked? Could it not simply be one already there - one of the checked ones - or a new one just subclassing ReflectiveOperationException? This is completely inconsistent to other core reflection APIs. Why have a totally unexpected new one?



Right, there is compatibility issue here (third item in the "Risks and Assumptions" section of JEP 261 [1]).

The setAccessible methods don't declare any checked exceptions. The only exception they declare is SecurityException. This exception make sense when running with a security manager and the specified permission check fails. This exception is the wrong exception to fail with when the member is not an an exported package as this is nothing to do with the security manager. Sadly, these methods do throw SecurityException when attempted on a constructor of java.lang.Class, I've been trying to dig into the history on this oddity.

Anyway, retrofitting these methods to throw ReflectiveOperationException (or sub-type) isn't really an option because this is a checked exception so it would add a source compatibility issue to the list. For now, the prototype API changes these methods to throw InaccessibleObjectException and we'll have to see the impact of this. I hope we aren't force to change this to throw SecurityException as it's the wrong exception, but less impact compatibility-wise because there is some chance that existing code might handle that already.

-Alan.

[1] http://openjdk.java.net/jeps/261