paulk-asert commented on PR #2773:
URL: https://github.com/apache/groovy/pull/2773#issuecomment-5260322520
Following up (AI-assisted analysis and patch) on one thread from the review
— @blackdrag's observation about debug info for hidden-but-allocated slots:
> 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.
We wrote a bytecode-level test to check, and the guess is correct — it is
the LocalVariableTable (the line number table itself is unaffected). On the
current PR head (9aa4ded), a pattern variable's LVT entry spans from its
definition in the condition to essentially the end of the method, ignoring the
flow-scoping hides:
| Shape | Problem observed |
|---|---|
| `if (o instanceof String s) {...} else {...}` | `s` range covers the
else-arm |
| same + `def s = ...` redeclared in the else | the two `s` entries
(different slots) have overlapping ranges — a debugger in the else block sees
both |
| `if (!(o instanceof String s)) { return }; ...s...` | `s` range covers the
then-arm |
| `o instanceof String s \|\| c` | `s` range covers both arms despite being
bound nowhere |
The root cause is structural: `hideVariable` frees the name but the variable
keeps its single `[start, end)` LVT range, whose end keeps being stamped by
later pops. A single contiguous range cannot express branch-shaped liveness
(e.g. live in condition + then-arm, dead in else, live again after), so no
tweak to the current emission can fix this — it needs one LVT entry per
visibility range, which the class-file format supports and is exactly what
javac emits for Java pattern variables.
Because these attributes are debug-only (no effect on execution or
verification), we'd suggest this as a **follow-up after this PR merges** rather
than a blocker. We have a working implementation ready (+142/−5 over two files)
that slots into the CompileStack-owned design from the redesign:
- `BytecodeVariable` keeps a list of completed ranges;
`startLabel`/`endLabel` describe only the open one. Ordinary locals never close
a range, so their emission is byte-for-byte unchanged.
- `CompileStack.hideVariable` closes the open range at the hide point;
`popState` re-opens a range when a pop re-exposes a hidden variable; `clear()`
emits one `visitLocalVariable` per non-zero-width range (type-annotation label
arrays extended to match).
With the patch, the emitted tables follow flow scoping — e.g. the
negated/abrupt-then idiom now produces two entries for the same slot, with the
dead then-arm in the hole between them, exactly like javac:
```text
if (!(o instanceof String s)) { print 'in-then'; return }
print s
LOCALVARIABLE s Ljava/lang/String; slot=2 range=[7, 21) // condition
LOCALVARIABLE s Ljava/lang/String; slot=2 range=[31, 44) // after the if
// then-arm (insn 24) in the hole
```
Verification: a new property-based `InstanceofPatternLvtTest` asserts both
directions (no entry covers a dead region; every live region is covered by some
entry), and the full main-module `:test` suite is green with the patch (16,600
passed, 0 failed).
If there's agreement this is the right follow-up, I'll raise a JIRA and open
a PR with the patch + test once this one lands.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
