fix(bundler): collect external imports from converted stmts for bytecode ModuleInfo#27456
fix(bundler): collect external imports from converted stmts for bytecode ModuleInfo#27456robobun wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughCollects external import records from converted statements during parallel printing: adds ImportInfoCollected, threads collected imports through DeclCollector and CompileResult, updates post-processing to build ModuleInfo from compile results (avoiding phantom imports), and adds a regression test; minor TLS hostname getter fix. Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bundler/linker_context/postProcessJSChunk.zig`:
- Around line 147-155: The code indexes arrays using import_info.source_index
without bounds checks; before using import_info.source_index to access
all_import_records, source_import_records, and c.graph.ast.items(.parts) ensure
the index is within each container's .len (e.g., check import_info.source_index
< all_import_records.len and < c.graph.ast.items(.parts).len) and skip/continue
if out of range, and apply the same guard for the other similar block around the
code referenced at lines ~193-197 to prevent OOB access when metadata is stale.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/bundler/bundle_v2.zigsrc/bundler/linker_context/generateCodeForFileInChunkJS.zigsrc/bundler/linker_context/generateCompileResultForJSChunk.zigsrc/bundler/linker_context/postProcessJSChunk.zigtest/bundler/bundler_compile.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this restructures how external imports are collected for bytecode ModuleInfo in the linker — core bundler plumbing that warrants human review.
Extended reasoning...
Overview
This PR fixes a bug where bun build --compile --bytecode --minify --format esm fails with "Cannot find module" errors for named imports from npm packages with re-export index files. The fix touches 4 source files in src/bundler/linker_context/ and adds a test scenario.
The core change: instead of scanning the original AST statements in postProcessJSChunk to find external imports (where source_index.isValid() was unreliable for transitive async-resolved dependencies), the code now extends DeclCollector to capture s_import statements from the converted output (after convertStmtsForChunk removes bundled imports). These collected imports are threaded through CompileResult to postProcessJSChunk.
Security risks
No security concerns — this is internal bundler plumbing that affects which module metadata is recorded in bytecode caches. No user input handling, auth, or external communication changes.
Level of scrutiny
High scrutiny warranted. This is production-critical bundler linker code that affects all --compile --bytecode ESM builds. The change restructures a key data flow (how external imports are identified for ModuleInfo), introducing a new struct (ImportInfoCollected), adding a field to DeclCollector and CompileResult.javascript, and rewriting the import collection loop in postProcessJSChunk.
The logic appears correct: convertStmtsForChunk calls shouldRemoveImportExportStmt which removes bundled imports, so only truly-external s_import statements survive to be collected by DeclCollector. The implicit skip of CJS-wrapped modules is also correct (their imports are inside wrapper closures, not in out_stmts). The O(parts×stmts) lookup to find the original s_import by import_record_index is acceptable since it only runs for external imports in bytecode+ESM+compile mode.
Other factors
- The PR is labeled
claude(AI-generated), which increases the value of human review for subtle correctness issues. - All files are CODEOWNER-protected.
- A test is included (
PackageReExportNamedImport) that reproduces the exact scenario. - The PR description mentions all existing tests pass.
|
@robobun Rebase main and get CI green |
|
Rebase done on current main. Fixed build error ( |
…ode ModuleInfo When building with `--compile --bytecode --minify --format esm`, named imports from npm packages caused "Cannot find module" errors at runtime. The root cause was in `postProcessJSChunk` section 2 which scanned original AST statements to collect external imports for ModuleInfo. It relied on `record.source_index.isValid()` to skip bundled imports, but the async resolution pipeline may not patch `source_index` on import records from transitive dependencies. This caused fully-inlined bundled imports to be incorrectly recorded as external dependencies in ModuleInfo, creating a mismatch with what JSC's parser sees in the actual emitted code. The fix extends DeclCollector to also capture `s_import` statements from the **converted** output (after `convertStmtsForChunk`). Since bundled imports are removed during conversion, only truly-external imports survive. `postProcessJSChunk` now uses these collected imports instead of scanning the original AST, ensuring ModuleInfo accurately reflects the emitted code. Closes #27454 Co-Authored-By: Claude <noreply@anthropic.com>
robobun
force-pushed
the
claude/fix-bytecode-esm-module-info-27454
branch
from
5740da8 to
7cf2d8e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bundler/linker_context/postProcessJSChunk.zig (1)
202-218:⚠️ Potential issue | 🟡 MinorStop re-adding wrapper refs after collecting final output decls.
js.declsnow comes from finalout_stmts, so wrapped files already contributevar init_* = __esm(...)/var require_* = __commonJS(...)in step 1. This pass adds the same binding a second time toModuleInfo.🛠️ Suggested fix
- // 3. Add wrapper-generated declarations (init_xxx, require_xxx) that are - // not in any part statement. - const all_flags = c.graph.meta.items(.flags); - const all_wrapper_refs = c.graph.ast.items(.wrapper_ref); - for (chunk.content.javascript.parts_in_chunk_in_order) |part_range| { - const source_index = part_range.source_index.get(); - if (all_flags[source_index].wrap != .none) { - const wrapper_ref = all_wrapper_refs[source_index]; - if (!wrapper_ref.isEmpty()) { - const name = chunk.renamer.nameForSymbol(wrapper_ref); - if (name.len > 0) { - const string_id = mi.str(name) catch continue; - mi.addVar(string_id, .declared) catch continue; - } - } - } - } + // 3. Wrapper-generated declarations are already present in `js.decls` + // because DeclCollector now runs on final `out_stmts`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bundler/linker_context/postProcessJSChunk.zig` around lines 202 - 218, The loop that re-adds wrapper refs (for chunk.content.javascript.parts_in_chunk_in_order using all_flags/all_wrapper_refs and calling chunk.renamer.nameForSymbol and mi.addVar) causes duplicate wrapper bindings; remove this post-collection pass or change it to skip adding if the binding already exists (e.g., check ModuleInfo via mi.hasVar/ equivalent before calling mi.addVar) so wrapper refs produced by final out_stmts are not added a second time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bundler/linker_context/postProcessJSChunk.zig`:
- Around line 202-218: The loop that re-adds wrapper refs (for
chunk.content.javascript.parts_in_chunk_in_order using
all_flags/all_wrapper_refs and calling chunk.renamer.nameForSymbol and
mi.addVar) causes duplicate wrapper bindings; remove this post-collection pass
or change it to skip adding if the binding already exists (e.g., check
ModuleInfo via mi.hasVar/ equivalent before calling mi.addVar) so wrapper refs
produced by final out_stmts are not added a second time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08bff5e5-fe9e-4fda-8750-3ae94b3847d1
📒 Files selected for processing (5)
src/bundler/bundle_v2.zigsrc/bundler/linker_context/generateCodeForFileInChunkJS.zigsrc/bundler/linker_context/generateCompileResultForJSChunk.zigsrc/bundler/linker_context/postProcessJSChunk.zigtest/bundler/bundler_compile.test.ts
PR #27838 changed tls_props from ?SSLConfig to ?SSLConfig.SharedPtr but missed updating getTlsHostname(), causing a build failure on main. https://claude.ai/code/session_01VUffTTZPCvYZwcopKhoiAf
There was a problem hiding this comment.
This PR refactors how the bundler collects external imports for bytecode ModuleInfo, moving from original AST scanning to converted-statement collection. The logic changes in postProcessJSChunk and DeclCollector are non-trivial and touch a critical code generation path, so a human reviewer should verify the approach.
Extended reasoning...
Overview
This PR fixes a bug where bun build --compile --bytecode --minify --format esm fails with "Cannot find module" errors for named imports from npm packages with re-export index files. It touches 4 Zig source files in the bundler linker context (bundle_v2.zig, generateCodeForFileInChunkJS.zig, generateCompileResultForJSChunk.zig, postProcessJSChunk.zig) and adds a test case. The core change is extending DeclCollector to also capture s_import statements from converted output, then using those collected imports in postProcessJSChunk instead of scanning the original AST.
Security risks
No direct security risks. The changes are internal to the bundler’s code generation pipeline and do not affect auth, permissions, or user input handling.
Level of scrutiny
High scrutiny warranted. This modifies the bundler’s linker context — specifically how external imports are collected for bytecode ModuleInfo. Incorrect collection could cause runtime failures (modules not found) or incorrect module metadata. The PR carries a claude label indicating AI authorship, which adds reason for careful human review of the Zig logic.
Other factors
- There is an unaddressed CodeRabbit suggestion about adding bounds checks on
import_info.source_indexbefore indexing intoall_import_recordsandpartsarrays. While the source_index originates frompart_range.source_index.get()(which should be valid), the defensive check is reasonable for a safety-critical path. - The test coverage looks good — a new
PackageReExportNamedImportscenario is added that exercises the exact bug fix, and it runs in both default and minified configurations via the existing test matrix. - CI was recently rebased onto main; results pending on commit
7cf2d8e.
There was a problem hiding this comment.
This PR changes how the bundler linker collects external imports for bytecode ModuleInfo, threading a new ImportInfoCollected struct through the parallel printing pipeline. The fix looks reasonable, but the architectural change to bundler internals warrants a human look before merging.
Extended reasoning...
Overview
This PR fixes #27454 where bun build --compile --bytecode --minify --format esm fails with "Cannot find module" errors for packages with re-exported named imports. It modifies 4 bundler linker files (bundle_v2.zig, generateCodeForFileInChunkJS.zig, generateCompileResultForJSChunk.zig, postProcessJSChunk.zig), plus an unrelated http.zig rebase fix and a new test case.
The core change: instead of scanning the original AST for external imports (where record.source_index.isValid() may not be patched for transitive dependencies resolved through the async pipeline), DeclCollector now captures s_import statements from converted output, and postProcessJSChunk iterates these collected imports. It still looks up the original AST to get binding info (default_name, items, namespace_ref).
Security risks
No security concerns — this is bundler-internal metadata collection for bytecode generation. No user input handling, no auth, no network-facing changes.
Level of scrutiny
This needs human review. The change is architecturally significant: it introduces a new data structure (ImportInfoCollected) threaded through the parallel printing pipeline (DeclCollector → CompileResult → postProcessJSChunk). The bundler linker context is a critical and complex code path. Additionally, this is an AI-generated PR (labeled claude) with no human code review yet — a human should verify the approach is the right architectural choice and that the interaction between collected imports and original AST lookups is correct in all edge cases.
Other factors
- The pre-existing bug (AST search missing
s_export_from/s_export_starconverted tos_import) is noted but not introduced by this PR - CodeRabbit's bounds-check suggestion was correctly dismissed by robobun
- The regression test (
PackageReExportNamedImport) covers the specific scenario - CI failures appear unrelated (tinycc warnings, CMakeLists warnings)
| const s_import = brk: { | ||
| for (source_parts) |part| { | ||
| for (part.stmts) |stmt| { | ||
| switch (stmt.data) { | ||
| .s_import => |s| { | ||
| if (s.import_record_index == import_info.import_record_index) | ||
| break :brk s; | ||
| }, | ||
| else => {}, | ||
| } | ||
| } | ||
| } | ||
| continue; |
There was a problem hiding this comment.
🟣 Pre-existing bug: The AST search at lines 156-168 only matches .s_import when looking up the original statement for a collected import, but convertStmtsForChunk converts s_export_from and s_export_star (with alias) into S.Import — the original AST still has the unconverted statement types, so the search misses them and silently drops the import from ModuleInfo via continue at line 168. This affects --compile --bytecode --format esm builds with re-exports of external modules (e.g., export { readFileSync } from "node:fs"). This issue exists in the old code as well and is not introduced by this PR.
Extended reasoning...
What the bug is
In postProcessJSChunk.zig lines 156-168, the code iterates over collected imports from DeclCollector and then searches the original AST parts for a matching s_import statement by import_record_index. The search only matches the .s_import variant (line 160). If no match is found, it falls through to continue at line 168, silently skipping the import.
Why it fails for re-exports
convertStmtsForChunk transforms s_export_from into S.Import (lines 308-329 of convertStmtsForChunk.zig) and s_export_star with alias into S.Import (lines 108-118) when shouldStripExports is true (always true in bundle mode). DeclCollector correctly captures these converted s_import statements with their import_record_index. However, the original AST — which is what postProcessJSChunk searches — still has .s_export_from or .s_export_star, not .s_import. The search at line 160 never matches these statement types.
Step-by-step proof
- A source file has
export { readFileSync } from "node:fs"in a--compile --bytecode --format esmbuild. - The parser creates an
s_export_fromstatement with animport_record_indexpointing tonode:fs. convertStmtsForChunkconverts this toS.Import(line 320-329) becauseshouldStripExportsis true andshouldRemoveImportExportStmtreturns false (external ESM import).DeclCollector.collectFromStmtssees the converted.s_importand records{ import_record_index, source_index }intodc.imports.postProcessJSChunkiteratesimportsand finds this entry. It looks up the import record — valid, not disabled, not bun-tagged.- It searches the original AST parts for
.s_importwith matchingimport_record_index(line 160). The original AST has.s_export_from, not.s_import. - The search exhausts all parts and falls through to
continueat line 168. The import is silently dropped from ModuleInfo.
Impact
In --compile --bytecode --format esm builds, re-exports of external modules (export { ... } from "ext" or export * as ns from "ext") will have incomplete ModuleInfo — their dependency tracking metadata is missing. The bytecode itself still works correctly; only the dependency metadata is affected. This is an edge case requiring external re-export syntax in a bytecode ESM compile build.
Pre-existing nature
The old code (before this PR) also only matched .s_import when scanning the original AST — s_export_from fell into the else branch. Both old and new code have the same limitation. The PR does not introduce or worsen this bug.
Fix
The search block (lines 159-164) should also match .s_export_from and .s_export_star by import_record_index, extracting the relevant fields (items, namespace_ref) from those statement types to reconstruct the import bindings for ModuleInfo.
3 participants
Summary
bun build --compile --bytecode --minify --format esmfailing with "Cannot find module" errors when using named imports from npm packages (e.g.,import { createTwoFilesPatch } from 'diff')postProcessJSChunkscanning original AST statements for external imports and relying onrecord.source_index.isValid()which may not be patched for transitive dependencies resolved through the async pipelineDeclCollectorto captures_importstatements from converted output (where bundled imports are already removed), then uses these inpostProcessJSChunkinstead of the original ASTTest plan
PackageReExportNamedImporttest scenario totest/bundler/bundler_compile.test.ts(tests both default and minified variants)bundler_compile.test.tstests passbundler_compile_splitting.test.tstests passCloses #27454
🤖 Generated with Claude Code