fix(ownership): clear the enclosing killed set when a while condition contains break or continue - #13523
Open
asterite wants to merge 2 commits into
Open
fix(ownership): clear the enclosing killed set when a while condition contains break or continue#13523asterite wants to merge 2 commits into
asterite wants to merge 2 commits into
Conversation
…while-condition break/continue A break or continue written in a nested while loop's condition targets the ENCLOSING loop, so it can skip a reassignment below it. The break/continue arm clears the killed set to record exactly that, but find_last_uses_in_loop_body moves the enclosing killed set aside (std::mem::take) before traversing the body and condition and restores it unconditionally afterwards, discarding the clear. Loop-exit truncation then wrongly exempts the variable, its use is classified as a move, and the required clone is never emitted: a release nargo silently returns [9, 9] instead of [1, 9], and a debug build rejects the program via the rc_invariant validator. Red tests only (residual of noir#13153, reported in noir-claude#1702): two execution_success programs (break and continue spellings) and two ownership unit tests whose snapshots record the buggy move decision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g to the enclosing loop's killed set A variable is exempt from loop-exit clone-forcing only while its in-loop reassignment is unconditional; a reachable break/continue voids that, which the Break/Continue arm records with killed.clear(). But find_last_uses_in_loop_body saves the enclosing killed set away (std::mem::take) while traversing the body and condition, so a break/continue in a while condition — which targets the enclosing loop — cleared the loop-local set, and the stale enclosing kills were restored afterwards. The variable then stayed exempt, its use was classified as a move, and the required clone was never emitted. Mirror the existing has_break propagation: when the condition contained a break/continue, clear the restored killed set too. Fixes noir-lang/noir-claude#1702 (residual of noir#13153). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The last-use analysis parks the enclosing loop's "guaranteed reassigned" facts (
killed) on the side while it walks a nestedwhile, so abreak/continuewritten in that inner condition — which targets the enclosing loop — erased a scratch copy instead of the real facts. The compiler then kept a move decision thebreakhad invalidated: the clone protectingwas never emitted,
y[0] = 9wrote throughx's buffer in place, and a releasenargosilently returned[9, 9]instead of[1, 9](a debug build rejects the program via therc_invariantvalidator instead).This is the residual half of noir#13153: that fix propagated
has_breakacross exactly this save/restore boundary but notkilled, and thebreak_dependent_usesset it introduced is consulted only by theconfirmed_movespartition, never by loop-exit truncation.Fixes noir-lang/noir-claude#1702.
Fix
Mirror the existing
has_breakpropagation forkilled: after restoring the enclosing set, clear it when the condition contained abreak/continue— the same effect the jump would have had if the enclosing facts had been in place when it was traversed.Tests
The first commit is red (against the buggy compiler), the second is the fix:
execution_success/regression_while_condition_break_killedandregression_while_condition_continue_killed(thecontinuewitness fires on the final iteration, so no later reassignment masks the corruption). Both sit alongside the three regressions noir#13153 shipped, which keep passing — the fix does not over-clone the shapes they pin.let mut y$l5 = x$l2;tolet mut y$l5 = x$l2.clone();.Validation: full
noirc_frontend(2228) andnoirc_evaluator(1961) suites, all 128 loop/break-related execute tests, AST fuzzer smoke, clippy, fmt.🤖 Generated with Claude Code