Enable implicit non-top-level conditions (#1160)#2250
Conversation
|
While fixing the verify methods also stumbled over the sole line that needs to be changed to enable non-top-level implicit conditions. |
Vampire
force-pushed
the
vampire/implicit-non-top-level-conditions
branch
from
007a5a9 to
70fb92e
Compare
|
Ok, two tests. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2250 +/- ##
============================================
+ Coverage 82.16% 82.23% +0.07%
- Complexity 4822 4827 +5
============================================
Files 472 472
Lines 15036 15037 +1
Branches 1905 1905
============================================
+ Hits 12354 12366 +12
+ Misses 1989 1983 -6
+ Partials 693 688 -5
🚀 New features to boost your workflow:
|
|
Very nice, but should we maybe wait for or plan a Spock 3 version? So I would be for delaying that PR until we released 2.4 finally and then we plan a 3.0 as next version. |
|
While I also want to get rid of this confusing behavior, I think we should postpone it until Spock 3.0, as the breaking change is quite significant. |
|
Up to @leonard84 I'd say. In my personal opinion I think the change is not that much breaking. And the cases where this is not the case, but the behavior used like in the two cases we had in the codebase, you can easily use We were seldomly shy to do breaking changes, why become shy now? :-) If you really think it is too breaking to bring it in 2.4 now, no problem, just delay it until later. As I said, I just thought I'll do the changes now that I have the details and easy solution in mind. and did it in a way it could still make it into 2.4 (regarding the release notes changes). |
Vampire
force-pushed
the
vampire/implicit-non-top-level-conditions
branch
2 times, most recently
from
5633a59 to
34517fe
Compare
Vampire
force-pushed
the
vampire/implicit-non-top-level-conditions
branch
from
34517fe to
b38799d
Compare
Vampire
force-pushed
the
vampire/implicit-non-top-level-conditions
branch
from
b38799d to
00ed347
Compare
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughExpression statements in then/expect/filter blocks are now treated as implicit conditions even when nested (e.g., inside closures or condition blocks), a new closure-specific sentinel was added, rewriter logic adjusted, docs/tests updated, and an opt-out Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as Parser/AST
participant Rewriter as DeepBlockRewriter
participant Sentinel as NoSpecialMethodCall
participant Condition as ConditionRewriter
Parser->>Rewriter: emit ExpressionStatement node
Note right of Rewriter: determine current block (Then/Expect/Filter),\ncurrSpecialMethodCall state, and interactionClosureDepth
Rewriter->>Sentinel: read currSpecialMethodCall (INSTANCE / CLOSURE_INSTANCE / other)
alt eligible for implicit condition
Rewriter->>Condition: check & rewrite ExpressionStatement -> implicit condition
Condition->>Parser: produce assertion/verification AST
else not eligible
Rewriter->>Parser: leave ExpressionStatement unchanged
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
spock-core/src/main/java/org/spockframework/compiler/DeepBlockRewriter.java (2)
204-208:⚠️ Potential issue | 🟠 MajorMake the new interaction guard exception-safe.
This branch now depends on
insideInteractionandinteractionClosureDepthbeing restored perfectly, but both are still mutated withoutfinallyin the surrounding traversal. If rewriting throws while visiting an interaction, leaked state can make later statements look like implicit conditions.Possible hardening
private InteractionRewriter visitInteractionAwareExpressionStatement(ExpressionStatement stat) { InteractionRewriter rewriter = new InteractionRewriter(resources, getCurrentWithOrMockClosure()); if (isInteractionExpression(rewriter, stat)) { insideInteraction = true; - super.doVisitExpressionStatement(stat); - insideInteraction = false; + try { + super.doVisitExpressionStatement(stat); + } finally { + insideInteraction = false; + } } else { super.doVisitExpressionStatement(stat); } return rewriter; } `@Override` protected void doVisitClosureExpression(ClosureExpression expr) { - if (insideInteraction) interactionClosureDepth++; - closureDepth++; - super.doVisitClosureExpression(expr); - defineRecorders(expr); - closureDepth--; - if (insideInteraction) interactionClosureDepth--; + boolean inInteraction = insideInteraction; + if (inInteraction) interactionClosureDepth++; + closureDepth++; + try { + super.doVisitClosureExpression(expr); + defineRecorders(expr); + } finally { + closureDepth--; + if (inInteraction) interactionClosureDepth--; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spock-core/src/main/java/org/spockframework/compiler/DeepBlockRewriter.java` around lines 204 - 208, The new interaction guard relies on mutable fields insideInteraction and interactionClosureDepth remaining correctly restored even if rewriting throws; make this exception-safe by ensuring any mutations to insideInteraction and interactionClosureDepth in DeepBlockRewriter (the traversal/visit methods that enter interaction closures) are wrapped so the previous values are always restored in a finally block (or use a try/finally helper/RAII-like pattern) around the body that can throw; update the methods that set these flags (the visit/enter methods handling interaction closures referenced in the diff) to save the old values, set the new ones, execute the traversal, and restore the saved values in finally.
204-208:⚠️ Potential issue | 🟠 Major
interactionClosureDepth == 1is too broad for interaction-condition detection.This also matches arbitrary first-level closures inside an interaction argument, e.g.
1 * svc.call(items.every { it > 0 }). In that caseit > 0gets rewritten as a Spock condition even though it lives in an unknown closure, which breaks the promised.every { ... }safety and changes valid interaction code. Please narrow this branch to actual argument-constraint closures and add a regression around a collection-method closure inside an interaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spock-core/src/main/java/org/spockframework/compiler/DeepBlockRewriter.java` around lines 204 - 208, The current check in DeepBlockRewriter that treats any first-level closure inside an interaction as an interaction-condition (using insideInteraction && interactionClosureDepth == 1) is too broad; update the condition so it only treats closures that are actual argument-constraint closures as interaction conditions — i.e., verify the closure node is a direct argument of the interaction method call and represents a constraint-style closure (instead of any first-level closure) before rewriting; modify the logic around insideInteraction and interactionClosureDepth to inspect the parent AST node (the method call / argument position) to confirm it's the interaction argument-constraint, and add a regression test that ensures a collection-method closure inside an interaction like `1 * svc.call(items.every { it > 0 })` is not rewritten into a Spock condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@spock-core/src/main/java/org/spockframework/compiler/DeepBlockRewriter.java`:
- Around line 204-208: The new interaction guard relies on mutable fields
insideInteraction and interactionClosureDepth remaining correctly restored even
if rewriting throws; make this exception-safe by ensuring any mutations to
insideInteraction and interactionClosureDepth in DeepBlockRewriter (the
traversal/visit methods that enter interaction closures) are wrapped so the
previous values are always restored in a finally block (or use a try/finally
helper/RAII-like pattern) around the body that can throw; update the methods
that set these flags (the visit/enter methods handling interaction closures
referenced in the diff) to save the old values, set the new ones, execute the
traversal, and restore the saved values in finally.
- Around line 204-208: The current check in DeepBlockRewriter that treats any
first-level closure inside an interaction as an interaction-condition (using
insideInteraction && interactionClosureDepth == 1) is too broad; update the
condition so it only treats closures that are actual argument-constraint
closures as interaction conditions — i.e., verify the closure node is a direct
argument of the interaction method call and represents a constraint-style
closure (instead of any first-level closure) before rewriting; modify the logic
around insideInteraction and interactionClosureDepth to inspect the parent AST
node (the method call / argument position) to confirm it's the interaction
argument-constraint, and add a regression test that ensures a collection-method
closure inside an interaction like `1 * svc.call(items.every { it > 0 })` is not
rewritten into a Spock condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e4efe5c-ac53-4d5f-bf40-021d3c7e5cb9
📒 Files selected for processing (8)
docs/release_notes.adocdocs/spock_primer.adocspock-core/src/main/java/org/spockframework/compiler/AbstractDeepBlockRewriter.javaspock-core/src/main/java/org/spockframework/compiler/DeepBlockRewriter.javaspock-core/src/main/java/org/spockframework/compiler/NoSpecialMethodCall.javaspock-junit4/src/test/groovy/org/spockframework/junit4/junit/JUnitFixtureMethods.groovyspock-specs/src/test/groovy/org/spockframework/smoke/condition/ConditionEvaluation.groovyspock-specs/src/test/groovy/org/spockframework/smoke/extension/TempDirExtensionSpec.groovy
Vampire
force-pushed
the
vampire/implicit-non-top-level-conditions
branch
from
00ed347 to
789cc32
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spock-core/src/main/java/org/spockframework/compiler/NoSpecialMethodCall.java (1)
25-26: Consider adding documentation to clarify the distinction.Both
INSTANCEandCLOSURE_INSTANCEare functionally identicalNoSpecialMethodCallobjects—distinguished only by object identity. This identity distinction is critical forDeepBlockRewriter.handleImplicitCondition(line 204), which usescurrSpecialMethodCall == NoSpecialMethodCall.INSTANCEto differentiate top-level blocks from closure scopes.A brief Javadoc explaining this would help future maintainers understand the design intent.
📝 Suggested documentation
public class NoSpecialMethodCall implements ISpecialMethodCall { + /** + * Sentinel instance used for top-level block scope. + * Distinguished from {`@link` `#CLOSURE_INSTANCE`} by identity check in + * {`@link` DeepBlockRewriter#handleImplicitCondition}. + */ public static final ISpecialMethodCall INSTANCE = new NoSpecialMethodCall(); + /** + * Sentinel instance used inside unrelated closures. + * Expressions in closures bound to this instance are not treated as implicit conditions. + */ public static final ISpecialMethodCall CLOSURE_INSTANCE = new NoSpecialMethodCall();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spock-core/src/main/java/org/spockframework/compiler/NoSpecialMethodCall.java` around lines 25 - 26, Add Javadoc to the NoSpecialMethodCall static fields clarifying that INSTANCE and CLOSURE_INSTANCE are intentionally distinct by object identity (even though they are functionally identical) so callers can detect top-level vs closure scopes; specifically document that DeepBlockRewriter.handleImplicitCondition uses identity comparison (currSpecialMethodCall == NoSpecialMethodCall.INSTANCE) to recognize top-level blocks, and that CLOSURE_INSTANCE is used for closure-scoped blocks to prevent that match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@spock-core/src/main/java/org/spockframework/compiler/NoSpecialMethodCall.java`:
- Around line 25-26: Add Javadoc to the NoSpecialMethodCall static fields
clarifying that INSTANCE and CLOSURE_INSTANCE are intentionally distinct by
object identity (even though they are functionally identical) so callers can
detect top-level vs closure scopes; specifically document that
DeepBlockRewriter.handleImplicitCondition uses identity comparison
(currSpecialMethodCall == NoSpecialMethodCall.INSTANCE) to recognize top-level
blocks, and that CLOSURE_INSTANCE is used for closure-scoped blocks to prevent
that match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c814360e-e56e-4d8e-9bd3-53374ccd39d6
📒 Files selected for processing (8)
docs/release_notes.adocdocs/spock_primer.adocspock-core/src/main/java/org/spockframework/compiler/AbstractDeepBlockRewriter.javaspock-core/src/main/java/org/spockframework/compiler/DeepBlockRewriter.javaspock-core/src/main/java/org/spockframework/compiler/NoSpecialMethodCall.javaspock-junit4/src/test/groovy/org/spockframework/junit4/junit/JUnitFixtureMethods.groovyspock-specs/src/test/groovy/org/spockframework/smoke/condition/ConditionEvaluation.groovyspock-specs/src/test/groovy/org/spockframework/smoke/extension/TempDirExtensionSpec.groovy
🚧 Files skipped from review as they are similar to previous changes (2)
- spock-specs/src/test/groovy/org/spockframework/smoke/extension/TempDirExtensionSpec.groovy
- spock-specs/src/test/groovy/org/spockframework/smoke/condition/ConditionEvaluation.groovy
Vampire
force-pushed
the
vampire/implicit-non-top-level-conditions
branch
from
789cc32 to
4abd191
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/java/org/spockframework/compiler/DeepBlockRewriter.java (1)
204-208: 🛠️ Refactor suggestion | 🟠 MajorDecouple implicit-condition logic from singleton identity.
The gate at lines 204–208 restricts block-level implicit conditions using
currSpecialMethodCall == NoSpecialMethodCall.INSTANCE, butNoSpecialMethodCallremains publicly instantiable. SinceINSTANCEandCLOSURE_INSTANCEare behaviorally identical, any freshnew NoSpecialMethodCall()would silently fail the identity check and disable implicit-condition rewriting inthen/expect/filterblocks.Currently the codebase only uses the two static instances, but this design is fragile. Encode the "no special method call" state explicitly (e.g., a dedicated flag or method on
ISpecialMethodCall) instead of inferring it from identity checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spock-core/src/main/java/org/spockframework/compiler/DeepBlockRewriter.java` around lines 204 - 208, The identity check against NoSpecialMethodCall.INSTANCE is fragile; add an explicit predicate to ISpecialMethodCall (e.g., boolean isNoSpecialMethodCall()) implemented to return true in NoSpecialMethodCall (both INSTANCE and CLOSURE_INSTANCE) and false in all other implementations, then replace the identity check in DeepBlockRewriter (the condition using currSpecialMethodCall == NoSpecialMethodCall.INSTANCE together with isThenOrExpectOrFilterBlock()) with currSpecialMethodCall.isNoSpecialMethodCall() so implicit-condition logic no longer depends on singleton identity; keep the other branches (isConditionMethodCall(), isConditionBlock(), isGroupConditionBlock(), insideInteraction && interactionClosureDepth == 1) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/release_notes.adoc`:
- Around line 11-14: The release note is too broad; change the wording to scope
the behavior to assertion/condition contexts only by clarifying that expression
statements are treated as implicit conditions only in assertion-related contexts
handled by DeepBlockRewriter.handleImplicitCondition — specifically in
then/expect/filter blocks, known condition blocks/methods (e.g.,
`@ConditionBlock`, verifyAll/with), and first-level interaction closures, and not
globally for all expression statements or unknown closures.
---
Outside diff comments:
In `@spock-core/src/main/java/org/spockframework/compiler/DeepBlockRewriter.java`:
- Around line 204-208: The identity check against NoSpecialMethodCall.INSTANCE
is fragile; add an explicit predicate to ISpecialMethodCall (e.g., boolean
isNoSpecialMethodCall()) implemented to return true in NoSpecialMethodCall (both
INSTANCE and CLOSURE_INSTANCE) and false in all other implementations, then
replace the identity check in DeepBlockRewriter (the condition using
currSpecialMethodCall == NoSpecialMethodCall.INSTANCE together with
isThenOrExpectOrFilterBlock()) with
currSpecialMethodCall.isNoSpecialMethodCall() so implicit-condition logic no
longer depends on singleton identity; keep the other branches
(isConditionMethodCall(), isConditionBlock(), isGroupConditionBlock(),
insideInteraction && interactionClosureDepth == 1) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd1e7819-cc8f-4486-a5ea-8d4e70ef622f
📒 Files selected for processing (10)
docs/release_notes.adocdocs/spock_primer.adocspock-core/src/main/java/org/spockframework/compiler/AbstractDeepBlockRewriter.javaspock-core/src/main/java/org/spockframework/compiler/DeepBlockRewriter.javaspock-core/src/main/java/org/spockframework/compiler/NoSpecialMethodCall.javaspock-junit4/src/test/groovy/org/spockframework/junit4/junit/JUnitFixtureMethods.groovyspock-specs/src/test/groovy/org/spockframework/smoke/condition/ConditionEvaluation.groovyspock-specs/src/test/groovy/org/spockframework/smoke/extension/TempDirExtensionSpec.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/VerifyMethodsAstSpec/transforms_conditions_in_private_methods.groovy
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/spock_primer.adoc
- spock-specs/src/test/groovy/org/spockframework/smoke/condition/ConditionEvaluation.groovy
| * It is no longer true that only top-level expression statements are treated as implicit conditions. | ||
| Just like within `with { ... }`, `verifyAll { ... }`, `@ConditionBlock` methods, and other places, | ||
| each expression statement is now considered an implicit condition, except for statements in unknown | ||
| closures. |
There was a problem hiding this comment.
Scope this note to assertion contexts.
The implementation does not make expression statements implicit globally; DeepBlockRewriter.handleImplicitCondition() still limits this to then/expect/filter, known condition blocks/methods, and first-level interaction closures. As written, “each expression statement” reads broader than the compiler change actually is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/release_notes.adoc` around lines 11 - 14, The release note is too broad;
change the wording to scope the behavior to assertion/condition contexts only by
clarifying that expression statements are treated as implicit conditions only in
assertion-related contexts handled by DeepBlockRewriter.handleImplicitCondition
— specifically in then/expect/filter blocks, known condition blocks/methods
(e.g., `@ConditionBlock`, verifyAll/with), and first-level interaction closures,
and not globally for all expression statements or unknown closures.
4abd191 to
64b1e59
Compare
This comment has been minimized.
This comment has been minimized.
Vampire
force-pushed
the
vampire/implicit-non-top-level-conditions
branch
from
64b1e59 to
ab01700
Compare
🔎 No tests executed 🔎🏷️ Commit: ab01700 Learn more about TestLens at testlens.app. |
3 participants
Fixes #1160
Summary by CodeRabbit
Breaking Changes
!!to opt out of implicit-condition handling.Tests
Documentation