[
https://issues.apache.org/jira/browse/GROOVY-12063?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18133711#comment-18133711
]
ASF GitHub Bot commented on GROOVY-12063:
-----------------------------------------
codecov-commenter commented on PR #3067:
URL: https://github.com/apache/groovy/pull/3067#issuecomment-6476022923
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/3067?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `67.74194%` with `20 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 73.6153%. Comparing base
([`cfd9b2f`](https://app.codecov.io/gh/apache/groovy/commit/cfd9b2f0222a8e1a74473f7c5b2cb45b5bdd3b5b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`2d623f0`](https://app.codecov.io/gh/apache/groovy/commit/2d623f06b9cf92895fe2a884a3be150f873bffea?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3067?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[...codehaus/groovy/classgen/VariableScopeVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/3067?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2FVariableScopeVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL1ZhcmlhYmxlU2NvcGVWaXNpdG9yLmphdmE=)
| 67.7419% | [9 Missing and 11 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3067?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/3067?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #3067 +/- ##
==================================================
+ Coverage 73.6047% 73.6153% +0.0105%
- Complexity 42348 42365 +17
==================================================
Files 1657 1657
Lines 145708 145770 +62
Branches 27498 27524 +26
==================================================
+ Hits 107248 107309 +61
+ Misses 28479 28474 -5
- Partials 9981 9987 +6
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3067?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...codehaus/groovy/classgen/VariableScopeVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/3067?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2FVariableScopeVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL1ZhcmlhYmxlU2NvcGVWaXNpdG9yLmphdmE=)
| `91.6409% <67.7419%> (-2.5372%)` | :arrow_down: |
... and [13 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/3067/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> Anonymous inner class extending Map: enclosing-class field references in the
> instance initializer compile to dynamic getProperty on 'this' and silently
> resolve to null - regression in 5.0.x
> ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12063
> URL: https://issues.apache.org/jira/browse/GROOVY-12063
> Project: Groovy
> Issue Type: Bug
> Components: class generator, Compiler
> Affects Versions: 6.0.0-alpha-1, 5.0.5, 5.0.6, 5.0.7
> Environment: JDK 21; reproduces with and without @CompileStatic, and
> in both indy and classic codegen
> Reporter: James Fredley
> Assignee: Daniel Sun
> Priority: Major
>
> h2. Summary
> In an anonymous inner class that extends {{java.util.Map}} (e.g.
> {{HashMap}}), bareword references to the *enclosing class's fields* inside
> the AIC instance-initializer block are no longer resolved to those fields. As
> of Groovy 5.0.x they compile to dynamic property reads on {{this}}
> ({{this.getProperty("FIELD")}}); because {{this}} is a {{Map}}, the MOP turns
> that into a key lookup {{this.get("FIELD")}} on the still-empty map, so the
> reference silently evaluates to {{null}}. This is a regression: 3.0.x and
> 4.0.x resolve it correctly. The {{Map}} base class makes it fail *silently* -
> a non-Map AIC would instead throw {{MissingPropertyException}}.
> h2. Steps to reproduce
> Single self-contained script:
> {code:groovy}
> class Holder {
> static final String VALUE = 'expected'
> static final Map<String, String> MAP = new HashMap<String, String>() {
> {
> put('literalKey', VALUE) // 'VALUE' is the enclosing class's
> static field
> }
> }
> }
> println "Groovy ${GroovySystem.version} -> ${Holder.MAP}"
> assert Holder.MAP.get('literalKey') == 'expected'
> {code}
> h2. Actual result (Groovy 5.0.5 / 5.0.6 / 5.0.7-SNAPSHOT / 6.0.0-alpha-1)
> {noformat}
> Groovy 5.0.7-SNAPSHOT -> [literalKey:null]
> Assertion failed:
> assert Holder.MAP.get('literalKey') == 'expected'
> | | | |
> | | null false
> {noformat}
> h2. Expected result (as on Groovy 3.0.24 / 4.0.32)
> {noformat}
> Groovy 4.0.32 -> [literalKey:expected]
> (assertion passes)
> {noformat}
> h2. Affected versions
> || Groovy || Holder.MAP || Result ||
> | 3.0.24 | {{[literalKey:expected]}} | correct |
> | 4.0.32 | {{[literalKey:expected]}} | correct |
> | 5.0.5 | {{[literalKey:null]}} | broken |
> | 5.0.6 | {{[literalKey:null]}} | broken |
> | 5.0.7-SNAPSHOT | {{[literalKey:null]}} | broken |
> | 6.0.0-alpha-1 | {{[literalKey:null]}} | broken |
> Notes: reproduces for a plain class, a {{@CompileStatic}} class, and a
> {{@CompileStatic}} interface - so it is not specific to {{@CompileStatic}}.
> The literal key survives (an {{LDC}} constant); only the bareword field
> reference degrades to {{null}}. If both key and value are field references,
> the map collapses to {{[null:null]}}.
> h2. Bytecode evidence
> {{Holder$1.<init>}} (the anonymous HashMap initializer), {{javap -c}}, Groovy
> 5.0.7-SNAPSHOT:
> {noformat}
> 13: aload_0 // receiver = this (the
> HashMap)
> 14: ldc "literalKey" // key: literal ->
> correct
> 16: aload_0
> 17: invokedynamic #0:getProperty:(LHolder$1;) // value:
> this.getProperty("VALUE") -> BUG
> 22: invokedynamic #1:invoke:(...) // this.put(key, value),
> dynamic
> 28: return
> {noformat}
> The BootstrapMethods table confirms the call-site name is the source
> identifier ({{IndyInterface.bootstrap(..., "VALUE", 12)}}). The enclosing
> {{<clinit>}} is correct - {{VALUE}} is assigned before {{new Holder$1()}}
> runs - so the defect is purely the AIC initializer compiling the bareword as
> a dynamic read on {{this}} instead of {{GETSTATIC}}. At runtime
> {{MetaClassImpl.getProperty}} special-cases {{Map}} receivers as {{((Map)
> this).get(name)}}; the map is empty during its own initializer, so the read
> returns {{null}}. The JDK is not involved - the emitted bytecode is wrong.
> h2. Root cause
> * The bareword {{VALUE}} inside the AIC initializer is resolved as a dynamic
> property read on {{this}} ({{invokedynamic getProperty}}) rather than a
> {{GETSTATIC}} of the enclosing class's field.
> * Because the AIC extends {{Map}}, Groovy's MOP turns that property read into
> a map-key lookup {{this.get("VALUE")}}; the map is empty while its own
> initializer runs, so it returns {{null}} instead of failing fast.
> * Groovy 3.0.x / 4.0.x resolve the bareword to the enclosing-class field
> correctly.
> h2. Real-world impact
> Apache Grails {{grails.gorm.validation.ConstrainedProperty}} builds its
> default-messages map with exactly this idiom ({{new HashMap<String,String>()
> { { put(CODE, MESSAGE) } }}}); under Groovy 5 the map ends up effectively
> empty ({{[null:null]}}), so default validation messages fail to resolve
> (apache/grails-core PR #15557).
> h2. Workarounds
> * Avoid the anonymous-Map-with-instance-initializer idiom; use
> {{Map.of(...)}}, a literal map, or a statically compiled static block.
> * Or fully qualify the references: {{put('literalKey', Holder.VALUE)}}.
> h2. Suggested fix
> Resolve bareword identifiers inside an AIC instance-initializer against the
> lexical scope (enclosing-class members) before falling back to a dynamic
> property read on {{this}}, regardless of whether {{this}} is a {{Map}} - i.e.
> restore 4.0.x name-resolution for AIC initializer blocks.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
