Repository navigation
fix(#1928): banner reads planned requirements off the emission plan - #1996
Conversation
registerPlannedToolchain copied the float-assert and IRQ-wrapper blocks' requirements from IEmissionPlan into ToolchainRequirements, so the banner read a transcription of 2.2's decision. The plan is now held on TranspileState for the file's turn (cleared per file), and CodeGenerator.getToolchainRequirements composes the one per-file list: what Render recorded at its emission sites, unioned with EmissionPlan.requirements(plan). The banner and IFileResult.requirements both read that list. The copy and its stale CodeGenState comment are gone. RequirementAggregator.union is the shared key/site union that merge now builds on. Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
jlaustill
left a comment
There was a problem hiding this comment.
Review Summary
PR: #1996 — fix(#1928): banner reads planned requirements off the emission plan
Reviewed by: Claude (automated review)
Overview
This deletes registerPlannedToolchain and builds each file's requirement list in one place: Render's records ∪ EmissionPlan.requirements(plan). The banner and IFileResult.requirements both read it. The change does what #1928 option (a) asks. Three things are left over: a null default that should be an invariant, a misplaced doc comment, and docs that still describe the instrumentation ledger as the whole list.
Verified (not taken from the PR body)
- CI on
a1b3e92b: every check-runsuccessexceptDeploy Coverage(skipped) (gh api …/commits/a1b3e92b…/check-runs, queried 2026-10-11T14:09Z). Sonar:total 0open issues, gateOK. - I re-ran mutation 2 in a scratch clone. Replacing
EmissionPlan.requirements(plan)with[]madetests/criticalfail 0/8 andbit-range-const-width-floatfail 1/13. After the restore, both are green again (8/8, 13/13). - I mutation-checked both new unit tests.
unionwithoutaddUniquefails "does not repeat a site both lists carry".requirements()without the float block fails "lists every planned block's keys…". - Key order: the plan's keys now come after
overflow-panic-hosted-libc(recorded inaddGeneratedHelpers) where they used to come before it. The output does not change, becauseToolchainRequirementUtils.reportable()sorts by feature. A--debugprobe that has a clamp op insidecritical {}gives a byte-identical.cand an identical CLI requirements report on0cd96994band ona1b3e92b. - "Clears #1933 §0a's last parallel path": the other open paths in §0a's inventory are #1927, #1929 and #1659, and all three are closed (queried 14:10Z).
Findings
| Severity | Count |
|---|---|
| Critical | 0 |
| Important | 1 |
| Moderate | 3 |
| Nit | 1 |
Key Issues
- [important]
CodeGenerator.ts:249:plan === null ? []is a default for a decision that has not been made yet. Production can never reach it, and if it ever did, the banner would lose the interrupt-masking line andRequires: C11.with no error. Use an assertingemissionPlan()accessor, the same waydeclarationPlan()works. (Inline.) - [moderate]
EmissionPlan.ts:94:requirements()was inserted betweenbuild()'s JSDoc andbuild(). (Inline.) - [moderate]
ToolchainRequirements.ts:collect()and the module docs still say this module holds the file's whole list. (Inline.) - [moderate]
docs/architecture/README.md:143-144(not in the diff): its proof that "the ledger IS read back" is the banner'sRequires: C11.. After this PR, nothing in the ledger produces that line.float-assert-c11is the only C11 key (TOOLCHAIN_REQUIREMENTS.ts:56-60), and only the plan carries it (git grep '"float-assert-c11"' -- src ':!**/__tests__/**'finds onlyEmissionPlan.ts:167outside the registry and the key union). The argument still holds, because the banner still reads the ledger. For example,Requires: C++20.comes fromcpp-designated-initializer, recorded atCodeGenWalker.ts:3233. Change the example to that one. Lines 125-127 ("which toolchain features a run required … read once by the host") also need a clause saying the planned blocks' features are 2.2's and are read off the plan. Line 45 already lists toolchain requirements under 2.2. - [nit]
ToolchainRequirements.ts:34: the staleCodeGenStaterow and the measurement SHA. (Inline, with a suggestion.)
What Looks Good
RequirementAggregator.union()makes per-file and per-project merging one key/site union, somerge()can no longer drift from the per-file composition.- The PR deletes the stale "still recorded into
CodeGenState" paragraph and the duplicated header aboveaddGeneratedHelpers, and corrects theCodeGenWalkersite count (3recordcalls: lines 1593, 3233, 3271). - The mutation table includes a control (
bit-indexing, which has no IRQ block), and mutation 2 reproduces exactly as reported.
🤖 Automated review by Claude Code
Review of #1996: - TranspileState.emissionPlan() asserts the plan exists, like declarationPlan(); getToolchainRequirements() reads it instead of defaulting a missing plan to no requirements. A unit test calls it before generate() and expects the invariant's message. - EmissionPlan.requirements() moves below build(), so build() keeps its own JSDoc. - ToolchainRequirements' header, deferred-sites and collect() docs say it holds Render's records only; the table names TranspileState.requireInclude and the tree it was measured on. - Transpiler.ts's read comment and RequirementSites' history stop naming the deleted requireToolchain/CodeGenState as current. - docs/architecture/README.md: the ledger read-back example is the C++20 line from cpp-designated-initializer, and planned-block features are noted as 2.2's. Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
|
Re review item 4 (
Items 1, 2, 3 and 5 are answered in their threads. |
The two holders (recorded, deferred) are unchanged; the header and deferred-sites docs added in the previous commit moved them from lines 56/67 to 61/74. Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
jlaustill
left a comment
There was a problem hiding this comment.
Review Summary
PR: #1996 — fix(#1928): banner reads planned requirements off the emission plan
Reviewed by: Claude (automated review), head cc8fd41cf
Overview
registerPlannedToolchain is gone. The file's requirement list is now built in one place, CodeGenerator.getToolchainRequirements(), as Render's records ∪ EmissionPlan.requirements(plan), and both readers (the banner and IFileResult.requirements) use it. That meets #1928's option (a). The change is correct. One guard is missing, and one gap the PR uncovered needed filing.
Verified
- CI: all 17 check runs completed on
cc8fd41cf, withAll Checks Passedat 2026-10-11T14:48:15Z (queried 14:54:19Z). Sonar:total: 0open issues on the PR, gateOK, new-code coverage 100.0. - Scratch clone at
cc8fd41cf:npm run buildandnpx tsc --noEmitexit 0.npm run unit: 427 files, 8497 tests passed.npm run test:q: 1615/1615 passed, withgit status --porcelainempty afterwards (no snapshot changed). - The PR's composition mutation reproduces: with
EmissionPlan.requirements(...)replaced by[], everytests/criticalfixture fails at banner line 4, and so doesbit-range-const-width-float. - The new invariant cannot fire on a real path:
getToolchainRequirements()has exactly two callers (CodeGenWalker.ts:1693,Transpiler.ts:1034).generate()always reachesassembleGeneratedOutputor throws intobuildCatchResult, and parse-only returns before it. - Key order is not observable: plan keys now follow post-plan Render records such as
overflow-panic-hosted-libc, but the banner and the CLI report both go throughreportable(), which sorts by feature and then key. - The doc claims hold: holder pins
ToolchainRequirements.ts:61/74matchrecorded/deferred.CodeGenWalkerhas 3recordsites. Nothing under3-Render/headers/reaches a requirement write, directly or throughrequireInclude,applyEffectsorHelperGenerator. The README'sRequires: C++20.example matches real snapshots (for exampletests/issue-502/*.expected.cpp). - #1933 §0a: #1927, #1929 and #1659 closed on 2026-10-08, so #1928 is the last standing row of that inventory, as the body says.
Findings
| Severity | Count |
|---|---|
| Critical | 0 |
| Important | 1 |
| Moderate | 1 |
| Nit | 0 |
Key Issues
- [important]
EmissionPlan.requirements()lists the planned blocks by hand. A third block compiles with its cost silently missing from the banner (mutation: 0 tsc errors). The suggested map, keyed by the plan's own type, turns that intoTS2741. The suggestion is compiler-verified. - [moderate] The deleted "does not yet own" note was the only record that Render still authors five keys §1 gives to 2.2, and the README now describes that split as the design. Filed as #1997. Cite it here.
What Looks Good
- There is one composition with two readers, so the banner and the file result cannot disagree. The accessor asserts the plan exists, so a missing plan throws instead of the old
=== null ? []default answering "costs nothing". RequirementAggregator.union()is extracted, so per-file and per-project merging share one key/site union.- The mutation table in the PR body reproduces as described.
🤖 Automated review by Claude Code
Review of #1996: - EmissionPlan.requirements() reads PLANNED_BLOCKS, a map keyed by every IEmissionPlan field typed IPlannedBlock | null, so a block added to the plan without an entry fails to compile (TS2741) instead of shipping with its cost missing from the banner. - docs/architecture/README.md and the ToolchainRequirements header cite #1997 for the keys Render still authors against §1's Ownership table. - Re-pin the ToolchainRequirements holders the header growth moved. Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
jlaustill
left a comment
There was a problem hiding this comment.
Review Summary
PR: #1996 — fix(#1928): banner reads planned requirements off the emission plan
Reviewed by: Claude (automated review)
Overview
This PR deletes the copy of 2.2's planned-block requirements into ToolchainRequirements. The file's requirement list is now composed once, in CodeGenerator.getToolchainRequirements(), which is option (a) on #1928. The direction is right, and the box-2 evidence holds up independently. Two things should change before merge. First, the plan's sites are a live alias that this PR makes load-bearing. Second, the extracted union() is still one of three copies of the same fold.
Findings
| Severity | Count |
|---|---|
| Critical | 0 |
| Important | 2 |
| Moderate | 1 |
| Nit | 1 |
Key Issues
EmissionPlan.ts:133: planned blocks hold the livedeferredarray, so a frozen plan's sites change afterbuild(). The snapshot thatregisterPlannedToolchainused to take is gone. The probe and a verified two-line fix are inline.RequirementAggregator.ts:30:record,noteDeferredSiteandunioneach implement the same key→sites fold, and they share an order-dependent quirk. Measurements are inline.EmissionPlan.ts:115(moderate): thePLANNED_BLOCKStype guard misses an optional block, and a key listed with the wrong accessor still compiles. A measured suggestion is inline.
What Looks Good
- Box 2 evidence, reproduced in a
git clone --localatf59644ad. With the composition ignoring the plan (EmissionPlan.requirements(...)→[]),tests/criticalwent 0/8 andtests/bit-indexingwent 12/13; the one failure wasbit-range-const-width-float. Unmutated, they were 8/8 and 13/13, with a clean tree. - Banner order is unaffected. The planned keys now come after every Render record, where before they came ahead of
addGeneratedHelpers' records.describeForBannergoes throughreportable(), which sorts by feature and then key, so the order cannot show. - The new invariant cannot fire on a real run.
generate()always reachesassembleGeneratedOutput, which stores the plan, or it throws intobuildCatchResult. So theemissionPlan()read atTranspiler.ts:1034always has a plan. - CI and Sonar. On
f59644ad, 16 check-runs aresuccessand Deploy Coverage isskipped(gh api repos/jlaustill/c-next/commits/f59644ad…/check-runs, 2026-10-11T15:30:11Z). Sonar reportstotal: 0open issues forpullRequest=1996, gateOK,new_coverage100.0. The SonarCloud Code Analysis check-run completed on that SHA.
Already tracked, so not re-filed: the stale CodeGenState wording in captureEmissionFacts' doc (CodeGenWalker.ts:1840) is #1750, and the keys Render still authors are #1997.
🤖 Automated review by Claude Code
Review of #1996: - decideFloatStaticAssert/decideIrqWrappers copy the facts' site lists, as build() already does for its helper lists, so a site noted after the plan is frozen cannot join a block it did not cause. - RequirementSites.accumulate is the one key-to-sites fold; record, noteDeferredSite and RequirementAggregator.union call it. It drops repeats in the first list for a key too, so union no longer depends on list order. - PLANNED_BLOCKS is keyed by field and read as plan[field]: an optional block without an entry fails to compile, and there is no accessor to mis-read. - ToolchainRequirements.deferred no longer lists clamp helpers. Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
jlaustill
left a comment
There was a problem hiding this comment.
Review Summary
PR: #1996 — fix(#1928): banner reads planned requirements off the emission plan
Reviewed by: Claude (automated review), head 49349834c
Overview
This deletes registerPlannedToolchain, so the plan's block requirements no longer pass through a copy in ToolchainRequirements. CodeGenerator.getToolchainRequirements() becomes the one place the file's list is composed, and both readers (the banner and IFileResult.requirements) go through it. The code change is correct. The two findings below are both stale prose: each describes the old mechanism, and each has a twin this PR already rewrote.
Findings
| Severity | Count |
|---|---|
| Critical | 0 |
| Important | 0 |
| Moderate | 2 |
| Nit | 0 |
Key Issues
TranspileState.requireInclude(lines 143-149) still says "the emitter that finally produces the block" attributes the deferred sites, and calls the two keys "headers that have a claiming emitter". The same wording appears inTranspileState.includes.test.ts:8and:52. After this PR, no emitter claims those sites:captureEmissionFactstakes them and the plan block carries them. The PR rewrote the twin sentence inToolchainRequirements.deferredbut left this one. Details are inline onTranspileState.ts:317.TOOLCHAIN_REQUIREMENTS.ts:7-9still says "the emitting code records that entry's key at the moment it produces the text". That is no longer true forfloat-assert-*andcritical-*. The PR corrected the identical claim inIRecordedRequirement.tsbut not this one. Details are inline onIRecordedRequirement.ts:8.
Both are in scope for this PR. It touches TranspileState.ts and rewrote the matching sentences elsewhere, so leaving these makes the doc set disagree with itself.
Verified (not taken from the PR body)
- CI on
49349834c(check-runs queried 2026-10-11 16:18 UTC): every check in the latest run passed. The cancelled set is a superseded duplicate run. - SonarCloud
pullRequest=1996&statuses=OPEN,CONFIRMED,REOPENED:total: 0, gateOK. Analysis completed on the head SHA at 16:16:55Z. - Local
tsc --noEmiton the head: exit 0. The 23 targeted unit tests (EmissionPlan,RequirementAggregator,RequirementSites, and the newCodeGenerator.getToolchainRequirementscase) pass. - Mutation: composition ignores the plan (
EmissionPlan.requirements(...).slice(0, 0), applied and confirmed by grep).tests/criticalfailed 8/8 andtests/bit-indexingfailed onlybit-range-const-width-float. Restored (grep count 0), the control run passes 8/8 and 13/13 with a clean tree. This matches row 2 of the PR's table. - Mutation: optional
mutationBlock?: IPlannedBlockadded toIEmissionPlan.tscreportsTS2741: Property 'mutationBlock' is missingatEmissionPlan.ts(116,27). The-?mapped type catches the optional case as claimed. - Banner order can't drift. The union puts Render keys recorded after plan-build (
overflow-panic-hosted-libcfromaddGeneratedHelpers) ahead of the plan's keys, where the deleted registrar had put them after.ToolchainRequirementUtils.reportable()sorts by feature then key beforedescribeForBannerreads them, so insertion order never reaches the banner. - The
[...facts.*Sites]copy guards something real.takeDeferredSitesreturns the live array from thedeferredmap without removing it, so a laternoteDeferredSitewould otherwise grow a frozen plan's block. - Every read happens after plan-build.
Transpiler.ts:1034is the only production caller ofgetToolchainRequirements()outside the banner, and it runs aftergenerate().assembleGeneratedOutputhas no return before the plan is stored, so the new invariant can't fire on a normal path. - Doc claims checked:
CodeGenWalkerhas 3recordsites (1593, 3233, 3271).Requires: C++20.appears in real.expected.cppbanners (e.g.tests/structs/struct-const.expected.cpp). #1997 is open and covers the Render-authored keys.
What Looks Good
- The exhaustiveness guard on
PLANNED_BLOCKSis type-level and covers nullable and optional fields. A new planned block can't render with its cost missing from the banner, which was #1143's failure. RequirementSites.accumulatemerges three hand-written key-to-sites folds into one and removes their shared first-list ordering quirk.addUniquegoing private keeps it that way.- The mutation table imitates the defect rather than fitting the guard: dropping a key from the plan's constant goes red, and
bit-indexingstays green as the negative control.
🤖 Automated review by Claude Code
TranspileState.requireInclude, its test comments and the TOOLCHAIN_REQUIREMENTS header still described the emitter claiming the deferred sites / recording every key; planned-block keys are carried by the 2.2 block that decided the text. Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
jlaustill
left a comment
There was a problem hiding this comment.
Review Summary
PR: #1996 — fix(#1928): banner reads planned requirements off the emission plan
Reviewed by: Claude (automated review), head eae03552
Overview
This is the fifth pass. The two commits since the last review (829a957b, eae03552) change only comments: git diff 49349834..eae03552 touches 3 files, and every changed line is a comment. The code answers every thread from the earlier rounds. The change still does what #1928 option (a) asks, and I found nothing that blocks it. There is one nit, inline.
Findings
| Severity | Count |
|---|---|
| Critical | 0 |
| Important | 0 |
| Moderate | 0 |
| Nit | 1 |
Key Issues
- Nit:
EmissionPlan.test.ts:178calls the facts' site lists "1.x-era accumulators". They areToolchainRequirements.deferred's own arrays, written in 2.3 Render. The suggested wording is inline.
Verified (not taken from the PR body)
- CI on
eae03552: 16 check runs reportsuccessandDeploy Coveragereportsskipped(gh api --paginate repos/jlaustill/c-next/commits/eae03552…/check-runs, queried 2026-10-11T18:02Z). - Sonar (
pullRequest=1996&statuses=OPEN,CONFIRMED,REOPENED):total=0, gateOK. - Doc claims at head:
CodeGenWalkerhas 3ToolchainRequirements.recordcalls (1593, 3233, 3271).- The holder pins
ToolchainRequirements.ts:65/:78point atrecorded/deferred. - Nothing under
3-Render/headers/referencesToolchainRequirements. Requires: C++20.appears in real snapshots, for exampletests/adr-014/struct-inferred-every-position.expected.cpp.
- Banner order: the union now puts
overflow-panic-hosted-libc(recorded inaddGeneratedHelpers) ahead of the plan's keys. That cannot reach the output, becauseToolchainRequirementUtils.reportable()sorts by feature and then key beforedescribeForBannerreads the list. - A gap I suspected, measured and found guarded: the IRQ block's four keys (
EmissionPlan.ts:64) and the four#ifarms thatgenerateIrqWrappersemits are separate lists. In a scratch clone ateae03552, I deleted the#elif defined(__AVR__)arm from the template and left the key list alone. A minimalcritical {}program then transpiled with no AVR code, its banner still listedavr-libc, andtsc --noEmitexited 0. However,RequirementProbeInvariant.test.tsdirection A failed 4 cases ("every recorded requirement is present in the output"), and its direction B covers the reverse edit. The two lists are already tied by a test, so I filed nothing. After restoring the template,git diffis empty and the invariant test passes.
What Looks Good
- There is one composition with two readers, behind an asserting accessor. A read before 2.2 builds the plan throws instead of answering "costs nothing".
PLANNED_BLOCKSis keyed by the plan's own type. A new planned block, nullable or optional, fails to compile until it is listed.RequirementSites.accumulateis now the only key→sites fold. That removed the first-list ordering quirk the three copies shared.
🤖 Automated review by Claude Code
Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
|



Summary
The banner no longer reads a copy of 2.2's decision.
registerPlannedToolchaincopied the float-assert and IRQ-wrapper blocks' requirements fromIEmissionPlanintoToolchainRequirements; it is deleted, and the file's requirement list is composed in one place from Render's own records plus the plan's blocks, read off the plan (option (a), owner ruling on #1928).Changes
src/TRANSPILE/TranspileState.ts- holdsemissionPlanOrNullfor the file's turn, cleared in the per-file reset besidedeclarationPlanOrNull.src/TRANSPILE/CodeGenWalker.ts- stores the plan instead of callingregisterPlannedToolchain; the method, its stale "still recorded intoCodeGenState" paragraph and a duplicated doc header are deleted.src/TRANSPILE/2-Plan/EmissionPlan.ts-EmissionPlan.requirements(plan): each planned block's keys with that block's sites, read asplan[field]for each field inPLANNED_BLOCKS, which is keyed by everyIEmissionPlanfield holding a planned block (nullable or optional), so a new block without an entry fails to compile. The decisions copy their sites, so a site noted afterbuild()cannot join a frozen block.src/TRANSPILE/3-Render/codegen/CodeGenerator.ts-getToolchainRequirements()is the single composition:ToolchainRequirements.collect()∪EmissionPlan.requirements(plan). The banner andIFileResult.requirementsboth read it (they were its only two callers before and still are).src/utils/RequirementAggregator.ts-union()extracted;merge()(project level) now builds on it.src/utils/RequirementSites.ts-accumulate()is the one key-to-sites fold;ToolchainRequirements.record,.noteDeferredSiteandRequirementAggregator.unionall call it. It drops repeats in the first list for a key too, sounionno longer depends on list order.IRecordedRequirement,IFileResult.requirements, and theToolchainRequirementscall-site table (CodeGenWalker4 sites → 3).src/TRANSPILE/TranspileState.ts-emissionPlan()asserts the plan exists (likedeclarationPlan()); the composition reads it, so a read before 2.2 builds the plan throws instead of answering "no planned requirements".ToolchainRequirementsheader /deferred/collect()docs (Render's records only;getToolchainRequirements()is the one list), theTranspiler.tsread comment,RequirementSiteshistory note, anddocs/architecture/README.md(ledger read-back example is nowRequires: C++20.fromcpp-designated-initializer; planned-block features are 2.2's; the keys Render still authors against §1's Ownership table cite 2.3 Render authors five toolchain-requirement keys that README §1 gives to 2.2 Plan alone; #1996 deletes the only comment that marked it 'not yet' #1997).EmissionPlan.test.ts(2) forrequirements(),RequirementAggregator.test.ts(2) forunion(),CodeGenWalker.test.ts(1) for the invariant (mutation: restoring the=== null ? []default makes it fail),EmissionPlan.test.ts(1) for sites fixed at the decision,RequirementSites.test.ts(4) andRequirementAggregator.test.ts(1) foraccumulate.Testing
Generated output is unchanged:
tests/critical8/8,tests/bit-indexing13/13,tests/regression/issue-1927-stdint-recording3/3,tests/const-params9/9 pass with no snapshot diff;git statusclean after the runs.Mutations (each asserted to change exactly one line, restored after):
"critical-avr-libc"from the plan'sIRQ_WRAPPER_REQUIREMENTStests/critical0/8,issue-19272/3,const-params8/9 fail on the banner;bit-indexing(no IRQ, control) stays 13/13EmissionPlan.requirements(plan)→[])bit-range-const-width-float(the float-assert block)mutationBlock: IPlannedBlock | nulltoIEmissionPlanandmutationBlock: nulltobuild()(one line each)tsc --noEmit:TS2741: Property 'mutationBlock' is missingmutationBlock?: IPlannedBlocktoIEmissionPlantsc --noEmit:TS2741[...]copy of both blocks'sitesEmissionPlan.test.ts: sites-as-decided case failsaccumulatecopies the first list as givenAlso run locally:
tsc --noEmit, oxlint on changed files, prettier,plan-render-state:check(0 writes, 0 reads; control 3),layout:check,knip, cspell, and the targeted unit suites (58 tests). Full suite left to CI per the push rule.Related Issues
Closes #1928. Part of #1313; clears #1933 §0a's last parallel path. Render-authored keys that §1 gives to 2.2: #1997.
Link to Devin session: https://app.devin.ai/sessions/f8dad7b6b64443cd859a9c5715f51417
Open in Devin Desktop: https://app.devin.ai/desktop/session/f8dad7b6b64443cd859a9c5715f51417?variant=devin
Requested by: @jlaustill