parboiled: Parboiled stopped working in Java 16 due to enforcement of encapsulation

@david-shao I can create PR that would force using the MethodHandles.lookup() instead.

As I’m switching to Java 17 on my projects as well, I’ve tested whether --add-opens still works for Java internal classes and I can confirm using --add-opens java.base/java.lang=ALL-UNNAMED still works for Parboiled, as @SethTisue suggested. So when you specify it for the runtime, everything still works as it should.

Just released parboiled 1.4.0 which should work fine on Java 17.

Adding runtime flags does create deployment & project sharing headaches, so a long-term solution hopefully won’t require them.

@bgalek I’d be happy to look at and merge a PR that helps with overcoming the long-standing issues around dynamic class loading. But I’m afraid I’m too far away from the technical details (> 10 years!) to be of any help myself, unfortunately…

(shameless plug) If anybody is going to use Java 17 (soon), take a look at Rekex (which doesn’t work in Java 16 or lower:)

I’ve had some time do look into this a bit deeper:

1. --illegal-access=permit will work as workaround for Java 16, however it will be removed in Java 17 (which is the upcoming LTS version - https://openjdk.java.net/jeps/403)
2. The troublemaker in this case is ClassLoader and its methods findLoadedClass / defineClass - which does class lookup / new class definition from bytecode produced by ASM

When Parboiled gets rid of the usage of those two methods, it starts working without any issues. How to achieve this is however a little problematic:

• Direct replacement for findLoadedClass and defineClass is with MethodHandles$Lookup class -> MethodHandles.lookup().findClass(className) and MethodHandles.lookup().defineClass(code)
• The usage of MethodHandles is pretty straightforward, but it ensures the class can;t define / lookup class outside its capability
  • Lookup class constructor is with package visibility
  • MethodHandles.lookup() instantiates Lookup class with Reflection.getCallerClass(), which restricts the usage to the package the caller is from
  • Parboiled is loading classes with util class org.parboiled.transform.AsmUtils
  • Parboiled loads parsers from custom packages, which results in IllegalAccessException, when AsmUtils tries to define class in different package than org.parboiled.transform

So far I could think of three possible scenarios:

1. Replace AsmUtils.findLoadedClass and AsmUtils.loadClass implementation directly with MethodHandles.lookup().* methods a. Pros: change in only few lines, easy fix b. Cons: all parsers need to be defined in org.parboiled.transform package, otherwise it won’t work

2. Replace AsmUtils.findLoadedClass and AsmUtils.loadClass implementation directly with MethodHandles.lookup().* and update ASM to generate new updated classes inside org.parboiled.transform, not in the original parser package a. Pros: works without any necessary modification to existing parsers b. Cons: all classes that are defined dynamically need to be generated in different package and all related calls need to be updated as well (class can be “renamed” with ClassVisitor, but all references to the original class need to be updated / copied, otherwise similar IllegalAccessException will be thrown) -> I suspect this would require major rewrite of the ASM handling

3. Replace AsmUtils.findLoadedClass and AsmUtils.loadClass implementation with calls to similar static methods defined in parser class (see below) a. Pros: change in only few lines, easy fix b. Cons: all parsers need to define their own static findLoadedClass and loadClass static methods (creating custom method in BaseParser doesn’t help as it gets detected as different package)

All options mentioned above require at least Java 9, because that’s when Lookup.defineClass was added (https://docs.oracle.com/javase/9/docs/api/java/lang/invoke/MethodHandles.Lookup.html#defineClass-byte:A- )

The third option can be achieved with following patch:

diff --git a/build.sbt b/build.sbt
index c1bc1a6d..0397bc96 100644
--- a/build.sbt
+++ b/build.sbt
@@ -14,8 +14,8 @@ val basicSettings = Seq(
 
   javacOptions          ++= Seq(
     "-deprecation",
-    "-target", "1.7",
-    "-source", "1.7",
+    "-target", "11",
+    "-source", "11",
     "-encoding", "utf8",
     "-Xlint:unchecked"
   ),
diff --git a/parboiled-java/src/main/java/org/parboiled/transform/AsmUtils.java b/parboiled-java/src/main/java/org/parboiled/transform/AsmUtils.java
index b1f9e9e7..621c84d0 100644
--- a/parboiled-java/src/main/java/org/parboiled/transform/AsmUtils.java
+++ b/parboiled-java/src/main/java/org/parboiled/transform/AsmUtils.java
@@ -199,20 +199,11 @@ class AsmUtils {
      * @param classLoader the class loader to use
      * @return the class instance or null
      */
-    public static Class<?> findLoadedClass(String className, ClassLoader classLoader) {
+    public static Class<?> findLoadedClass(String className, ClassLoader classLoader, Class<?> origClass) {
         checkArgNotNull(className, "className");
-        checkArgNotNull(classLoader, "classLoader");
+        checkArgNotNull(origClass, "origClass");
         try {
-            Class<?> classLoaderBaseClass = Class.forName("java.lang.ClassLoader");
-            Method findLoadedClassMethod = classLoaderBaseClass.getDeclaredMethod("findLoadedClass", String.class);
-
-            // protected method invocation
-            findLoadedClassMethod.setAccessible(true);
-            try {
-                return (Class<?>) findLoadedClassMethod.invoke(classLoader, className);
-            } finally {
-                findLoadedClassMethod.setAccessible(false);
-            }
+            return (Class<?>) origClass.getMethod("findLoadedClass", String.class).invoke(null, className);
         } catch (Exception e) {
             throw new RuntimeException("Could not determine whether class '" + className +
                     "' has already been loaded", e);
@@ -231,22 +222,11 @@ class AsmUtils {
      * @param classLoader the class loader to use
      * @return the class instance
      */
-    public static Class<?> loadClass(String className, byte[] code, ClassLoader classLoader) {
-        checkArgNotNull(className, "className");
+    public static Class<?> loadClass(String className, byte[] code, ClassLoader classLoader, Class<?> origClass) {
         checkArgNotNull(code, "code");
-        checkArgNotNull(classLoader, "classLoader");
+        checkArgNotNull(origClass, "origClass");
         try {
-            Class<?> classLoaderBaseClass = Class.forName("java.lang.ClassLoader");
-            Method defineClassMethod = classLoaderBaseClass.getDeclaredMethod("defineClass",
-                    String.class, byte[].class, int.class, int.class);
-
-            // protected method invocation
-            defineClassMethod.setAccessible(true);
-            try {
-                return (Class<?>) defineClassMethod.invoke(classLoader, className, code, 0, code.length);
-            } finally {
-                defineClassMethod.setAccessible(false);
-            }
+            return (Class<?>) origClass.getMethod("loadClass", byte[].class).invoke(null, code);
         } catch (Exception e) {
             throw new RuntimeException("Could not load class '" + className + '\'', e);
         }
diff --git a/parboiled-java/src/main/java/org/parboiled/transform/GroupClassGenerator.java b/parboiled-java/src/main/java/org/parboiled/transform/GroupClassGenerator.java
index eeeb4d11..2e534681 100644
--- a/parboiled-java/src/main/java/org/parboiled/transform/GroupClassGenerator.java
+++ b/parboiled-java/src/main/java/org/parboiled/transform/GroupClassGenerator.java
@@ -58,12 +58,12 @@ abstract class GroupClassGenerator implements RuleMethodProcessor {
 
         Class<?> groupClass;
         synchronized (lock) {
-            groupClass = findLoadedClass(className, classLoader);
+            groupClass = findLoadedClass(className, classLoader, classNode.getParentClass());
             if (groupClass == null || forceCodeBuilding) {
                 byte[] groupClassCode = generateGroupClassCode(group);
                 group.setGroupClassCode(groupClassCode);
                 if (groupClass == null) {
-                    loadClass(className, groupClassCode, classLoader);
+                    loadClass(className, groupClassCode, classLoader, classNode.getParentClass());
                 }
             }
         }
diff --git a/parboiled-java/src/main/java/org/parboiled/transform/ParserTransformer.java b/parboiled-java/src/main/java/org/parboiled/transform/ParserTransformer.java
index b31e4f2f..6ce19861 100644
--- a/parboiled-java/src/main/java/org/parboiled/transform/ParserTransformer.java
+++ b/parboiled-java/src/main/java/org/parboiled/transform/ParserTransformer.java
@@ -33,7 +33,7 @@ public class ParserTransformer {
         checkArgNotNull(parserClass, "parserClass");
         // first check whether we did not already create and load the extension of the given parser class
         Class<?> extendedClass = findLoadedClass(
-                getExtendedParserClassName(parserClass.getName()), parserClass.getClassLoader()
+                getExtendedParserClassName(parserClass.getName()), parserClass.getClassLoader(), parserClass
         );
         return (Class<? extends T>)
                 (extendedClass != null ? extendedClass : extendParserClass(parserClass).getExtendedClass());
@@ -44,7 +44,7 @@ public class ParserTransformer {
         new ClassNodeInitializer().process(classNode);
         runMethodTransformers(classNode);
         new ConstructorGenerator().process(classNode);
-        defineExtendedParserClass(classNode);
+        defineExtendedParserClass(classNode, parserClass);
         return classNode;
     }
 
@@ -93,7 +93,7 @@ public class ParserTransformer {
         );
     }
 
-    private static void defineExtendedParserClass(final ParserClassNode classNode) {
+    private static void defineExtendedParserClass(final ParserClassNode classNode, Class<?> origClass) {
         ClassWriter classWriter = new ClassWriter(ASMSettings.FRAMES) {
             @Override
             protected ClassLoader getClassLoader() {
@@ -105,7 +105,8 @@ public class ParserTransformer {
         classNode.setExtendedClass(loadClass(
                 classNode.name.replace('/', '.'),
                 classNode.getClassCode(),
-                classNode.getParentClass().getClassLoader()
+                classNode.getParentClass().getClassLoader(),
+                origClass
         ));
     }

And following methods need to be defined in all parsers:

@BuildParseTree
class TestParser extends BaseParser<Integer> {

    public static Class<?> findLoadedClass(String className) throws IllegalAccessException {
        try {
            return MethodHandles.lookup().findClass(className);
        } catch (ClassNotFoundException e) {
            return null;
        }
    }

    public static Class<?> loadClass(byte[] code) throws IllegalAccessException {
        return MethodHandles.lookup().defineClass(code);
    }

}

While this is far from optimal, it works 🤷‍♂️