GROOVY-12570: let super.m() reach a package-private method of a superclass of another class loader

COMPATIBILITY.md

@@ -961,6 +961,33 @@ a field written by a compound assignment, `++`, `--` or a tuple assignment.
 
 The field descriptors are unchanged, so `japicmp` does not report this.
 
+### Groovy 7 — statically compiled code respects the run-time package of a class defined by another `GroovyClassLoader` compilation (GROOVY-12568)
+
+The JVM allows access to a protected or package-private member only within the
+same run-time package, which is the package name and the class loader. A
+`GroovyClassLoader` defines each compilation in memory with a class loader of
+its own: a `GroovyShell` script, `parseClass`, a script run with `groovy`,
+JSR-223. Under `@CompileStatic`, such a compilation now reaches a protected or
+package-private constructor, method or field of a class of an earlier
+compilation, or of a precompiled class, with the same package name through the
+meta-class, as dynamic code does, where it failed with `IllegalAccessError`.
+Code compiled to class files (groovyc, Gradle, Maven, Ant, joint compilation
+with Java) keeps direct access, as javac does.
+
+**Who is affected.** Statically compiled code that reaches such members of a
+class defined by another compilation now follows meta-class semantics for them,
+like dynamic code; in a GraalVM native image those accesses need reflection
+metadata.
+
+**New compile errors.** Under `@CompileStatic`, `new A(...)` of a protected
+constructor of another package in a subclass is now reported ("Cannot call
+protected constructor"); as in Java, only `super(...)` and `new A(...) {}` can
+call it. A protected method of another package called through a receiver of
+the superclass type (`a.m()`, `a.name`, `a::m`, `A::m`) is reported as "Cannot
+access method". Both used to compile and then fail verification, so no working
+code is affected. Dynamic and `@TypeChecked` code, which call them through the
+meta-class, are unchanged.
+
 ## The binary-compatibility check
 
 The [`subprojects/binary-compatibility/`](subprojects/binary-compatibility)

src/main/java/groovy/lang/MetaClassImpl.java

@@ -577,7 +577,12 @@ public void methodNameAction(final Class<?> c, final MetaMethodIndex.Cache e) {
                         if (matchedMethod >= 0) {
                             methods.set(i, mopMethods[matchedMethod]);
                         } else if (!useThis && notSuperUseful(method, c)) {
-                            methods.remove(i--); // not fit for super usage
+                            MetaMethod superMethod = superMethodOfAnotherRuntimePackage(method, c);
+                            if (superMethod != null) {
+                                methods.set(i, superMethod);
+                            } else {
+                                methods.remove(i--); // not fit for super usage
+                            }
                         }
                     }
                     if (!useThis) {
@@ -592,7 +597,7 @@ public void methodNameAction(final Class<?> c, final MetaMethodIndex.Cache e) {
                         if (useThis) e.methods = mopMethods[matchedMethod];
                         else e.methodsForSuper = mopMethods[matchedMethod];
                     } else if (!useThis && notSuperUseful(method, c)) {
-                        e.methodsForSuper = null; // not fit for super usage
+                        e.methodsForSuper = superMethodOfAnotherRuntimePackage(method, c); // or null: not fit for super usage
                     }
                 }
             }
@@ -635,6 +640,29 @@ private int mopArrayIndex(final MetaMethod method, final String mopName) {
                 return -1;
             }
 
+            /**
+             * Finds the superclass method that a method declared by class c
+             * replaced in the index, if it is a package-private method of a
+             * class with the same package name but another class loader, so of
+             * another run-time package. The JVM does not let the method of c
+             * override it, and no super$ method can call it, but invoked
+             * reflectively it is the method a super call should reach.
+             */
+            private MetaMethod superMethodOfAnotherRuntimePackage(final MetaMethod method, final Class<?> c) {
+                if (c != method.getDeclaringClass().getTheClass()) return null;
+                for (Class<?> sc = c.getSuperclass(); sc != null; sc = sc.getSuperclass()) {
+                    for (CachedMethod cm : ReflectionCache.getCachedClass(sc).getMethods()) {
+                        if (cm.getName().equals(method.getName()) && !cm.isStatic()
+                                && MetaMethod.equal(cm.getParameterTypes(), method.getParameterTypes())) {
+                            // the nearest method decides; if it is not of another run-time package, it hides any deeper one
+                            return cm.isPackagePrivate() && sc.getClassLoader() != c.getClassLoader()
+                                    && sc.getPackageName().equals(c.getPackageName()) ? cm : null;
+                        }
+                    }
+                }
+                return null;
+            }
+
             private boolean notSuperUseful(final MetaMethod method, final Class<?> c) {
                 return !isDGM(method) && (isBridge(method)
                     || method.isAbstract() // GROOVY-11929

src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java

@@ -160,7 +160,6 @@
 import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
 import static org.codehaus.groovy.ast.tools.GeneralUtils.classX;
 import static org.codehaus.groovy.ast.tools.GeneralUtils.fieldX;
-import static org.codehaus.groovy.ast.tools.GeneralUtils.inSamePackage;
 import static org.codehaus.groovy.ast.tools.GeneralUtils.maybeFallsThrough;
 import static org.codehaus.groovy.ast.tools.GeneralUtils.thisPropX;
 import static org.codehaus.groovy.ast.tools.ParameterUtils.isVargs;
@@ -1324,9 +1323,22 @@ private static String getStaticFieldName(final ClassNode type) {
     /**
      * Determines if the given field can be directly accessed by the given class
      * (via {@code GETFIELD}, {@code GETSTATIC}, etc. bytecode instructions).
+     * <p>
+     * Unlike {@link #isMemberDirectlyAccessible}, the package name is not
+     * enough for a protected or package-private field; it needs the same
+     * run-time package (see {@link #isInSameRuntimePackage}).
      */
     public static boolean isFieldDirectlyAccessible(final FieldNode field, final ClassNode accessingClass) {
-        return field != null && isMemberDirectlyAccessible(field.getModifiers(), field.getDeclaringClass(), accessingClass);
+        if (field == null) return false;
+        ClassNode declaringClass = field.getDeclaringClass();
+        if (field.isPublic() || field.isPrivate()) { // nestmates can access a private field
+            return isMemberDirectlyAccessible(field.getModifiers(), declaringClass, accessingClass);
+        }
+        // JVMS 5.4.4: a protected or package-private field is accessible from the run-time package of its declaring
+        // class, which includes the declaring class, and a protected field also from a subclass; access from a class
+        // of the same package name but another run-time package fails with IllegalAccessError
+        return isInSameRuntimePackage(declaringClass, accessingClass)
+            || (field.isProtected() && accessingClass.isDerivedFrom(declaringClass));
     }
 
     /**
@@ -1535,7 +1547,7 @@ public void visitPropertyExpression(final PropertyExpression expression) {
                 } else {
                     fieldNode = classNode.getSuperClass().getDeclaredField(name);
                     // GROOVY-4497, GROOVY-11764: do not visit super class field if it is private or package-private
-                    if (fieldNode != null && (fieldNode.isPrivate() || !(fieldNode.isPublic() || fieldNode.isProtected() || inSamePackage(classNode, classNode.getSuperClass())))) {
+                    if (fieldNode != null && (fieldNode.isPrivate() || !(fieldNode.isPublic() || fieldNode.isProtected() || isFieldDirectlyAccessible(fieldNode, classNode)))) {
                         fieldNode = null;
                     }
                 }

src/main/java/org/codehaus/groovy/classgen/asm/MopWriter.java

@@ -23,6 +23,7 @@
 import org.codehaus.groovy.ast.MethodNode;
 import org.codehaus.groovy.ast.Parameter;
 import org.codehaus.groovy.ast.tools.ParameterUtils;
+import org.codehaus.groovy.classgen.AsmClassGenerator;
 import org.objectweb.asm.MethodVisitor;
 
 import java.util.Collections;
@@ -154,6 +155,8 @@ private void visitMopMethodList(final Iterable<MethodNode> methods, final boolea
             // super$ method for non-private isThis=false
             // --> results in XOR
             if (isThis ^ mn.isPrivate()) continue;
+            // a super$ method cannot call a package-private method of another run-time package
+            if (!isThis && mn.isPackageScope() && !AsmClassGenerator.isInSameRuntimePackage(mn.getDeclaringClass(), controller.getClassNode())) continue;
 
             String methodName = mn.getName();
             Parameter[] parameters = mn.getParameters();

src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticInvocationWriter.java

@@ -61,6 +61,7 @@
 import org.codehaus.groovy.classgen.asm.TypeChooser;
 import org.codehaus.groovy.classgen.asm.VariableSlotLoader;
 import org.codehaus.groovy.classgen.asm.WriterController;
+import org.codehaus.groovy.runtime.ScriptBytecodeAdapter;
 import org.codehaus.groovy.syntax.SyntaxException;
 import org.codehaus.groovy.transform.sc.StaticCompilationMetadataKeys;
 import org.codehaus.groovy.transform.sc.StaticCompilationVisitor;
@@ -85,6 +86,7 @@
 
 import static org.apache.groovy.ast.tools.ClassNodeUtils.formatTypeName;
 import static org.apache.groovy.ast.tools.ClassNodeUtils.isSubtype;
+import static org.apache.groovy.ast.tools.ClassNodeUtils.samePackageName;
 import static org.apache.groovy.ast.tools.ExpressionUtils.isNullConstant;
 import static org.apache.groovy.ast.tools.ExpressionUtils.isSuperExpression;
 import static org.apache.groovy.ast.tools.ExpressionUtils.isThisExpression;
@@ -121,6 +123,8 @@
  */
 public class StaticInvocationWriter extends InvocationWriter {
 
+    private static final MethodCallerMultiAdapter invokeNew = MethodCallerMultiAdapter.newStatic(ScriptBytecodeAdapter.class, "invokeNew", true, true);
+
     private final AtomicInteger labelCounter = new AtomicInteger();
 
     /**
@@ -155,6 +159,19 @@ public void writeInvokeConstructor(final ConstructorCallExpression call) {
         }
         ClassNode declaringClass = cn.getDeclaringClass();
         ClassNode enclosingClass = controller.getClassNode();
+        if (!cn.isPrivate() && !isDirectlyAccessible(cn, enclosingClass)) {
+            if (samePackageName(declaringClass, enclosingClass)) {
+                writeDynamicConstructorCall(call, declaringClass);
+                return;
+            }
+            // JLS 6.6.2.2: outside its package, only super(...) or an anonymous inner class can call a protected constructor;
+            // reported here, while the class is generated, so @TypeChecked code is not checked for it and works dynamically
+            controller.getSourceUnit().addError(new SyntaxException(
+                    "Cannot call " + (cn.isProtected() ? "protected" : "package-private") + " constructor for "
+                            + declaringClass.toString(false) + " from class " + enclosingClass.toString(false),
+                    call
+            ));
+        }
         List<Expression> argList = call.getArguments() instanceof TupleExpression
                 ? ((TupleExpression) call.getArguments()).getExpressions() : List.of(call.getArguments());
         if (!AsmClassGenerator.isMemberDirectlyAccessible(cn.getModifiers(), declaringClass, enclosingClass)) {
@@ -192,8 +209,9 @@ public void writeInvokeConstructor(final ConstructorCallExpression call) {
      * <p>
      * Unlike {@link AsmClassGenerator#isMemberDirectlyAccessible}, there is no
      * subclass rule: the verifier rejects {@code NEW} and {@code INVOKESPECIAL}
-     * of a protected constructor of another package in a subclass too, so a
-     * constructor reference to it creates the object through the meta-class.
+     * of a protected constructor of another package in a subclass too, so
+     * {@code new} of it is reported and a constructor reference to it creates
+     * the object through the meta-class.
      */
     static boolean isDirectlyAccessible(final ConstructorNode ctor, final ClassNode accessingClass) {
         if (ctor.isPublic()) return true;
@@ -204,6 +222,18 @@ static boolean isDirectlyAccessible(final ConstructorNode ctor, final ClassNode
         return AsmClassGenerator.isInSameRuntimePackage(declaringClass, accessingClass);
     }
 
+    /**
+     * Creates the object through the meta-class, which selects the constructor
+     * at run time, for a constructor that the current class cannot call directly.
+     */
+    private void writeDynamicConstructorCall(final ConstructorCallExpression call, final ClassNode type) {
+        Expression arguments = call.getArguments();
+        makeUncachedCall(call, classX(controller.getClassNode()), classX(type), null, arguments,
+                invokeNew, false, false, false, AsmClassGenerator.containsSpreadExpression(arguments));
+        BytecodeHelper.doCast(controller.getMethodVisitor(), type);
+        controller.getOperandStack().replace(type);
+    }
+
     /** {@inheritDoc} */
     @Override
     public void writeSpecialConstructorCall(final ConstructorCallExpression call) {
@@ -371,8 +401,19 @@ protected boolean writeDirectMethodCall(final MethodNode target, final boolean i
             receiverType = receiverType.getGenericsTypes()[0].getType(); // GROOVY-11694
         }
 
+        if (!target.isPublic() && !target.isPrivate() && samePackageName(declaringClass, enclosingClass)
+                && !AsmClassGenerator.isInSameRuntimePackage(declaringClass, enclosingClass)
+                && !(target.isProtected() && enclosingClass.isDerivedFrom(declaringClass)
+                        && (target.isStatic() || isSuperExpression(receiver) || isSubtype(enclosingClass, receiverType)))) {
+            // a protected or package-private method of a class of another run-time package, which is the package
+            // name and the class loader, is selected at run time (except for the subclass rule of a protected method)
+            return false;
+        }
+
         if (AsmClassGenerator.isMemberDirectlyAccessible(target.getModifiers(), declaringClass, enclosingClass)
-                && !(target.isProtected() && !inSamePackage(declaringClass, enclosingClass) && !isSubtype(receiverType, enclosingClass))) { // GROOVY-7325
+                // GROOVY-7325, JVMS 4.10.1.8: outside its package, a protected instance method needs a receiver of the current class
+                && !(target.isProtected() && !target.isStatic() && !inSamePackage(declaringClass, enclosingClass) && receiver != null
+                        && !isSuperExpression(receiver) && !isClassWithSuper(receiver) && !isSubtype(enclosingClass, receiverType))) {
             boolean isThisImplicit = implicitThis;
             Expression theReceiver = receiver;
             MethodNode theTarget = target;

src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java

@@ -234,11 +234,29 @@ && getField(receiverType, propertyName, FieldNode::isPublic) == null) {
             }
         }
 
+        if (isFieldOfAnotherRuntimePackage(receiverType, propertyName)) {
+            makeDynamicGetProperty(receiver, propertyName, safe);
+            return;
+        }
+
         addPropertyAccessError(receiver, propertyName, receiverType);
         controller.getMethodVisitor().visitInsn(ACONST_NULL);
         controller.getOperandStack().push(OBJECT_TYPE);
     }
 
+    /**
+     * Determines if the property is a protected or package-private field of a
+     * class with the same package name but of another run-time package (see
+     * {@link AsmClassGenerator#isInSameRuntimePackage}). Such a field is read
+     * through the meta-class when the current class cannot access it directly.
+     */
+    private boolean isFieldOfAnotherRuntimePackage(final ClassNode receiverType, final String propertyName) {
+        FieldNode field = getField(receiverType, propertyName);
+        return field != null && !field.isPublic() && !field.isPrivate()
+            && samePackageName(field.getDeclaringClass(), controller.getClassNode())
+            && !AsmClassGenerator.isInSameRuntimePackage(field.getDeclaringClass(), controller.getClassNode());
+    }
+
     private void makeDynamicGetProperty(final Expression receiver, final String propertyName, final boolean safe) {
         MethodNode target = safe ? INVOKERHELPER_GETPROPERTYSAFE_METHOD : INVOKERHELPER_GETPROPERTY_METHOD;
         MethodCallExpression call = callX(
@@ -406,6 +424,11 @@ && getField(receiverType, propertyName, FieldNode::isPublic) == null
             return;
         }
 
+        if (isFieldOfAnotherRuntimePackage(receiverType, propertyName)) {
+            makeDynamicGetProperty(receiver, propertyName, safe);
+            return;
+        }
+
         boolean isScriptVariable = (receiverType.isScript() && receiver instanceof VariableExpression && ((VariableExpression) receiver).getAccessedVariable() == null);
         // a packed closure's hoisted body keeps the closure-context contract: like the inner
         // closure class it replaces, it falls through to the dynamic property sequence (e.g.
@@ -537,7 +560,7 @@ boolean makeGetField(final Expression receiver, final ClassNode receiverType, fi
                 ClassNode currentClass = controller.getClassNode();
                 ClassNode castType = field.getOwner();
                 if (field.isProtected() && !currentClass.equals(field.getOwner())
-                        && !samePackageName(currentClass, field.getOwner())) {
+                        && !AsmClassGenerator.isInSameRuntimePackage(field.getOwner(), currentClass)) {
                     if (receiverType.isDerivedFrom(currentClass)) castType = currentClass;
                     else return false;
                 }
@@ -862,6 +885,12 @@ private boolean setField(final PropertyExpression expression, final Expression o
 
         FieldNode fn = AsmClassGenerator.getDeclaredFieldOfCurrentClassOrAccessibleFieldOfSuper(controller.getClassNode(), receiverType, name, false);
         if (fn == null) return false;
+        ClassNode currentClass = controller.getClassNode();
+        if (!fn.isStatic() && fn.isProtected() && !currentClass.equals(fn.getOwner())
+                && !AsmClassGenerator.isInSameRuntimePackage(fn.getOwner(), currentClass)
+                && !receiverType.isDerivedFrom(currentClass)) {
+            return false; // JVMS 4.10.1.8: in another run-time package, the receiver must be of the current class
+        }
 
         OperandStack os = controller.getOperandStack();
         os.doGroovyCast(fn.getType());