[jira] [Commented] (GROOVY-12144) AstNodeToScriptAdapter decompiler drops or corrupts several constructs when rendering AST back to source

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GROOVY-12144?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18095255#comment-18095255 ] 

ASF GitHub Bot commented on GROOVY-12144:
-----------------------------------------

leonard84 opened a new pull request, #2682:
URL: https://github.com/apache/groovy/pull/2682

   Upstreams eight rendering-fidelity fixes to the Groovy Console's AST decompiler (`AstNodeToScriptAdapter` / `AstNodeToScriptVisitor`), plus a prerequisite fix that re-enables its test class which a JUnit 5 migration had silently disabled.
   
   Two commits, two JIRAs (12145 first, as it's a prerequisite):
   
   ## GROOVY-12145 — re-enable `AstNodeToScriptAdapterTest`
   
   The "Convert to JUnit5" migration (`a1897e0811`) removed `extends GroovyTestCase` but never added `@Test`. Since the build uses `useJUnitPlatform()`, JUnit Platform discovers tests only by `@Test`, so all 63 tests silently stopped executing — a `--tests` filter reports *"No tests found for given includes"*. The `import org.junit.jupiter.api.Test` was also mis-placed inside a triple-quoted script string.
   
   This commit adds `@Test` to every method, moves the import to the top, and refreshes two assertions whose expected output had drifted while the tests were dark (`@Immutable`'s hashCode accumulator `Object`→`Integer`; `@Lazy` now emits `@Internal` before the backing field).
   
   > GROOVY-12145 also surveys a broader fallout of the same migration (~195 dead test methods across ~32 files). This PR fixes only the `groovy-console` instance, which is the prerequisite for the fidelity tests below; the full survey is attached to the JIRA.
   
   ## GROOVY-12144 — decompiler rendering fidelity
   
   `AstNodeToScriptVisitor` rendered several constructs so they did not round-trip — re-parsing produced a different type or different semantics. Eight independent defects, each fixed in a single visitor method:
   
   | # | Construct | Rendered before | Rendered after |
   |---|---|---|---|
   | 1 | nested generic type arguments | `Map<String, List>` | `Map<String, List<Integer>>` |
   | 2 | range boundary exclusions | `(1..5)` (all) | `(1..<5)` / `(1<..5)` / `(1<..<5)` |
   | 3 | elvis operator | `c ? c : d` (evaluates `c` twice) | `c ?: d` |
   | 4 | attribute (direct field) access | `obj.f` | `obj.@f` / `obj*.@f` / `obj?.@f` |
   | 5 | explicit zero-arg closure | `{ ... }` (gains implicit `it`) | `{ -> ... }` |
   | 6 | safe index access | `list[0]` | `list?[0]` |
   | 7 | numeric literal type suffixes | `42` / `2.5` / `3.5` / `10` | `42L` / `2.5F` / `3.5D` / `10G` |
   | 8 | explicit method-call type arguments | `Collections.emptyList()` | `Collections.<String>emptyList()` |
   
   Defects 3, 5, 6 and 7 changed the *meaning* of the recovered source, not just its appearance. Each defect has a regression test (marked `// GROOVY-12144`) that fails before the fix and passes after.
   
   **Reviewer note:** fix 4 makes `AttributeExpression` render faithfully everywhere. `@Log` builds `Level.FINE` as an `AttributeExpression`, so it now renders `Level.@FINE` — a faithful view of the actual AST node (previously mis-shown as property access); `testLogAnnotation` is updated accordingly. Two existing tests are updated to the corrected output (`testTernaryOperaters`; `testGenericsInMethods`, whose own comments had asked whether the nested type args should be dropped).
   
   These fidelity issues were originally found and fixed downstream in Spock's transpiler (derived from this class; both projects are Apache-2.0), and this upstreams them.
   
   ## Testing
   
   `./gradlew :groovy-console:test` → **138/138 green**. The GROOVY-12145 commit stands on its own (63/63 with the unmodified adapter), and the GROOVY-12144 commit adds the eight regression tests on top.
   
   ---
   🤖 Prepared with Claude Code (Opus 4.8); commits carry `Assisted-by` trailers per the ASF generative-tooling guidance.
   




> AstNodeToScriptAdapter decompiler drops or corrupts several constructs when rendering AST back to source
> --------------------------------------------------------------------------------------------------------
>
>                 Key: GROOVY-12144
>                 URL: https://issues.apache.org/jira/browse/GROOVY-12144
>             Project: Groovy
>          Issue Type: Bug
>          Components: groovy-console
>            Reporter: Leonard Brünings
>            Priority: Major
>
> h2. Description
> {{groovy.console.ui.AstNodeToScriptAdapter}} (class {{{}AstNodeToScriptVisitor{}}}) turns a compiled AST back into Groovy source; it backs the Groovy Console's AST browser and is usable from the command line. Several node kinds are rendered incorrectly, producing source that does not round-trip - it either fails to re-parse, or re-parses to a *different* program (changed type or changed semantics).
> Eight distinct rendering defects are present on {{{}master{}}}. They are independent and each has a one-line cause in a single visitor method.
> || #  ||Construct||Input||Rendered now (wrong)||Should render||
> |1|Nested generic type arguments|{{Map<String, List<Integer>>}}|{{Map<String, List>}}|{{Map<String, List<Integer>>}}|
> |2|Range boundary exclusions|{{1..<5}} / {{1<..5}} / {{1<..<5}}|{{(1..5)}} (all three)|{{(1..<5)}} / {{(1<..5)}} / {{(1<..<5)}}|
> |3|Elvis operator|{{c ?: d}}|{{c ? c : d}} (evaluates {{c}} twice)|{{c ?: d}}|
> |4|Attribute (direct field) access|{{obj.@f}} / {{obj*.@f}} / {{obj?.@f}}|{{obj.f}} / {{obj*.f}} / {{obj?.f}} (silently becomes property access)|{{obj.@f}} / {{obj*.@f}} / {{obj?.@f}}|
> |5|Explicit zero-arg closure|{\{ { -> foo() }}}\|{{{}{ foo() }{}}} (re-parses with an implicit {{{}it{}}})\|\{{{ -> foo() }
> }}|
> |6|Safe index access|{{{{{}list?[0]{}}}}}|{{list[0]}}|{{list?[0]}}|
> |7|Numeric literal type suffixes|{{42L}} / {{2.5f}} / {{3.5d}} / {{10G}}|{{42}} / {{2.5}} / {{3.5}} / {{10}} (re-parses as {{{}Integer{}}}/{{{}BigDecimal{}}})|{{42L}} / {{2.5F}} / {{3.5D}} / {{10G}}|
> |8|Explicit method-call type arguments|{{Collections.<String>emptyList()}}|{{Collections.emptyList()}}|{{Collections.<String>emptyList()}}|
> Defects 3, 5, 6 and 7 change the *meaning* of the recovered source, not just its appearance.
> h3. Root cause (per defect)
>  * *1* - {{visitGenerics}} prints {{it.name}} only and never recurses into a concrete type argument's own generics.
>  * *2* - {{visitRangeExpression}} prints {{'..'}} unconditionally, ignoring {{RangeExpression.isExclusiveLeft()}} / {{{}isExclusiveRight(){}}}.
>  * *3* - {{visitShortTernaryExpression}} delegates to {{{}visitTernaryExpression{}}}, duplicating the condition.
>  * *4* - {{visitAttributeExpression}} delegates to {{{}visitPropertyExpression{}}}, which always prints {{{}'.'{}}}.
>  * *5* - {{visitClosureExpression}} treats {{parameters == null}} (explicit \{{{} \{ -> }
> {}}}) the same as {{parameters == Parameter.EMPTY_ARRAY}} (implicit {{{}it{}}}); only the latter should omit the arrow.
>  * *6* - {{visitBinaryExpression}} detects the {{LEFT_SQUARE_BRACKET}} access but prints a hardcoded {{{}'['{}}}, ignoring {{{}isSafe(){}}}.
>  * *7* - {{visitConstantExpression}} prints {{value.toString()}} with no type suffix.
>  * *8* - {{visitMethodCallExpression}} ignores {{{}MethodCallExpression.getGenericsTypes(){}}}.
> h2. Steps to reproduce
> {code:groovy}
> import groovy.console.ui.AstNodeToScriptAdapter
> import org.codehaus.groovy.control.CompilePhase
> def src = 'def x = Collections.<String>emptyList(); def y = 1..<5; def z = a?.@f'
> println new AstNodeToScriptAdapter().compileToScript(src, CompilePhase.SEMANTIC_ANALYSIS.phaseNumber)
> // observe: Collections.emptyList(), (1..5), a.f  -- all three wrong
> {code}
> h2. Suggested fix
> One localized change per visitor method (all in {{{}subprojects/groovy-console/src/main/groovy/groovy/console/ui/AstNodeToScriptAdapter.groovy{}}}), plus one regression test per defect in {{{}AstNodeToScriptAdapterTest{}}}.
> *Note for reviewers:* fix 4 makes {{AttributeExpression}} render faithfully as {{{}.@{}}}. This changes the rendered output of AST-transform-generated code that builds attribute nodes - e.g. {{@Log}} generates {{Level.@FINE}} for its {{isLoggable}} guard. That output is now a faithful representation of the actual AST node (previously mis-shown as property access {{{}Level.FINE{}}}); {{{}testLogAnnotation{}}}'s expectation is updated accordingly.
> h2. Notes
>  * These fidelity bugs were originally found and fixed downstream in Spock's transpiler (a fork of this class); this ticket upstreams them.
>  * Both projects are Apache-2.0 and the Spock file derives from this one, so the contribution is clean.
>  * Depends on the test class actually running - see the companion ticket GROOVY-12145 on the JUnit 5 migration having left {{AstNodeToScriptAdapterTest}} unexecuted (commit {{{}a1897e0811{}}}).
> {panel:title=Provenance}
> Assisted-by: Claude Code (Opus 4.8). Contributor is responsible for correctness, licensing, and style.
> {panel}



--
This message was sent by Atlassian Jira
(v8.20.10#820010)