Attach importer sourceURL/line/column to module link SyntaxErrors - #378
Attach importer sourceURL/line/column to module link SyntaxErrors#378robobun wants to merge 2 commits into
Conversation
When a named import cannot be resolved during InitializeEnvironment (the imported module does not export that name, or the binding is ambiguous), the thrown SyntaxError previously carried only the message text, which named the imported-from module but not the importing file. In a multi-file project the user had no way to locate the failing import statement. ImportEntry now records the specifier's source offset (one unsigned, populated from ImportSpecifierNode::startOffset() in NodesAnalyzeModule). At throw time initializeEnvironment computes the 1-based line/column by scanning the importing module's SourceCode up to that offset, and sets sourceURL / line / column on the ErrorInstance both as the internal m_sourceURL/m_lineColumn members and as DontEnum own properties so the embedder's uncaught-error printer can read them when no JS frames were captured (module linking runs from a C++ microtask with an empty JS stack). The indirect-export and ambiguous cases go through the same helper with no offset, so they at least carry the importing module's sourceURL. Entirely under USE(BUN_JSC_ADDITIONS). For oven-sh/bun#5582.
|
Warning Review limit reached
Next review available in: 14 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked: setSourceURL takes its argument by value so the later WTF::move(sourceURL) into jsString is safe; the offset scan is clamped to source.length() so there's no OOB read; and jsModule / provider() are null-checked before dereference. Deferring to a human since this adds ~50 lines of new logic in the module-linking runtime path.
Extended reasoning...
Verified the move-after-copy pattern on sourceURL (ErrorInstance::setSourceURL takes String by value, so the local remains valid for the subsequent jsString(vm, WTF::move(sourceURL))), the std::min<unsigned>(sourceOffset, source.length()) bound on the scan, and the std::max(0, specifier->startOffset()) clamp given startOffset() returns signed int. All new code is under USE(BUN_JSC_ADDITIONS) and only runs on link-error paths, so happy-path module evaluation is unaffected. Not auto-approving because it's non-trivial new C++ in JSC's initializeEnvironment, which warrants a human glance.
Preview Builds
|
The diagnostic scan previously counted only LF, so sources using lone CR or U+2028/U+2029 line separators would report line = firstLine and a column equal to the absolute offset. CRLF happened to work because the LF is still seen. Match Lexer::isLineTerminator / shiftLineTerminator.
There was a problem hiding this comment.
LGTM — the line-terminator nit is addressed in aac0d8c and the scan now matches Lexer::isLineTerminator including CRLF collapse.
Checked: the new throwLinkSyntaxError helper only runs on link-failure paths and is fully gated by USE(BUN_JSC_ADDITIONS); happy-path linking is unchanged. Verified ErrorInstance::setSourceURL/setLine/setColumn and vm.propertyNames->{sourceURL,line,column} exist, and that the source scan is bounded by min(sourceOffset, source.length()) so there's no OOB read. setSourceURL takes its argument by value, so the later WTF::move(sourceURL) into jsString is safe.
Extended reasoning...
Overview
Three files touched, all under #if USE(BUN_JSC_ADDITIONS):
AbstractModuleRecord.h: addsunsigned sourceOffset { 0 }toImportEntry.NodesAnalyzeModule.cpp: populates it fromImportSpecifierNode::startOffset()(clamped non-negative;startOffset()returnsint).CyclicModuleRecord.cpp: adds athrowLinkSyntaxErrorlambda insideinitializeEnvironmentthat scans the importing module's source to the recorded offset, computes 1-based line/column, and attachessourceURL/line/columnto the thrownSyntaxErrorboth asErrorInstanceinternal fields and asDontEnumown properties. All eight Bun-branchthrowSyntaxErrorsites in this function now route through it; the message strings are unchanged.
Security risks
None identified. This is diagnostic metadata on an already-thrown error. The source scan indexes a StringView up to std::min(sourceOffset, source.length()), so no out-of-bounds access. No user-controlled data reaches privileged operations; putDirect on a fresh ErrorInstance cannot invoke user setters.
Level of scrutiny
Moderate-low. While CyclicModuleRecord::initializeEnvironment is core module-linking code, every added line is either (a) a lambda definition with no cost until invoked, or (b) inside a branch that already ended in throwSyntaxError(...); return;. The successful-link path is byte-for-byte unchanged. The whole thing is Bun-gated, so upstream WebKit semantics are untouched.
Other factors
- The one issue from the previous review pass (
\n-only line counting) was fixed in aac0d8c; the loop now matches\r,\n, U+2028, U+2029 and collapses\r\n— verified against the current diff. - Confirmed the referenced APIs exist:
ErrorInstance::setSourceURL/setLine/setColumn(ErrorInstance.h:109-115) andCommonIdentifiersentries forsourceURL/line/column. setSourceURL(String)takes by value, sosourceURLis still valid when it's later moved intojsString(...).- The
if (sourceOffset)guard means a specifier at absolute offset 0 would skip line/column — a degenerate case (the specifier of an import statement is never at byte 0), and it still getssourceURL, which is strictly better than the pre-PR behavior. - Test coverage lands on the Bun side per the PR description.
- Preview build for b5a21f5 succeeded; the follow-up commit only tweaks the terminator comparison inside the same loop.
The change is self-contained, error-path only, and the prior feedback was addressed. Approving.
| } | ||
| } | ||
| if (!sourceURL.isEmpty()) | ||
| error->putDirect(vm, vm.propertyNames->sourceURL, jsString(vm, WTF::move(sourceURL)), attrs); |
There was a problem hiding this comment.
@robobun don't set both here. just set the one.
2 participants
When a named import cannot be resolved during
InitializeEnvironment(the imported module does not export that name, or the binding is ambiguous), the thrownSyntaxErrorcarried only the message text, which names the imported-from module but not the importing file. In a multi-file project there is no way to locate the failing import statement. For oven-sh/bun#5582.Change
ImportEntrynow records the specifier's source offset (oneunsigned, populated fromImportSpecifierNode::startOffset()inNodesAnalyzeModule). At the throw siteinitializeEnvironmentcomputes the 1-based line/column by scanning the importing module'sSourceCodeup to that offset and setssourceURL/line/columnon theErrorInstance:m_sourceURL/m_lineColumnsoformatStackTraceprepends anat <parse> (file:line)frame when a JS stack was captured, andDontEnumown properties so the embedder's uncaught-error printer can read them when no JS frames were captured (module linking runs from a C++ microtask with an empty JS stack).The indirect-export and ambiguous-export cases route through the same helper with no offset so they at least carry the importing module's
sourceURL. The message text is unchanged.Entirely under
USE(BUN_JSC_ADDITIONS).Verification
Covered by the new
test/js/bun/resolve/esm-link-error-location.test.tson the bun side (lands with theWEBKIT_VERSIONbump). The three cases:index.mjs:3main→middle→dep): stderr namesmiddle.mjs, not the entry pointimport()rejection: the caught error's.sourceURL/.linepoint at the importing file