module loader: mark has_loaded before transpiler-cache early return - #35482
module loader: mark has_loaded before transpiler-cache early return#35482robobun wants to merge 2 commits into
Conversation
When the entry point is >= MINIMUM_CACHE_SIZE (4 KiB) and the runtime transpiler cache hits, transpile_source_code_inner returns early before reaching the has_loaded = true assignment. The synchronous_loader block in transpile_file then falls through to the "potentially the main module" Tsx fallback for any subsequent unknown-extension import, so `import x from "./file.c"` is parsed as JS instead of using the file loader on every run after the first. Move the has_loaded assignment to just after the parse-error check so it is set before any of the successful early returns (bytecode cache, empty .cjs/.cts, transpiler-cache hit, async module queue).
|
Updated 4:23 PM PT - Jul 24th, 2026
@autofix-ci[bot], your commit 1443a0d is building: |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Closing in favor of #33371, which is the same fix (move |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // On the warm run the entry point is restored from cache; the `.c` import | ||
| // must still go through the file loader. | ||
| const warm = run("warm run (cache hit)"); | ||
| expect(warm.stderr).not.toContain("Unexpected #include"); |
There was a problem hiding this comment.
🟡 This assertion can never fail: if the warm run printed Unexpected #include, the subprocess would exit non-zero and run() would already have thrown at if (!result.success) throw new Error(...) before this line is reached. The next line's expect(warm.stdout).toBe("file-loader-ok") is the load-bearing check — either drop this line, or have run() return {stdout, stderr, exitCode} and assert the combined object so this becomes reachable.
Extended reasoning...
What the issue is
The assertion expect(warm.stderr).not.toContain("Unexpected #include") is vacuous — there is no execution path on which it can fail. REVIEW.md's test rules say "Every assertion must be able to fail, and assert the strongest invariant. Hunt vacuous patterns."
Why it can't fail
The local run() helper does:
if (!result.success) throw new Error(`${label}: ${stderr}\n${stdout}`);
return { stdout, stderr };So the only way execution reaches the .not.toContain(...) line is if result.success is true (exit code 0). But Unexpected #include is the parse error produced when Bun tries to parse impl.c as TSX via a static ESM import — a fatal module-load error that always causes a non-zero exit. There is no realistic path where Bun exits 0 and prints that parse-error text to stderr.
Step-by-step proof
- Regression case (unfixed build, warm run):
import src from "./impl.c"is parsed as TSX →error: Unexpected #includeon stderr → process exits non-zero →result.successis false →run()throwsError("warm run (cache hit): error: Unexpected #include ..."). The test fails at the throw, before line 257 is evaluated. - Fixed build, warm run: file loader is used → stdout is
file-loader-ok, stderr has no parse error → exit 0 →run()returns → line 257 evaluatesexpect(<no error text>).not.toContain("Unexpected #include")→ trivially passes. - There is no third branch: a static-import parse error cannot coexist with exit 0.
Therefore in every reachable state the assertion passes; it contributes nothing to the test's ability to catch the regression.
Impact
Low. The test as a whole is not vacuous — on regression it still fails via the throw inside run() (which helpfully embeds stderr, so the failure message even contains "Unexpected #include"), and expect(warm.stdout).toBe("file-loader-ok") on the next line is a strict positive assertion. Only this single line is dead weight. It doesn't hurt correctness or coverage; it just reads as though it's guarding something when it isn't.
How to fix
Two options:
- Delete the line. The throw + the
.toBe("file-loader-ok")assertion already cover both the negative and positive contract. - Make it reachable. Change
run()to return{ stdout, stderr, exitCode }without throwing, then assert on a combined object per REVIEW.md's subprocess guidance ("assert a combined { stdout, stderr, exitCode } object"), e.g.expect({ stdout: warm.stdout, exitCode: warm.exitCode }).toEqual({ stdout: "file-loader-ok", exitCode: 0 }). That way a regression produces a diff showing the actual stderr instead of a thrown Error.
| // `Tsx` to `File`; leaving it unset on a cache hit made a second | ||
| // run of `bun entry.ts` parse `import x from "./a.c"` as JS. |
There was a problem hiding this comment.
🟡 The trailing clause of this comment ("leaving it unset on a cache hit made a second run of bun entry.ts parse import x from \"./a.c\" as JS") is past-tense bug history — REVIEW.md's Comments section says that belongs in the PR description, not the source. Keep the first five lines (the placement invariant and the cross-file dependency on transpile_file's synchronous_loader block) and drop just the trailing repro clause. Same applies to the 5-line block at test/cli/run/transpiler-cache.test.ts:227-231, which narrates the historical failure the test name already states as an invariant.
Extended reasoning...
What the finding is
Two of the new comments in this PR mix durable invariant documentation with past-tense bug narration, which REVIEW.md's Comments section explicitly separates:
Comments carry only durable non-obvious content: invariants, ownership/lifetime contracts, SAFETY justifications, deliberate deviations from upstream. No narrating what the code does, no bug history — that belongs in the PR description. Regression tests get exactly one comment: the issue URL.
Location 1 — src/runtime/jsc_hooks.rs:2655-2661
The first ~5 lines are exactly what the rule wants kept: they state the placement invariant ("before any of the early Ok(...) returns below") and the non-local, cross-file dependency ("has_loaded is what flips the loader fallback for unknown extensions — transpile_file's synchronous_loader block — from Tsx to File"). That's durable, non-obvious, and prevents a future refactor from re-hoisting the assignment.
The trailing clause, however, is bug history:
leaving it unset on a cache hit made a second run of
bun entry.tsparseimport x from "./a.c"as JS.
That's a past-tense description of the pre-fix failure mode — precisely what the PR description's Repro and Cause sections already record verbatim. Dropping it loses nothing; the invariant half of the comment already tells a future reader everything they need to keep the assignment where it is.
Location 2 — test/cli/run/transpiler-cache.test.ts:227-231
The 5-line block:
The runtime-transpiler-cache early return used to skip marking the VM's
has_loadedflag, so on a warm run … an ESMimport x from "./file.c"fell into the "potentially the main module" loader fallback and tried to parse the C source as TSX …
is entirely past-tense narration of what was broken. The test name — "cache hit on the entry point does not change loader selection for unknown-extension imports" — already states the invariant, and the two surviving inline comments (line 233 "Entry file large enough to be cached, importing an unknown-extension file" and the pre-warm-run comment "the .c import must still go through the file loader") already cover the non-obvious setup constraints (why 50 KiB of filler, why a .c import, why two runs).
Addressing the counter-argument ("local convention allows explanatory comments")
One reading holds that this file's convention permits multi-line explanatory blocks — the ignores files under the minimum cache size test explains the 4 KiB threshold, the --feature flag test annotates each stage, and the rejects cached module records test documents the on-disk cache-entry byte layout. But each of those is present-tense reference material (a threshold's rationale, a serialized layout, per-stage expected state) — durable content a reader needs to modify the test. None of them narrates a historical bug in the past tense. The distinction REVIEW.md draws is not "long comments vs short comments" but "invariants vs bug history", and the new block falls on the bug-history side ("used to skip", "fell into", "tried to parse").
The counter-argument also notes there is no GitHub issue URL to substitute. That's fine — the rule doesn't require one; it says regression tests get at most the issue URL. With no issue, the setup comments already present at line 233 and before the warm run are sufficient, or the block can be rephrased as a one-line present-tense invariant if the author prefers.
Suggested edit
jsc_hooks.rs — end the comment after "…from Tsx to File.":
// Mark the entry point as loaded as soon as it has parsed
// successfully, before any of the early `Ok(...)` returns below
// (bytecode, empty-cjs, runtime-transpiler-cache hit, etc.).
// `has_loaded` is what flips the loader fallback for unknown
// extensions (`transpile_file`'s `synchronous_loader` block) from
// `Tsx` to `File`.transpiler-cache.test.ts — drop lines 227-231; the test name plus the two remaining inline comments already carry the durable content.
Severity
Nit. This is comment hygiene enforced by an explicit repo rule, but merging as-is causes no behavioral, correctness, or maintenance failure — it's a one-clause trim in the source and a block deletion in the test.
1 participant
What
When the entry point is >= 4 KiB (the runtime transpiler cache's
MINIMUM_CACHE_SIZE) and a cache entry exists for it, an ESMimport x from "./file.c"(or any other unknown extension) is parsed as TSX instead of going through the file loader. The first run of the program works; every subsequent run fails with:Repro
Cause
transpile_source_code_innersetsvm.has_loaded = trueonly after the printer runs, but several success paths return earlier: the bytecode/already-bundled arm, the empty.cjs/.ctsarm, and the runtime-transpiler-cache hit. On a warm run the cache-hit return fires beforehas_loadedis set.transpile_file'ssynchronous_loaderblock useshas_loaded || is_in_preloadto decide betweenLoader::File(unknown extension inside the module graph) andLoader::Tsx("potentially the main module"). Withhas_loadedstillfalse, the.cimport takes theTsxbranch.Fix
Move the
has_loaded = trueassignment to immediately after the parse-error check, so every successful parse of the entry point (cache hit or not) sets it before any early return.Test
Added a case to
test/cli/run/transpiler-cache.test.tsthat runs a >4 KiB entry importing a.cfile twice against an isolated cache dir, asserting the file-loader result on both the cold and warm run and that the cache was populated in between.