[
https://issues.apache.org/jira/browse/GROOVY-11986?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18077887#comment-18077887
]
Jochen Theodorou commented on GROOVY-11986:
-------------------------------------------
I consider the Groovy6 behavior a breaking change and I think we should
consider undoing it. Why did we make this change in the first place?
> genericGetMethod registration too permissive: matches any get(X) where X is a
> supertype of String
> -------------------------------------------------------------------------------------------------
>
> Key: GROOVY-11986
> URL: https://issues.apache.org/jira/browse/GROOVY-11986
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Priority: Major
>
> h2. Summary
> On Groovy 6.0.0-SNAPSHOT, dynamic property access on an instance whose class
> inherits a single-arg {{get(X)}} method — where {{X}} is any supertype of
> {{String}} ({{Serializable}}, {{Object}}, {{CharSequence}}, {{Comparable}},
> {{Cloneable}}) — is incorrectly routed through that {{get}} method as the
> {{genericGetMethod}}, bypassing {{propertyMissing(String)}}. The same code
> works on Groovy 4.x and 5.x.
> Discovered during the Grails 8 / Groovy 6 canary ([apache/grails-core PR
> #15558|https://github.com/apache/grails-core/pull/15558]). GORM's
> {{GormEntity}} trait declares {{static D get(Serializable id)}} as the
> entity-by-ID lookup; on Groovy 6, {{book.someConnection}}, {{book.errors}},
> etc. now hit {{get(Serializable)}} with the property name as the "id"
> argument instead of falling through to {{propertyMissing}}. CI failures: 16
> in {{DataServiceConnectionRoutingSpec}}, similar pattern in
> {{CrossLayerMultiDataSourceSpec}}.
> h2. Reproducer
> Standalone repro:
> [https://github.com/jamesfredley/groovy6-get-as-generic-getter]
> {code:title=EntityTrait.groovy}
> trait EntityTrait {
> Object get(Serializable id) { "no-such-entity:${id}".toString() }
> Object propertyMissing(String name) { new DelegatingApi(name: name) }
> }
> {code}
> {code:title=Book.groovy}
> @CompileStatic
> class Book implements EntityTrait {
> String title
> }
> {code}
> {code:title=Driver}
> def book = new Book(title: 'Effective Java')
> def r = book."secondary"
> // Groovy 4.x / 5.x: r is DelegatingApi[secondary] (PASS)
> // Groovy 6.0: r is "no-such-entity:secondary" (FAIL — hit
> get(Serializable))
> {code}
> h2. Observed behaviour
> || Groovy version || {{book."secondary"}} ||
> | 4.0.27 | DelegatingApi (PASS) — propertyMissing wins |
> | 5.0.5 | DelegatingApi (PASS) — propertyMissing wins |
> | 6.0.0-SNAPSHOT | *"no-such-entity:secondary" (FAIL) — get(Serializable)
> wins* |
> h2. Root cause
> Commit {{7bc29825bc}} (GROOVY-11829, "check param types of standard methods
> and keep best fit", Jan 2026, master only) replaced the strict
> {{isGenericGetMethod}} check with an assignability-based {{checkMatch}}.
> *Pre-change* ({{MetaClassImpl.isGenericGetMethod}}, Groovy 4.x and 5.x):
> {code:java}
> return "get".equals(method.getName())
> && parameterTypes.length == 1
> && parameterTypes[0].getTheClass() == String.class; // strict equality
> {code}
> The candidate had to take *exactly {{String}}*. {{get(Serializable)}},
> {{get(Object)}}, etc. were rejected.
> *Post-change* ({{MetaClassImpl.checkMatch}} / {{checkIfStdMethod}}, Groovy
> 6.0.0-SNAPSHOT):
> {code:java}
> case "get":
> if (checkMatch(metaMethod, genericGetMethod, GETTER_MISSING_ARGS)
> && metaMethod.getReturnType() != Void.TYPE) {
> genericGetMethod = metaMethod;
> }
> private static boolean checkMatch(MetaMethod newMethod, MetaMethod oldMethod,
> Class<?>[] arguments) {
> return newMethod.isValidExactMethod(arguments) && (oldMethod == null
> || MetaClassHelper.calculateParameterDistance(arguments, newMethod)
> <= MetaClassHelper.calculateParameterDistance(arguments,
> oldMethod));
> }
> {code}
> {{isValidExactMethod({String.class})}} accepts any method whose parameter is
> a *supertype of {{String}}* — {{String}}, {{CharSequence}}, {{Comparable}},
> {{Serializable}}, {{Object}}, {{Cloneable}}. So inherited methods like
> {{get(Serializable)}} now register as the generic getter, and dynamic
> property reads route through them before {{propertyMissing}} is consulted.
> h2. Scope of impact
> Only signatures where String is assignable to the param trip the regression.
> {{get(File)}}, {{get(Long)}}, {{get(UUID)}} are unaffected. Affected:
> {{get(String)}}, {{get(CharSequence)}}, {{get(Comparable)}},
> {{get(Serializable)}}, {{get(Object)}}, {{get(Cloneable)}}. {{Map}}-typed
> classes ({{get(Object)}}) are within the impact zone in principle but Map
> dispatch is special-cased elsewhere; GORM-style {{get(Serializable)}} is the
> visible casualty.
> h2. GROOVY-11829's intent vs side effect
> The added tests target {{def get(String name)}} and a setter-precedence case
> ({{set(Object, Object)}} vs {{set(String, Object)}} — the more-specific one
> needs to win). The "best-fit" {{<= calculateParameterDistance}} tie-breaker
> is the actual goal. Folding the getter side into the same
> {{isValidExactMethod}} check inadvertently relaxed the getter's historically
> strict "must be {{String}} exactly" rule.
> h2. Suggested options
> # Restore the strict {{String}}-exact check on the getter side, while keeping
> the new best-fit logic for the setter side. (The getter regression doesn't
> appear to have been a GROOVY-11829 target.)
> # Or keep the assignability check but require {{parameter == String.class}}
> for *initial* registration, then use {{<=}} distance for tie-breaking further
> candidates.
> # Add a test mirroring this scenario — class with inherited
> {{get(Serializable)}} (or {{get(Object)}}) + {{propertyMissing(String)}} — so
> the regression can't recur.
> h2. Workaround (already applied in Grails)
> Per-entity AST-injected shim that gives Groovy's "best-fit" tie-breaker a
> more-specific {{get(String)}} candidate than the inherited
> {{get(Serializable)}}:
> {code}
> public Object get(String name) {
> propertyMissing(name)
> }
> {code}
> Verified locally: with this shim added, parameter distance to {{{String}}} is
> 0 for {{get(String)}} vs ≥1 for {{get(Serializable)}}, so the new tie-breaker
> picks the shim and {{book."secondary"}} correctly routes to
> {{propertyMissing}}. The shim should be removed once the upstream fix lands.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
