[
https://issues.apache.org/jira/browse/GROOVY-12041?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18083468#comment-18083468
]
Paul King commented on GROOVY-12041:
------------------------------------
h2. Proposed Resolution
Close as *Won't Fix* / by design. The 2023 change
([1ea6927ad9|https://github.com/apache/groovy/commit/1ea6927ad9]) is a
deliberate STC improvement that has shipped in every {{GROOVY_5_0_X}} release
since the branch was cut. It correctly informs STC that {{this.foo}} on a
receiver with {{getProperty(String)}} is dynamic, fixes GROOVY-7024 and
GROOVY-10985, and has at least one follow-up (GROOVY-11817) layered on top that
relies on {{DYNAMIC_RESOLUTION}} being propagated from the synthesised {{pexp}}
onto the {{vexp}}. A blanket reversal would regress all of these.
A surgical change — fire {{unresolvedVariable}} / {{unresolvedProperty}} *in
addition to* marking the node {{DYNAMIC_RESOLUTION}}, so extensions can still
observe it — is the principled "have-both" fix, but it changes the extension
contract in the opposite direction (callbacks fire for things that STC
nonetheless considers resolved) and risks breaking other extensions that today
rely on those callbacks indicating "STC gave up." Not pursued here without a
concrete need.
h2. Workaround for extension authors
No Groovy-side change is required. Extensions using the cross-callback
node-identity bookkeeping pattern can recover the lost cases by adding a single
fallback in {{methodNotFound}} that consults the receiver's
{{DYNAMIC_RESOLUTION}} metadata:
{code:groovy}
import org.codehaus.groovy.transform.stc.StaticTypesMarker
methodNotFound { receiver, name, argList, argTypes, call ->
def oe = call.objectExpression
if (oe != null && (currentScope.dynamicProperties.contains(oe)
||
oe.getNodeMetaData(StaticTypesMarker.DYNAMIC_RESOLUTION))) {
return makeDynamic(call)
}
}
{code}
Verified against the standalone reproducer on Groovy 5.0.6: every
previously-failing case ({{extends Script}}, {{extends Base}} with
{{getProperty}}) now compiles cleanly. The unchanged cases continue to pass.
h2. Recommendation for downstream projects
* *Grails* ({{GroovyPageTypeCheckingExtension}}): apply the fallback above,
then the {{@IgnoreIf({ instance.isGroovy5OrLater() && data.gDotPrefix })}}
guards on the three {{GspCompileStaticSpec}} cases in [apache/grails-core PR
#15557|https://github.com/apache/grails-core/pull/15557] can be dropped. No
Groovy upgrade required.
* *Other DSL / builder extensions* (Gradle, Spock data-driven specs, anything
that mixes {{@CompileStatic}} with a {{getProperty(String)}}-using base class)
should audit any cross-callback identity bookkeeping. If an extension assumes
"{{unresolvedVariable}} always fires for any name that isn't a real
field/method," that assumption is no longer true on Groovy 5.
h2. Documentation follow-up
Worth a paragraph in the [type-checking extensions
documentation|https://docs.groovy-lang.org/latest/html/documentation/type-checking-extensions.html]
noting that, since Groovy 5, identifiers resolvable via the receiver's
{{getProperty(String)}} / {{setProperty(String, Object)}} are pre-resolved by
STC as dynamic and do *not* trigger the {{unresolvedVariable}} /
{{unresolvedProperty}} callbacks; extensions wishing to react to such receivers
should look for {{StaticTypesMarker.DYNAMIC_RESOLUTION}} metadata on the
receiver expression.
h2. References
* Standalone reproducer:
[jamesfredley/groovy5-stc-extension-node-identity-bug|https://github.com/jamesfredley/groovy5-stc-extension-node-identity-bug]
* Triggering Groovy commit:
[1ea6927ad9|https://github.com/apache/groovy/commit/1ea6927ad9] — _GROOVY-7024,
GROOVY-10985: self property precedence_
* Related: GROOVY-7024, GROOVY-10985, GROOVY-7996, GROOVY-11817
* Downstream impact: [apache/grails-core PR
#15557|https://github.com/apache/grails-core/pull/15557]
({{GspCompileStaticSpec}} {{@IgnoreIf}} guards)
> STC: unresolvedVariable/unresolvedProperty no longer fires when receiver
> inherits getProperty(String)
> -----------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12041
> URL: https://issues.apache.org/jira/browse/GROOVY-12041
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Priority: Major
>
> A {{TypeCheckingDSL}} extension that captures the receiver in
> {{unresolvedVariable}} / {{unresolvedProperty}} and later matches it by node
> identity in {{methodNotFound}} stops working in Groovy 5 when the enclosing
> class inherits {{getProperty(String)}} (e.g. extends {{Script}} or a
> GSP-style {{GroovyPage}} base). The {{unresolvedVariable}} /
> {{unresolvedProperty}} callback never fires, so the extension has no chance
> to record the receiver, and the downstream {{methodNotFound}} rejects the
> chained call.
> h2. Observed behaviour
> Reproducer extension (abridged):
> {code:groovy}
> newScope { dynamicProperties = [] as Set }
> unresolvedVariable { VariableExpression ve ->
> if (ve.name == 'g') { currentScope.dynamicProperties << ve; return
> makeDynamic(ve) }
> }
> unresolvedProperty { PropertyExpression pe ->
> if (pe.propertyAsString == 'g') { currentScope.dynamicProperties << pe;
> return makeDynamic(pe) }
> }
> methodNotFound { receiver, name, argList, argTypes, call ->
> if (call.objectExpression != null
> &&
> currentScope.dynamicProperties.contains(call.objectExpression)) {
> return makeDynamic(call)
> }
> }
> {code}
> Results across versions (running {{g.message(code: 'World')}} inside the
> {{Subject}}):
> ||Subject shape||4.0.27||5.0.5 / 5.0.6||
> |Plain class, {{g.message(...)}}|PASS ({{unresolvedVariable}} fires)|PASS
> (fires twice, but works)|
> |Plain class, {{this.g.message(...)}}|PASS ({{unresolvedProperty}}
> fires)|PASS|
> |{{extends Script}}, {{g.message(...)}}|*PASS*|*FAIL* —
> {{unresolvedVariable}} never fires|
> |{{extends Base}} with {{Object getProperty(String)}},
> {{g.message(...)}}|*PASS*|*FAIL* — same|
> |{{extends Script}}, {{this.g.message(...)}}|PASS|PASS|
> The Groovy 5 failure is:
> {noformat}
> [Static type checking] - Cannot find matching method
> java.lang.Object#message(LinkedHashMap<String,String>)
> {noformat}
> Diagnostic prints confirm that on the failing cases the extension is *not*
> called for {{g}} at all; {{methodNotFound}} fires with an empty
> {{dynamicProperties}} set. Where the callbacks do fire, AST node identity is
> *preserved* across callbacks on Groovy 5 — i.e. the originally reported "node
> identity changes between callbacks" hypothesis is not what is happening.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
