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

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

codecov-commenter commented on PR #2684:
URL: https://github.com/apache/groovy/pull/2684#issuecomment-4939550477

   ## 
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2684?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 `76.19048%` with `5 lines` in your changes missing 
coverage. Please review.
   :white_check_mark: Project coverage is 67.2451%. Comparing base 
([`ee772e0`](https://app.codecov.io/gh/apache/groovy/commit/ee772e01f546b3961020581bb064a1f4e3df40fc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
 to head 
([`3a4bd6d`](https://app.codecov.io/gh/apache/groovy/commit/3a4bd6dbe68fd05bb51f31133b03c9b06430234d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   
   | [Files with missing 
lines](https://app.codecov.io/gh/apache/groovy/pull/2684?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Patch % | Lines |
   |---|---|---|
   | 
[...vy/groovy/console/ui/AstNodeToScriptAdapter.groovy](https://app.codecov.io/gh/apache/groovy/pull/2684?src=pr&el=tree&filepath=subprojects%2Fgroovy-console%2Fsrc%2Fmain%2Fgroovy%2Fgroovy%2Fconsole%2Fui%2FAstNodeToScriptAdapter.groovy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LWNvbnNvbGUvc3JjL21haW4vZ3Jvb3Z5L2dyb292eS9jb25zb2xlL3VpL0FzdE5vZGVUb1NjcmlwdEFkYXB0ZXIuZ3Jvb3Z5)
 | 76.1905% | [0 Missing and 5 partials :warning: 
](https://app.codecov.io/gh/apache/groovy/pull/2684?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>
   
   
   
   [![Impacted file tree 
graph](https://app.codecov.io/gh/apache/groovy/pull/2684/graphs/tree.svg?width=650&height=150&src=pr&token=1r45138NfQ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)](https://app.codecov.io/gh/apache/groovy/pull/2684?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@                  Coverage Diff                   @@
   ##             GROOVY_5_0_X      #2684        +/-   ##
   ======================================================
   - Coverage         67.2477%   67.2451%   -0.0027%     
   + Complexity          29571      29568         -3     
   ======================================================
     Files                1382       1382                
     Lines              116972     116990        +18     
     Branches            20539      20551        +12     
   ======================================================
   + Hits                78661      78670         +9     
   - Misses              31788      31792         +4     
   - Partials             6523       6528         +5     
   ```
   
   | [Files with missing 
lines](https://app.codecov.io/gh/apache/groovy/pull/2684?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Coverage Δ | |
   |---|---|---|
   | 
[...vy/groovy/console/ui/AstNodeToScriptAdapter.groovy](https://app.codecov.io/gh/apache/groovy/pull/2684?src=pr&el=tree&filepath=subprojects%2Fgroovy-console%2Fsrc%2Fmain%2Fgroovy%2Fgroovy%2Fconsole%2Fui%2FAstNodeToScriptAdapter.groovy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LWNvbnNvbGUvc3JjL21haW4vZ3Jvb3Z5L2dyb292eS9jb25zb2xlL3VpL0FzdE5vZGVUb1NjcmlwdEFkYXB0ZXIuZ3Jvb3Z5)
 | `71.4932% <76.1905%> (+0.1754%)` | :arrow_up: |
   
   ... and [3 files with indirect coverage 
changes](https://app.codecov.io/gh/apache/groovy/pull/2684/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>




> 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)

Reply via email to