Improve rendering fidelity of the AST-to-source transpiler#2393
Improve rendering fidelity of the AST-to-source transpiler#2393leonard84 wants to merge 18 commits into
Conversation
AstNodeToScriptVisitor.visitGenerics printed each type argument by name only, dropping any nested type arguments the argument itself carried. A signature such as Iterator<Tuple2<Integer, Integer>> was rendered as Iterator<Tuple2>, and Map<String, List<Integer>> as Map<String, List>. Recurse into the type argument's own generics (for concrete arguments, which are the only ones that can carry them) so the rendered source faithfully reflects the declared signature. Placeholders and wildcards are unchanged.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe transpiler now renders additional Groovy AST forms, and the smoke snapshots and release notes were updated to match the new output. Empty ChangesAST source rendering updates
Estimated code review effort: 5 (Critical) | ~90 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2393 +/- ##
============================================
- Coverage 82.32% 82.28% -0.04%
Complexity 4877 4877
============================================
Files 474 474
Lines 15205 15251 +46
Branches 1935 1959 +24
============================================
+ Hits 12518 12550 +32
- Misses 1993 2002 +9
- Partials 694 699 +5
🚀 New features to boost your workflow:
|
…spiler visitShortTernaryExpression delegated to visitTernaryExpression, so c ?: d was rendered as c ? c : d. The duplicated condition misrepresents the source: an elvis operator evaluates its condition once, while re-compiling the rendered ternary would evaluate it twice if it has side effects. Render the ?: form directly instead.
visitAttributeExpression delegated to visitPropertyExpression without any distinction, so this.@order was rendered as this.order. Direct field access and property access are different operations, the rendered source silently changed the semantics. Print .@ for attribute expressions, including the safe (?.@) and spread (*.@) variants.
…nspiler
visitClosureExpression printed the arrow unconditionally, so { it * 2 } was
rendered as { -> it * 2 }. That form declares an explicit zero-arg closure,
so the rendered source no longer has the implicit it parameter.
A null parameter array denotes an explicit zero-arg closure { -> }, while an
empty array denotes the implicit it (see ClosureWriter). Only render the
arrow for the former.
visitBinaryExpression only appended the closing bracket for the plain '[' operation, so a safe index access was rendered unbalanced: l?[0] became l ?[ 0 under Groovy 4. Under Groovy 3, where the parser keeps the '[' token and sets the safe flag instead, the safe navigation was silently dropped, rendering a plain index access. Map the safe flag to the ?[ operator and close the bracket for both index tokens. BinaryExpression.isSafe() only exists in Groovy 3+, so it is probed dynamically.
visitConstantExpression printed only the value, so 42L, 2.5f, 3.5d, and 10G were rendered as 42, 2.5, 3.5, and 10. Re-parsing the rendered source would change their types to Integer or BigDecimal. Append the literal type suffix based on the constant's value type.
visitRangeExpression only checked the right boundary, so the Groovy 4 range 1<..5 was rendered as (1..5), silently including the excluded lower bound. Print '<' before the range operator for exclusive-left ranges. RangeExpression.isExclusiveLeft() only exists in Groovy 4+, so it is probed dynamically.
…ranspiler visitMethodCallExpression ignored the call's generics types, so Collections.<String>emptyList() was rendered as Collections.emptyList().
…transpiler A switch statement without a default case carries EmptyStatement as its default statement, which is truthy, so a spurious default label with an empty body was rendered. Adopted from apache/groovy commit dc020e06c8.
…ource transpiler A try statement without a finally block carries EmptyStatement, but an empty finally block was rendered unconditionally. This mainly cluttered the rendering of Spock's own condition rewriting, which generates try/catch without finally. Adopted from apache/groovy commit db888a0537.
…iler Groovy 5 supports for (index, value in iterable) loops (GROOVY-10683), but visitForLoop only rendered the value variable, silently dropping the index. ForStatement.getIndexVariable() only exists in Groovy 5+, so it is probed dynamically. Adopted from apache/groovy commit 876fbed302.
compileScript never closed the GroovyClassLoader it creates when the caller does not pass one, leaking resources on every transpilation. Adopted from apache/groovy commit 49fb716307.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
spock-core/src/main/groovy/spock/util/SourceToAstNodeAndSourceTranspiler.groovy (2)
809-810: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting a shared "is real statement" helper.
Both
visitSwitch's default-guard andvisitTryCatchFinally's finally-guard repeat the samestmt && !(stmt instanceof EmptyStatement)check. A small private helper (e.g.isPresent(Statement)) would remove the duplication.♻️ Optional refactor
+ private static boolean isPresent(Statement statement) { + statement != null && !(statement instanceof EmptyStatement) + } + void visitSwitch(SwitchStatement statement) { ... - if (statement?.defaultStatement && !(statement.defaultStatement instanceof EmptyStatement)) { + if (isPresent(statement?.defaultStatement)) { ... void visitTryCatchFinally(TryCatchStatement statement) { ... - if (statement?.finallyStatement && !(statement.finallyStatement instanceof EmptyStatement)) { + if (isPresent(statement?.finallyStatement)) {Also applies to: 1248-1257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spock-core/src/main/groovy/spock/util/SourceToAstNodeAndSourceTranspiler.groovy` around lines 809 - 810, The same “present statement” check is duplicated in the switch default guard and the try/finally guard, so extract it into a small private helper on SourceToAstNodeAndSourceTranspiler (for example, a method like isPresent on Statement) and use that helper in both visitSwitch and visitTryCatchFinally. Keep the helper’s logic equivalent to the current null-and-EmptyStatement check so both call sites share one definition.
756-760: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication across version-detection helpers.
isSafeIndexAccess,forLoopIndexVariable, andisExclusiveLeftall follow the samerespondsTo(...) ? ... : defaultpattern for cross-Groovy-version API detection. Could be consolidated into one generic dynamic helper, but each guards a different receiver type/signature so the payoff is limited.Also applies to: 914-918, 991-995
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spock-core/src/main/groovy/spock/util/SourceToAstNodeAndSourceTranspiler.groovy` around lines 756 - 760, There is repeated Groovy-version API detection logic in isSafeIndexAccess, forLoopIndexVariable, and isExclusiveLeft, all using the same respondsTo(...) ternary pattern. Refactor the shared pattern into a single dynamic helper in SourceToAstNodeAndSourceTranspiler, and update those three methods to delegate to it while preserving each method’s specific receiver type and fallback value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@spock-core/src/main/groovy/spock/util/SourceToAstNodeAndSourceTranspiler.groovy`:
- Around line 809-810: The same “present statement” check is duplicated in the
switch default guard and the try/finally guard, so extract it into a small
private helper on SourceToAstNodeAndSourceTranspiler (for example, a method like
isPresent on Statement) and use that helper in both visitSwitch and
visitTryCatchFinally. Keep the helper’s logic equivalent to the current
null-and-EmptyStatement check so both call sites share one definition.
- Around line 756-760: There is repeated Groovy-version API detection logic in
isSafeIndexAccess, forLoopIndexVariable, and isExclusiveLeft, all using the same
respondsTo(...) ternary pattern. Refactor the shared pattern into a single
dynamic helper in SourceToAstNodeAndSourceTranspiler, and update those three
methods to delegate to it while preserving each method’s specific receiver type
and fallback value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83894986-c74c-4070-8f85-b2fc0786546a
📒 Files selected for processing (74)
docs/release_notes.adocspock-core/src/main/groovy/spock/util/SourceToAstNodeAndSourceTranspiler.groovyspock-specs/src/test/groovy/org/spockframework/smoke/ast/AstSpec.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/attribute_access_is_rendered_with_the_at_sign.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/closure_parameter_lists_are_rendered_faithfully.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/elvis_operator_is_rendered_in_short_form.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/explicit_type_arguments_of_method_calls_are_rendered.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/for_in_loops_with_an_index_variable_render_both_variables.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/full_feature_exercise-groovy5.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/full_feature_exercise.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/numeric_literals_keep_their_type_suffix.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/range_boundary_exclusions_are_rendered_on_both_sides.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/safe_index_accesses_are_rendered_with_the_closing_bracket.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/switch_without_a_default_case_is_rendered_without_a_default_label.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/try_without_a_finally_block_is_rendered_without_an_empty_one.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/BlocksAst/all_observable_blocks_with_GString_labels.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/CleanupBlocksAstSpec/cleanup_rewrite_keeps_correct_method_reference.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/CleanupBlocksAstSpec/cleanup_rewrite_keeps_correct_method_reference_for_multi_assignments-groovy5.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/CleanupBlocksAstSpec/cleanup_rewrite_keeps_correct_method_reference_for_multi_assignments.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/multi_parameterization.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/nested_multi_parameterization.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables_combined_with_data_table__data_pipe__derived_variables__cross_multiplication_and_filter.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables_with_multiple_assignment-groovy3_4.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables_with_multiple_assignment-groovy5.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/data_tables_with__separators_can_be_combined-[0].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/data_tables_with__separators_can_be_combined-[1].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/data_tables_with__separators_can_be_combined-[2].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/filter_block_becomes_its_own_method.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/using_a_variable_in_a_cell_multiple_times_compiles.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/CollectionConditionAstSpec/collection_condition_matchCollectionsAsSet_is_transformed_correctly.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/CollectionConditionAstSpec/collection_condition_matchCollectionsInAnyOrder_is_transformed_correctly.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/CollectionConditionAstSpec/regex_find_conditions_are_transformed_correctly.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/CollectionConditionAstSpec/regex_match_conditions_are_transformed_correctly.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/GDK_method_that_looks_like_built_in_method_as_explicit_condition.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/GDK_method_that_looks_like_built_in_method_as_implicit_condition.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod-[0].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod-[1].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod-[2].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_with_exception-[0].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_with_exception-[1].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_with_exception-[2].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_with_only_exception-[0].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_with_only_exception-[1].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_with_only_exception-[2].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod-[0].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod-[1].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod-[2].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod_with_exception-[0].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod_with_exception-[1].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod_with_exception-[2].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod_with_only_exception-[0].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod_with_only_exception-[1].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod_with_only_exception-[2].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/ignores_conditions_in_overwritten_methods.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/ignores_methods_without_annotation.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/methods_without_condition_declarations_stay_unchanged_in_specs.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_conditions_at_all_nesting_levels.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_conditions_in_methods_inside_spec_classes.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_conditions_in_methods_outside_of_spec_classes.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_conditions_in_private_methods.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_conditions_in_static_methods.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_explicit_conditions.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/ignores_conditions_in_overwritten_methods.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/ignores_methods_without_annotation.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/methods_without_condition_declarations_stay_unchanged_in_specs.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_conditions_at_all_nesting_levels.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_conditions_in_methods_inside_spec_classes.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_conditions_in_methods_outside_of_spec_classes.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_conditions_in_private_methods.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_conditions_in_static_methods.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_explicit_conditions.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/parameterization/DataProviders/data_provider_with_asserting_closure_produces_error_rethrower_variable_in_data_provider_method.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/parameterization/DataProviders/data_variable_with_asserting_closure_produces_error_rethrower_variable_in_data_processor_method.groovy
💤 Files with no reviewable changes (34)
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_conditions_at_all_nesting_levels.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_conditions_in_static_methods.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_conditions_in_methods_outside_of_spec_classes.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_conditions_in_static_methods.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/CleanupBlocksAstSpec/cleanup_rewrite_keeps_correct_method_reference_for_multi_assignments-groovy5.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/nested_multi_parameterization.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/data_tables_with__separators_can_be_combined-[0].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/CleanupBlocksAstSpec/cleanup_rewrite_keeps_correct_method_reference.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/data_tables_with__separators_can_be_combined-[1].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/data_tables_with__separators_can_be_combined-[2].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/ignores_conditions_in_overwritten_methods.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/CollectionConditionAstSpec/collection_condition_matchCollectionsAsSet_is_transformed_correctly.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/methods_without_condition_declarations_stay_unchanged_in_specs.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/BlocksAst/all_observable_blocks_with_GString_labels.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_explicit_conditions.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/ignores_conditions_in_overwritten_methods.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/ignores_methods_without_annotation.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/methods_without_condition_declarations_stay_unchanged_in_specs.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_conditions_at_all_nesting_levels.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/CleanupBlocksAstSpec/cleanup_rewrite_keeps_correct_method_reference_for_multi_assignments.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/CollectionConditionAstSpec/regex_match_conditions_are_transformed_correctly.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/CollectionConditionAstSpec/regex_find_conditions_are_transformed_correctly.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_conditions_in_methods_outside_of_spec_classes.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/filter_block_becomes_its_own_method.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/ignores_methods_without_annotation.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_conditions_in_methods_inside_spec_classes.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/CollectionConditionAstSpec/collection_condition_matchCollectionsInAnyOrder_is_transformed_correctly.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/multi_parameterization.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables_with_multiple_assignment-groovy5.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables_combined_with_data_table__data_pipe__derived_variables__cross_multiplication_and_filter.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_conditions_in_methods_inside_spec_classes.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_explicit_conditions.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables_with_multiple_assignment-groovy3_4.groovy
✅ Files skipped from review due to trivial changes (32)
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/numeric_literals_keep_their_type_suffix.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_with_only_exception-[2].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_with_only_exception-[0].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/closure_parameter_lists_are_rendered_faithfully.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/switch_without_a_default_case_is_rendered_without_a_default_label.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/parameterization/DataProviders/data_provider_with_asserting_closure_produces_error_rethrower_variable_in_data_provider_method.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/explicit_type_arguments_of_method_calls_are_rendered.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/attribute_access_is_rendered_with_the_at_sign.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_with_exception-[0].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/parameterization/DataProviders/data_variable_with_asserting_closure_produces_error_rethrower_variable_in_data_processor_method.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod-[2].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod-[0].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/try_without_a_finally_block_is_rendered_without_an_empty_one.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_with_only_exception-[1].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/range_boundary_exclusions_are_rendered_on_both_sides.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod_with_only_exception-[1].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod_with_exception-[2].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod-[0].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod_with_only_exception-[2].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod-[1].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/GDK_method_that_looks_like_built_in_method_as_implicit_condition.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/elvis_operator_is_rendered_in_short_form.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_with_exception-[2].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod-[2].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod_with_exception-[0].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/using_a_variable_in_a_cell_multiple_times_compiles.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod_with_only_exception-[0].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod-[1].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_with_exception-[1].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/for_in_loops_with_an_index_variable_render_both_variables.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ConditionMethodsAstSpec/condition_method__conditionMethod_within_condition_method__conditionMethod_with_exception-[1].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_conditions_in_private_methods.groovy
🚧 Files skipped from review as they are similar to previous changes (1)
- spock-specs/src/test/groovy/org/spockframework/smoke/ast/AstSpec.groovy
… transpiler Port the end state of apache/groovy commits 88f9b2af56, 922f3475b7, ef8f3bd5e0, 145345e22a, 5a34d13798, 4a4ce4f4e1, 29de7e506e, and 4988329696: variables are no longer space-padded when used as assignment targets, unary or postfix or prefix operands, cast subjects, property or method call receivers, or list and argument elements, and index accesses are rendered without inner padding. Examples: '!( a )' becomes '!a', '( i )++' becomes 'i++', '[1, 2, 3] [ 0]' becomes '[1, 2, 3][0]', '(( c ) as long)' becomes '(c as long)'. The multiple-assignment declaration rewrite also fixes a fidelity bug: Groovy 5 represents the left side as a plain TupleExpression, which the previous ArgumentListExpression check missed, rendering the broken 'java.lang.Object( foobar , b ) = [null, null]' instead of 'def (java.lang.Object foobar, java.lang.Object b) = [null, null]'. Spock-specific behavior is kept: Show filtering, AnnotationConstantExpression handling, GString rendering, and the Groovy 2.5 compatible safe index logic.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spock-core/src/main/groovy/spock/util/SourceToAstNodeAndSourceTranspiler.groovy (1)
69-74: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap non-
GroovyClassLoaderinputs before building theCompilationUnit.
compileScriptacceptsClassLoader, but line 74 casts it toGroovyClassLoaderunconditionally. Passing a plainClassLoaderwill fail at runtime; create aGroovyClassLoaderwrapper for those cases and only close the instance created here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spock-core/src/main/groovy/spock/util/SourceToAstNodeAndSourceTranspiler.groovy` around lines 69 - 74, compileScript currently assumes classLoader is already a GroovyClassLoader, but it accepts any ClassLoader and casts it directly when creating the CompilationUnit. Update SourceToAstNodeAndSourceTranspiler.compileScript to wrap non-GroovyClassLoader inputs in a new GroovyClassLoader before constructing the CompilationUnit, and keep track of whether the loader was created locally so only that instance is closed afterward.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@spock-core/src/main/groovy/spock/util/SourceToAstNodeAndSourceTranspiler.groovy`:
- Around line 69-74: compileScript currently assumes classLoader is already a
GroovyClassLoader, but it accepts any ClassLoader and casts it directly when
creating the CompilationUnit. Update
SourceToAstNodeAndSourceTranspiler.compileScript to wrap non-GroovyClassLoader
inputs in a new GroovyClassLoader before constructing the CompilationUnit, and
keep track of whether the loader was created locally so only that instance is
closed afterward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8acd8fa8-ca50-4575-b6f6-d01f83ff1269
📒 Files selected for processing (30)
docs/release_notes.adocspock-core/src/main/groovy/spock/util/SourceToAstNodeAndSourceTranspiler.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/enums-groovy4.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/enums.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/full_feature_exercise-groovy5.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/full_feature_exercise.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/groovy_3_language_features.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/safe_index_accesses_are_rendered_with_the_closing_bracket.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/BlocksAst/all_observable_blocks_with_GString_labels.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/BlocksAst/all_observable_blocks_with_empty_labels.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/BlocksAst/all_observable_blocks_with_labels_and_blocks.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/CleanupBlocksAstSpec/cleanup_rewrite_keeps_correct_method_reference.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/CleanupBlocksAstSpec/cleanup_rewrite_keeps_correct_method_reference_for_multi_assignments-groovy5.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/CleanupBlocksAstSpec/cleanup_rewrite_keeps_correct_method_reference_for_multi_assignments.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/multi_parameterization.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/nested_multi_parameterization.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables_combined_with_data_table__data_pipe__derived_variables__cross_multiplication_and_filter.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables_with_multiple_assignment-groovy3_4.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables_with_multiple_assignment-groovy5.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/data_tables_with__separators_can_be_combined-[0].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/data_tables_with__separators_can_be_combined-[1].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/data_tables_with__separators_can_be_combined-[2].groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/filter_block_becomes_its_own_method.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/using_a_variable_in_a_cell_multiple_times_compiles.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ExceptionConditionsAstSpec/thrown_rewrite_keeps_correct_method_reference_for_multi_assignments-groovy5.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_conditions_at_all_nesting_levels.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_conditions_at_all_nesting_levels.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/parameterization/DataProviders/data_provider_with_asserting_closure_produces_error_rethrower_variable_in_data_provider_method.groovyspock-specs/src/test/resources/snapshots/org/spockframework/smoke/parameterization/DataProviders/data_variable_with_asserting_closure_produces_error_rethrower_variable_in_data_processor_method.groovy
✅ Files skipped from review due to trivial changes (20)
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/ExceptionConditionsAstSpec/thrown_rewrite_keeps_correct_method_reference_for_multi_assignments-groovy5.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/BlocksAst/all_observable_blocks_with_labels_and_blocks.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/BlocksAst/all_observable_blocks_with_empty_labels.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/groovy_3_language_features.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/parameterization/DataProviders/data_variable_with_asserting_closure_produces_error_rethrower_variable_in_data_processor_method.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables_with_multiple_assignment-groovy3_4.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/multi_parameterization.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/safe_index_accesses_are_rendered_with_the_closing_bracket.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables_with_multiple_assignment-groovy5.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/enums-groovy4.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/using_a_variable_in_a_cell_multiple_times_compiles.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/parameterization/DataProviders/data_provider_with_asserting_closure_produces_error_rethrower_variable_in_data_provider_method.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/AstSpec/enums.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables_combined_with_data_table__data_pipe__derived_variables__cross_multiplication_and_filter.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/filter_block_becomes_its_own_method.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/where_block_variables.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/CleanupBlocksAstSpec/cleanup_rewrite_keeps_correct_method_reference.groovy
- docs/release_notes.adoc
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/CleanupBlocksAstSpec/cleanup_rewrite_keeps_correct_method_reference_for_multi_assignments.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyAllMethodsAstSpec/transforms_conditions_at_all_nesting_levels.groovy
🚧 Files skipped from review as they are similar to previous changes (6)
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/data_tables_with__separators_can_be_combined-[2].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/data_tables_with__separators_can_be_combined-[0].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataAstSpec/nested_multi_parameterization.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/BlocksAst/all_observable_blocks_with_GString_labels.groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/DataTablesAstSpec/data_tables_with__separators_can_be_combined-[1].groovy
- spock-specs/src/test/resources/snapshots/org/spockframework/smoke/ast/condition/VerifyMethodsAstSpec/transforms_conditions_at_all_nesting_levels.groovy
compileScript declares a plain ClassLoader parameter but cast it directly to GroovyClassLoader when constructing the CompilationUnit, so passing any other classloader failed with a ClassCastException. Wrap such classloaders in a new GroovyClassLoader instead, and keep closing only locally created loaders.
The absent-statement check (null or EmptyStatement) was duplicated across the if/else, switch default, and finally guards; the Groovy version probing was duplicated across three respondsTo ternaries. Extract isPresent and invokeIfSupported and delegate to them.
✅ All tests passed ✅🏷️ Commit: a56e2bd Learn more about TestLens at testlens.app. |
This PR started as a fix for nested generic type arguments and grew into a general fidelity pass over
SourceToAstNodeAndSourceTranspiler, after exercising it with constructs the test suite never covered. Each fix is a separate commit with anAstSpecsnapshot test.Fidelity fixes
Map<String, List<Integer>>was rendered asMap<String, List>c ?: dwas rendered asc ? c : d, duplicating the condition expressionthis.@orderwas rendered asthis.order, silently turning direct field access into property access (also?.@and*.@)itclosures:{ it * 2 }was rendered as{ -> it * 2 }, which declares a zero-arg closure withoutit. Note the AST convention: null parameters mean{ -> }, an empty array means implicitit(seeClosureWriter)l?[0]was rendered unbalanced asl ?[ 0under Groovy 4+, and as a plain (non-safe)l [ 0]under Groovy 3, where the parser keeps the[token and sets the safe flag instead42L,2.5f,3.5d,10Gwere rendered without suffix, so re-parsing would change their types1<..5was rendered as(1..5)Collections.<String>emptyList()was rendered asCollections.emptyList()TupleExpressionas the left side, which the previousArgumentListExpressioncheck missed, rendering the brokenjava.lang.Object( foobar , b ) = [null, null]instead ofdef (java.lang.Object foobar, java.lang.Object b) = [null, null]Adopted from upstream
AstNodeToScriptAdapterdefault:label for switches without one (groovy@dc020e06c8)finally {}for try statements without one (groovy@db888a0537); this alone cleaned 56 snapshots of noise from Spock's condition rewritingfor (index, value in iterable)loops render the index variable (groovy@876fbed302)GroovyClassLoaderis closed after compilation (groovy@49fb716307)!a,i++,[1, 2, 3][0],(c as long),((long) c)instead of!( a ),( i )++,[1, 2, 3] [ 0],(( c ) as long),((long) c ). Since the fidelity fixes above already touched most snapshots, folding this in only added 6 more changed snapshot files (72 to 78 of 133)APIs that do not exist in Groovy 2.5 (
BinaryExpression.isSafe(),RangeExpression.isExclusiveLeft(),ForStatement.getIndexVariable()) are probed dynamically.Verification
The full snapshot suite (19 spec classes) passes under all variants (2.5, 3.0, 4.0, 5.0); shared snapshots are byte-identical across them. Version-specific constructs are guarded with
@Requireson the Groovy major version.Upstream still has all the fidelity bugs listed above (verified against groovy master aa30a69065); contributing them back is prepared separately.