Repository navigation
GROOVY-12614: static properties and accessors of traits (bundles 12 PRs) and a specification of member access in trait code - #3297
paulk-asert wants to merge 16 commits into
Conversation
…ith the implementing class (apache#3115) Under @CompileStatic, trait code that reads or writes a static accessor of a trait with property syntax ("ready" for isReady(), "cnt = 1" for setCnt(int)) called the trait interface's forwarder, which passes the trait itself as the receiver class, so the accessor failed to reach the trait's static state. Call the trait helper with $static$self or $self.getClass() instead, as the method call form already does.
…t in trait code (apache#3123) In trait code, "this.ready" for a static "isReady()" of the trait is compiled as "$self.ready" (or "$static$self.ready" in a static method of a sub-trait), and the type checker reported "static method of interface T can only be accessed with class qualifier", although "this" stands for the implementing class, which has the static accessor. The implicit form "ready" is checked the same way, but its error was dropped because the generated expression has no source position. Skip the check when the receiver is the trait's $self or $static$self parameter and the accessor is declared by a trait.
…erty in a closure with the implementing class (apache#3135) A closure or lambda in trait code leaves a property name that does not resolve to a property of the trait itself unchanged (GROOVY-8049, GROOVY-9386), so that the closure's delegate can provide it. Under @CompileStatic a static accessor of a super trait reached that way ("cnt" for getCnt()) was called on the super trait interface, which passes the trait itself to the helper as the receiver class, so an accessor that reads trait state failed. A static setter written as a property ("cnt = n") was rejected by the type checker, which looked for setters on the receiver type only, where a static interface method is not inherited. In a closure, call the trait helper with the class of the closure's thisObject, which is $self, or $static$self in a static trait method. A lambda has no thisObject, so rewrite the name there to "$self.name" or "$static$self.name", as for a property of the trait itself. Let trait code see the static setters of a super trait.
…ty in a static scope of the implementing class (apache#3132)
…ritten as a property (apache#3121)
…n a closure or for an instance field (apache#3136)
…lf clearly (apache#3124) A promoted trait static called as Trait.m() runs with the trait interface as its static self. The trait has no static state of its own, so a read or write of a static field failed with a MissingMethodException naming the internal accessor (T__flag$get). The trait interface now declares those accessors and they throw a MissingFieldException that says the state belongs to each implementing class.
…va.lang.Class (apache#3120) In a static trait method the receiver is typed Class<T>, and TraitReceiverTransformer.tryGetFieldNode asked that type for the field first. A static property named like a declared field of java.lang.Class (name, module, componentType, packageName) resolved to that private field, so the assignment called a setter named after Object (java_lang_Object__name$set), which fails at run time when dynamic and is rejected under @TypeChecked/@CompileStatic.
…ugh the closure's delegate (apache#3125)
…clares a method of the same name (apache#3128)
…tic is called through the trait (apache#3131)
…of the accessor In the body of a trait method, "label" and "this.label" for a static accessor getLabel() of the trait were resolved through the implementing class in dynamic and @TypeChecked code, so a static getLabel() of the implementing class was used, while the method form getLabel() and the property form under @CompileStatic used the trait's own accessor. The property form is now rewritten to the method form, which is then dispatched as any call of a trait static method is: to the trait's own method, or through the implementing class for a @virtual method. An assignment calls the setter and a compound assignment calls the setter with the value computed from the getter. Names in a closure or lambda are still left for the closure to resolve, since its delegate may provide them, and ++ and -- are not changed. TraitStaticDispatchMatrix row09acc asserted the old dynamic behaviour; it now uses @virtual, and row09accp covers the plain accessor.
…es the accessor A declared property of a trait referenced by its simple name in a closure is resolved by the closure, and so reaches the accessor of the implementing instance. The qualified form this.name in a closure still read and wrote the trait's backing field. It is now a property access on the implementing instance, as it is in a closure in a class that declares the property. A lambda keeps the direct field reference.
Adds "Member access in trait code" to the traits guide: defined terms, numbered rules for instance and static members, and a summary table of what a property name or accessor call in trait code refers to in a method body, a closure and a lambda. The rules cover GROOVY-12590, GROOVY-12594, GROOVY-12596, GROOVY-12598, GROOVY-12605, GROOVY-12608 and GROOVY-12614. The spec tests behind the examples run as dynamic, @TypeChecked and @CompileStatic code. TraitMemberAccessMatrix asserts the whole table in the three modes and records the cells where the implementation does not yet follow the rules. The @virtual javadoc now says that the marker does not apply to calls from a closure or lambda.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3297 +/- ##
==================================================
+ Coverage 74.4894% 74.5202% +0.0307%
- Complexity 44906 45094 +188
==================================================
Files 1703 1703
Lines 152180 152501 +321
Branches 29166 29292 +126
==================================================
+ Hits 113358 113644 +286
Misses 28408 28408
- Partials 10414 10449 +35
🚀 New features to boost your workflow:
|
daniellansun
left a comment
There was a problem hiding this comment.
Thank you for gathering the twelve fixes and for writing the member-access rules down so carefully. The matrix, with the open cells that fail if they start to follow the rule, is a very useful companion to the new guide section. The GROOVY-12614 and GROOVY-12598 changes match the description in COMPATIBILITY.md, and the @Virtual migration example there is clear.
I would like to request two changes before this becomes the published specification, and I have a few further notes that I hope are useful.
1. Sticky inLambda and instance rule 4. When a regular { } closure is nested inside a lambda, inLambda stays true, so isClosurePropertyReference and isDeclaredInstancePropertyInClosure both return false. A declared instance property in that inner closure then goes to the backing field in all three compilation modes, which does not match instance rule 4. The reverse nesting (a lambda inside { }) already follows rule 5. I may have missed a test for this; the matrix and the NOTE do not cover it. If an implementing class's setter is used as a validation hook, this path would skip it. Could inLambda track only the innermost construct (the existing inLambdaOnly flag already answers a related question), and could a test for { name } / this.name inside a lambda, in all three modes, lock the intended rule? If the present behaviour is left as an open cell, I think the guide should say so plainly, including that the implementing-class accessor is not called.
2. Terms versus ++ / -- on a plain static accessor. The Terms say that an increment or decrement of p is a reference of the same form as the name it is applied to, and static rule 4 already treats assignment and compound assignment as accessor calls. Prefix and postfix expressions are marked as assignmentTarget and then passed to super.transform, so a plain static accessor is not rewritten — which COMPATIBILITY.md already records as deliberate. With a class override, assignment and += use the trait accessor (already tested), but ++ / -- in dynamic and @TypeChecked code use the class getter and setter, and under @CompileStatic the getter is the trait's and the setter is the class's. That mixed @CompileStatic result looks like the older property-form split that GROOVY-12614 left in place for increment and decrement, rather than a new regression. Would you be willing either to bring ++ / -- in line with +=, or to drop the synonymy claim from the Terms and lock the current behaviour with tests in all three modes?
Other notes, if you are willing to take them now or in a follow-up, are on the inline comments: the summary NOTE does not mention that a @Virtual static accessor uses the implementing class for label / this.label in a closure or lambda under @CompileStatic as well; the official GROOVY-12608 lambda-with-delegate test does not lock @CompileStatic (the same script returns 1 there); a super-trait static accessor in a statically compiled method body does not yet get the same invokestatic as the closure path; and the clearer MissingFieldException from GROOVY-12597 may be worth a COMPATIBILITY.md entry if that fix is also backported.
| boolean oldInClosure = inClosure, oldInLambdaOnly = inLambdaOnly, oldInLambda = inLambda; | ||
| inLambdaOnly = exp instanceof LambdaExpression && (!inClosure || inLambdaOnly); | ||
| inClosure = true; | ||
| inLambda = oldInLambda || exp instanceof LambdaExpression; |
There was a problem hiding this comment.
I may have missed a test for this, but inLambda is set with oldInLambda || exp instanceof LambdaExpression, so it stays true when a regular { } closure is nested inside a lambda. isClosurePropertyReference and isDeclaredInstancePropertyInClosure then return false, and a declared instance property in that inner closure goes to the backing field in all three modes, which does not match instance rule 4. The reverse nesting already follows rule 5. If an implementing-class setter is used as a validation hook, this path would skip it.
Could this flag track only the innermost construct? A test for { name } and this.name inside a lambda, in all three modes, would make the intended rule visible. If the present behaviour is kept, I think the guide should say so plainly.
| } else if (exp instanceof PrefixExpression || exp instanceof PostfixExpression) { | ||
| Expression oldTarget = assignmentTarget; | ||
| assignmentTarget = exp instanceof PrefixExpression ? ((PrefixExpression) exp).getExpression() : ((PostfixExpression) exp).getExpression(); | ||
| try { | ||
| return super.transform(exp); | ||
| } finally { | ||
| assignmentTarget = oldTarget; | ||
| } |
There was a problem hiding this comment.
For a plain static accessor, ++ and -- are marked as assignmentTarget and then left to super.transform, so they are not rewritten as getter plus setter, unlike +=. With a class override, assignment and += use the trait accessor, but ++ / -- use the class getter and setter in dynamic and @TypeChecked code, and under @CompileStatic the getter is the trait's and the setter is the class's. That looks like the older property-form split left in place, as COMPATIBILITY.md already notes that increment and decrement are unchanged.
The Terms, however, say that increment and decrement have the same meaning as the name they apply to. Would you consider either bringing these in line with +=, or adjusting the Terms and adding tests in all three modes?
| or `setP(v)`, with or without `this.`. An assignment, compound assignment, increment or decrement of `p` is a | ||
| reference of the same form as the name it is applied to. |
There was a problem hiding this comment.
As noted on the transformer, this synonymy does not currently hold for ++ and -- on a plain static accessor-only property. Could the Terms or static rule 4 mention that these are not brought in line with the new rule, unless the rewrite is completed?
| * A _name_ (`p` or `this.p`) of a static accessor-only property in a closure or a lambda is resolved by the closure, | ||
| so that its delegate can supply the name. When no delegate does, dynamic and `@TypeChecked` code reach the | ||
| implementing class's accessor, with or without `@Virtual`, where the rules give the trait's own. The method form | ||
| `getP()` follows the rules in every mode. |
There was a problem hiding this comment.
Would it be worth spelling out the @CompileStatic case for a @Virtual static accessor as well? TraitMemberAccessMatrix.virtualStaticAccessorOnlyProperty records that label and this.label in a closure or a lambda use the implementing class in all three modes, whereas a plain static accessor does so only in dynamic and @TypeChecked code. The table can stay as the rule; I think the NOTE should list that cell the same way the matrix does.
| @ParameterizedTest | ||
| @ValueSource(strings=['@CompileDynamic','@TypeChecked']) | ||
| void testSuperTraitStaticAccessorAsPropertyInLambdaWithDelegate(String mode) { | ||
| assertScript shell, """ | ||
| class P { | ||
| int cnt = 42 | ||
| } | ||
| $mode | ||
| trait T { | ||
| private static int count = 1 | ||
| static int getCnt() { count } | ||
| static void setCnt(int n) { count = n } | ||
| } | ||
| $mode | ||
| trait U extends T { | ||
| int get(P p) { p.with((x) -> cnt) } | ||
| } | ||
| $mode | ||
| class C implements U { | ||
| } | ||
|
|
||
| assert new C().get(new P()) == 42 |
There was a problem hiding this comment.
This official GROOVY-12608 case locks only @CompileDynamic and @TypeChecked, where the delegate supplies 42. The same script under @CompileStatic returns 1 (the delegate is ignored). Could @CompileStatic be locked as well, and the NOTE or an open cell record that difference?
| private static MethodNode findTraitHelperMethod(final MethodNode target) { | ||
| if (target.isStatic() && isTrait(target.getDeclaringClass())) { | ||
| Parameter[] parameters = target.getParameters(); | ||
| for (MethodNode method : Traits.findHelper(target.getDeclaringClass()).getDeclaredMethods(target.getName())) { | ||
| Parameter[] helperParameters = method.getParameters(); | ||
| if (method.isStatic() && helperParameters.length == parameters.length + 1 | ||
| && ClassHelper.isClassType(helperParameters[0].getType()) | ||
| && ParameterUtils.parametersEqual(Arrays.copyOfRange(helperParameters, 1, helperParameters.length), parameters)) { | ||
| return method; | ||
| } | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
A super-trait static accessor in a statically compiled method body does not yet take the same invokestatic path as a closure. This lookup only runs when target.isStatic() && isTrait(target.getDeclaringClass()), and only when closureThis || isTraitSelf(receiver). transformMethodCallOnThis writes a super-trait getN() as this.getN($static$self) without setMethodTarget, so the writer sees the child helper. The result is still correct, but the method body goes through invokedynamic and boxing, while the closure path is already invokestatic on the super-trait helper. Aligning the two would need the call to be bound to the super-trait helper, not only a wider lookup. I mention this in case you want to take it in a follow-up.
| private static MethodNode createTraitStaticStateAccessor(final ClassNode traitClass, final FieldNode field, final String name, final Parameter[] params) { | ||
| String message = "Cannot access static field " + field.getName() + " of trait " + traitClass.getName() | ||
| + " through the trait itself: trait static state belongs to each implementing class," | ||
| + " so call the trait static method on an implementing class"; | ||
| Statement body = throwS(ctorX(ClassHelper.make(MissingFieldException.class), | ||
| args(constX(message), constX(field.getName()), classX(traitClass)))); |
There was a problem hiding this comment.
The failure mode through the trait itself is now a clearer MissingFieldException that uses the source field name and the trait name, and does not leak the helper name. The guide already describes this; would a short COMPATIBILITY.md entry be useful as well, especially if the fix is also taken to 6.0.x?
| private static boolean isStaticallyCompiled(final AnnotatedNode member, final ClassNode trait) { | ||
| for (AnnotatedNode node : new AnnotatedNode[]{member, trait}) { | ||
| boolean compileStatic = false; | ||
| for (AnnotationNode annotation : node.getAnnotations()) { | ||
| String name = annotation.getClassNode().getName(); | ||
| Expression mode = annotation.getMember("value"); | ||
| if (name.equals("groovy.transform.CompileDynamic") || (mode != null && mode.getText().equals("groovy.transform.TypeCheckingMode.SKIP") | ||
| && (name.equals("groovy.transform.CompileStatic") || name.equals("groovy.transform.TypeChecked")))) { | ||
| return false; | ||
| } | ||
| compileStatic = compileStatic || name.equals("groovy.transform.CompileStatic"); | ||
| } | ||
| if (compileStatic) return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
isStaticallyCompiled only looks at annotations on the member or the trait. A method body still ends up as invokestatic when @CompileStatic is applied only through a compiler customizer, so the main effect is whether the GROOVY-12608 statically compiled lambda rewrite is triggered. It may be enough to document or test that case.
…uses the accessor Where closures and lambdas are nested in trait code, the innermost one decides how a declared instance property is accessed: a closure within a lambda now uses the accessor, as a closure does elsewhere. The simple name is not offered to the closure's delegate there. Also state in the user guide that an increment or a decrement of a static accessor-only property is not a call of the trait's accessors, extend the note on where compilation modes differ from the rules, and record the GROOVY-12608 change for a statically compiled lambda in COMPATIBILITY.md. Tests lock each of these in all compilation modes.
|
Thanks Daniel, all of these checked out. I've pushed 3ef9259 in response. 1. Closure within a lambda. Fixed: the innermost construct now decides, so a declared property in a closure within a lambda goes through the accessor for reads and writes, and a lambda within a closure still uses the field. 2. Other notes.
|
✅ All tests passed ✅🏷️ Commit: 3ef9259 Learn more about TestLens at testlens.app/docs. |
daniellansun
left a comment
There was a problem hiding this comment.
Thanks Paul. I have gone back over the whole of #3297 at 3ef9259, including the twelve bundled fixes, the later behaviour changes, the guide, and your follow-up to the earlier review.
The two changes from the earlier review are in place. The innermost construct now decides how a declared instance property is accessed when closures and lambdas are nested, testPropertyInClosureWithinLambdaUsesTheAccessor locks that in all three modes, and the NOTE and the GROOVY-12598 compatibility entry record that the simple name in a closure within a lambda is not offered to the delegate (because leaving it for the closure hits GROOVY-12602 under @CompileStatic). The Terms no longer treat increment and decrement as the same form of reference as assignment; static rule 4 and the “Prefix and postfix operations” section describe the present behaviour, and testIncrementOfStaticAccessorIsNotACallOfTheTraitAccessor locks it in all three modes, including the mixed @CompileStatic result. I agree it is better to handle ++ / -- on static accessors together with GROOVY-6689 than to complete only half of that here.
The other notes from last time look good as well. The summary NOTE now includes the @Virtual @CompileStatic cell; the official GROOVY-12608 lambda-with-delegate case is locked under @CompileStatic and has a compatibility entry; and the customizer path is tested and recorded as an open cell, including the MissingFieldException for a plain lambda () -> cnt. I am content to leave the super-trait invokestatic alignment in a method body, and a compatibility entry for the GROOVY-12597 message, for a later follow-up.
One residual case you may want to pick up later, or to list as an open cell: a SAM lambda nested in a closure does not yet take the GROOVY-12608 statically compiled rewrite, because that path still keys off inLambdaOnly (a lambda that is not inside a closure) rather than the innermost construct. A top-level Supplier s = () -> cnt under @CompileStatic correctly reads the super-trait accessor, but the same lambda inside { -> … } fails at run time with the GROOVY-12597 MissingFieldException. Dynamic and @TypeChecked code return the accessor value. This fails closed, and it is not covered by the official 12608 tests, so I do not think it needs to block this change. Using inLambda for that rewrite, or recording the nest in the NOTE with a locking test, would close the gap.
I also noticed that onMethodSelection always asks findVirtualStaticAccessorType for a read, unlike handleUnresolvedProperty. I could not turn that into a user-visible type-checking hole, so I am happy to leave it as an optional tidy-up.
I am happy to approve this version of the PR.
This bundles twelve open fixes for static properties, static accessors and
@Virtualstatics in traits, so that their behaviour can be checked as a whole across dynamic,@TypeCheckedand@CompileStaticcode, and adds a specification of the rules to the traits guide. It targets master; which commits go toGROOVY_6_0_Xis noted at the end.The "Decisions taken" section lists what was put into this PR, not decisions of the Groovy project. Each bundled PR is one commit with its original author, so the bundle can be merged as it is, trimmed or squashed.
What is in it
One commit per PR, in this order:
this.namefor a static accessor of a trait in trait code@Virtualstatic accessor read or written as a propertyjava.lang.Class@Virtualstatic call dynamic whenClassdeclares a method of the same name@Virtualstatic when a promoted static is called through the traitThree commits on top, described below: two that change behaviour and one for the guide, spec tests and a behaviour matrix.
Combining the PRs
TraitReceiverTransformer. All of the changes are kept.onMethodSelectionoverride toTraitTypeCheckingExtension. They merge without a textual conflict but do not compile together, so the two are one method here (in the GROOVY-12601 commit).Changes on top of the twelve PRs
1. A static accessor read as a property is a call of the accessor (GROOVY-12614)
For a trait with
static String getLabel(), and an implementing class that declares its own staticgetLabel():@TypeChecked@CompileStaticlabel,this.label(before)getLabel()(before and now)label,this.label(now)The guide and the
@Virtualjavadoc already say that a trait static is not overridden by the implementing class unless it is@Virtual. The property form now follows that in every mode: it is rewritten to the method form and dispatched as that is. Assignment calls the setter, and a compound assignment calls the setter with the value computed from the getter. With@Virtualthe property form uses the implementing class's accessor, as the method form does (that needs GROOVY-12594 above under@TypeCheckedand@CompileStatic).TraitStaticDispatchMatrix.row09accasserted the old dynamic behaviour for a plain accessor. It now uses@Virtual, androw09accpcovers the plain accessor.2.
this.namefor a declared trait property in a closure uses the accessor (follow-up to GROOVY-12598)With #3125, the simple name of a declared instance property in a closure is resolved by the closure and reaches the accessor of the implementing instance.
this.namein a closure still read and wrote the trait's backing field. It now uses the accessor too, as both forms do in a closure in a class that declares the property.Decisions taken
@Virtualis the way to make it so. This changes dynamic and@TypeCheckedcode that relied on the property form (seeCOMPATIBILITY.md, which shows the@Virtualmigration). A@Virtualstatic is not on the trait interface, soTrait.labelis not available after migrating.@Virtualdoes not apply to a call made from a closure or lambda (VirtualAnnotationTest.virtual_insideClosure_stillHelperBound) is kept and is now in the guide and the javadoc.Not changed
labelorthis.label) of a static accessor in a closure or lambda is still left for the closure to resolve, so that its delegate can supply it. When no delegate does, dynamic and@TypeCheckedcode reach the implementing class's accessor, where the rules give the trait's own. Changing that belongs with the open PRs on closures in trait code (GROOVY-12611: resolve a private or static trait method called from a closure through the closure's delegate #3138, GROOVY-12618: resolve a public static super trait method called from a closure through the closure's delegate #3144, GROOVY-12624: resolve calls that the trait transformation adds in a closure on the trait, not the closure's delegate #3150).ClassCastException(GROOVY-12602, GROOVY-12602: SC: pass $self as the receiver of a lambda in a trait method #3129).++and--on a static accessor property.Documentation
A new section of the traits guide, "Member access in trait code", states which accessor or field a name in trait code refers to: defined terms, five rules for instance members, five for static members, and a summary table for a method body, a closure and a lambda. A note names the two places above where the implementation does not follow the table yet.
@TypeCheckedand@CompileStaticcode.TraitMemberAccessMatrixasserts the whole table: 7 kinds of member, 10 forms of reference, 3 modes. Cells where the implementation differs from the rule are listed with what it does; a cell that starts to follow the rule fails its test until it is removed from the list.COMPATIBILITY.mdhas entries for the two behaviour changes.Testing
:test: 20,181 tests, 0 failures. Subproject tests were not run locally.GROOVY_6_0_XThe twelve PR commits cherry-pick onto
GROOVY_6_0_Xin order without conflicts; they have not been tested there. The two behaviour changes on top are meant for master only, and #3125 (GROOVY-12598) also changes behaviour, so it is a candidate to leave out of a backport.Supersedes #3115, #3120, #3121, #3123, #3124, #3125, #3128, #3131, #3132, #3135, #3136 and #3162.