GROOVY-4721: variable declared in try block is in scope in finally block

[daniellansun]

https://issues.apache.org/jira/browse/GROOVY-4721

[Copilot]

Pull Request Overview

This pull request addresses GROOVY-4721 by adjusting the scope handling of variables declared in try blocks to ensure their correct accessibility within the corresponding finally block.

• Updated imports for new AST classes
• Refactored makeBlockRecorder to utilize a consistent compileStack instance and incorporate explicit handling for empty finally statements

Files not reviewed (2)

• src/test/groovy/bugs/Groovy4721.groovy: Language not supported
• subprojects/groovy-groovysh/src/test/groovy/org/apache/groovy/groovysh/GroovyshTest.groovy: Language not supported

Comments suppressed due to low confidence (1)

src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java:409

• By returning early for a null or EmptyStatement, the lambda may bypass necessary compileStack state management that is performed in the non-empty path. Consider verifying whether any push/pop operations should still occur to maintain a consistent compileStack state.

if (finallyStatement == null || finallyStatement instanceof EmptyStatement) return;

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.9776%. Comparing base (6ce967f) to head (fef0b8c).
Report is 25 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                 @@
##               master      #2172        +/-   ##
==================================================
+ Coverage     68.8443%   69.9776%   +1.1333%     
- Complexity      29533      31746      +2213     
==================================================
  Files            1421       1422         +1     
  Lines          113424     119714      +6290     
  Branches        19644      21835      +2191     
==================================================
+ Hits            78086      83773      +5687     
- Misses          28754      29326       +572     
- Partials         6584       6615        +31     

Files with missing lines	Coverage Δ
.../codehaus/groovy/classgen/asm/StatementWriter.java	97.5104% <100.0000%> (+0.3757%)	⬆️
.../groovy/org/apache/groovy/groovysh/Groovysh.groovy	60.0790% <100.0000%> (ø)

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[eric-milles]

Seems reasonable. Nice compact change in StatementWriter.

[paulk-asert]

This isn't the existing behavior of groovysh with interpreterMode set.

groovy:000> :set interpreterMode
groovy:000> int x = 3
===> 3
groovy:000> x
===> 3
groovy:000> int a, b, c
===> 0
groovy:000> b
===> 0

[paulk-asert]

I would like to fix the original issue, but it would be good to not break interpreterMode. I'll try to find time to investigate a little more.

[daniellansun]

groovy/subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/Groovysh.groovy

Line 294 in 8739056

buff = [importsSpec] + ['try {', 'true'] + current + ['} finally {' + variableBlocks + '}']

Groovysh will try to collect variables in the finally block, but variables declared in try block can not be accessed in finally block after GROOVY-4721 is fixed.

[paulk-asert]

I added the change to StatementWriter and changed groovysh to just have the "variableBlocks" execute directly after "current" (i.e. just removed the try...finally) then interpreterMode still seemed to work. The current test suite and the new tests all passed. There could be variations that we don't have coverage for in our test suite that may become broken with that change.

[daniellansun]

if the code of current throws an exception, the variables will still fail to collect.

[daniellansun]

@paulk-asert I've polished the PR as you suggested:
fef0b8c