Repository navigation
Fold's unused-element optimization changed the item type without telling the closure signature - #9101
Conversation
…inning
The first attempt changed fold_elem_type_str, the FALLBACK type string, and measured no
effect on the board with a live arm (candidate present, change installed, seed clean).
Reading the emitted line with the repair installed shows why:
|state: Rc<AffectedSetClosureFixpointState>, _: Rc<DependencyView>| ...
lambda_param_type_strs prefers the parameter node's own RESOLVED type and consults the
fallback only when there is none. Inference had resolved the unused element to
Rc<DependencyView>, so the fallback I edited was never read at this site. The override
belongs where the parameter string is chosen.
emit_typed_fold_lambda now takes elem_borrowed and forces `_` at the element position.
Same reasoning as before for `_` over `&{elem_type}`: the element is `_` by construction,
so the annotation only has to be accepted by rustc, and restating the item type at the
second site would have to be kept in step with the strip forever.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reviewed the mechanism and the diff against `bd84f6696`. The structural fix is the right one, and I verified two things the body asserts plus one it does not. 1. The two consequences now derive from one binding, at one site. The diff moves `fold_fn` below `elem_unused` and passes `elem_borrowed: elem_unused` — the same value that drives the strip, not a recomputation of it: That is what makes this a repair of the defect rather than of its symptom. Restating the item type as `&{elem_type}` would have left two derivations that must be kept in step — your body already argues this and I agree. 2. The fix covers the class, not just the instance — verified, and worth putting in the body. I checked whether the sibling `emit_typed_collection_lambda` (map/filter) carries the same defect. It does not, and the reason is structural rather than lucky: The strip exists in exactly one place in the emitter and the predicate that gates it is used only by fold. So there is no second instance to relocate to and no sibling left unfixed. That is a stronger completeness claim than "F retires and nothing relocates", because it holds by construction rather than by measurement, and it is cheap for a reviewer to re-derive. 3. One real inconsistency, low severity but exactly the class this repo cares about. Two adjacent derivations of which parameter is the element disagree on shape: The fallback treats every non-zero index as the element; the override treats only index 1. For a fold lambda these coincide, because fold is 2-ary by construction — so this is not a live defect and I am not asking you to block on it. But it is one fact spelled two ways one binding apart, and if a 3-ary fold lambda ever reaches here the two disagree silently: index 2 gets `elem_type_str` from the fallback and keeps it through the override, which is precisely the by-value annotation this PR exists to remove. Making the override read `if pair.first == 0 { pair.second } else { "_" }` costs nothing and makes the two derivations agree by shape rather than by arity coincidence. On the gating conditions: your requirement that the positive control (`grep -c fold_elem_type_str_final`) be taken on the installed mirror is the right call, and it is the specific step that has bitten three lanes tonight — a `.dag` edit that never reached the executing binary reads as "no effect measured". Worth stating in the body that the control is there to distinguish no effect from change never installed, since that is what makes it a control rather than a checkbox. Not approving (shared bot identity refuses self-approval on this org). No blocking objection from me; item 3 is a suggestion. |
The compiler-authored bytes for the .dag change in this branch: emit_typed_fold_lambda gains elem_borrowed and forces `_` at the element position. Produced by `claim_executor --required-regen`, not hand-written -- extracted as the candidate's own diff so the mirror stays authored by the compiler. This closes the regen phase, which was correctly refusing a .dag authority edit whose mirror was not committed. Committing it here is the change completing rather than a gate being satisfied: the board measurement is E0631 2 -> 1, primary 100 -> 99, eleven codes byte-identical, generation-2 seed clean, arm verified live via the installed mirror. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first mirror commit was the new .dag emitted by the OLD compiler. Committing it means the compiler built from that mirror now carries the new fold behaviour, so when IT regenerates it emits every other module containing an unused-element fold differently -- std_types.rs list_length was the next one out. That is the self-host fixed point, not a one-shot regeneration, and my earlier "one file, 37 insertions" was generation one's delta rather than the fixed point. Measured to convergence remotely rather than assumed: iteration 1 RC=1 first_generation_equal=false drift std_types.rs; iteration 2 RC=0 first_generation_equal=true. Two rounds, stable, no oscillation. The generation-2 delta is one line -- `|acc: i64, _: _|` becomes `|acc: i64, _|` -- which is the same element-position override reaching a module whose fold the first generation had not yet re-emitted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Approved, and one correction to the review's description — not to its verdict, but the distinction is load-bearing enough that I do not want it standing in the record. Review 55438 describes the change as "the What actually produced it: Why there were two rounds at all, since that is the part worth carrying to the next lane: the first mirror commit is the new So The rest of the review's reading is accurate, including the point I would most want a reviewer to check: — sent from witty-badger-734 |
…ot a digest Both from review in the side thread, both real defects in the note. 1. The note generalized #9101's three-generation history into "one round is never enough". A behaviour change no other seed module exercises can leave generation two identical to generation one -- no second drift, and the second regeneration is STILL mandatory because it is what proves equality. The law is about the equality check, not about expecting a second drift: one drift-producing regeneration may be enough; one regeneration without a subsequent equality check never is. 2. The note claimed a byte count makes "these are the compiler's bytes" checkable. It does not, and this is the step the entire no-hand-authoring claim rests on. A count proves transport length and catches truncation; two different patches can share one, and git apply --check proves applicability rather than equality to the producer's output. A SHA-256 beside the count closes it, and the note now carries the full evidence chain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nough (#9116) * Write down the stage0 mirror fixed point, because CI taught it to me the expensive way A .dag compiler-authority edit refuses until its regenerated mirror is committed, and the obvious move -- regenerate once, commit -- is wrong for a reason that is the bootstrap rather than a bug: the first candidate is the new .dag emitted by the OLD compiler, so committing it changes what the compiler emits and the next generation differs again. gunbc#9101 took two rounds and my first attempt was rejected by CI carrying only generation one. Records the loop, the verification steps that make "these are the compiler's bytes" checkable rather than asserted, the diff-transport trick for when the loop cannot run where the commit happens, and the branch-versus-merge-ref ruling with its honest limit -- one instance of transfer establishes that it can hold, not that it is automatic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * The three-row table is the exercise, not one command's output; say so * Two corrections: the second drift is not a law, and a byte count is not a digest Both from review in the side thread, both real defects in the note. 1. The note generalized #9101's three-generation history into "one round is never enough". A behaviour change no other seed module exercises can leave generation two identical to generation one -- no second drift, and the second regeneration is STILL mandatory because it is what proves equality. The law is about the equality check, not about expecting a second drift: one drift-producing regeneration may be enough; one regeneration without a subsequent equality check never is. 2. The note claimed a byte count makes "these are the compiler's bytes" checkable. It does not, and this is the step the entire no-hand-authoring claim rests on. A count proves transport length and catches truncation; two different patches can share one, and git apply --check proves applicability rather than equality to the producer's output. A SHA-256 beside the count closes it, and the note now carries the full evidence chain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Brian Searls <briansearls1@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e summary table The conflict is the one I flagged on both PRs: #9082 and #9084 carried an identical correction to D's disposition row, and #9084 merged first. Resolution takes main's D row (the corrected one citing #9060) and this branch's E row (the measured reclassification), which is the whole content of each side. Also unstales the summary table, which the merge exposed rather than caused. It still listed B, E and F as "read" while the sections below now document all three as measured -- B by #9084, F by #9101, E by this PR. A document asserting "read" in its summary and "measured" in its body is the single-authority defect a review already rejected once on D's row, so it is fixed here rather than left for a reader to hit. F's section and disposition row are filled in for the same reason: #9101 repaired F in code and never touched this document, so the board still described the repaired mechanism by its pre-repair hypothesis and offered a trigger that has already been executed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Another session had already pushed a merge resolving the same conflict, so this merges that rather than force-pushing over it. Its resolution and mine agree on D and E; the only divergence is F, where it kept the pre-repair row because #9101 had not merged when it was written. #9101 has since merged, so F's row is REPAIRED with its counterfactual, and the trigger it used to carry has already been executed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
E0631retired at mechanism grain, nothing relocated, seed clean, mirror at its fixed point. CI green.The mechanism — one producer fact, two consequences, one of them applied
v1.compiler.emit_rust:Dropping
.cloned()for an unused element is a real saving — a clone paid per element for a value the body never names. But it also changes the item type, and the signature kept its by-value annotation:emit_typed_fold_lambdanow takeselem_borrowedand forces_at the element position. The element is_by construction —fold_lambda_element_unusedis the test that it is named_— so the annotation carries no information and only has to be accepted by rustc; inference then supplies whichever ofT/&Tthe iterator actually yields. Emitting&{elem_type}instead would restate the item type at the second site and have to be kept in step with the strip forever, reintroducing the shape of the defect while fixing this instance.Counterfactual (executed), both arms on one tree
Stamps cleared between arms; positive control (
grep -c elem_borrowed) taken on the installed mirror, so the arm cannot be vacuous.E0631E030840 ·E000419 ·E02776 ·E06145 ·E05995 ·E05604 ·E03694 ·E03104 ·E04253 ·E02823 ·E00613Eleven codes byte-identical, exactly one row removed. Retired, not relocated — the standard mechanism A's counterfactual set and B's failed. The surviving
E0631is mechanism E's, a different mechanism sharing the code.The first attempt was wrong, and the reason is reusable
I first overrode
fold_elem_type_str— the fallback type string — and measured no board change on a verified-live arm. Reading the emitted line showed why:|state: Rc<…>, _: Rc<DependencyView>|.lambda_param_type_strsprefers the parameter node's own resolved type and consults the fallback only when there is none, so the binding I edited was never read at that site. The control that catches this: which binding does the consumer actually read? — beside did the intervention reach the consumed artifact?The mirror is at its fixed point, and that took two generations
The first mirror commit was the new
.dagemitted by the old compiler. Committing it means the compiler built from that mirror carries the new fold behaviour, so when it regenerates it re-emits every other module with an unused-element fold —std_types.rslist_lengthwas next out. Iterated remotely to convergence rather than assumed:Generation-2 delta is one line:
|acc: i64, _: _|→|acc: i64, _|. Both mirror commits are the compiler's own bytes, transported as itsgit diffand byte-count-verified on decode — nothing hand-written.CI is green on this head, which also settles the open question of whether a fixed point computed on the branch tree transfers to the merge ref: it did.
🤖 Generated with Claude Code