[
https://issues.apache.org/jira/browse/GROOVY-12062?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18086240#comment-18086240
]
ASF GitHub Bot commented on GROOVY-12062:
-----------------------------------------
codecov-commenter commented on PR #2590:
URL: https://github.com/apache/groovy/pull/2590#issuecomment-4628115084
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2590?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 68.2831%. Comparing base
([`13b009a`](https://app.codecov.io/gh/apache/groovy/commit/13b009a3243716c000d3ceb7db681883d98bf76b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`0dec4e0`](https://app.codecov.io/gh/apache/groovy/commit/0dec4e03bc51296f36feb7c038dab2031af9d678?dropdown=coverage&el=desc&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/2590?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2590 +/- ##
==================================================
+ Coverage 68.2396% 68.2831% +0.0436%
- Complexity 33337 33367 +30
==================================================
Files 1518 1518
Lines 126815 126819 +4
Branches 22995 22995
==================================================
+ Hits 86538 86596 +58
+ Misses 32601 32543 -58
- Partials 7676 7680 +4
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2590?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...org/codehaus/groovy/classgen/asm/CompileStack.java](https://app.codecov.io/gh/apache/groovy/pull/2590?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FCompileStack.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9Db21waWxlU3RhY2suamF2YQ==)
| `86.5854% <100.0000%> (+0.3011%)` | :arrow_up: |
|
[.../codehaus/groovy/classgen/asm/StatementWriter.java](https://app.codecov.io/gh/apache/groovy/pull/2590?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FStatementWriter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9TdGF0ZW1lbnRXcml0ZXIuamF2YQ==)
| `96.8912% <100.0000%> (-0.0398%)` | :arrow_down: |
... and [21 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2590/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>
> OptimizingStatementWriter __$stMC slow branch resolves a try-block local as
> getProperty when the method has a non-empty finally (classic non-indy
> codegen) - regression in 5.0.x
> --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12062
> URL: https://issues.apache.org/jira/browse/GROOVY-12062
> Project: Groovy
> Issue Type: Bug
> Components: bytecode, class generator
> Affects Versions: 6.0.0-alpha-1, 5.0.5, 5.0.6, 5.0.7
> Environment: JDK 21 (Corretto 21.0.11); optimizationOptions.indy =
> false (classic codegen)
> Reporter: James Fredley
> Priority: Major
>
> h2. Summary
> Under classic (non-{{indy}}) codegen, {{OptimizingStatementWriter}} emits an
> optimizable statement behind a {{__$stMC}} fast/slow guard built from the
> *same* AST. When the method has a non-empty {{finally}} block, a local
> variable declared in the {{try}} block and passed as a method-call argument
> is compiled inconsistently: the fast branch ({{__$stMC=true}}) loads it from
> its local slot, but the slow branch ({{__$stMC=false}}) re-resolves it as
> {{this.getProperty("x")}} and throws {{MissingPropertyException}} at runtime.
> The slow branch is taken depending on the receiver's metaclass state, so
> correct source fails at runtime. {{indy=true}} has no {{__$stMC}} fork and is
> unaffected. This is a regression: Groovy 4.0.x compiles both branches
> correctly.
> h2. Steps to reproduce
> Two pure-Groovy files - no Grails dependency, no AST transform.
> *Target.groovy*
> {code:groovy}
> class Target {
> String compute(int x) {
> "ok${x}"
> }
> Object compute() {
> try {
> int x = 7
> return this.compute(x) // 'x' is a try-block local
> } finally {
> 123 // a NON-EMPTY finally is required to
> trigger the bug
> }
> }
> }
> {code}
> *RunRepro.groovy* - compiles {{Target.groovy}} with {{indy=false}}, then
> exercises both branches:
> {code:groovy}
> import org.codehaus.groovy.control.CompilationUnit
> import org.codehaus.groovy.control.CompilerConfiguration
> File out = new File('build/repro-classes'); out.mkdirs()
> def config = new CompilerConfiguration()
> config.optimizationOptions.put('indy', false) // classic codegen
> config.targetDirectory = out
> def cu = new CompilationUnit(config)
> cu.addSource(new File('Target.groovy'))
> cu.compile()
> def loader = new URLClassLoader([out.toURI().toURL()] as URL[],
> this.class.classLoader)
> Class<?> t = loader.loadClass('Target')
> def stmc = t.getDeclaredField('__$stMC'); stmc.accessible = true
> [false, true].each { boolean v ->
> stmc.setBoolean(null, v)
> def obj = t.getDeclaredConstructor().newInstance()
> try {
> println "__\$stMC=${v} -> ${t.getMethod('compute').invoke(obj)}"
> } catch (java.lang.reflect.InvocationTargetException e) {
> println "__\$stMC=${v} -> ${e.cause.class.name}: ${e.cause.message}"
> }
> }
> {code}
> Run it with: {{groovy RunRepro.groovy}}
> h2. Actual result (Groovy 5.0.5 / 5.0.7-SNAPSHOT / 6.0.0-alpha-1)
> {noformat}
> __$stMC=false -> groovy.lang.MissingPropertyException: No such property: x
> for class: Target
> __$stMC=true -> ok7
> {noformat}
> A bare {{new Target().compute()}} (no reflection) also throws
> {{MissingPropertyException}} on 5.0.x, because {{__$stMC}} is {{false}} on
> first use.
> h2. Expected result (as on Groovy 4.0.32)
> {noformat}
> __$stMC=false -> ok7
> __$stMC=true -> ok7
> {noformat}
> h2. Affected versions
> || Groovy || stMC=false (slow path) || stMC=true (fast path) || Result ||
> | 4.0.32 | ok7 | ok7 | correct |
> | 5.0.5 | MissingPropertyException | ok7 | broken |
> | 5.0.7-SNAPSHOT | MissingPropertyException | ok7 | broken |
> | 6.0.0-alpha-1 | MissingPropertyException | ok7 | broken |
> h2. Bytecode evidence
> {{Target.compute()}} disassembled with {{javap -c}} (Groovy 5.0.7-SNAPSHOT,
> indy=false):
> {noformat}
> public java.lang.Object compute();
> 8: getstatic #60 // Field __$stMC:Z (the fast/slow
> guard)
> ... __$stMC=true (FAST branch):
> 28: iload_2 // loads local x
> -> correct
> 44: areturn
> ... __$stMC=false (SLOW branch):
> 55: invokeinterface CallSite.callGroovyObjectGetProperty //
> this.getProperty("x") -> BUG
> 63: invokevirtual compute:(I)String
> 75: areturn
> {noformat}
> The two branches are not semantically equivalent for the same local {{x}}.
> h2. Root cause (Groovy 5.0.5 sources, org.codehaus.groovy.classgen.asm)
> * {{OptVisitor.visitMethodCallExpression}} marks the {{this.compute(x)}} call
> optimizable, so the enclosing {{return}} becomes a {{__$stMC}} fork and is
> emitted twice.
> * Whether a variable compiles to a local load or a property read is decided
> at codegen by {{CompileStack.getVariable(name, false)}}
> ({{AsmClassGenerator.visitVariableExpression}}); a miss is rewritten to
> {{this.<name>}} and emitted as {{getProperty}}.
> * Emitting the non-empty {{finally}} restores/unwinds the {{CompileStack}}
> scope and drops {{x}}, so on one of the two emissions {{getVariable("x")}}
> misses and that branch falls back to {{this.getProperty("x")}}.
> * Net: the finally-driven scope restore makes the two {{__$stMC}} emissions
> of the same {{return}} see different {{CompileStack}} state for {{x}}. Groovy
> 4.0.x does not exhibit this.
> h2. Real-world impact
> Apache Grails {{ControllerActionTransformer}} generates a zero-arg action
> wrapper of exactly this shape (locals bound from request params, a delegating
> {{this.action(p1, ...)}} call, wrapped in {{try/catch/finally}}). Under
> {{-PgrailsIndy=false}} a parameterized action throws
> {{MissingPropertyException}} in any live app (apache/grails-core PR #15557).
> Standalone Grails reproducer:
> https://github.com/jamesfredley/groovy5-controller-action-param-scope-bug
> h2. Workaround
> Tag the {{ClassNode}} with {{OptimizingStatementWriter.ClassNodeSkip}} to
> suppress the fork (apache/grails-core PR #15557, commit fb725b178b).
> h2. Suggested fix
> Keep {{try}}-block locals registered in the {{CompileStack}} across the
> non-empty {{finally}} scope restore so both {{__$stMC}} emissions resolve
> them identically; or skip the fast/slow fork for statements whose locals
> cross a {{finally}} boundary.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
