Clojure

Reflection on internal classes fails under Java 9

Details

  • Patch:
    Code and Test
  • Approval:
    Ok

Description

Due to changes in reflective access for the Jigsaw module system in Java 9, the Reflector will now fail on some cases that worked in previous Java versions.

(def fac (javax.xml.stream.XMLInputFactory/newInstance))
(.createXMLStreamReader fac (java.io.StringReader. ""))

Here fac will be an instance of com.sun.xml.internal.stream.XMLInputFactoryImpl, which is an extension of javax.xml.stream.XMLInputFactory. In the new java.xml module, javax.xml.stream is an exported package, but the XMLInputFactoryImpl is an internal extension of the public abstract class in that package. The invocation of createXMLStreamReader will be reflective and the Reflector will attempt to invoke the method based on the implementation class, which is not accessible outside the module, yielding:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by clojure.lang.Reflector (file:/Users/alex/.m2/repository/org/clojure/clojure/1.10.0-alpha8/clojure-1.10.0-alpha8.jar) to method com.sun.xml.internal.stream.XMLInputFactoryImpl.createXMLStreamReader(java.io.Reader)
WARNING: Please consider reporting this to the maintainers of clojure.lang.Reflector
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

A workaround here is to avoid the reflective call by type-hinting to the public exported interface but that defeats the point of reflection support:

(.createXMLStreamReader ^javax.xml.stream.XMLInputFactory fac (java.io.StringReader. ""))

Another (undesirable) workaround is to export the private package from java.xml to the unnamed module (which is the module used when code is loaded from the classpath rather than from a module) when invoking java/javac:

java --add-exports=java.xml/com.sun.xml.internal.stream=ALL-UNNAMED --add-exports=java.xml/com.sun.xml.internal.stream.writers=ALL-UNNAMED --add-exports=java.xml/com.sun.org.apache.xerces.internal.impl=ALL-UNNAMED ...

Alternatives:

We have investigated a number of options to enhance the Reflector. When we are trying to reflect, the only class we know is the class of the target object (here a non-public class, but one we can look at due to the default settings of --illegal-access). Some options considered include:

  • checking canAccess() on the Method (here it returns true, so no help)
  • invoking and looking for IllegalAccessError (pretty gross)
  • traversing the super class and interface hierarchy to find "more public" instances (this really is replicating logic that already exists in the Java method lookup logic)

None of these is "good".

Proposed:

This continues to be a warning on Java 9, 10, and (just released) 11. The warning is accurate - you are using reflection to a method on a non-exported class. The default setting in Java 9, 10, 11 is --illegal-access=permit which allows this but reports a warning on the first occurrence (other settings are warn, debug, and deny). In some future Java, the default may be deny.

If at some point in the future the JVM changes the default to deny, we still want these reflective calls to work. In that case, we will know this situation has occurred because Method#canAccess will return false. When that happens, we need to look up through the superclasses of the target object to find one that has an accessible version of the method.

Patch: clj-2066-6.patch - when invoking an instance method, map each method to an accessible super class method, then filter non-null methods found (presumably all of them). When on Java 8 or when --illegal-access=permit (the default on Java 9, 10, and 11), then the canAccess() check on each method will return true, allowing it to occur. If --illegal-access=deny, then canAccess() will be false and super class methods will be used instead.

Cases to test for screening (all with example at top):

  • Java 8 - should work with no warnings
  • Java 9, 10, and 11 with default JVM options (should succeed, but warn per --illegal-access=permit)
  • Java 9, 10, and 11 with --illegal-access=deny (should succeed, with no warnings)
  • Java 9, 10, and 11 with "--add-opens java.xml/com.sun.xml.internal.stream=ALL-UNNAMED" - should succeed, with no warnings

More info:

  1. clj-2066-4.patch
    21/Sep/18 3:44 PM
    3 kB
    Alex Miller
  2. clj-2066-5.patch
    02/Oct/18 4:14 PM
    3 kB
    Alex Miller
  3. clj-2066-6.patch
    03/Oct/18 2:09 PM
    4 kB
    Alex Miller
  4. clj-2066-check-accessibility.patch
    25/Aug/18 9:18 PM
    12 kB
    Ghadi Shayban
  5. tcrawley.CLJ-2066.2017-03-16.patch
    16/Mar/17 9:20 AM
    7 kB
    Toby Crawley
  6. tcrawley.CLJ-2066.2017-09-07.patch
    07/Sep/17 7:23 AM
    7 kB
    Toby Crawley

Activity

Hide
Toby Crawley added a comment -
Show
Toby Crawley added a comment - This is the root cause of http://dev.clojure.org/jira/browse/DXML-32
Hide
Toby Crawley added a comment -

I don't see any indication that Java 9 will change in our favor, so I'm attaching a patch to fix this in Clojure itself.

Show
Toby Crawley added a comment - I don't see any indication that Java 9 will change in our favor, so I'm attaching a patch to fix this in Clojure itself.
Hide
Alex Miller added a comment -

Screening comments:

I think by just inspecting the patch that the new code will attempt to invoke the method on all super classes and interfaces, many of which will not have the applicable method and will thus throw IllegalArgumentException, possibly before finding the correct one. Can we reduce the effort here by only attempting the call on the super types that actually have the method? It would be good to expand the tests to also include this case if possible if I'm just mis-reading things (I did not attempt to construct this scenario).

Also, is there a method we can call to determine whether the method is accessible before invoking and needing to catch IllegalAccessException? (Obviously, we would want something that is not new in Java 9 so that this call worked on older JDKs too.)

Show
Alex Miller added a comment - Screening comments: I think by just inspecting the patch that the new code will attempt to invoke the method on all super classes and interfaces, many of which will not have the applicable method and will thus throw IllegalArgumentException, possibly before finding the correct one. Can we reduce the effort here by only attempting the call on the super types that actually have the method? It would be good to expand the tests to also include this case if possible if I'm just mis-reading things (I did not attempt to construct this scenario). Also, is there a method we can call to determine whether the method is accessible before invoking and needing to catch IllegalAccessException? (Obviously, we would want something that is not new in Java 9 so that this call worked on older JDKs too.)
Hide
Herwig Hochleitner added a comment -

This may be a stupid question, but has there been any research, whether [1] and [2] could be utilized to handle this without catching exceptions?

[1] http://download.java.net/java/jdk9/docs/api/java/lang/Class.html#getModule--
[2] http://download.java.net/java/jdk9/docs/api/java/lang/reflect/Module.html#isExported-java.lang.String-java.lang.reflect.Module-

Show
Herwig Hochleitner added a comment - This may be a stupid question, but has there been any research, whether [1] and [2] could be utilized to handle this without catching exceptions? [1] http://download.java.net/java/jdk9/docs/api/java/lang/Class.html#getModule-- [2] http://download.java.net/java/jdk9/docs/api/java/lang/reflect/Module.html#isExported-java.lang.String-java.lang.reflect.Module-
Hide
Alex Miller added a comment -

Nope, that's the kind of thing I was asking about in my comment. However, those are jdk 9 only methods and right now Clojure supports jdk 6+. That's not impossible to deal with but does increase the difficulty.

Show
Alex Miller added a comment - Nope, that's the kind of thing I was asking about in my comment. However, those are jdk 9 only methods and right now Clojure supports jdk 6+. That's not impossible to deal with but does increase the difficulty.
Hide
Toby Crawley added a comment -

Alex: you are correct, an intermediate ancestor that does not provide the method will cause the lookup to fail - I'll rework and provide a new patch with better tests.

Herwig: we could build Clojure as a multi-release jar in order to have code that can use Java 9 features, but that would definitely complicate the build. However, there may be other Java 9 issues that may require us to take that option (I'm investigating one potential issue now).

Show
Toby Crawley added a comment - Alex: you are correct, an intermediate ancestor that does not provide the method will cause the lookup to fail - I'll rework and provide a new patch with better tests. Herwig: we could build Clojure as a multi-release jar in order to have code that can use Java 9 features, but that would definitely complicate the build. However, there may be other Java 9 issues that may require us to take that option (I'm investigating one potential issue now).
Hide
Alex Miller added a comment -

Yeah, I was looking at the multi release jar stuff yesterday. I would really like to avoid that if at all possible as it adds a bunch of work for build, ci, and test.

Show
Alex Miller added a comment - Yeah, I was looking at the multi release jar stuff yesterday. I would really like to avoid that if at all possible as it adds a bunch of work for build, ci, and test.
Hide
Toby Crawley added a comment -

I've attached a new patch (tcrawley.CLJ-2066.2017-03-13.patch) and removed the old one (tcrawley.CLJ-2066.2017-02-14.patch). The new patch checks for any matching methods before calling invokeMatchingMethod; this will prevent the lack of the method on an ancestor from throwing IllegalArgumentException and aborting the lookup process.

I wasn't able to provide a test for the case where the method was missing on an ancestor, since that requires trying to invoke the method through an interface that doesn't provide it (all subclasses will provide methods from parent classes, so you need an ancestor tree that pulls in an interface), and I couldn't find anything in Java's stdlib that did that and was a non-exposed class. To implement a proper test for this, I believe we'd need to construct a module ourselves, which would require changes to the build process to build that module (and run the test) only under Java 9. I did, however, confirm locally that the old code was broken in this regard, and that the new patch fixes it.

Show
Toby Crawley added a comment - I've attached a new patch (tcrawley.CLJ-2066.2017-03-13.patch) and removed the old one (tcrawley.CLJ-2066.2017-02-14.patch). The new patch checks for any matching methods before calling invokeMatchingMethod; this will prevent the lack of the method on an ancestor from throwing IllegalArgumentException and aborting the lookup process. I wasn't able to provide a test for the case where the method was missing on an ancestor, since that requires trying to invoke the method through an interface that doesn't provide it (all subclasses will provide methods from parent classes, so you need an ancestor tree that pulls in an interface), and I couldn't find anything in Java's stdlib that did that and was a non-exposed class. To implement a proper test for this, I believe we'd need to construct a module ourselves, which would require changes to the build process to build that module (and run the test) only under Java 9. I did, however, confirm locally that the old code was broken in this regard, and that the new patch fixes it.
Hide
Toby Crawley added a comment -

After reviewing tcrawley.CLJ-2066.2017-03-13.patch, I can see a new issue with it: if the method being invoked exists only on the non-public class, then the patch will throw an IllegalArgumentException claiming that the method doesn't exist instead of the original, more accurate IllegalAccessException. I'll rework and provide another patch.

Show
Toby Crawley added a comment - After reviewing tcrawley.CLJ-2066.2017-03-13.patch, I can see a new issue with it: if the method being invoked exists only on the non-public class, then the patch will throw an IllegalArgumentException claiming that the method doesn't exist instead of the original, more accurate IllegalAccessException. I'll rework and provide another patch.
Hide
Toby Crawley added a comment -

New patch attached (tcrawley.CLJ-2066.2017-03-16.patch) that will properly throw the original IllegalAccessException when the method only exists on the non-public class. Patch tcrawley.CLJ-2066.2017-03-13.patch has been deleted.

Show
Toby Crawley added a comment - New patch attached (tcrawley.CLJ-2066.2017-03-16.patch) that will properly throw the original IllegalAccessException when the method only exists on the non-public class. Patch tcrawley.CLJ-2066.2017-03-13.patch has been deleted.
Hide
Ghadi Shayban added a comment -

In JDK9 public classes housed in a non-exported package are now inaccessible. Previously public classes might not be public any more. Clojure should link to them neither non-reflectively nor reflectively – perhaps a slightly larger adjustment to the compiler/reflector method lookup?

Show
Ghadi Shayban added a comment - In JDK9 public classes housed in a non-exported package are now inaccessible. Previously public classes might not be public any more. Clojure should link to them neither non-reflectively nor reflectively – perhaps a slightly larger adjustment to the compiler/reflector method lookup?
Hide
Herwig Hochleitner added a comment -

Ghadi: does that mean, that we can't even get at the Class object to call http://download.java.net/java/jdk9/docs/api/java/lang/Class.html#getModule-- ?

Show
Herwig Hochleitner added a comment - Ghadi: does that mean, that we can't even get at the Class object to call http://download.java.net/java/jdk9/docs/api/java/lang/Class.html#getModule-- ?
Hide
Alex Miller added a comment -

My understanding (which may be faulty) is that the new module system is independent from the classloader setup (although the base classloader hierarchy has changed a bit) and that reflection is still largely the same with respect to examining classes and methods. But you may run into issues with invoking constructors or methods that are not allowed according to access restrictions. I'm not sure if the ability to load classes is affected (but I didn't think so).

Show
Alex Miller added a comment - My understanding (which may be faulty) is that the new module system is independent from the classloader setup (although the base classloader hierarchy has changed a bit) and that reflection is still largely the same with respect to examining classes and methods. But you may run into issues with invoking constructors or methods that are not allowed according to access restrictions. I'm not sure if the ability to load classes is affected (but I didn't think so).
Hide
Alex Miller added a comment -

Toby Crawley comments on the current patch:

  • make ancestors() and isAccessException() private
  • ancestors() is recursive and could blow the stack. should be iterative instead.
  • in invokeInstanceMethod/invokeNoArgInstanceMember, couldn't you have two catch blocks there, one for IllegalAccessException and one for Exception? Or does the type checker not like that?
  • seems like the logic in invokeInstanceMethod/invokeNoArgInstanceMember could get pushed into invokeMatchingMethod rather than have two copies?
Show
Alex Miller added a comment - Toby Crawley comments on the current patch:
  • make ancestors() and isAccessException() private
  • ancestors() is recursive and could blow the stack. should be iterative instead.
  • in invokeInstanceMethod/invokeNoArgInstanceMember, couldn't you have two catch blocks there, one for IllegalAccessException and one for Exception? Or does the type checker not like that?
  • seems like the logic in invokeInstanceMethod/invokeNoArgInstanceMember could get pushed into invokeMatchingMethod rather than have two copies?
Hide
Toby Crawley added a comment - - edited

I've added a new patch (tcrawley.CLJ-2066.2017-09-07.patch), and left the old patch (tcrawley.CLJ-2066.2017-03-16.patch) attached for comparison.

make ancestors() and isAccessException() private

Done.

ancestors() is recursive and could blow the stack. should be iterative instead.

Done.

in invokeInstanceMethod/invokeNoArgInstanceMember, couldn't you have two catch blocks there, one for IllegalAccessException and one for Exception? Or does the type checker not like that?

Since the methods we're calling use Util.sneakyThrow(), and don't declare that they throw any exceptions, the compiler won't let us catch IllegalAccessException there. That's also why I had to add isAccessException().

seems like the logic in invokeInstanceMethod/invokeNoArgInstanceMember could get pushed into invokeMatchingMethod rather than have two copies?

I considered that, but I use invokeMatchingMethod inside invokeAncestorMethod, and was concerned about complicating invokeMatchingMethod further with managing the current state to know if it should call invokeAncestorMethod or not (since it shouldn't if it's currently within the stack of invokeAncestorMethod). Instead, I've moved some of the logic into maybeInvokeAncestorMethod (renamed from invokeAncestorMethod) to simplify invokeInstanceMethod/invokeNoArgInstanceMember a bit. Let me know if you want me to take another pass at that.

Interesting side note - the build output under Java 9 now gives the output that users will see when reflecting:

     [java] Testing clojure.test-clojure.compilation
     [java] WARNING: An illegal reflective access operation has occurred
     [java] WARNING: Illegal reflective access by clojure.lang.Reflector (file:/Users/tcrawley/oss/clojure/clojure/target/classes/) to method com.sun.xml.internal.stream.XMLInputFactoryImpl.getXMLReporter()
     [java] WARNING: Please consider reporting this to the maintainers of clojure.lang.Reflector
     [java] WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
     [java] WARNING: All illegal access operations will be denied in a future release
Show
Toby Crawley added a comment - - edited I've added a new patch (tcrawley.CLJ-2066.2017-09-07.patch), and left the old patch (tcrawley.CLJ-2066.2017-03-16.patch) attached for comparison.
make ancestors() and isAccessException() private
Done.
ancestors() is recursive and could blow the stack. should be iterative instead.
Done.
in invokeInstanceMethod/invokeNoArgInstanceMember, couldn't you have two catch blocks there, one for IllegalAccessException and one for Exception? Or does the type checker not like that?
Since the methods we're calling use Util.sneakyThrow(), and don't declare that they throw any exceptions, the compiler won't let us catch IllegalAccessException there. That's also why I had to add isAccessException().
seems like the logic in invokeInstanceMethod/invokeNoArgInstanceMember could get pushed into invokeMatchingMethod rather than have two copies?
I considered that, but I use invokeMatchingMethod inside invokeAncestorMethod, and was concerned about complicating invokeMatchingMethod further with managing the current state to know if it should call invokeAncestorMethod or not (since it shouldn't if it's currently within the stack of invokeAncestorMethod). Instead, I've moved some of the logic into maybeInvokeAncestorMethod (renamed from invokeAncestorMethod) to simplify invokeInstanceMethod/invokeNoArgInstanceMember a bit. Let me know if you want me to take another pass at that. Interesting side note - the build output under Java 9 now gives the output that users will see when reflecting:
     [java] Testing clojure.test-clojure.compilation
     [java] WARNING: An illegal reflective access operation has occurred
     [java] WARNING: Illegal reflective access by clojure.lang.Reflector (file:/Users/tcrawley/oss/clojure/clojure/target/classes/) to method com.sun.xml.internal.stream.XMLInputFactoryImpl.getXMLReporter()
     [java] WARNING: Please consider reporting this to the maintainers of clojure.lang.Reflector
     [java] WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
     [java] WARNING: All illegal access operations will be denied in a future release
Hide
Toby Crawley added a comment -

Alex Miller any chance we can get this into 1.9? Let me know if there is anything I can do to help that happen.

Show
Toby Crawley added a comment - Alex Miller any chance we can get this into 1.9? Let me know if there is anything I can do to help that happen.
Hide
Alex Miller added a comment -

Still hoping to!

Show
Alex Miller added a comment - Still hoping to!
Hide
Toby Crawley added a comment -

I realized while responding to the mailing-list thread for beta4 that this patch actually does nothing under non-EA builds of Java 9, since it no longer throws on illegal reflective access, it just warns. I believe the only way to avoid the warning (without code compiled for Java 9) would be to calculate the ancestor chain, then start from the top of that, asking for the method. However, we'd have to do that for every reflective call, since we'd have no way to know when we have to do it to avoid the warning.

Show
Toby Crawley added a comment - I realized while responding to the mailing-list thread for beta4 that this patch actually does nothing under non-EA builds of Java 9, since it no longer throws on illegal reflective access, it just warns. I believe the only way to avoid the warning (without code compiled for Java 9) would be to calculate the ancestor chain, then start from the top of that, asking for the method. However, we'd have to do that for every reflective call, since we'd have no way to know when we have to do it to avoid the warning.
Hide
Ghadi Shayban added a comment -

Since illegal reflective ops are going to be denied in the near future, this is still a relevant issue. You can simulate the pending future behavior by adding the switch --illegal-access=deny on JDK9+

Now that Clojure is JDK1.8 minimum, a new way around the IllegalAccess catching is available to us, and it doesn't involve multirelease jars or anything messy like that.

The predicate j.l.reflect.Method::canAccess(targetObj) allows you to determine accessibility, and it respects modules. It will return false if you are calling a non-exported Method (e.g. on the XML impl subclass), but true on the public method upstairs in the chain. pseudocode:

static final MethodHandle ACCESSIBLE;

static {
   ACCESSIBLE = if jdk8? then constantly true
                  else MH.Lookup("canAccess")  
}

This will help avoid the catching and rethrowing. We'll still need to have the ancestor lookup logic as in the current patch.

Show
Ghadi Shayban added a comment - Since illegal reflective ops are going to be denied in the near future, this is still a relevant issue. You can simulate the pending future behavior by adding the switch --illegal-access=deny on JDK9+ Now that Clojure is JDK1.8 minimum, a new way around the IllegalAccess catching is available to us, and it doesn't involve multirelease jars or anything messy like that. The predicate j.l.reflect.Method::canAccess(targetObj) allows you to determine accessibility, and it respects modules. It will return false if you are calling a non-exported Method (e.g. on the XML impl subclass), but true on the public method upstairs in the chain. pseudocode:
static final MethodHandle ACCESSIBLE;

static {
   ACCESSIBLE = if jdk8? then constantly true
                  else MH.Lookup("canAccess")  
}
This will help avoid the catching and rethrowing. We'll still need to have the ancestor lookup logic as in the current patch.
Hide
Ghadi Shayban added a comment -

Adding a patch with the approach I suggested above. Relevant trace output of the motiving repro case.

Java 10 Denying Access – Future default setting
/usr/lib/jvm/java-10-openjdk/bin/java --illegal-access=deny -jar ./clojure.jar - <<EOF  
(def fac (javax.xml.stream.XMLInputFactory/newInstance))
(.createXMLStreamReader fac (java.io.StringReader. ""))
EOF

looking up createXMLStreamReader on com.sun.xml.internal.stream.XMLInputFactoryImpl
looking up createXMLStreamReader on javax.xml.stream.XMLInputFactory
found on javax.xml.stream.XMLInputFactory
Java 10 Default Setting
/usr/lib/jvm/java-10-openjdk/bin/java -jar ./clojure.jar - <<EOF
(def fac (javax.xml.stream.XMLInputFactory/newInstance))
(.createXMLStreamReader fac (java.io.StringReader. ""))
EOF

looking up createXMLStreamReader on com.sun.xml.internal.stream.XMLInputFactoryImpl
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by clojure.lang.InjectedInvoker/319670866 (file:/home/ghadi/dev/clojure/clojure.jar) to method com.sun.xml.internal.stream.XMLInputFactoryImpl.createXMLStreamReader(java.io.Reader)
WARNING: Please consider reporting this to the maintainers of clojure.lang.InjectedInvoker/319670866
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
found on com.sun.xml.internal.stream.XMLInputFactoryImpl

Java 8
/usr/lib/jvm/java-8-openjdk/bin/java -jar ./clojure.jar - <<EOF 
(def fac (javax.xml.stream.XMLInputFactory/newInstance))
(.createXMLStreamReader fac (java.io.StringReader. ""))
EOF

looking up createXMLStreamReader on com.sun.xml.internal.stream.XMLInputFactoryImpl
found on com.sun.xml.internal.stream.XMLInputFactoryImpl
%

Show
Ghadi Shayban added a comment - Adding a patch with the approach I suggested above. Relevant trace output of the motiving repro case.
Java 10 Denying Access – Future default setting
/usr/lib/jvm/java-10-openjdk/bin/java --illegal-access=deny -jar ./clojure.jar - <<EOF  
(def fac (javax.xml.stream.XMLInputFactory/newInstance))
(.createXMLStreamReader fac (java.io.StringReader. ""))
EOF

looking up createXMLStreamReader on com.sun.xml.internal.stream.XMLInputFactoryImpl
looking up createXMLStreamReader on javax.xml.stream.XMLInputFactory
found on javax.xml.stream.XMLInputFactory
Java 10 Default Setting
/usr/lib/jvm/java-10-openjdk/bin/java -jar ./clojure.jar - <<EOF
(def fac (javax.xml.stream.XMLInputFactory/newInstance))
(.createXMLStreamReader fac (java.io.StringReader. ""))
EOF

looking up createXMLStreamReader on com.sun.xml.internal.stream.XMLInputFactoryImpl
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by clojure.lang.InjectedInvoker/319670866 (file:/home/ghadi/dev/clojure/clojure.jar) to method com.sun.xml.internal.stream.XMLInputFactoryImpl.createXMLStreamReader(java.io.Reader)
WARNING: Please consider reporting this to the maintainers of clojure.lang.InjectedInvoker/319670866
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
found on com.sun.xml.internal.stream.XMLInputFactoryImpl
Java 8
/usr/lib/jvm/java-8-openjdk/bin/java -jar ./clojure.jar - <<EOF 
(def fac (javax.xml.stream.XMLInputFactory/newInstance))
(.createXMLStreamReader fac (java.io.StringReader. ""))
EOF

looking up createXMLStreamReader on com.sun.xml.internal.stream.XMLInputFactoryImpl
found on com.sun.xml.internal.stream.XMLInputFactoryImpl
%
Hide
Alex Miller added a comment - - edited

Apply the patch and {{mvn clean test }} (with Java 8, since that's our build setup).

To test, I am using a deps.edn in my clojure repo like this:

{:deps
 {org.clojure/clojure {:mvn/version "1.10.0-alpha8"}
  org.clojure/test.check {:mvn/version "0.9.0"}}
 :aliases
 {:dbg {:classpath-overrides {org.clojure/clojure "/Users/alex/code/clojure/target/classes"}}}} ;; <-- change

You can then test in the various modes (swapping JDK independently) with:

;; Java 8: should succeed with no warnings
clj -A:dbg -e '(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))'

;; Java 9, 10, 11: should warn but succeed
clj -A:dbg -e '(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))'

;; Java 9, 10, 11: should NOT warn and succeed
clj -A:dbg -J--add-opens -Jjava.xml/com.sun.xml.internal.stream=ALL-UNNAMED -e '(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))'

;; Java 9, 10, 11: should NOT warn and succeed (new code exercised in this case)
clj -A:dbg -J--illegal-access=deny -e '(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))'
Show
Alex Miller added a comment - - edited Apply the patch and {{mvn clean test }} (with Java 8, since that's our build setup). To test, I am using a deps.edn in my clojure repo like this:
{:deps
 {org.clojure/clojure {:mvn/version "1.10.0-alpha8"}
  org.clojure/test.check {:mvn/version "0.9.0"}}
 :aliases
 {:dbg {:classpath-overrides {org.clojure/clojure "/Users/alex/code/clojure/target/classes"}}}} ;; <-- change
You can then test in the various modes (swapping JDK independently) with:
;; Java 8: should succeed with no warnings
clj -A:dbg -e '(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))'

;; Java 9, 10, 11: should warn but succeed
clj -A:dbg -e '(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))'

;; Java 9, 10, 11: should NOT warn and succeed
clj -A:dbg -J--add-opens -Jjava.xml/com.sun.xml.internal.stream=ALL-UNNAMED -e '(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))'

;; Java 9, 10, 11: should NOT warn and succeed (new code exercised in this case)
clj -A:dbg -J--illegal-access=deny -e '(def fac (javax.xml.stream.XMLInputFactory/newInstance)) (.createXMLStreamReader fac (java.io.StringReader. ""))'
Hide
Alex Miller added a comment -

Small tweaks in -6 patch

Show
Alex Miller added a comment - Small tweaks in -6 patch

People

Vote (7)
Watch (14)

Dates

  • Created:
    Updated:
    Resolved: