Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions docs/architecture/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,9 @@ files at the root: an entry point lives inside the root it starts.
construct the pipeline. Each is entered through its own `index.ts`: the command-line
tool and the library are separate concerns and do not share a starting point.
- **Instrumentation** -- `instrumentation/`. Records facts about the RUN, never about the
program: where an ADR's rule fired, which toolchain features a run required. Written
program: where an ADR's rule fired, which toolchain features a run's Render emitters
required (a 2.2 planned block's features are 2.2's decision, read off the plan, #1928;
that Render still authors the rest, against the Ownership table, is tracked by #1997). Written
from any layer, read once by the host, delivered on the run's result. It authors no fact
the program has, so it is not a state container, and it decides nothing, so it is not a
pass.
Expand All @@ -141,7 +143,8 @@ files at the root: an entry point lives inside the root it starts.

**"Read once at the end" is not the test -- "nothing branches on it" is**, and the
distinction matters because the ledger IS read back. `CodeGenWalker.buildBanner` puts
` * Requires: C11.` into the generated `.c`, and `captureEmissionFacts` carries
` * Requires: C++20.` into a generated `.cpp` that uses a designated initializer
(`cpp-designated-initializer`, recorded where `CodeGenWalker` emits it), and `captureEmissionFacts` carries
`takeDeferredSites(...)` into `IEmissionFacts`. Checked, because a rule stated loosely
reads as violated: `EmissionPlan` branches on `facts.needsFloatStaticAssert` and
`facts.needsIrqWrappers` -- both render state -- and the sites travel beside them as
Expand Down
4 changes: 2 additions & 2 deletions scripts/__tests__/passes-hold-no-mutable-state.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ const INSTRUMENTATION_ROOT = "src/instrumentation";
const INSTRUMENTATION_HOLDERS = [
"src/instrumentation/AdrProvenance.ts:58",
"src/instrumentation/AdrProvenance.ts:59",
"src/instrumentation/ToolchainRequirements.ts:56",
"src/instrumentation/ToolchainRequirements.ts:67",
"src/instrumentation/ToolchainRequirements.ts:65",
"src/instrumentation/ToolchainRequirements.ts:78",
];

/**
Expand Down
28 changes: 28 additions & 0 deletions src/TRANSPILE/2-Plan/EmissionPlan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import type IEmissionFacts from "../../types/IEmissionFacts";
import type IEmissionPlan from "../../types/IEmissionPlan";
import type IPlannedBlock from "../../types/IPlannedBlock";
import type IRecordedRequirement from "../../types/IRecordedRequirement";
import type TRequirementKey from "../../types/TRequirementKey";
import SYSTEM_INCLUDE_TARGETS from "./SYSTEM_INCLUDE_TARGETS";
import CTypeIncludes from "./CTypeIncludes";
Expand Down Expand Up @@ -106,6 +107,33 @@ class EmissionPlan {
return Object.freeze(plan);
}

/**
* One accessor per planned block, keyed by its `IEmissionPlan` field. A
* block added to the plan and not listed here fails to compile, instead of
* rendering with its cost missing from the banner -- #1143's failure.
*/
private static readonly PLANNED_BLOCKS: {
readonly [K in keyof IEmissionPlan as IEmissionPlan[K] extends IPlannedBlock | null
? K
: never]: (plan: IEmissionPlan) => IPlannedBlock | null;
} = {
floatStaticAssert: (plan) => plan.floatStaticAssert,
irqWrappers: (plan) => plan.irqWrappers,
};

/**
* The requirements the plan's blocks carry, each with the sites that asked
* for its block. A view of decisions already in the plan, not a new one.
*/
static requirements(plan: IEmissionPlan): readonly IRecordedRequirement[] {
return Object.values(EmissionPlan.PLANNED_BLOCKS).flatMap((blockOf) => {
const block = blockOf(plan);
return block === null
? []
: block.requirements.map((key) => ({ key, sites: block.sites }));
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
});
}
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.

/**
* Which system headers the implementation file emits, already deduplicated
* against what its own source includes.
Expand Down
27 changes: 27 additions & 0 deletions src/TRANSPILE/2-Plan/__tests__/EmissionPlan.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,33 @@ describe("EmissionPlan (2.2 Plan)", () => {
});
});

describe("requirements -- read off the blocks, never copied (#1928)", () => {
it("lists every planned block's keys with that block's sites", () => {
const site = { sourcePath: "a.cnx", line: 3 };
const plan = EmissionPlan.build(
facts({
needsFloatStaticAssert: true,
needsIrqWrappers: true,
floatAssertSites: [site],
}),
);

expect(EmissionPlan.requirements(plan)).toEqual([
{ key: "float-assert-c11", sites: [site] },
{ key: "critical-arm-gnu", sites: [] },
{ key: "critical-arduino", sites: [] },
{ key: "critical-avr-libc", sites: [] },
{ key: "critical-cmsis-fallback", sites: [] },
]);
});

it("is empty when the plan has no block that costs anything", () => {
expect(EmissionPlan.requirements(EmissionPlan.build(NOTHING))).toEqual(
[],
);
});
});

describe("ISR typedef", () => {
it.each([
[false, true],
Expand Down
17 changes: 12 additions & 5 deletions src/TRANSPILE/3-Render/codegen/CodeGenerator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ import PassByValueAnalyzer from "../../2-Plan/PassByValueAnalyzer";
// Issue #797: Centralized C-style name generation
import type IRecordedRequirement from "../../../types/IRecordedRequirement";
import ToolchainRequirements from "../../../instrumentation/ToolchainRequirements";
import RequirementAggregator from "../../../utils/RequirementAggregator";
import EmissionPlan from "../../2-Plan/EmissionPlan";
import TranspileState from "../../TranspileState";
import StructDefaultInitializer from "./helpers/StructDefaultInitializer";
import StructDefault from "../../../utils/StructDefault";
Expand Down Expand Up @@ -232,14 +234,19 @@ export default class CodeGenerator implements IOrchestrator {
}

/**
* Issue #1143: Snapshot the toolchain requirements recorded during the last
* generate() call.
* Issue #1143: what the last generate() call's output requires.
*
* Must be read before the next file's this.state.reset(), which clears the
* recording map.
* Two sources, each the owner of its facts: what Render recorded at the
* sites that emitted text, and what 2.2's planned blocks carry, read off the
* plan (#1928). The banner and the file result both read this one list.
*
* Must be read before the next file's this.state.reset(), which clears both.
*/
getToolchainRequirements(): readonly IRecordedRequirement[] {
return ToolchainRequirements.collect();
return RequirementAggregator.union([
ToolchainRequirements.collect(),
EmissionPlan.requirements(this.state.emissionPlan()),
]);
}

/**
Expand Down
35 changes: 3 additions & 32 deletions src/TRANSPILE/CodeGenWalker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1650,11 +1650,9 @@ class CodeGenWalker {
this.captureEmissionFacts(sourceIncludeTargets),
);

// The plan decided WHICH toolchain capabilities these helpers need and at
// which sites; registering them is part of that decision, not part of
// formatting. `addGeneratedHelpers` used to do it while pushing the text,
// which made the renderer a writer of state something downstream reads.
CodeGenWalker.registerPlannedToolchain(plan);
// #1928: the banner and the file result read the planned blocks'
// requirements off the plan itself, so nothing copies them anywhere.
this.host.state.emissionPlanOrNull = plan;

// 2.3 Render: format the plan. These two decide nothing.
this.addAutoIncludes(output, plan);
Expand Down Expand Up @@ -1887,9 +1885,6 @@ class CodeGenWalker {
return match ? match[1] : null;
}

/**
* Add generated helpers (static asserts, IRQ wrappers, typedefs, etc.).
*/
/**
* Print the deferred blocks and helpers the plan decided on.
*
Expand All @@ -1898,31 +1893,7 @@ class CodeGenWalker {
* the two cannot disagree -- #1143 kept them in step by computing both from
* one ternary at this site; the plan keeps them in step by making them two
* fields of one record, and the ternary is gone from here.
*
* Requirements are still recorded into `CodeGenState` rather than read off
* the plan by the banner, because the banner also carries requirements this
* plan does not yet own (the mode baseline, C++ initializer forms, atomics).
* Recording a decision someone else made is transcription, not derivation.
*/
/**
* Register the toolchain capabilities the plan's helper blocks need.
*
* Separated from `addGeneratedHelpers` so that rendering a block and
* declaring what the block requires are not the same act: the plan already
* holds both the keys and the sites, and a renderer that writes them is a
* renderer making state visible downstream. Order is unchanged -- this runs
* immediately after the plan is built, and nothing between reads a toolchain
* requirement.
*/
private static registerPlannedToolchain(plan: IEmissionPlan): void {
for (const block of [plan.floatStaticAssert, plan.irqWrappers]) {
if (block === null) continue;
for (const key of block.requirements) {
ToolchainRequirements.record(key, block.sites);
}
}
}

private addGeneratedHelpers(output: string[], plan: IEmissionPlan): void {
const floatAssert = plan.floatStaticAssert;
if (floatAssert !== null) {
Expand Down
25 changes: 25 additions & 0 deletions src/TRANSPILE/TranspileState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import type ITypeBindingDeps from "../types/ITypeBindingDeps";
import type IProgram from "../types/IProgram";
import type ITypingContext from "../types/ITypingContext";
import type IDeclarationPlan from "../types/IDeclarationPlan";
import type IEmissionPlan from "../types/IEmissionPlan";
import type IFunctionSignature from "../types/IFunctionSignature";
import invariant from "../utils/invariant";
import emittedLocalName from "../utils/emittedLocalName";
Expand Down Expand Up @@ -309,6 +310,14 @@ class TranspileState {
* silently-wrong default.
*/
declarationPlanOrNull: IDeclarationPlan | null = null;
/**
* 2.2 Plan's emission decisions for the file being generated.
*
* Held for the rest of the file's turn so the requirements its planned
* blocks carry are read off the plan, never copied into
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
* `ToolchainRequirements` (#1928). Null until the declarations have rendered.
*/
emissionPlanOrNull: IEmissionPlan | null = null;
/** ADR-013: Track function parameter const-ness for call-site validation */
functionSignatures: Map<string, IFunctionSignature> = new Map();
/** Callback field types: "Struct.field" -> callbackTypeName */
Expand Down Expand Up @@ -363,6 +372,21 @@ class TranspileState {
);
return plan;
}
/**
* 2.2 Plan's emission decisions, asserted present.
*
* Same reason as `declarationPlan()`: answering "no planned block costs
* anything" before the plan exists would drop the interrupt-masking and C11
* lines from the banner without failing.
*/
emissionPlan(): IEmissionPlan {
const plan = this.emissionPlanOrNull;
invariant(
plan !== null,
"2.2 Plan builds the emission plan before its requirements are read",
);
return plan;
}
/**
* Compute unmodified parameters for all functions on-demand.
* Returns a map of function name -> Set of parameter names NOT modified.
Expand Down Expand Up @@ -1219,6 +1243,7 @@ class TranspileState {
this.callbackTypeReferences = new Set();
this.publicCallbackTypeReferences = new Set();
this.declarationPlanOrNull = null;
this.emissionPlanOrNull = null;
this.usedClampOps = new Set();
this.usedSafeDivOps = new Set();
this.usedCastHelpers = new Set();
Expand Down
8 changes: 8 additions & 0 deletions src/TRANSPILE/__tests__/CodeGenWalker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17715,3 +17715,11 @@ describe("CodeGenWalker", () => {
});
});
});

describe("CodeGenerator.getToolchainRequirements (#1928)", () => {
it("refuses to answer before 2.2 has built the emission plan", () => {
expect(() => new CodeGenerator().getToolchainRequirements()).toThrow(
"2.2 Plan builds the emission plan before its requirements are read",
);
});
});
24 changes: 13 additions & 11 deletions src/cli/Transpiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1018,17 +1018,19 @@ class Transpiler {
this.headerEmissionFactsByPath.set(sourcePath, headerFacts);
}

// Issue #1143: read after header-facts CAPTURE, and before the next
// file's TranspileState.reset() clears the recording map. This covers a
// requirement that capturing a header's facts triggers (e.g. through
// HeaderEmissionCapture.convertToHeaderSymbols) -- it does NOT cover one the RENDER might
// trigger, since #1323 moved rendering to Stage 5.5, after every file's
// requirements have already been read here and reset() has run N times.
// Currently unreachable rather than wrong: TranspileState.requireToolchain
// has no caller under output/headers/, so no render path records one --
// but this read does not guarantee that stays true, and #1143 is
// precisely the bug class where an ordering assumption like that broke
// under a later refactor.
// Issue #1143: the file's one requirement list -- Render's records plus
// what the emission plan's blocks carry (#1928). Read after header-facts
// CAPTURE, and before the next file's TranspileState.reset() clears both.
// This covers a requirement that capturing a header's facts triggers
// (e.g. through HeaderEmissionCapture.convertToHeaderSymbols) -- it does
// NOT cover one the header RENDER might trigger, since #1323 moved
// rendering to Stage 5.5, after every file's requirements have already
// been read here and reset() has run N times. Currently unreachable
// rather than wrong: nothing under TRANSPILE/3-Render/headers/ calls
// ToolchainRequirements, so no header render records one -- but this
// read does not guarantee that stays true, and #1143 is precisely the
// bug class where an ordering assumption like that broke under a later
// refactor.
const requirements = this.codeGenerator.getToolchainRequirements();

return this.buildSuccessResult(
Expand Down
39 changes: 27 additions & 12 deletions src/instrumentation/ToolchainRequirements.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
/**
* Issue #1143: what the generated output for one file costs the reader's
* toolchain, recorded by each emitter at the moment it produces the text.
* toolchain, recorded by each 2.3 Render emitter at the moment it produces the
* text.
*
* Consumers read this; nothing re-derives it. PR #1141 failed by re-deriving --
* its `#error` guard keyed on `usedClampOps.size > 0` while the emission it
* described keyed on the template family, so 96 snapshots shipped a guard for a
* builtin they did not contain. Recording at the emission site is what makes a
* consumer unable to disagree with what was emitted.
* This holds Render's records only. The requirements 2.2's planned blocks
* carry (float asserts, IRQ wrappers) are read off the emission plan, never
* recorded here (#1928). The file's one list is
* `CodeGenerator.getToolchainRequirements()`, which unions the two; consumers
* read that, and nothing re-derives it. That Render still authors these keys
* at all, where §1's Ownership table gives toolchain requirements to 2.2, is
* tracked by #1997, not settled as the design.
*
* PR #1141 failed by re-deriving -- its `#error` guard keyed on
* `usedClampOps.size > 0` while the emission it described keyed on the template
* family, so 96 snapshots shipped a guard for a builtin they did not contain.
* Recording at the emission site is what makes a consumer unable to disagree
* with what was emitted.
*
* ## Why this is instrumentation (#1452)
*
Expand All @@ -24,14 +33,14 @@
* genuinely cross-pass, written at 17 `record` sites spanning 2.1 Analyze and
* 2.3 Render and read once at the end of the run. This module claims no such
* exemption, because it is not cross-pass state at all. Measured on
* `e16b5f03`, every write is inside 2.3 Render:
* `a1b3e92b`, every write is inside 2.3 Render:
*
* | Write site | Pass |
* | ------------------------------------- | ------------- |
* | `CodeGenWalker` (4 sites) | 2.3 Render |
* | `CodeGenWalker` (3 sites) | 2.3 Render |
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
* | `CodeGenerator.applyEffects` | 2.3 Render |
* | `HelperGenerator` | 2.3 Render |
* | `CodeGenState.requireInclude` | 2.3 Render |
* | `TranspileState.requireInclude` | 2.3 Render |
*
* and every read -- `takeDeferredSites` at plan-build, `collect` for the banner
* and for the result -- happens inside the same `generate()` call for the same
Expand Down Expand Up @@ -61,8 +70,10 @@ class ToolchainRequirements {
* the deferral ("irq_wrappers", "float_static_assert").
*
* Kept separate from `recorded` because at request time the code has not been
* emitted yet, so there is nothing to record a requirement for. The emitter
* claims these sites when it actually produces the block.
* emitted yet, so there is nothing to record a requirement for.
* `captureEmissionFacts` takes these sites into `IEmissionFacts`, and the
* plan block that decides the emission carries them from there (#1928); they
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
* never come back to `recorded`.
*/
private static deferred: Map<string, IRequirementSite[]> = new Map();

Expand Down Expand Up @@ -112,7 +123,11 @@ class ToolchainRequirements {
}

/**
* Snapshot what this file's generated output requires.
* Snapshot what Render recorded for this file.
*
* Not the file's whole list: the planned blocks' requirements live on the
* emission plan. `CodeGenerator.getToolchainRequirements()` is the one list,
* and its only reader of this (#1928).
*
* Must be read before the next file's reset, which clears the recording map.
*/
Expand Down
3 changes: 2 additions & 1 deletion src/types/IFileResult.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ interface IFileResult {

/**
* Issue #1143: Toolchain requirements this file's generated output actually
* carries, recorded by the emitters that produced the text.
* carries: recorded by the emitters that produced the text, plus what the
* emission plan's blocks carry (#1928).
*/
requirements?: readonly IRecordedRequirement[];
}
Expand Down
5 changes: 3 additions & 2 deletions src/types/IRecordedRequirement.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ import type TRequirementKey from "./TRequirementKey";
* Issue #1143: A requirement that generated output actually carries, together
* with every source location that incurred it.
*
* Recorded by the emitter at the moment the text is produced, so a consumer can
* never disagree with what was emitted.
* Recorded by the emitter at the moment the text is produced, or carried by the
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
* 2.2 plan block that decided the text (#1928), so a consumer can never
* disagree with what was emitted.
*/
interface IRecordedRequirement {
/** Registry key; look up the details in TOOLCHAIN_REQUIREMENTS. */
Expand Down
Loading
Loading