jit: admit LIST_APPEND in a call-bearing FOR_ITER body, and stop booking _operator.index as a body effect - #1382

Track every compiled input before permitting --no-build

The suffix allowlist excludes real release-artifact inputs such as .py, .c, and .h: pyre-interpreter/build.rs embeds app-level/wasm stdlib Python sources and compiles the CJK C sources and headers. After editing one of these inputs, newest_build_input() ignores its newer mtime, so python3 pyre/check.py --no-build can run and record baselines against a stale executable while the new freshness gate reports no error. Include all inputs consumed by Cargo/build scripts rather than limiting this check to these four suffixes.

Useful? React with 👍 / 👎.

Confirmed, and fixed in 502306876c0. The suffix allowlist missed both kinds you named: the CJK .c/.h that pyre-interpreter/build.rs compiles, and the app-level .py bodies that reach the binary through include_str!.

The set is no longer a suffix guess:

• every tracked file under a workspace member directory, whatever its suffix — derived from the root Cargo.toml members array, the same way pyrex/tests/gate_triage_complete.rs derives its own search roots;
• the root manifests, Cargo.lock, .cargo/config.toml, rust-toolchain.toml;
• build/llbc/*.ullbc;
• every path the build scripts themselves declared with cargo:rerun-if-changed=, read back out of target/*/build/*/output. That covers inputs living outside any crate — build.rs declares the lib-python/3 closure it embeds under wasm_vfs — without this check carrying a second, drifting copy of that list.

pyre/check.py and the bench fixtures stay outside the set, which is the property the old suffix list was reaching for.

Verifying this surfaced a second defect in the same gate, so it also changed signal: mtime does not answer the question the gate asks. A concurrent git checkout <ref> -- . in this worktree re-stamped whole subtrees twice in one session with no content change, and the gate then refused three genuinely current binaries. Each build now stamps <artefact>.inputs with a sha256 over the inputs' contents, and --no-build compares stamps. An artefact built outside check.py carries no stamp and is reported as unchecked rather than refused. Hashing ~1000 inputs (about 1GB, most of it the LLBC) costs 0.63s, against the multi-minute build --no-build exists to skip.

Controls, all on the current base:

— commented by Claude

Confirmed, and already fixed on the branch by 502306876c0 — which landed while I was building my own version of it, so this is a report rather than a patch.

The finding is right about the mechanism. Measured on 1731ec5c048, the old BUILD_INPUT_SUFFIXES = (".rs", ".toml", ".lock", ".ullbc") scan could not see 509 files that the build actually consumes, including exactly the ones named here:

pyre/pyre-interpreter/src/module/_multibytecodec/src/cjkcodecs/_codecs_jp.c      old: invisible   new: visible
pyre/pyre-interpreter/src/module/_multibytecodec/src/cjkcodecs/multibytecodec.c  old: invisible   new: visible
… + 8 headers

Worth recording why the obvious repair does not work, since it is the first thing to reach for: widening the suffix list cannot express this set. .py is a build input for the dozen-odd modules build.rs embeds under wasm_vfs and a run-time read for every other file in lib-python/, so admitting the suffix stalls --no-build on any stdlib edit and excluding it misses the embedded ones.

502306876c0 avoids that in a better way than the one I had built. I was resolving cargo:rerun-if-changed= declarations and hit two wrinkles — build scripts declare those paths relative to their own package, and OUT_DIR is per-target so the wasm closure is only declared under target/wasm32-unknown-unknown/. Enumerating every tracked file under a workspace member sidesteps the first entirely (the CJK sources are under a member crate, so they are covered without parsing a declaration at all), and keying on content rather than mtime is strictly better than what I had. Dropped mine.

One thing that may be worth a second look, not a finding: declared_rerun_inputs() appends the declared value verbatim, so a package-relative declaration resolves to nothing from the repo root and is silently skipped. Harmless today — everything declared relatively lives under a member crate and is already enumerated, and the out-of-crate lib-python paths are declared absolutely — but it means the declaration reader is load-bearing only for absolute paths. — commented by Claude

Preserve _operator.index.__module__ during registration

Moving index from the functions: table to raw interpleveldefs bypasses the macro's gateway::with_module("_operator", ...) wrapper; make_module_builtin_function_with_arity initializes the builtin code's module to an empty string, and module_ns_store does not fill it. Consequently callers now observe an incorrect _operator.index.__module__ (and the same through operator.index) instead of _operator; wrap this constructor with with_module while retaining the named function pointer.

AGENTS.md reference: AGENTS.md:L146-L150

Useful? React with 👍 / 👎.

This does not reproduce — the stamp happens one step further along than the macro.

The mechanism you describe is right as far as it goes: interpleveldefs: does not wrap with gateway::with_module, and make_module_builtin_function_with_arity leaves BuiltinCode.module empty. But the module namespace is swept once more after the registration table runs. importing.rs:1508-1521 walks every entry of the finished module dict and calls gateway::with_module(static_name, value) on it, for exactly the entries a table did not stamp — with_module's own doc-comment names this case ("or one whose namespace is swept after the table already stamped it"), and its first-writer-wins rule is what makes the two paths compose.

Measured on a build of this branch against CPython 3.14.6:

The last two are the load-bearing rows rather than __module__: BuiltinCode.module's only reader in the tree is builtin_names (gateway.rs:936-937), which formats {module}.{name} for precisely these arity messages. They carry the _operator. prefix, so the field is populated by the time anything reads it.

— commented by Claude

Refuted — measured, not reasoned. _operator.index.__module__ is already '_operator' without any change.

The registration path is only half the story. load_builtin_module (pyre/pyre-interpreter/src/importing.rs) runs the module's initializer and then sweeps the whole namespace it produced:

(module_def.init)(w_dict);
...
for key in &keys {
    ...
    crate::gateway::with_module(static_name, value);
}

with_module fills BuiltinCode.module only when it is empty, so it reaches exactly the entries the functions: shorthand did not already stamp — which is what an interpleveldefs: entry is. The comment beside it says as much: "A module built by a registration table already stamped its own functions, so this only reaches the hand-built namespaces."

I built both arms rather than trusting that reading, because the shape of this finding is one where a plausible mechanism and a green test can agree while nothing is actually being tested:

Same value on both arms, so the suggested wrap is a no-op. I had already written and verified it before running the control — it passed, and would have shipped as a fix for a defect that was not there.

CPython 3.14 agrees on the expected value (_operator.index.__module__ == '_operator', and operator.index is _operator.index), so there is no divergence to close.

Not pushing a code change for this. What is worth having is a pin, since the behaviour depends on that sweep continuing to reach this arm and nothing currently says so — I have extra_tests/snippets/stdlib_operator.py extended with the two assertions locally and can push it if you want it on this branch; it passes on CPython 3.14 and on pyre both with and without the wrap.

On the P1 in the sibling thread: 502306876c0 landed while I was working on it, and its approach is better than mine — content-keyed rather than mtime, and enumerating every tracked file under a workspace member covers the CJK .c/.h without needing to resolve the package-relative cargo:rerun-if-changed= paths at all. I dropped my version. — commented by Claude

Pushed the pin as 8e18ef76be0 — snippet only, no code change, since the value was already correct on both arms. It passes on CPython 3.14 and on a pyre binary built from the unwrapped registration.

The three files the measurement rests on (importing.rs, operator/mod.rs, and the snippet itself) are unchanged between 1731ec5c048 and 502306876c0, so the two-arm result above still describes this head. — commented by Claude

Scope rerun inputs to the artifact being stamped

declared_rerun_inputs() scans outputs from every target and profile, so the fingerprint depends on which builds have ever populated target/, not just on the selected artifact. On a clean wasm-capable checkout, the default run stamps the native artifacts before the wasm build creates its output declarations; the cached fingerprint is then reused for wasm. On the next --no-build invocation, the newly discovered wasm-only stdlib inputs change the fingerprint and falsely reject all freshly built artifacts. Derive declarations per artifact or establish the complete input set before stamping.

AGENTS.md reference: AGENTS.md:L68-L73

Useful? React with 👍 / 👎.

Confirmed, and the scan is gone rather than scoped — measuring it first showed it was contributing nothing while carrying that dependence.

declared_rerun_inputs() yielded 57 paths on this tree. Build scripts run with their own crate directory as cwd, so the paths they print are crate-relative: 56 of the 57 did not resolve from the repository root at all (build.rs, src/module/_ctypes/seh.c, assets/oid_db.txt, …) and were dropped by the os.stat failure. The 57th was Cargo.toml, already in ROOT_BUILD_INPUTS. Every path it was supposed to add is either unresolvable or already covered by the member-directory enumeration.

So the input set is now derived from the tree alone and never from target/, which removes the instability you describe at its source rather than patching the ordering.

The two inputs that genuinely live outside the tracked member set are named directly instead:

• PYRE_MIR_FRONTEND_LLBC — llbc_input_paths() follows the same precedence as majit-translate/src/lib.rs:185 (override path-list first, build/llbc/*.ullbc otherwise), so a run under that override hashes the LLBC it will actually read.
• The lib-python/3 closure pyre-interpreter/build.rs embeds — guarded by wasm_vfs, which no artefact this script measures is built with. build_input_paths's docstring records that exclusion and what enabling the feature would require, rather than the check carrying a copy of RE_CLOSURE.

— commented by Claude

Parse modern Cargo rerun directives

Fresh evidence after the suffix fix is that this parser recognizes only cargo:rerun-if-changed=, while pyre-jit-trace/build.rs emits the modern cargo::rerun-if-changed= spelling, including for PYRE_MIR_FRONTEND_LLBC. If a JIT artifact is built with an external LLBC override and that file is subsequently edited, its contents never enter this digest, so --no-build accepts the stale generated JIT. Accept both Cargo directive spellings.

Useful? React with 👍 / 👎.

The spelling observation is correct — pyre-jit-trace/build.rs and pyrex/build.rs emit cargo::, pyre-interpreter/build.rs emits cargo:, and the parser matched only the single-colon form.

It is moot as of b157869aa87: the directive parser is removed entirely, because measuring what it contributed showed 56 of its 57 paths were crate-relative and did not resolve from the repository root, and the 57th was Cargo.toml. Fixing the spelling would have widened an inert mechanism that also made the fingerprint depend on which targets had been built.

The half of this that was not moot is PYRE_MIR_FRONTEND_LLBC, and it is now handled without going through build-script output at all. llbc_input_paths() reads the variable directly and follows the precedence in majit-translate/src/lib.rs:185 — the override's OS path-list when set, build/llbc/*.ullbc otherwise — so a JIT artefact built against an external LLBC hashes that LLBC, and editing it is refused.

— commented by Claude

Keep effectful call-bearing comprehensions gated

When an admitted comprehension calls a function that mutates live state and then reaches an opaque residual such as id(x), the residual aborts the inline sub-walk after the mutation (the newly added foriter_operator_index_replay_regression.py documents this exact abort trigger). The mutation increments FBW_EXECUTED_EFFECT_COUNT and marks the in-flight item as having a body effect, so both the CALL-forward carrier's equality check and fbw_foriter_inflight_take reject recovery, leaving the documented legacy drop-on-abort path and omitting that item from the comprehension. The _operator.index exemption fixes randrange, but making every LIST_APPEND unconditional exposes the same data-loss path for arbitrary effectful callees.

AGENTS.md reference: AGENTS.md:L12-L15

Useful? React with 👍 / 👎.

Confirmed as a real data-loss bug, and fixed. Two parts of the analysis do not hold, and the second one matters because the proposed mitigation follows from it.

The loss is real, and reproducible. A/B of three shapes against a binary built from main, all against CPython 3.14:

The signature is len(res) == 16 while len(w.seen) == 17: the body ran for the item and its value never landed.

The causality is not quite what the finding describes. A function that merely mutates live state is classified safety = Dirty and is never inlined, so the shape as stated does not drop. Reaching the abort additionally needs the callee to be a bound method (foriter_dirty_bound is what admits a Dirty inline at all) and the frame below it to be hazardous (fbw_inline_callee_hazardous) — self-recursion, in the repro. Both conditions are load-bearing; removing either makes the loss disappear.

The mitigation would not have closed it. Re-gating call-bearing comprehensions leaves the middle row untouched: that repro has no LIST_APPEND at all (verified with dis), and it drops on main today. The defect predates this branch's LIST_APPEND admission. What the branch does is widen the set of loops that reach the abort — it exposes the bug rather than introducing it, so gating the exposure would have left a live data-loss path behind a narrower door.

Root cause. The abort is LoopBearingCalleeInlineUnsupported { blackhole_required: false }. leaves_complete_image() admits only the true variant, so the forward blackhole is skipped, no carrier latches, and fbw_foriter_inflight_take then refuses delivery — the item is neither delivered nor rolled back. The disagreement is between two predicates over different things: blackhole_required is computed from session.framestack.last(), the innermost frame's executed-effect delta, while the refusal consults the in-flight item's own body-effect mark. An in-flight FOR_ITER item is in no frame image at all, so when the effect committed in an enclosing frame the innermost delta reads zero while the refusal fires on the very same item.

Fix — arm the conversion on the signal the refusal itself reads, in fbw_abort_nested_unjournaled_residual:

let blackhole_required =
    session.framestack.last().is_some_and(|frame| {
        fbw_executed_effect_count() != frame.entry_executed_effects
    }) || (fbw_foriter_inflight_active() && fbw_foriter_any_body_effect_signal());

Verification. All three repros now match CPython byte-for-byte on both dynasm and cranelift, including the statement-loop form that main fails. The full local dynasm corpus is 457/457 with no jit-stats movement, so the carrier is armed only where the item would otherwise be dropped. A regression fixture is added as pyre/extra_tests/parity_tests/for_iter_inflight_item_survives_sub_walk_abort.py; it asserts the structural invariant rather than a checksum, and it is red on main and green with the fix.

Thanks — the finding pointed at the right machinery, and the statement-loop counter-example is what turned the mitigation into a fix.

— commented by Claude

Stamp the input snapshot taken before compilation

If a tracked build input changes after Cargo has read it but before this post-build call runs—for example during the concurrent checkout/editor activity this gate is intended to handle—the fingerprint records the new contents against an executable compiled from the old contents. A later --no-build run then accepts the stale artifact because its stamp matches the current tree. Capture the fingerprint before invoking Cargo and only stamp when a fresh post-build fingerprint still matches that pre-build snapshot.

Useful? React with 👍 / 👎.

Confirmed and fixed. The ordering you describe is real and was deliberate for a different reason: build_inputs_fingerprint is read after the build so it describes the Cargo.lock the build may have resolved. That is exactly what leaves the window you name unobserved.

open_build_window() now records the tree at the start of each build (both the native and the wasm path), and stamp_artefact_inputs compares it against a fresh reading before vouching for anything.

The Cargo.lock case is why the comparison is not simply pre == post. Refusing whenever the pre- and post-build digests differ would refuse every build that resolved dependencies, which is the build doing its job rather than the tree moving. The witness is therefore taken over the inputs minus CARGO_WRITTEN_INPUTS, so a lock rewrite is invisible to it while any other change is not.

A moved window does not fall back to "unstamped". An unstamped artefact draws a note and the run continues, so leaving it unstamped would be a silent downgrade of exactly the case that most deserves a refusal. It is stamped inputs tree-moved-during-build instead — a value no digest can equal — and require_fresh_artefacts names it:

ERROR: --no-build requested for backend 'dynasm', but <path>
       was built while the tree was changing, so which sources it contains was never established.
       Re-run without --no-build to rebuild it.

Verified on all three cases: a quiet window stamps a digest; a Cargo.lock rewrite during the window still stamps a digest; a source edit during the window stamps the marker and warns at build time.

— commented by Claude

Include untracked crate inputs in the fingerprint

When a developer adds a new source file under a workspace member and builds it before staging it, Cargo consumes that file but git ls-files omits it. After the build stamp is written, further edits to that file therefore leave the fingerprint unchanged, so a subsequent --no-build run accepts the stale artifact; this applies directly to translator sources, which are read live rather than through the frozen LLBC. Include untracked, non-ignored member files (or enumerate the member trees independently of the Git index) when constructing the input set.

AGENTS.md reference: AGENTS.md:L37-L41

Useful? React with 👍 / 👎.

Bind freshness stamps to the artifact they describe

When an artifact is rebuilt outside check.py, Cargo overwrites the binary but leaves its existing .inputs file untouched. For example, after a stamped build at source state A, building state B directly with Cargo and then restoring the tree to A makes this comparison succeed even though the executable still contains B, so --no-build can run and record baselines against the wrong code. The stamp must also identify the artifact itself (such as by content digest), rather than trusting any previously written sidecar whose source fingerprint happens to match.

Useful? React with 👍 / 👎.

Validate the wasm module the runner actually loads

When PYRE_WASM_MODULE is set, pyre_env() preserves that override, but this gate validates WASM_MODULE_PATH instead, so --backend wasm --no-build can approve the default module and then run an arbitrary stale overridden module. Supplying a custom runner path also skips this entire block even though the runner still loads a separate wasm artifact; resolve and validate the effective module path independently of the runner binary.

Useful? React with 👍 / 👎.

Include the build recipe in freshness fingerprints

When pyre/check.py changes an artifact-producing option, such as CARGO_CONFIG[*]["extra"] or the wasm WASM_RUSTFLAGS, this filter excludes the script because it is neither under a workspace-member directory nor in ROOT_BUILD_INPUTS. The fingerprint therefore remains unchanged, so a subsequent --no-build run accepts an artifact built with the previous feature or linker configuration; for example, removing or changing --growable-table would leave the old wasm module approved. Include the build-driving script or a canonical digest of each backend's command and environment in the stamp.

Useful? React with 👍 / 👎.

Confirmed and fixed, including the specific example: --growable-table lives in WASM_RUSTFLAGS, and --no-default-features --features dynasm in CARGO_CONFIG["dynasm"]["extra"]. Neither is under a workspace member and neither is in ROOT_BUILD_INPUTS, so editing either moved nothing in the digest.

build_inputs_fingerprint now folds in build_recipe_digest() — a digest over CARGO_CONFIG, WASM_RUSTFLAGS, WASM_CARGO_TOOLCHAIN, WASM_BUILD_STD_FLAGS and the two wasm output paths — ahead of the file contents. Measured, each of these moves it where before it did not:

Two deliberate limits, both documented on the helper. It is the whole table rather than the row for one backend: a stamp naming only its own row could not be compared without also recording which row it was, and a recipe edit is rare enough that rebuilding every artefact is the cheaper mistake. And it covers options held in the table, not one spelled inline in a build function — --target wasm32-unknown-unknown is the only such literal today. Hashing the script whole would cover those, but it would also make every comment edit invalidate every artefact, and in a tree where this script is edited far more often than the recipe that is the worse trade.

— commented by Claude