GROOVY-12242: instanceof pattern variable scope is not aligned with Java flow scoping (JEP 394) - #2773
GROOVY-12242: instanceof pattern variable scope is not aligned with Java flow scoping (JEP 394)#2773daniellansun wants to merge 4 commits into
Conversation
…ava flow scoping (JEP 394)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: d332081 | Previous: a251ce9 | Ratio |
|---|---|---|---|
org.apache.groovy.bench.NonCapturingLambdaBench.capturingLambdaApply |
58237.69930250118 ops/ms |
33225.81394512552 ops/ms |
1.75 |
org.apache.groovy.bench.AryBench.java ( {"n":"1000"} ) |
0.1183010262049575 ms/op |
0.059300010387627757 ms/op |
1.99 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2773 +/- ##
==================================================
+ Coverage 69.9945% 70.0153% +0.0208%
- Complexity 35540 35566 +26
==================================================
Files 1557 1557
Lines 131696 131924 +228
Branches 24174 24230 +56
==================================================
+ Hits 92180 92367 +187
- Misses 31171 31191 +20
- Partials 8345 8366 +21
🚀 New features to boost your workflow:
|
JMH summary — classic (commit
|
| Group | Speedup | Calibrated | n |
|---|---|---|---|
| bench | 0.988 × | 0.983 × | 99 |
| core | 1.028 × | 1.003 × | 83 |
| grails | 0.874 × | 0.853 × | 80 |
Runner calibration (this run vs baseline hardware): bench 1.00× (26 rulers) · core-ag 1.00× (3 rulers) · core-hz 1.05× (3 rulers) · grails-ad 1.10× (3 rulers) · grails-ez 0.97× (3 rulers)
Baseline: dev/bench/jmh/<part>/classic/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data
JMH summary — indy (commit
|
| Group | Speedup | Calibrated | n |
|---|---|---|---|
| bench | 0.946 × | 1.000 × | 99 |
| core | 2.865 × | 2.812 × | 83 |
| grails | 5.469 × | 4.865 × | 80 |
⚠️ Runner speed differs ≥15% from the historical baseline hardware for: grails-ez. Raw speedups are not meaningful for those parts — use the calibrated column.
Runner calibration (this run vs baseline hardware): bench 0.95× (26 rulers) · core-ag 0.98× (3 rulers) · core-hz 1.07× (3 rulers) · grails-ad 0.98× (3 rulers) · grails-ez 1.26× (3 rulers)
Baseline: dev/bench/jmh/<part>/indy/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data
| * @see org.codehaus.groovy.classgen.asm.InstanceofFlowSlotPublisher | ||
| * @since 6.0.0 | ||
| */ | ||
| public final class InstanceofFlowBindings { |
There was a problem hiding this comment.
if we start adding helper classes that are especially and only for VariablescopeVisitor we should maybe consider a new package like org.apache.groovy.classgen.varscope. Disclaimer: I am bad with names! Also is it an @internal? I don´t think we have established yet when exactly to use it.
Another problem I see is what is VariableScopeVisitor for? The idea is that it resolves the variable scopes and then downstream we can easily use the information. But that would imply not to have InstanceofFlowBindings exposed later on, since it is completely different from the normal structure of VariableScopeVisitor and thus requires special checks and handling downstream. Or you use this class itself downstream... which you did - as shown in StatementWriter. So I am a bit worried about the general design, especially about separation of concerns.
There was a problem hiding this comment.
The concern is well-taken and worth explaining carefully.
@Internal annotation: InstanceofFlowBindings is a compiler-internal type that is not
part of Groovy's public API surface. The @Internal annotation (groovy.transform.Internal)
has been added to the class declaration in the follow-up to make this intent explicit and
machine-checkable (binary-compatible check tooling already honours the annotation).
Dual-phase design — why sharing is intentional: The class is consumed by two distinct
compiler phases, not only by VariableScopeVisitor:
VariableScopeVisitor(semantic analysis) — uses the binding sets to declare
pattern-variable names only on the definitively-live path, so that subsequent name
resolution resolves them correctly.InstanceofFlowSlotPublisher/StatementWriter(code generation) — uses the
same binding sets to publish/hide CompileStack slots on the matching control-flow arm,
keeping bytecode slot visibility consistent with the resolved scopes.
Both phases need to answer exactly the same question: "which pattern-variable names are
definitely bound on the true path vs. the false path of this boolean condition?" Answering
that question is a single, pure semantic analysis with no side effects. Sharing the answer
via one value type avoids two independent implementations that could silently diverge — which
is the actual separation-of-concerns risk. Placing the class in org.codehaus.groovy.classgen
(the parent package of both VariableScopeVisitor and the asm sub-package) follows
naturally from that shared role.
A new sub-package (e.g. varscope) would be a reasonable long-term home if the project
decides to modularise the classgen layer more finely. This can be deferred — the @Internal
marker already prevents accidental API entrenchment.
The class-level Javadoc has been updated with a "Design note — dual use across compiler
phases" section that makes this reasoning explicit in the source code so future readers do
not have to reconstruct it from PR threads.
| for (VariableExpression ve : whenTrue) names.add(ve.getName()); | ||
| for (VariableExpression ve : whenFalse) names.add(ve.getName()); | ||
| return names; | ||
| } |
There was a problem hiding this comment.
Did you not use names.append(names(whenTrue)); names.append(names(whenFalse)) because of the additional collection creation?
There was a problem hiding this comment.
Yes, exactly. Calling names(whenTrue) followed by names(whenFalse) would allocate two
intermediate LinkedHashSet instances (one per call to names()), only to merge them into
a third result set. The direct loop populates a single LinkedHashSet in one pass.
The Javadoc for allNames() has been updated to document this rationale explicitly:
/**
* All pattern-variable names appearing in either path (stable encounter order).
* <p>
* Implemented by iterating both lists directly rather than composing
* {@link #whenTrueNames()} and {@link #whenFalseNames()}: that would
* allocate two intermediate {@link Set} objects only to merge them into a
* third, whereas the direct loop allocates only the result set.
*/| List.of(), | ||
| union(left.whenFalse, right.whenFalse)); | ||
| } | ||
| } |
There was a problem hiding this comment.
I wonder if we really have to travel the complete tree of expressions.
There was a problem hiding this comment.
There are two methods with deliberately different traversal strategies:
containsPattern(Expression) — intentionally traverses the full subtree.
This method answers a conservative question: "does any descendant node in this expression
tree contain a type pattern?" It is used for expression-statement isolation (deciding
whether the statement's value needs CompileStack sandboxing). Even a pattern buried
arbitrarily deep (e.g. inside a method-call argument: print(o instanceof String s)) may
have caused evaluateInstanceof to define a CompileStack slot. That worst-case question
requires a full subtree scan. Short-circuit optimisation (found[0] early-return) keeps
the typical cost proportional to the depth of the first match, not the full tree size.
analyse(Expression) — does not traverse the full tree. It is a recursive structural
descent that follows only the operators that propagate definite-assignment (instanceof,
!instanceof, &&, ||, !) and returns EMPTY conservatively for any other shape.
For a typical condition o instanceof String s && s.length() > 0 the descent visits exactly
three nodes.
Both methods now have explicit "Why a full subtree walk?" / "Why not a full subtree
walk?" Javadoc paragraphs to document this distinction for future contributors.
| assert C.m(1) == 'not' | ||
| true | ||
| ''') | ||
| } |
There was a problem hiding this comment.
I wonder if the tests are complete. I think not. Basically we have to consider the following case:
- simple instanceof x
- simple !instanceof x
- simple !instanceof x, plus return in else block
- instanceof x && cond
- instanceof x || cond
- !instanceof x && cond
- !instanceof x && cond plus return in else block
- !instanceof x || cond
- !instanceof x || cond plus return in else block
for each case we have to ask where x is visible. in the if-block, else-block, after the if-else condition? De Morgan would help to simplify maybe. My feeling is also that we do not need to traverse the complete expression tree in all cases to decide for x. A complex expression with an instanceof deep inside makes things more difficult, but would also be solvable with De Morgan. Just not seeing the test cases reflecting all of this.
There was a problem hiding this comment.
This is the most actionable review point, and the coverage was indeed incomplete.
One important nuance about dynamic-mode Groovy: evaluateInstanceof always allocates the
CompileStack slot during condition evaluation (needed for && short-circuit). In dynamic
mode this means the slot can be accessed at runtime even for condition shapes where flow
scoping says it is not "definitely bound" (e.g. instanceof s || true in the if-block).
@TypeChecked / @CompileStatic enforces the stricter Java rule at compile time. The
cases below are therefore tested in whichever mode is the appropriate signal for that shape.
| # | Condition shape | if-block | else-block | after if-else | Test method |
|---|---|---|---|---|---|
| 1 | o instanceof String s |
✅ visible (existing) | ❌ new | ❌ new | testSimpleInstanceof_notInElse_notAfterIf |
| 1b | o instanceof String s, else throws |
— | — | ✅ visible new | testSimpleInstanceof_visibleAfterIf_whenElseAbrupt |
| 2 | !(o instanceof String s) |
❌ new | ✅ visible (existing) | — | testNegatedInstanceof_truePathHides_falsePathBinds |
| 3 | !(o instanceof s) + return in if |
— | — | ✅ new (explicit cell) | testNegatedInstanceof_earlyReturnInTrue_visibleAfter |
| 4 | o instanceof String s && cond |
✅ visible (existing) | ❌ new | ❌ new | testAndChain_ifBlockVisible_elseNotVisible_afterNotVisible |
| 5 | o instanceof String s || cond |
❌ (TypeChecked) new | ❌ | ❌ | testOrChain_noVisibilityAnywhere |
| 6 | !(o instanceof String s) && cond |
❌ (TypeChecked) new | — | — | testNegatedAndCond_noVisibility |
| 7 | !(o instanceof s) && cond, else return |
— | — | ❌ (TypeChecked) new | testNegatedAndCond_withElseReturn_noVisibilityAfter |
| 8 | !(o instanceof String s) || cond |
— | ✅ visible new | — | testNegatedOr_elseBlockSees_afterAbruptIfBlockSees |
| 9 | !(o instanceof s) || cond, else return |
— | — | ❌ new | testNegatedOr_elseReturn_noVisibilityAfter |
Cases 5, 6, 7 use
@TypeCheckedbecause in dynamic Groovy the slot is physically
allocated byevaluateInstanceofregardless of flow scoping. TypeChecked enforces the
Java-compatible rule at compile time, which is the correct enforcement layer.
Additional De Morgan / compound cases added:
!(o instanceof String s && cond)— conservative: no binding anywhere
(testDeMorgan_notAndNegation_noBinding)!!(o instanceof String s)— double negation restores positive binding
(testDoubleNegation_positiveBinding)
Unit-test coverage for the analysis logic (InstanceofFlowBindingsTest) has been
extended with the same systematic matrix at the pure binding-analysis level, testing
whenTrue(), whenFalse(), and isEmpty() for each condition shape. Multi-pattern
&&/|| combinations (two distinct pattern variables s and i) and allNames()
deduplication are also covered.
There was a problem hiding this comment.
So I hope I read the table right, I assume "-" means not visible and is existing behavior.
Case 1b is incomplete. if the if-block also has an abrupt return, then - well technically the code after is unreachable - but we pass that in compilation. Which means normally s should not be visible. Maybe it does not matter and can be ignored.
But I think this case here is missing:
if (!(o instanceof String s)) {
println "not String"
} else {
println "String"
return
}
println "still not String"
println s // invalid
It is not case 2,3,6, 7 or 9. Case 8 has the same behavior, but I doubt it covers this variant. I know I did not mention this before, I had not enough time to ensure the list is complete.
Then on TypeChecked... the comment made me think that the approach has a major problem I did see a bit before already, but it becomes more clear to me. I think there should be no reason as of why TypeChecked should enable more cases. Especially not because of an implementation detail that exists in the dynamic compiler.
My wish would be now the following... yes I am aware that this is a big wish:
Have the scoping logic completely in or through VariableScopeVisitor. Have AST level tests that check the scope for correctness for all combinations of instanceof (negated or not), with condition after or not, with return/throw in if-block, in else-block. And for each case of that matrix we need to check and define is the variable visible in the condition if it exists, in the if-block, in the else-block or after. The code here goes in the right direction but I think we need this on the AST level right after VariableScopeVisitor to ensure downstream transforms do this right. Or maybe a simple VariableScope is not enough anymore. The original purpose is to determine if a variable is referencing something outside the scope or not.
And what does it mean for a Variable to be in the scope:
{ // block1
....
def s = ...
...
}
s is added to the scope of block1. This does not mean s is available in all of block1. Declaring another variable s in that block can be easily checked as not allowed, since the active scope already defines it.
{ // block1
if (!(o instanceof String s) {
return
}
println s
}
Now we have again our block1, but where is c declared? If we stay with the VariableScope system we would then have to make a scope for block1, that define s and remove it from the scope in the if-block. I think we currently have no remove. Well or we do not add it the the child scope
{ // block1
if (o instanceof String s) {
return
}
println s // different s
}
If block1 declares s, then the if-block references it, but after the if we are back in block1 and now s is supposed to be invalid. The problem could maybe solved by adding an invisible block containing the if-else for such a case, but that means to conditionally rewrite a potentially big AST part, which I also feel not so well about. I mean in principle the AST is an "abstract" tree, not a parser tree, so I think technically it would be ok, just weird because we are normally not doing that. (the primitive optimization part did copy large parts of the AST, but mostly by visiting it twice)
So I cannot right away suggest a good solution, just that the actions in StatementWriter raise warning flags for me and two phases using the same logic on an AST that is supposed to have been enhanced for not requiring that logic anymore, well that raises a red flag to me.
When I mentioned De Morgan, I was actually not thinking of !!. I was thinking of for example
(a==b && (a instanceof X x && x.isFoo()) which is... well De Morgan helps with the escape analysis here maybe, but does not solve the problem of the usage of x is valid, but a and b cannot reference that x. But I think you have that one actually covered.
One more word to TypeChecked. We have basically lexical scopes for variables in Groovy. The difference is that the scope parenting the method level is an open scope (similar for Closure), while it is not for TypeChecked. This means a random reference s is always valid in dynamic Groovy, but the semantics are influenced by the lexical scope. That involves double declaration and also shadowing rules. TypeChecked and not TypeChecked should behave here the same, except for a vanilla s not always being valid.
There was a problem hiding this comment.
Thank you for the detailed and precise feedback. All three points have been addressed.
1. Missing test case
You identified this shape, which was absent from the matrix:
if (!(o instanceof String s)) {
println "not String" // S can complete normally
} else {
println "String"
return // T cannot complete normally
}
println s // must be INVALIDJLS §6.3.2.2-200-C analysis:
e = !(o instanceof String s):whenTrue = {},whenFalse = {s}- C-A:
e.whenTrue = {}— nothing to introduce even with abrupt T. - C-B: requires
e.whenFalse = {s}and S cannot complete normally.
Here S can complete normally (it falls through) — C-B does not apply. - →
sis not introduced after the if-else.
Added:
testNegatedInstanceof_abruptElseOnly_noVisibilityAfterinInstanceofTest.groovytestCase7b_negatedInstanceof_abruptElseOnly_afterDynamicin the new
InstanceofScopeTest.groovy(AST-level, see §3 below)
2. @TypeChecked should not be special for scoping
You are correct. Tests 5, 6, and 7 incorrectly used @TypeChecked, implying it
enables enforcement that the base compiler does not provide. The real picture:
VariableScopeVisitor.visitIfElse already correctly scopes pattern variables in
all modes. For shapes where e.whenTrue = {} (e.g. instanceof s || cond,
!(instanceof s) && cond), declarePatternVariables(bindings.whenTrue()) declares
nothing in the if-block scope. The undeclared reference s therefore resolves
via findVariableDeclaration to a DynamicVariable — in both dynamic and
@TypeChecked modes. In dynamic Groovy, DynamicVariable resolution at runtime
yields MissingPropertyException — the same observable effect.
Tests 5, 6, and 7 have been rewritten to use plain
shouldFail MissingPropertyException without @TypeChecked. The comments and the
VariableScopeVisitor Javadoc now state explicitly:
References to a pattern variable outside its flow scope resolve to
DynamicVariable, which at runtime produces aMissingPropertyException—
the same behaviour as@TypeChecked's compile-time error, without
requiring that annotation.
3. Architecture: VariableScopeVisitor as single source of truth
You raised the concern that two phases independently apply the same flow-scoping
logic on an AST that should already have been enhanced by the first.
Here is the precise split:
-
VariableScopeVisitor.visitIfElseis the authoritative source of scope
decisions. It declares each pattern variable only in the scope(s) where the
JLS says it is definitely assigned. All subsequent compiler phases — AST
transforms, type-checking, code generation — see the correct name bindings
without re-deriving the logic. -
InstanceofFlowSlotPublisherinStatementWritersolves a distinct
bytecode-level problem:evaluateInstanceofallocates a CompileStack slot
during condition evaluation (required for&&short-circuit RHS). Without the
publisher, that slot would be visible to both branches in the bytecode, even
thoughVariableScopeVisitorhas already excluded it from the else-branch's
scope. The publisher hides and re-exposes slots to keep CompileStack slot
visibility consistent with the VariableScope declarations — it does not
independently decide which variables are in scope.
In short: VariableScopeVisitor says which names are in scope;
InstanceofFlowSlotPublisher says which bytecode slots are exposed on each
branch. Both are driven by the same InstanceofFlowBindings analysis to stay
in sync.
The Javadoc on VariableScopeVisitor (class level and visitIfElse) now makes
this split explicit and is the canonical description of the design.
Longer term, if the dynamic compiler were changed to allocate pattern slots
lazily (only after the full condition has been evaluated, with initial visibility
controlled by the taken branch), the slot publisher could be removed entirely.
That would require changes to CompileStack / evaluateInstanceof and is left
as a tracked future improvement.
4. AST-level scope tests (InstanceofScopeTest.groovy)
As you requested, a new InstanceofScopeTest.groovy has been added. It compiles
each condition shape to Phases.SEMANTIC_ANALYSIS and inspects
VariableExpression.getAccessedVariable() on every s reference to verify:
- In-scope references resolve to the pattern variable's
VariableExpression
(a declared local). - Out-of-scope references resolve to
DynamicVariable.
| Test | Condition shape | Assertions |
|---|---|---|
testCase1_* |
o instanceof String s |
if-block: local; else/after: dynamic |
testCase1b_* |
same, abrupt else | after: local (§6.3.2.2-200-C-A) |
testCase2_* |
!(o instanceof String s) |
if-block: dynamic; else-block: local |
testCase3_* |
negated, abrupt if | after: local (§6.3.2.2-200-C-B) |
testCase4_* |
instanceof s && cond |
&& RHS: local; if-block: local; else/after: dynamic |
testCase5_* |
instanceof s || cond |
if-block: dynamic; || RHS (false path): local |
testCase6_* |
!(instanceof s) && cond |
if-block: dynamic |
testCase7a_* |
same, abrupt else | after: dynamic (C-B does not apply) |
testCase7b_* |
!(instanceof s), abrupt-else-only |
else-block: local; after: dynamic (new missing case) |
testCase8_* |
!(instanceof s) || cond |
|| RHS: local; if-block: dynamic; else-block: local |
testDoubleNegation_* |
!!(instanceof s) |
if-block: local |
testDeMorgan_* |
!(instanceof s && cond) |
if-block: dynamic; else-block: local |
testTernaryExpression_* |
(instanceof s) ? s : s |
true-expr: local; false-expr: dynamic |
Summary of changes
| File | Change |
|---|---|
InstanceofTest.groovy |
Tests 5/6/7 rewritten without @TypeChecked; new testNegatedInstanceof_abruptElseOnly_noVisibilityAfter |
InstanceofScopeTest.groovy |
New — AST-level scope assertions via VariableExpression.getAccessedVariable(), full visibility matrix |
VariableScopeVisitor.java |
Class-level and visitIfElse Javadoc clarified: authoritative-source role; relationship with InstanceofFlowSlotPublisher; JLS §6.3.2.2 rule labels on inline comments |
5. Bug fix: slot leakage for ||-shaped conditions in dynamic Groovy
During the test rewrite, a genuine bug was uncovered and fixed:
For conditions like o instanceof String s || cond where
InstanceofFlowBindings.of() returns EMPTY (neither whenTrue nor whenFalse
has any bindings), InstanceofFlowSlotPublisher.captureAndHide previously
returned early (NONE) because it only looked at bindings.allNames() — which
is empty for this shape. However, evaluateInstanceof still allocates a
CompileStack slot for s while evaluating the condition. Without capture-and-hide,
that slot remained visible to both branches in bytecode, allowing s to be read
in the if-block even though VariableScopeVisitor had correctly excluded it from
the if-block scope.
Fix: A new InstanceofFlowBindings.allPatternNames(Expression) static method
walks the full condition expression tree to collect all pattern variable names,
regardless of which flow path they are definitely assigned on. The
captureAndHide method now uses this to capture and hide all allocated slots,
not just those in bindings.allNames(). Publishing still uses only the
path-specific sets (whenTrueNames(), whenFalseNames()), so the net effect is:
- Before fix:
o instanceof String s || true—sslot visible in if-block
(bytecode leak;VariableScopeVisitorsaid dynamic but bytecode said local). - After fix: slot is captured and hidden after condition evaluation; never
re-published for the if-block (becausewhenTrue={}) → correctly invisible.
This fix closes the gap between VariableScopeVisitor's scope decision and the
bytecode slot visibility — the same result in both dynamic and @TypeChecked
modes, without needing @TypeChecked to enforce the rule.
6. Refactoring: InstanceofFlowBindings → VariableScopeVisitor.InstanceofFlowBindings
To make the architectural intent explicit — that VariableScopeVisitor is the
single authoritative source, and that the flow analysis exists in service of
that visitor — InstanceofFlowBindings has been refactored into a public static final nested class named InstanceofFlowBindings inside VariableScopeVisitor.
Why a nested class?
- Co-location signals authorship. The class physically living inside
VariableScopeVisitormakes it immediately clear that this is the
visitor's analysis helper, not a free-standing utility that happens to
be referenced from two places. - Reference from the asm layer is still clean. The code-generation layer
(InstanceofFlowSlotPublisher,StatementWriter,BinaryExpressionHelper)
importsVariableScopeVisitor.InstanceofFlowBindings— theVariableScopeVisitor.
prefix makes explicit that these classes are consuming the scope decisions
established byVariableScopeVisitor, not independently deriving them. - Single file to read. Anyone reading
VariableScopeVisitor.javanow sees
the complete picture: scope declaration logic and the flow analysis that
drives it — no need to navigate to a second file.
Changes
| File | Change |
|---|---|
VariableScopeVisitor.java |
Added public static final class InstanceofFlowBindings (moved from the deleted file); added @Internal; Javadoc updated with @see InstanceofFlowBindings |
InstanceofFlowBindings.java |
Deleted |
InstanceofFlowSlotPublisher.java |
Import updated to VariableScopeVisitor.InstanceofFlowBindings; all InstanceofFlowBindings usages → FlowBindings |
StatementWriter.java |
Same import + usage update |
BinaryExpressionHelper.java |
Same import + usage update |
InstanceofFlowBindingsTest.groovy |
Import updated; all InstanceofFlowBindings.xxx → InstanceofFlowBindings.xxx; class name kept as InstanceofFlowBindingsTest |
InstanceofScopeTest.groovy |
Same import + usage update |
There was a problem hiding this comment.
[...]
* **`InstanceofFlowSlotPublisher`** in `StatementWriter` solves a distinct **bytecode-level problem**: `evaluateInstanceof` allocates a CompileStack slot during condition evaluation (required for `&&` short-circuit RHS). Without the publisher, that slot would be visible to both branches in the bytecode, even though `VariableScopeVisitor` has already excluded it from the else-branch's scope. The publisher hides and re-exposes slots to keep _CompileStack slot visibility_ consistent with the _VariableScope declarations_ — it does not independently decide which variables are in scope.
my problem with this is that it describes what it does, but it does not postulate why it has to be like that. And then it sounds like trying to solve a tooling problem with more tooling instead of fixing the original tool in the first place. There is then a number of things to mention as result of that.
- if the variable is visible via compile stack even though it should not via scoping, does it matter?
- If the variable is not used then I guess the line number table would be wrong for it, though it is not proofed yet, that the current solution does this correctly. My guess would be that not.
- If there is another variable of the same name in the block or we reference a former hidden variable. Here I do see that we have to do something.
- Entry point for should be CompileStack.
- InstanceofFlowSlotPublisher seems to exist parallel to it. Assume we give the compile stack the ability to hide a variable, meaning name becomes free, but index is unchanged, plush pushing the state and poping when done. Does this mean more code that is using compile stack? Probably yes, but I don´t think we need the bindings or the publisher here anymore.
- CompileStack is the administrator for handling bytecode tables and variable slots and the design idea is to have something like a stack for the different states. Those do not exist in bytecode but they are reflected by what slots are free and how the tables are written.Bypassing the push/pop logic as the additions of removeVariable and putVariable bend that idea. But putVariable basically tells me compile stack is no longer producing the variables in all cases (it comes originally from compile stack in this code, but nobody guarantees that) and removeVariable is supposed to be handled by push/pop
- I am not against adding helper classes, but the asm part should not have to redo things that had been done before already. Which means any classgen.asm class referencing something from even just classgen, that redoes something non-trivial already done by a previous visitor, that was supposed to enrich the AST with that kind of information, is looking really really problematic to me. It would be different if in a asm class we suddenly need to expand the AST, maybe create a parallel AST, and need to change information in there. But this is not the case here and also sounds like a bad idea frankly.
In short:
VariableScopeVisitorsays which names are in scope;InstanceofFlowSlotPublishersays which bytecode slots are exposed on each branch. Both are driven by the sameInstanceofFlowBindingsanalysis to stay in sync.
as explained before, InstanceofFlowBindings should not be required concept wise.
The Javadoc on
VariableScopeVisitor(class level andvisitIfElse) now makes this split explicit and is the canonical description of the design.
Mentioning InstanceofFlowSlotPublisher in VariableScoppeVisitor is imho not good. If there is a need for that, then there is a problem. You should never have to name a specific class in a classgen transform that is specific to a later classgen transform, especially not in classgen.asm.
Longer term, if the dynamic compiler were changed to allocate pattern slots lazily (only after the full condition has been evaluated, with initial visibility controlled by the taken branch), the slot publisher could be removed entirely. That would require changes to
CompileStack/evaluateInstanceofand is left as a tracked future improvement.
how does the lazy approach solve the problem? Also as long as this code is a break of the design it should not wait... Unless you convince me that it is not breaking, not even bending strongly.
InstanceofScopeTest.groovyNew — AST-level scope assertions viaVariableExpression.getAccessedVariable(), full visibility matrix
[...]
VariableScopeVisitor.javaClass-level andvisitIfElseJavadoc clarified: authoritative-source role; relationship withInstanceofFlowSlotPublisher; JLS §6.3.2.2 rule labels on inline comments
I think the testing has now improved a lot... but I am still missing something InstanceofScopeTest does only positive testing, VariableScopeVisitor does some negative and positive testing. I think we are missing still two tings related to additional variable declarations. (1) some shouldNotCompileTests that check you cannot declare a variable s where s is supposed to be visible through the instance-of. (2) more tests in InstanceOfScopeTest that use variable declarations of s where s is not visible through the instance-of and asserting they are local. And we have to ensure those examples actually are done correctly in class generation as well, thus similar tests should also be added to InstanceOfTest
There was a problem hiding this comment.
Thank you for the careful design review. Your points on why slot visibility
matters, and on keeping CompileStack as the single administrator of name↔slot
state, were correct. The earlier InstanceofFlowSlotPublisher +
putVariable/removeVariable design bent that model; the current code follows
the direction you described.
1. When does CompileStack visibility matter?
if the variable is visible via compile stack even though it should not via
scoping, does it matter?
| Case | Does it matter? | Approach now |
|---|---|---|
| Name unused on that path | Mostly LVT / debug ranges | Improved as a side effect of hide + push/pop; not the primary driver |
| Same name redeclared where the pattern is not live | Yes — name must be free | CompileStack.hideVariable (name free, index unchanged) |
| Reference that should be dynamic | Yes — AsmClassGenerator looks up locals by name |
(1) path hide; (2) if accessedVariable instanceof DynamicVariable, never load a same-named local (also covers mid-condition || RHS) |
So action is required exactly for the two cases you flagged: redeclaration and
reference to a name that scoping has already rejected.
2. Entry point is CompileStack; publisher removed
Entry point should be CompileStack… hide… name free, index unchanged… push/pop…
I don’t think we need the bindings or the publisher here anymore.
Done:
InstanceofFlowSlotPublisherdeleted.putVariable/removeVariabledeleted (they bypassed push/pop and implied
CompileStack was no longer the sole producer of locals).- CompileStack now owns:
recordPatternVariable— slots produced only byevaluateInstanceof;hideVariable— name free, index kept;hidePatternVariablesExcept(candidates, live)— hide among names
introduced by this condition (outer pattern names untouched);patternVariablesIntroducedSince(before)— identity-based introduced set
(same name re-bound by a later condition still path-hides correctly).
Control flow (if/else):
// condition on outer frame → slots defined & recorded
pushBreakable() // arms only (GROOVY-7463)
pushState / hide(except whenTrue) / then / pop
pushState / hide(except whenFalse) / else / pop
pop() // outer still holds the slots
hidePatternVariablesExcept(introduced, survivors) // permanent on outer
Then/else use ordinary push → hide → pop. Survivors need no put-back: the
condition runs before pushBreakable, so after pop the outer map still
holds those slots and we only hide non-survivors.
3. asm must not re-run flow analysis
the asm part should not have to redo things that had been done before already
InstanceofFlowBindings should not be required concept wise [in classgen]
Agreed. Layering now:
| Phase | Role |
|---|---|
VariableScopeVisitor + InstanceofFlowBindings (internal analysis) |
Declare names into scopes; attach InstanceofPathLiveNames (path-live name sets) as AST metadata |
| classgen.asm | Read metadata only; drive CompileStack hide/push/pop — no InstanceofFlowBindings.of(...) |
InstanceofFlowBindings remains a private analysis helper of the visitor (and its
unit tests). It is not a cross-phase concept that asm re-executes. Classgen only
consumes the enriched name sets on the AST — the same idea as using
VariableScope after the visitor, adapted to flow-sensitive bindings.
4. No SlotPublisher in VariableScopeVisitor
Mentioning InstanceofFlowSlotPublisher in VariableScopeVisitor is imho not good
Done. That class is gone; VSV Javadoc no longer names any classgen.asm type.
The visitor documents only its own products: scopes + InstanceofPathLiveNames.
5. Lazy allocation
how does the lazy approach solve the problem? … as long as this is a break of
the design it should not wait
Agreed that a design break should not be deferred. The redesign above is meant to
remove the layering break now, not postpone it.
Lazy define (slot only after the condition, on the taken path) could still
simplify evaluateInstanceof later, but it is not required to restore
CompileStack’s push/pop ownership. Early define remains for short-circuit &&
RHS; path visibility is administered by CompileStack hide/push/pop + metadata.
Lazy allocation is therefore an optional micro-optimisation, not a fix for a
broken design.
6. Tests you asked for
(1) shouldNotCompile where s is visible
(2) ScopeTest: declare s where not visible, assert local
(3) same for classgen / InstanceofTest
Added:
InstanceofScopeTest: redeclare where pattern is not live → new local
(accessedVariablelocal and ≠ pattern); where pattern is live →
already contains a variable of the name s.InstanceofTest: matching runtime/classgen cases; successive ifs reusing
the same pattern name; isolated pattern expression then later if.
Visibility matrix and flow-bindings tests remain green; main-module :test is green
on this change set.
7. Point → change (summary)
| Your point | Response |
|---|---|
| Explain why, not only what | Free name for redeclare + correct load vs dynamic access |
| CompileStack entry; hide + push/pop; drop publisher | Done; publisher deleted |
| put/remove bend the model | Removed; no put-back API |
| asm must not redo visitor work | Metadata only; no re-analysis |
| Don’t name later asm types from VSV | Done |
| Design break must not wait on “lazy” | Layering fixed without lazy |
| Redeclare tests (visible / not) | Scope + runtime tests added |
Happy to adjust further if any of the above still looks like a strong bend of the
CompileStack model from your point of view.
d18beef to
5738634
Compare
|
✅ All tests passed ✅🏷️ Commit: d332081 Learn more about TestLens at testlens.app. |
3 participants
https://issues.apache.org/jira/browse/GROOVY-12242