fix(lowering): dedup panic_destruct calls for a var at a shared panic location - #10205
Conversation
… location When several control-flow arms converge on a single block that materializes the `Panic`, `PanicState::merge` concatenates their panic-location lists without deduplication. A variable that is live into that convergence then gets a `PanicDeconstructionEntry` per duplicate location, so `add_destructs` emits two `panic_destruct` calls that both consume the same variable. For panic-destruct-only types (a manual `PanicDestruct` impl, no `Drop`/ `Destruct`) this double-move reaches sierra generation as `dup<T>` on a non-duplicatable type and ICEs, e.g. `dup<Felt252Dict<u64>>`. The borrow checker uses a scalar `PanicState` so it never observes the duplication. Dedup the destructions per group (by the destructed variable) in `add_destructs`, where entries are already grouped by location, so each variable is panic-destructed at most once per convergence point. Adds a `test_data/destruct` golden covering an `if/else` that converges on a shared `panic!` over a `Felt252Dict`-wrapping panic-destruct-only type.
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryLow Risk Overview In A new lowering snapshot test covers an Reviewed by Cursor Bugbot for commit 8ecc01e. Bugbot is set up for automated code reviews on this repo. Configure here. |
TomerStarkware
left a comment
There was a problem hiding this comment.
@TomerStarkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on eytan-starkware).
Summary
Deduplicate
DestructionEntryitems byvar_idbefore generating panic-destruct calls, preventing the same variable from being destructed more than once when control-flow arms converge on a shared panic path.Type of change
Please check one:
Why is this change needed?
When multiple control-flow branches converge at a shared panic site, the lowering pass could emit duplicate
panic_destructcalls for the same variable. This produced redundant (and potentially incorrect) destructor invocations in the lowered IR.What was the behavior or documentation before?
The
destructionslist was iterated as-is, so a variable that appeared in the destruction list more than once (due to branch convergence) would have its panic destructor emitted multiple times.What is the behavior or documentation after?
Before iterating over
destructions, entries are deduplicated byvar_idusingunique_by. Each variable's panic destructor is now emitted exactly once, even when multiple branches share the same panic exit point.A new test case (
Panic destruct is not duplicated when arms converge on a shared panic) verifies that aFelt252Dict-wrapping struct with a customPanicDestructimpl is squashed only once in the flat lowering output when anif/elseconverges before apanic_with_felt252call.Related issue or discussion (if any)
Additional context
The fix is a one-liner using
unique_byfrom theitertoolscrate, applied directly to thedestructionsiterator inadd_destructs.