-
Notifications
You must be signed in to change notification settings - Fork 55
[JSC] Module linking: name the binding and the module when an indirect export does not resolve - #581
[JSC] Module linking: name the binding and the module when an indirect export does not resolve #581robobun wants to merge 1 commit into
Conversation
...t export does not resolve
The InitializeEnvironment check for IndirectExportEntries (export { a as b } from
"m", or import { a } from "m"; export { a as b }) printed the export alias and
the raw specifier, and for an unresolvable 'default' it printed "export default
cannot be used with export *" even when no module has an export *.
Use the same messages as the ImportEntries check in step 7: name the requested
binding (importName) and the requested module by its key.
export { nope as y } from "./p.mjs"
before: export 'y' not found in './p.mjs'
after: Export named 'nope' not found in module '/abs/p.mjs'.
export { default } from "./p.mjs" (p.mjs has no default)
before: export default cannot be used with export *
after: Missing 'default' export in module '/abs/p.mjs'.
export { dup as z } from "./root.mjs" (two export * in root provide dup)
before: Cannot export 'z' multiple times in './root.mjs'
after: Export named 'dup' cannot be resolved due to ambiguous multiple
bindings in module '/abs/root.mjs'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM — Bun-only diagnostic text change that mirrors the existing step-7 ImportEntry messages in the same function.
What was reviewed:
- Confirmed
ExportEntrycarriesimportName,moduleName, andmoduleRequestType, so the newmakeStringcalls and thehostResolveImportedModulelookup compile against the existing struct. - Verified
hostResolveImportedModuleis a purem_loadedModulesmap lookup returningnullptron miss — no exceptions, so the lambda's fallback toe.moduleName.string()is safe and noRETURN_IF_EXCEPTIONis needed on the error path. - Control flow is unchanged: each case still
returns after throwing, and them_isTypeScriptNotFound leniency is preserved.
Extended reasoning...
Overview
The PR touches a single file, Source/JavaScriptCore/runtime/CyclicModuleRecord.cpp, and only within #if USE(BUN_JSC_ADDITIONS) blocks. It rewrites the SyntaxError message text for the three failure cases (NotFound, Ambiguous, Error) when resolving IndirectExportEntries in initializeEnvironment. A small helper lambda requestedModuleKey is added that calls hostResolveImportedModule (a m_loadedModules HashMap lookup, per AbstractModuleRecord.cpp:230-235) to resolve the specifier to its module key, falling back to the raw specifier on a miss. The messages now use e.importName (the binding requested from the target module) instead of e.exportName (the local alias), and the resolved module key instead of the raw specifier — matching the ImportEntry messages at lines 221-260 of the same function.
Security risks
None. This is diagnostic string construction on an already-failing path that is about to throw a SyntaxError and return. No parsing, no allocation pattern changes beyond makeString, no new inputs from untrusted sources beyond what was already being formatted into these messages. hostResolveImportedModule takes a JSGlobalObject* but ignores it and cannot throw, so there is no exception-handling gap introduced by calling it inside the message construction.
Level of scrutiny
Low. The change is guarded to Bun builds, alters only user-facing error text, and copies its shape directly from the ImportEntry handling ~100 lines below in the same function (which already calls hostResolveImportedModule and formats importedModule->moduleKey().string() into identical message templates). The Error branch's e.importName == vm.propertyNames->defaultKeyword split is a reasonable refinement: when the direct request was default, it says "Missing 'default' export"; when the failure came from deeper in a re-export chain, it falls back to the generic not-found phrasing — either way strictly better than the previous fixed "export default cannot be used with export *" string.
Other factors
The repo's .github/CODEOWNERS lists /Source/JavaScriptCore @ WebKit/jsc-reviewers, but the file's own header states it does not confer ownership and exists to auto-add upstream WebKit reviewers — not a gate in this Bun fork. The PR description references companion Bun-side tests (test/js/bun/typescript/type-export.test.ts) covering these shapes. No prior reviews or objections appear in the timeline, and the bug hunt exited on dry_streak with no candidates.
|
No actionable comments were generated in the recent review. 🎉 i️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesIndirect export diagnostics
Merge Risk: ⚪ Minimal · up to Indirect-export diagnostics now identify the requested binding and resolved module path without an evidenced remaining merge-readiness risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, implementation, preserved behavior, and test coverage. It does not follow the repository template because it omits a bug title, Bugzilla link, review status, and the required changed-file and function list.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
Problem
export { default } from "./p.mjs"(orexport { default as x } from) wherep.mjshas no default export, bun printsSyntaxError: export default cannot be used with export *. No module has anexport *, and the message names neither the binding nor the module. Node printsThe requested module './p.mjs' does not provide an export named 'default'.IndirectExportEntriescheck inCyclicModuleRecord::initializeEnvironment(runtime/CyclicModuleRecord.cpp:109-132,BUN_JSC_ADDITIONSbranch).resolveExportanswersErrorfor any unresolvabledefault(anexport *never provides one), and the branch had a fixed string for it. TheNotFoundandAmbiguousmessages in the same switch printede.exportName(the alias) instead ofe.importName:export { nope as y } from "./p.mjs"saidexport 'y' not found in './p.mjs', and a conflictingexport *saidCannot export 'z' multiple times in './root.mjs'.Fix
ImportEntriescheck in step 7 of the same function. They name the requested binding (importName) and the requested module by its key:Export named 'nope' not found in module '/abs/p.mjs'.Missing 'default' export in module '/abs/p.mjs'.Export named 'dup' cannot be resolved due to ambiguous multiple bindings in module '/abs/root.mjs'.hostResolveImportedModule, a map lookup thatresolveExportalready did for this entry. If the lookup misses, the raw specifier is printed.m_isTypeScriptleniency forNotFoundand the non-Bun messages do not change. Failing is still correct in every case. Only the text changes.test/js/bun/typescript/type-export.test.ts), which cover the three shapes above plus the existing through-export cases.