Skip to content

module loader: mark has_loaded before transpiler-cache early return #35482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 2 commits into from
+48 −5
Closed

module loader: mark has_loaded before transpiler-cache early return #35482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f5bc8d
module loader: mark has_loaded before transpiler-cache early return
robobun Jul 24, 2026
1443a0d
[autofix.ci] apply automated fixes
autofix-ci[bot] Jul 24, 2026
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
17 changes: 12 additions & 5 deletions src/runtime/jsc_hooks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2652,6 +2652,18 @@ fn transpile_source_code_inner(
return Err(crate::Error::ParseError);
}

// 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`; leaving it unset on a cache hit made a second
// run of `bun entry.ts` parse `import x from "./a.c"` as JS.
Comment on lines +2660 to +2661

@claude claude Bot Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.ts parse import 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_loaded flag, so on a warm run … an ESM import 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.

if is_main {
// SAFETY: per fn contract — `jsc_vm` is the live per-thread VM.
unsafe { (*jsc_vm).has_loaded = true };
}

let source = &parse_result.source;

// Raw JSON: hand the source bytes straight to JSC.
Expand Down Expand Up @@ -3050,11 +3062,6 @@ fn transpile_source_code_inner(
print_result?;
}

if is_main {
// SAFETY: per fn contract — `jsc_vm` is the live per-thread VM.
unsafe { (*jsc_vm).has_loaded = true };
}

// `module_info.asDeserialized()`: finalize the
// printer-filled record into the FFI shape consumed by C++
// (freed by C++ `~SourceProvider` via
Expand Down
36 changes: 36 additions & 0 deletions test/cli/run/transpiler-cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,42 @@ describe("transpiler cache", () => {
expect(b.stdout == "production 5");
expect(newCacheCount()).toBe(0);
});
test("cache hit on the entry point does not change loader selection for unknown-extension imports", () => {
// The runtime-transpiler-cache early return used to skip marking the VM's
// `has_loaded` flag, so on a warm run (>= MINIMUM_CACHE_SIZE entry file
// served from cache) an ESM `import x from "./file.c"` fell into the
// "potentially the main module" loader fallback and tried to parse the
// C source as TSX instead of using the file loader.
writeFileSync(join(temp_dir, "impl.c"), "#include <stdio.h>\nint main() { return 0; }\n");
// Entry file large enough to be cached, importing an unknown-extension file.
const code =
'import src from "./impl.c";\nconsole.log(typeof src === "string" && src.endsWith("impl.c") ? "file-loader-ok" : src);\n';
writeFileSync(join(temp_dir, "entry.ts"), code + "//" + Buffer.alloc(50 * 1024, "x").toString() + "\n");

const run = (label: string) => {
const result = Bun.spawnSync({
cmd: [bunExe(), "entry.ts"],
cwd: temp_dir,
env,
});
const stderr = result.stderr.toString();
const stdout = result.stdout.toString().trim();
if (!result.success) throw new Error(`${label}: ${stderr}\n${stdout}`);
return { stdout, stderr };
};

const cold = run("cold run");
expect(cold.stdout).toBe("file-loader-ok");
expect(existsSync(cache_dir)).toBeTrue();
expect(newCacheCount()).toBe(1);

// 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");

@claude claude Bot Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

  1. Regression case (unfixed build, warm run): import src from "./impl.c" is parsed as TSX → error: Unexpected #include on stderr → process exits non-zero → result.success is false → run() throws Error("warm run (cache hit): error: Unexpected #include ..."). The test fails at the throw, before line 257 is evaluated.
  2. Fixed build, warm run: file loader is used → stdout is file-loader-ok, stderr has no parse error → exit 0 → run() returns → line 257 evaluates expect(<no error text>).not.toContain("Unexpected #include") → trivially passes.
  3. 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.

expect(warm.stdout).toBe("file-loader-ok");
expect(newCacheCount()).toBe(0);
});
test("--feature flag invalidates cache", () => {
// feature() can only appear in an if/ternary, so wrap it
const code = `import { feature } from "bun:bundle";\nif (feature("SUPER_SECRET")) console.log("enabled"); else console.log("disabled");`;
Expand Down
Loading