Repository navigation
GROOVY-12570: let super.m() reach a package-private method of a superclass of another class loader - #3099
Open
codeconsole wants to merge 24 commits into
Open
GROOVY-12570: let super.m() reach a package-private method of a superclass of another class loader#3099codeconsole wants to merge 24 commits into
codeconsole wants to merge 24 commits into
Conversation
…h new directly only within the compilation unit Under static compilation, new A(...) of a protected or package-private constructor of a class with the same package name was compiled to NEW and INVOKESPECIAL. The JVM allows that call only within the same run-time package, which is the package name and the class loader, so it failed with IllegalAccessError when A came from another class loader: a class of an earlier parse of the same GroovyShell, which GroovyClassLoader defines with another InnerLoader, or a precompiled class used by a script. Dynamic and type-checked code create such an object through the meta-class and work. Such a constructor is now called directly only when its class is compiled in the same compilation unit, which defines all its classes with one class loader, as for a constructor reference. Otherwise the object is created through the meta-class, which selects the constructor at run time. This also applies to the no-argument constructor that a map-style construction such as new A(name: 'x') calls before it sets the properties. Public constructors, and constructors of classes of the same compilation unit, are still called directly. The constructor reference writer uses the same check.
… only within the compilation unit Under static compilation, a call to a protected or package-private method of a class with the same package name was compiled to a direct INVOKEVIRTUAL, INVOKESTATIC or INVOKESPECIAL. As for a constructor, the JVM allows that call only within the same run-time package, so it failed with IllegalAccessError when the class came from another class loader, for example a class of an earlier parse of the same GroovyShell. This also hit a protected or package-private getter or setter used as a property, such as a.name or a.name = value. Such a method is now called directly only when its class is compiled in the same compilation unit, or when the JVM allows the call from any package: a protected method called from a subclass on this, super or a receiver of the subclass type, or a protected static method called from a subclass. Otherwise the method is selected at run time through the meta-class, as dynamic code does. Public methods, and methods of classes of the same compilation unit, are still called directly.
…nother compilation unit through a synthetic method A statically compiled method reference such as m::pm, M::pm or M::spm gives LambdaMetafactory a method handle for the method. For a protected or package-private method of a class with the same package name, the JVM resolves that handle only within the same run-time package, so the reference failed with IllegalAccessError when the class came from another class loader, for example a class of an earlier parse of the same GroovyShell. For such a method of a class of another compilation unit, the reference now targets a synthetic static method of the current class that calls the method. The invocation writer compiles that call directly when the JVM allows it from any package (a protected method called through the subclass), or else selects the method at run time. Method references to methods of classes of the same compilation unit, and to public methods, still use a method handle for the method itself.
…ly within the compilation unit AsmClassGenerator.isFieldDirectlyAccessible decided by the package name if a protected or package-private field can be read or written with GETFIELD, PUTFIELD, GETSTATIC or PUTSTATIC. As for methods and constructors, the JVM allows that only within the same run-time package, so the access failed with IllegalAccessError when the class came from another class loader, for example a class of an earlier parse of the same GroovyShell. Under static compilation this hit a.field, a.@field, Type.field, an implicit field of a superclass, compound assignments and increments. Dynamic code hit it for super.field, super.@field and this.@field of a superclass. A protected field of a superclass with the same package name but of another compilation unit was also read through a receiver of the superclass type, which fails verification ("Bad access to protected data in getfield") when the classes are in different run-time packages. Such a field is now accessed directly only when its class is compiled in the same compilation unit, or through the subclass rule for a protected field (with a receiver of the current class). Otherwise it is read and written through the meta-class, as dynamic code does. Public fields, and fields of classes of the same compilation unit, are still accessed directly.
…ckage instead of failing verification
Outside its package, a protected constructor can only be called by
super(...) or by creating an anonymous inner class (JLS 6.6.2.2). The type
checker accepts new A(...) of such a constructor in a subclass of A, as it
accepts other protected members there, and type-checked code creates the
object through the meta-class. Under static compilation, the call was
compiled to NEW and INVOKESPECIAL, and the class failed verification when
it was loaded: "VerifyError: Bad access to protected <init> method". The
same happened for a map-style construction such as new A([:]), which calls
the protected no-argument constructor.
The static compiler now reports "Cannot call protected constructor for A
from class B", as it reports a protected method called on a receiver that
is not the subclass (GROOVY-7325). super(...), this(...) and
new A(...) {} still call the constructor directly. A constructor reference
A::new to such a constructor still selects it at run time.
… through a superclass receiver Outside its package, a protected instance method or field can be used only through a receiver of the subclass (JLS 6.6.2.1, JVMS 4.10.1.8). For a method, the static compiler meant to report such a call (GROOVY-7325), but its check compared the types the wrong way round: it tested whether the current class is a subtype of the receiver type instead of the reverse. So a.m() with a receiver of the superclass type, in a subclass in another package, was compiled to a direct call and the class failed verification: "VerifyError: Bad access to protected data in invokevirtual". This also hit a protected getter or setter used as a property (a.name, a.name = value). A method reference such as a::m or A::m to such a method failed with BootstrapMethodError, and an attribute assignment a.@field = value failed verification in putfield. The check now requires the receiver to be of the current class (or this, super or a static method), so such a call or method reference is reported as "Cannot access method", as GROOVY-7325 intended and as javac does. An attribute assignment through such a receiver is now made through the meta-class, as the attribute read a.@field already was. Calls through this, super or a receiver of the subclass type are still direct.
…amic cannot access the constructor A dynamic new of a protected constructor of another package, in a subclass, is accepted by Groovy and by the type checker. With invokedynamic, the constructor selector unreflected the selected constructor with the lookup of the calling class and turned an IllegalAccessException into GroovyBugError. For a constructor that the JVM refuses to the caller, such as the protected ObjectInputStream() of the encapsulated java.io package called by new in a subclass, the call failed with "BUG! UNCAUGHT EXCEPTION: member is protected". As the property selectors do since GROOVY-12314, the constructor selector now treats the refusal as an access-control outcome and leaves the call to the meta-class, which reports "could not access constructor", as the call does without invokedynamic. A constructor that reflection can reach, such as a protected constructor of a class of another package in the unnamed module, is still called.
…ethod of another run-time package For a dynamic super.m() call, the class gets a synthetic super$N$m method that calls the superclass method with INVOKESPECIAL. For a package-private method of a superclass with the same package name but of another compilation unit, the classes may be in different run-time packages, for example when the superclass comes from an earlier parse of the same GroovyShell, and the JVM refuses the call: super.m() failed with IllegalAccessError. Such a method, and a package-private method of a superclass of another package, no longer gets a super$ method, and the meta-class calls the superclass method reflectively, as it does for a statically compiled super.m() of such a method.
…ugh the meta-class only for classes defined in memory The previous commits accessed a protected or package-private member of a class with the same package name through the meta-class whenever that class was compiled by another compilation unit. That also hit class files: a Java class of a joint compilation, a main class used by a test, a class left over from an incremental compilation. Such classes are loaded by one class loader at run time, as javac assumes, so the direct access worked; the meta-class path was slower, followed meta-class semantics, needed reflection metadata in a native image, and made an incremental build give other bytecode than a full one. The run-time package of such a class only differs when the classes are defined in memory by a GroovyClassLoader, which defines each compilation (a GroovyShell script, parseClass, JSR-223) with an InnerLoader of its own. GroovyClassLoader now records that on the compilation unit, and AsmClassGenerator.isInSameRuntimePackage treats a class of another compilation unit as another run-time package only then. Constructors, map-style construction, methods, getters and setters, method references, fields, dynamic super and this.@ access and super$ methods all use this check. Compiling to class files keeps direct access, as before these changes and as javac does. (The rest of the change: new, map-style construction, methods, getters and setters, method references to methods, fields, dynamic super and this.@ access and super$ methods. The GroovyClassLoader marker, isInSameRuntimePackage and constructor references come first.)
The tests for protected and package-private members of a class of another compilation now compile it through a GroovyClassLoader, which defines it with another class loader; classes compiled to class files keep direct access. Name the tests accordingly.
…rules of constructor references - isInSameRuntimePackage: isPrimaryClassNode() stands for "defined by the class loader of the accessing class", not for "compiled from source". - StaticInvocationWriter.isDirectlyAccessible: unlike isMemberDirectlyAccessible it has no subclass rule, since the verifier rejects NEW and INVOKESPECIAL of a protected constructor of another package in a subclass too. - findConstructorReferenceTarget: a selected constructor is called directly, so a meta-class constructor override is not used, as for a statically compiled new. The guard comment now says why enums are left to the meta-class: the type checker accepts an enum constructor reference whose arguments match the generated constructor, but Java code cannot instantiate an enum. - addSyntheticMethod: the ACC_FINAL rule of JVMS 4.6 applies to every helper method of a method reference in an interface. (The part on statically compiled new; the rest comes with constructor references.)
… interface All helper methods that a method reference adds to the current class are created by addSyntheticMethod, which leaves out ACC_FINAL in an interface. The interface tests covered the constructor-reference and variadic helpers; they now also cover the array constructor reference, the static extension method helper (Thread::startDaemon), the Class method helper (String::getName) and the helper that calls a package-private method of a class of another class loader (M::spm, M::pm), in static and default methods, and check that every helper is private and not final. With ACC_FINAL put back, both tests fail with ClassFormatError (illegal modifiers 0x101A). (The case of a method of another run-time package; the other helper methods are tested with constructor references.)
…AbstractBytecodeTestCase MethodReferenceTest and StaticCompileRuntimePackageTest each had a copy of the helper that compiles a script against the classes of an earlier parse of the same GroovyClassLoader, and of the code that keeps only the instructions of one method. AbstractBytecodeTestCase now provides them as compileAgainstLibrary, extractMethodSequence and selectMethod, and both tests use them. Test code only; the tests are unchanged. (Here StaticCompileRuntimePackageTest; MethodReferenceTest was moved to the helpers with constructor references.)
…errors COMPATIBILITY.md describes how statically compiled code of a compilation that a GroovyClassLoader defines in memory reaches protected and package-private members of classes of other compilations, and the new compile errors for new of a protected constructor of another package and for a protected method of another package called through a superclass receiver. StaticInvocationWriter notes that the constructor error is reported during class generation, so @TypeChecked code is not checked for it and works dynamically.
…ass of another class loader Since GROOVY-12454, new A(map) with a Map variable, under static compilation, calls the no-argument constructor and then sets the properties of the map at run time. For a class of an earlier parse of the same GroovyShell whose no-argument constructor is package-private, the constructor is called through the meta-class, as for a map literal.
…class of another class loader A class that declares m() and calls super.m(), where m() is package-private in a superclass with the same package name but defined by another class loader, for example a class of an earlier parse of the same GroovyShell, failed with MissingMethodException. The JVM does not let the subclass method override a package-private method of another run-time package, so no super$ method can call it; without one, the meta-class removed the subclass method from the methods for super calls and kept nothing, as the superclass method had already been replaced by the subclass method in the index. The meta-class now keeps the superclass method for super calls in that case. Invoked reflectively it is the superclass method, because the JVM does not consider it overridden. Super calls that a super$ method serves, and package-private methods of another package, are unchanged.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3099 +/- ##
==================================================
+ Coverage 74.1374% 74.1745% +0.0372%
- Complexity 43718 43786 +68
==================================================
Files 1692 1692
Lines 149513 149612 +99
Branches 28359 28403 +44
==================================================
+ Hits 110845 110974 +129
+ Misses 28458 28422 -36
- Partials 10210 10216 +6
🚀 New features to boost your workflow:
|
daniellansun
left a comment
daniellansun
left a comment
Contributor
There was a problem hiding this comment.
GROOVY-12570 diagnosis matches the JVM rule: a subclass method does not override a package-private method of another run-time package, so no super$ MOP bridge can reach it; reflective invoke of the superclass method is what super.m() must do.
MetaClassImpl.superMethodOfAnotherRuntimePackage (replace in the super index when notSuperUseful would otherwise drop the entry) is the right place. Test covers dynamic + @CompileStatic, indy on/off, and both super.pm() and overriding pm().
Please before approve:
- Re-run
lts (17, windows-latest, 1)(60m timeout). - Confirm search order: the first superclass method with the same name+params wins, and returns
nullif that match is not package-private/other-loader — so an intermediate same-loader override correctly suppresses a deeper foreign package-private. Worth a one-line comment on that early-return, and maybe a Mid/B/C fixture if not already implied by #3097 tests. - Codecov still shows partials on the shared #3097 surface plus
MetaClassImpl(~62% on the new lines) — a direct assert on the super-index entry would help.
Contributor
Author
The new tests fail without the change ( |
# Conflicts: # src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesMethodReferenceExpressionWriter.java
…to the language rule For a public or private field the language rule of isMemberDirectlyAccessible decides. For a protected or package-private field, JVMS 5.4.4 allows access from the run-time package of the declaring class, or to a protected field from a subclass. That condition implies isMemberDirectlyAccessible, so it replaces it there instead of being checked after it.
Contributor
Author
…ivate-method # Conflicts: # COMPATIBILITY.md
✅ All tests passed ✅🏷️ Commit: 84c3d3d Learn more about TestLens at testlens.app/docs. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A class that declares
m()and callssuper.m(), wherem()is package-private in a superclass with the same package name but defined by another class loader (for example a class of an earlier parse of the sameGroovyShell), failed withMissingMethodException. The JVM does not let the subclass method override a package-private method of another run-time package, so nosuper$method can call it; without one, the meta-class removed the subclass method from the methods for super calls and kept nothing, as the superclass method had already been replaced by the subclass method in the index.The meta-class now keeps the superclass method for super calls in that case. Invoked reflectively it is the superclass method, because the JVM does not consider it overridden. Super calls that a
super$method serves, and package-private methods of another package, are unchanged.This needs #3097, which stops generating a
super$method for a package-private method of another run-time package and routes the statically compiled call through the meta-class; its commits are included until it is merged. It touches the same lines ofMetaClassImplas #3048, so whichever is merged second needs a rebase.Tests
bugs.Groovy12568.testSuperCallOfPackagePrivateMethodOfClassOfAnotherClassLoaderWithMethodOfSameName(marked// GROOVY-12570), dynamic and@CompileStatic, with and without invokedynamic; it fails without the change.bugs,groovy.ThisAndSuperTest,groovy.lang.*andStaticCompileRuntimePackageTestpass (1,144 tests), as do the Checkstyle and CodeNarc gates.