Repository navigation
GROOVY-12514, GROOVY-12517: qualified this and super in nested classes under static type checking and compilation - #3049
codeconsole wants to merge 8 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3049 +/- ##
==================================================
+ Coverage 73.6739% 73.6875% +0.0136%
- Complexity 42586 42655 +69
==================================================
Files 1678 1678
Lines 146911 147029 +118
Branches 27791 27836 +45
==================================================
+ Hits 108235 108342 +107
+ Misses 28525 28522 -3
- Partials 10151 10165 +14
🚀 New features to boost your workflow:
|
…ed class N.this inside a nested class N is this, as in Java. The type checker rejected it with "No such property: this for Class or static property for class: Box$N", under @TypeChecked and @CompileStatic, for a static nested, inner or enum class, and also inside a closure or lambda there; dynamic Groovy ran it, and javac accepts it. The type checker now types N.this inside N, outside a static context, as N. In a static context ResolveVisitor still reports its error. Top.this in a top-level class keeps its error in every mode (see StaticThisTest, GROOVY-4321).
…ling the same super method Under @CompileStatic a call to super.m() in a closure or lambda calls a generated method super$2$m of the enclosing class. The type checker records the target for every such call, and StaticCompilationMopWriter generated one method per record, so two closures or lambdas calling the same super method produced the method twice and the class failed to load with "ClassFormatError: Duplicate method name super$2$m". Dynamic Groovy and @TypeChecked run the same code. The writer now generates one method per name and descriptor; overloads of the same name keep one method each.
…lass N.super inside a nested class N is super, as in Java (JLS 15.11.2, 15.12.1). Under @TypeChecked and @CompileStatic, N.super.m() and N.super.p failed with "No such property: super for Class or static property for class: Box$N". Dynamic Groovy ran N.super.m(), but read N.super.p and N.super.q as N.this.p and N.this.q, the subclass's properties; javac reads the superclass's. ResolveVisitor now replaces N.super inside a nested class N with super, so every mode gives it the meaning of super: a call or property of the superclass, also inside a closure or lambda, for static nested, inner and enum classes. Under @CompileStatic a closure or lambda then calls the superclass method through the super$ helper (see GROOVY-12516). In a static context ResolveVisitor still reports its error. Top.super in a top-level class keeps its error in every mode (see StaticThisTest, GROOVY-4321), and Outer.super, naming an outer class, is not changed here. In dynamic Groovy, N.super.p inside N now reads the superclass's property instead of N.this.p.
…es, increments and compound assignments Under static compilation a property read or written through super inside a closure or lambda called the superclass accessor with invokespecial from the closure class, which fails verification. A super method call in a closure already goes through a super$ helper of the enclosing class; the type checker now asks for the same helper for the getter and setter of a super property (also for a property whose accessors are generated later), and the property access and assignment transformers call it. super.name++, ++super.name and super.name += value read the property through ScriptBytecodeAdapter.getPropertyOnSuper (and ++ also wrote it through setPropertyOnSuper), which throws MissingPropertyException when the class declares the same property. The call site writer now reads and writes such a super property through the superclass accessor, directly with invokespecial, or through the super$ helpers in a closure or lambda.
In a class nested in Outer that has an enclosing instance of Outer (an
inner class, an anonymous class, a closure or lambda in one of them, or an
inner class of an inner class), Outer.super names the superclass of that
instance (JLS 15.11.2, 15.12.1). The type checker rejected it ("No such
property: super"); it now types Outer.super as the superclass of Outer.
Where the class has no enclosing instance of Outer it stays an error.
Under static compilation, a method or property of Outer.super is called
through a super$ helper of Outer, as a super call in a closure is: the
JVM allows invokespecial on Outer's superclass only from within Outer.
Property reads and writes, increments and compound assignments use the
helpers for the accessors.
Dynamic Groovy read and wrote a property of Outer.super as a property of
Outer.this; it now goes to the superclass property, as Outer.super.m()
already went to the superclass method.
…name++ and compound assignments super.name++, ++super.name, super.name op= value and super.name ?= value wrote the property through the first one-argument setter the superclass declares, so with overloads such as setX(int) and setX(String) the value could go to the wrong one: a GroovyCastException, a character passed as an int, or, in a closure, a lambda or for Outer.super.name, the class's own property written instead. The call site writer now calls the setter the type checker selected for the value, which is also the one it asked the super$ helper for. The lookup of a property node whose accessors "are generated later" is removed: the accessors of every class of the compilation unit exist before bytecode is generated, so it only ran for a property with a non-default accessor name, and there it ignored static and private properties.
…and compound assignments super.name++, super.name op= value and their kin compiled to dynamic getPropertyOnSuper and setPropertyOnSuper calls when an accessor of the property is an extension method, such as getText() and setText(String) of a File, also in a closure, a lambda and for Outer.super.name. The call site writer now calls the extension methods directly: the setter the type checker selected for the value, and the getter it selects when the superclass declares none.
…uperclass Outer.super.x = v, ++ and compound assignments choose among the setters of the superclass, as super.x = v does since GROOVY-12521, in dynamic Groovy, under @TypeChecked and under @CompileStatic.
cd35874 to
cdb685d
Compare
✅ All tests passed ✅🏷️ Commit: cdb685d Learn more about TestLens at testlens.app/docs. |
daniellansun
left a comment
daniellansun
left a comment
There was a problem hiding this comment.
Thanks for the careful multi-ticket stack on qualified this/super — tip cdb685d.
The shape is right: ResolveVisitor rewrites N.super to super (GROOVY-12514) after the existing Class.this/Class.super checks; STC recognizes N.this and types Outer.super as the enclosing type’s superclass (GROOVY-12517); SC sends Outer.super (and super-from-closure) through Outer’s super$ helpers, with InvocationWriter’s existing Outer.this rewrite for the receiver; StaticCompilationMopWriter dedupes super$ by name+descriptor (GROOVY-12516); and the 12518 property / ++ / += / overloaded-setter / extension-accessor paths are backed by strong parameterized tests. CI is green on this tip.
A few items before I would approve:
-
COMPATIBILITY.md for the dynamic meaning change
N.super.p and Outer.super.p / Outer.super.f used to read the subclass / enclosing instance’s own member in dynamic Groovy; they now follow javac (superclass member). That is the same class of silent change we documented for GROOVY-12521. The PR description is clear; please add a short COMPATIBILITY section (Who is affected) before merge — also needed if we cherry-pick to GROOVY_6_0_X. -
Outer.super.@field asymmetry
visitPropertyExpression now uses isSuperOrOuterSuper → get/setPropertyOnSuper, but visitAttributeExpression still keys only on isSuperExpression. Outer.super.@q therefore still hits Outer’s own field. Please mirror the property fix (and test Outer.super.@x / N.super.@x), or file a follow-up JIRA and mention it explicitly so we do not leave the @ form silently wrong. makeSuperPropertySite’s AttributeExpression skip is the SC counterpart of the same gap. -
Landing next to #3071 (GROOVY-12526)
Complementary, not competing: #3071 rejects non-direct Iface.super; this PR enables class N.super / Outer.super. Expect a small ResolveVisitor / STC existsProperty conflict. Keep the interface check as an early path and the N.super rewrite afterward for non-interface nested classes. -
Nit: isSuperOrOuterSuper vs InvocationWriter.isOuterClassSuper
Same predicate in two places (and !isInterface() matters next to #3071). A shared helper would help; not a merge blocker. -
Cherry-pick
After COMPATIBILITY: full tip to GROOVY_6_0_X is appropriate (12516/12518 are pure fixes; 12514/12517 fix STC/SC hard failures and intentionally align dynamic Groovy with javac). Without COMPATIBILITY I would only be comfortable cherry-picking the 12516+12518 commits.
Happy to re-review quickly once COMPATIBILITY is in and the attribute path is either fixed or explicitly deferred with a ticket.
| MethodCallerMultiAdapter adapter; | ||
| if (controller.getCompileStack().isLHS()) { | ||
| adapter = isSuperExpression(objectExpression) ? setPropertyOnSuper : isGroovyObject(expression) ? setGroovyObjectProperty : setProperty; | ||
| adapter = isSuperOrOuterSuper(objectExpression) ? setPropertyOnSuper : isGroovyObject(expression) ? setGroovyObjectProperty : setProperty; |
There was a problem hiding this comment.
Property form now goes through isSuperOrOuterSuper → get/setPropertyOnSuper, but the attribute adapters in visitAttributeExpression (tip ~1550) still key only on isSuperExpression. Outer.super.@q will load Outer.this and then getField without useSuper, i.e. Outer's own field — the same bug shape this PR fixes for Outer.super.q.
Please mirror isSuperOrOuterSuper there (and cover Outer.super.@x / N.super.@x in InnerClassTest), or open a follow-up ticket and call it out in the PR so the @ form is not left silently wrong.
| * Determines if an expression is "super" or "Outer.super" for a class: a | ||
| * property of it is a property of the superclass of the instance. | ||
| */ | ||
| private static boolean isSuperOrOuterSuper(final Expression expression) { |
There was a problem hiding this comment.
isSuperOrOuterSuper matches InvocationWriter.isOuterClassSuper / isClassWithSuper (GROOVY-11611). Fine as a local copy for this fix, but consider one shared ExpressionUtils (or package) helper so the !isInterface() guard stays aligned with GROOVY-12526 (#3071).
| } | ||
|
|
||
| checkThisAndSuperAsPropertyAccess(xe); | ||
| if (isSuperOfCurrentNestedClass(xe)) { // "Type.super" within nested class Type is "super" |
There was a problem hiding this comment.
Good place for the rewrite (after checkThisAndSuperAsPropertyAccess). When rebasing onto or against #3071 (GROOVY-12526), keep the interface-super rejection as an early path for ClassExpression + interface + "super", and keep this rewrite only for non-interface nested classes — the interface early-return must not swallow N.super.
| return true; | ||
| } | ||
|
|
||
| private static MethodNode findSuperPropertyAccessor(final PropertyExpression expression, final ClassNode superType, final String name, final boolean setter) { |
There was a problem hiding this comment.
findSuperPropertyAccessor looks up isX then getX on the superclass, while the setter path trusts DIRECT_METHOD_CALL_TARGET. That matches Verifier-generated boolean properties and the new tests. Optional hardening: for getters, prefer DIRECT_METHOD_CALL_TARGET when present (as setters do) so a synthetic isX recorded on SUPER_MOP_METHOD_REQUIRED cannot disagree with a lone getX on the ClassNode.
This makes qualified
thisandsuperin nested classes (N.this,N.super,Outer.super) work under@TypeCheckedand@CompileStaticas in Java.Outer.superfrom a closure or an inner class goes through thesuper$helper methods that statically compiled closures use forsupercalls, so the two bugs of those helpers that it runs into are fixed first: GROOVY-12516 and GROOVY-12518. Each issue has its own commit (GROOVY-12514 one forthisand one forsuper, GROOVY-12518 two more, for overloaded setters and for extension accessors).Outer.superwithout an enclosing instance, in a static nested class or in an anonymous class in a static method, is a compile error in every mode since GROOVY-12512; the support forOuter.superhere relies on that check.Outer.super.x = v,++and compound assignments with overloaded setters of the superclass choose the setter assuper.x = vdoes since GROOVY-12521;InnerClassTest.testQualifiedSuperOfOuterClassWithOverloadedSetterscovers it in all three modes. Two cases combine this with other open pull requests, and the second of each pair to be merged adds their tests:Outer.super.sm()for a static method thatOuterhides needs GROOVY-12519 (GROOVY-12519: resolve super.name() and super.name to a static member the class hides #3045), assuper.sm()does;super.text += vunder@TypeChecked, where the accessors are extension methods that the class overrides, needs GROOVY-12522 (GROOVY-12522: super.name() and super.name find an extension method that the class overrides #3048).GROOVY-12514:
N.thisandN.superwithin a nested classNN.this, which isthis, failed under@TypeCheckedand@CompileStaticwith "No such property: this", andN.super.m()andN.super.pwith "No such property: super", in static nested, inner and enum classes and in closures and lambdas there. Both now compile, as in javac.N.supermeanssuperin every mode: dynamic Groovy readN.super.pasN.this.p. In a static context they remain errors.Top.thisandTop.superin a top-level class keep their error (GROOVY-4321,StaticThisTest).GROOVY-12517:
Outer.superin a class nested inOuterIn an inner class, an anonymous class, a closure or lambda in one of them, or an inner class of an inner class,
Outer.super.m()andOuter.super.pfailed under@TypeCheckedand@CompileStaticwith "No such property: super". They now call the superclass's member on the enclosingOuterinstance, as in javac. Under@CompileStaticthe calls go throughsuper$helper methods ofOuter, since onlyOutermay call its superclass's methods non-virtually. Property reads and writes,++and compound assignments work too. Dynamic Groovy read and wroteOuter.super.pasOuter.this.p; it now uses the superclass's property. Without an enclosing instance ofOuterit stays an error.GROOVY-12516 and GROOVY-12518:
superin closures under@CompileStaticsuper$helper twice, and the class failed to load withClassFormatError. The helper is generated once.super.p,super.nameandsuper.name = vin a closure or lambda failed withVerifyError(the superclass accessor was called withinvokespecialfrom the closure class); they now go throughsuper$helpers, like super method calls.super.n++,++super.nandsuper.n += vread the property through a dynamic call that failed withMissingPropertyExceptionwhen the class declares the same property; they now call the superclass's accessors. When the superclass overloads the setter (setX(int)andsetX(String)), the setter is the one the type checker selected for the value. When an accessor is an extension method, such asgetText()andsetText(String)of aFile,super.text += vcompiled to a dynamic property access; it now calls the extension methods directly.Behaviour changes
Code that changes meaning in dynamic Groovy and under
@TypeChecked, each time to javac's meaning:N.super.pinsideNreads the superclass's property instead ofN.this.p(GROOVY-12514);Outer.super.pandOuter.super.fin a class nested inOuteruse the superclass's member instead ofOuter.this's (GROOVY-12517).Code that failed to compile and now compiles: GROOVY-12514 and GROOVY-12517. Code that failed at run time and now runs under
@CompileStatic: GROOVY-12516 and GROOVY-12518.Tests
Each commit has tests that fail without it, marked with its JIRA key:
gls.innerClass.InnerClassTest,ClosuresSTCTest(andClosuresStaticCompileTest),FieldsAndPropertiesSTCTest(andFieldsAndPropertiesStaticCompileTest) and, for the overloaded setters and the extension accessors,FieldsAndPropertiesStaticCompileTestalone. 13,655 tests in thegroovy,gls,bugs,org.codehaus.groovy.classgen,transformandcontrolpackages pass, apart frombugs.Groovy12363, which fails on master too when it runs afterbugs.Groovy12142in the same JVM. The Checkstyle and CodeNarc gates pass.Backport
GROOVY-12514 and GROOVY-12517 change what
N.super.pandOuter.super.pmean in dynamic Groovy, so whether to backport them is left to the maintainers; GROOVY-12516 and GROOVY-12518 only make code that failed run.