Repository navigation
GROOVY-12522: super.name() and super.name find an extension method that the class overrides - #3048
codeconsole wants to merge 3 commits into
Conversation
…at the class overrides For super usage, a method the class declares is replaced by the MOP method super$N$name of the superclass method it overrides, or dropped when there is none. An extension method has no MOP method, so a class that declares its own first(), getProperties() or, extending File, getText() lost the extension method it had replaced in the method index: in dynamic Groovy and under @TypeChecked, super.first() threw a MissingMethodException, super.properties a MissingPropertyException, and super.getText() called getText(File, String) with a null charset. Under @CompileStatic, super.text += 'x' compiles to a dynamic property access and failed the same way. The method is now replaced by the extension method of the superclass, or of an interface it implements, with the same parameters, the one for the most specific type.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3048 +/- ##
==================================================
+ Coverage 73.6728% 73.6879% +0.0151%
- Complexity 42587 42597 +10
==================================================
Files 1678 1678
Lines 146913 146944 +31
Branches 27792 27806 +14
==================================================
+ Hits 108235 108280 +45
+ Misses 28526 28520 -6
+ Partials 10152 10144 -8
🚀 New features to boost your workflow:
|
daniellansun
left a comment
There was a problem hiding this comment.
Thanks — the failure mode is clear, and restoring the hidden extension into methodsForSuper is the right place to fix it.
I think getExtensionMethodsOfSuperclass is incomplete for a common shape:
class C implements List {
def first() { … }
def t() { super.first() }
}
Here c.getSuperclass() is Object, and Object's CachedClass interface set is empty, so List's extension methods are never candidates. ArrayList/File tests pass because the superclass already carries List/etc. in getInterfaces().
Could we also collect interfaces from c itself (or from every type in the hierarchy, including c)? A regression test of the form above would lock it in.
Smaller nits: when two unrelated interfaces both contribute the same extension, the "most specific" check does not order them — a short comment or a deterministic rule would help. Otherwise the approach (isDGM/NewMetaMethod, param equality, most-specific declaring type, one map per class) looks solid, and skipping the CS-closure case until GROOVY-12519 is fine.
Happy to +1 once the implements-only case is covered.
…lf implements super.name() looked for the overridden extension method among the superclasses and the interfaces they implement, so in a class whose superclass is Object, such as one that implements Iterable or delegates List with @DeleGate, super.first() still threw a MissingMethodException. The interfaces of the class itself are now searched as well. The candidates are collected in the order the classes declare their interfaces, breadth first, after the superclasses, instead of from CachedClass.getInterfaces(), which is a hash set. Of two extension methods for unrelated types, the one met first is used.
|
Fixed in 39b195c. The interfaces of the class itself are now searched too, so On the ordering nit: candidates are now collected in declaration order, starting with the superclasses (nearest first), then the interfaces of the class and of each superclass, breadth first. They no longer come from The new test runs only in dynamic mode. Under |
# Conflicts: # src/test/groovy/groovy/ThisAndSuperTest.groovy
✅ All tests passed ✅🏷️ Commit: 8184868 Learn more about TestLens at testlens.app/docs. |
When a class declares its own version of an extension method of its superclass, such as
first()in anArrayListsubclass,getProperties(), orgetText()in aFilesubclass,super.first(),super.propertiesandsuper.getText()in dynamic Groovy and under@TypeCheckedthrewMissingMethodExceptionorMissingPropertyException, or calledgetText(File, String)with a null charset. Under@CompileStatic,super.text += vcompiles to a dynamic property access and failed the same way. They now call the superclass's extension method, as they do when the class does not declare one. This happens in 4.0.33 to master.How
For
superusage, the meta-class replaces a method the class declares with the MOP methodsuper$N$nameof the superclass method it overrides, and drops it when there is none. An extension method has no MOP method, so the class's method was dropped, and the extension method it had replaced in the method index was not restored. It now is: the extension method of the superclass, or of an interface it implements, with the same parameters, the one for the most specific type. The lookup is built once per class, by name.Tests
groovy.ThisAndSuperTest, in all three modes, and in a closure in dynamic Groovy and under@TypeChecked. They fail without the change. Under@CompileStatic, asupercall to an extension method in a closure fails on master for another reason, withNoSuchMethodError; GROOVY-12519 (#3045) fixes that, so the closure case here runs in dynamic Groovy and under@TypeCheckedonly. 14,936 tests in thegroovy,gls,bugs,org.codehaus.groovy.classgen,transform,runtime,reflection,vmplugin,controlandastpackages and the stub generator tests pass.groovy-contracts,groovy-typecheckers,groovy-ginq,groovy-macro-library,groovy-json,groovy-xmlandgroovy-templatespass their tests, and every subproject compiles its main and test sources. The Checkstyle and CodeNarc gates pass.