appendSourceToErrorMessage: skip builtin code blocks - #369
Conversation
|
Warning Review limit reached
Next review available in: 16 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 (1)
Comment |
100422d to
bce3c3e
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, targeted guard that mirrors the existing isBuiltinFunction() check in FindFirstCallerFrameWithCodeblockFunctor (Error.cpp:153), and only affects error-message text. The inline nit about isBuiltinFunction() vs isPrivateBuiltinFunction() is worth a look but either choice is safe.
Extended reasoning...
Overview
Adds a single early-return in appendSourceToErrorMessage() (ErrorInstance.cpp) so that builtin code blocks never have their source text spliced into user-visible TypeError messages. Two-line guard plus a four-line explanatory comment; no other changes.
Security risks
None. The change strictly reduces the amount of internal source text exposed in error messages — it removes the leak of @-prefixed private-identifier source from ASSERT_ENABLED builds. No new inputs, no allocation, no control-flow beyond an early return of the unmodified message string.
Level of scrutiny
Low. This is error-message cosmetics on a well-understood slow path (Call IC "not a function" errors). The predicate (codeBlock->unlinkedCodeBlock()->isBuiltinFunction()) is the same one already used by the other entry into this function via FindFirstCallerFrameWithCodeblockFunctor in Error.cpp, so the two paths are now consistent. Worst case for the raised nit is that Bun's non-private builtins (created via createBuiltinExecutable with a sourceURL) also stop getting source snippets appended in release builds — an error-message-text-only regression, and arguably desirable since those can also contain @-intrinsics.
Other factors
- The PR description includes a concrete repro (
Iterator.from({}).next()) and before/after output showing the fix aligns debug/ASAN behavior with release. - Verifiers cross-checked
BytecodeGenerator::emitExpressionInfogating and theisPrivateBuiltinFunction()definition underUSE(BUN_JSC_ADDITIONS); the inline nit captures the one open question (predicate breadth + comment accuracy), which is the author's call and doesn't block merging.
Preview Builds
|
Builtin JS functions use @-prefixed private identifiers in their source text. In release builds their code blocks carry no expression info (BytecodeGenerator::emitExpressionInfo skips them) so the existing hasExpressionInfo() check already short-circuits here. ASSERT_ENABLED builds do emit expression info for builtins, and when a .@call() inside a builtin hits a non-callable value the Call IC slow path (throwNotAFunctionErrorFromCallIC) passes the builtin code block and bytecode index straight into appendSourceToErrorMessage, which then splices the private-identifier source text into the user-visible TypeError message: Iterator.from({}).next() // TypeError: @getWrapForValidIteratorInternalField(this, // @wrapForValidIteratorFieldIteratedNextMethod).@call is not a // function. (In '...') Bail out of source appending for builtin code blocks so ASSERT_ENABLED builds match release output: TypeError: undefined is not a function
bce3c3e to
61b90ca
Compare
There was a problem hiding this comment.
LGTM — the updated comment now accurately covers the non-private-builtin case under USE(BUN_JSC_ADDITIONS), and keeping isBuiltinFunction() to match FindFirstCallerFrameWithCodeblockFunctor makes sense.
Extended reasoning...
Overview
Adds a single early-return in appendSourceToErrorMessage() (ErrorInstance.cpp) so builtin code blocks never get their source text spliced into user-visible TypeError messages. One conditional plus a comment; no other files touched.
Follow-up on prior review
My earlier nit was that the original comment misstated the release-build invariant under USE(BUN_JSC_ADDITIONS) (non-private builtins do carry expression info there). The author responded that isBuiltinFunction() is intentional — it matches the existing check at Error.cpp:153 in FindFirstCallerFrameWithCodeblockFunctor so both callers of this function behave identically, and Bun's non-private builtins can also contain @-intrinsics. The comment has been rewritten to state exactly that, so the concern is resolved.
Security risks
None. This only affects the text of thrown TypeError messages. If anything it slightly reduces exposure by preventing internal builtin source snippets from leaking into error strings. No auth, crypto, memory, or control-flow surface is touched.
Level of scrutiny
Low. The change is a cosmetic guard on an error-message helper. The accessor chain codeBlock->unlinkedCodeBlock()->isBuiltinFunction() is identical to the one already used in Error.cpp:153, so there is no new null-deref or lifetime concern beyond what the existing caller path already exercises. Worst case is a slightly less descriptive error message for Bun's non-private builtins in release builds, which the author has explicitly accepted.
Other factors
- The inline review thread is resolved.
- The bug hunting system found no issues on the updated revision.
- Change is self-contained and mirrors an established pattern in the same subsystem.
2 participants
What
Skip source-text appending in
appendSourceToErrorMessage()when the code block is a builtin function.Why
Builtin JS functions use
@-prefixed private identifiers in their source text. In release builds their code blocks carry no expression info (becauseBytecodeGenerator::emitExpressionInfoearly-returns forisPrivateBuiltinFunction()under#if !ASSERT_ENABLED), so the existinghasExpressionInfo()check already short-circuits here.ASSERT_ENABLED builds (debug and ASAN) do emit expression info for builtins. When a
.@call()inside a builtin hits a non-callable value, the Call IC slow path (throwNotAFunctionErrorFromCallICinRepatchInlines.h) passes the builtin code block and bytecode index straight intoappendSourceToErrorMessage, which then splices the private-identifier source text into the user-visibleTypeErrormessage.Repro
ASSERT_ENABLED (before):
After (matches release output):
Also affects
Iterator.from({[Symbol.iterator]: 5})(getIteratorFlattenable) and%WrapForValidIteratorPrototype%.returnwith a non-callablereturn, and any other builtin that reaches the Call IC slow path.The
FindFirstCallerFrameWithCodeblockFunctorpath inError.cppalready had a check that avoids settingbytecodeIndexfor builtins; this covers the directcallLinkInfo->retrieveCaller()path used by the Call IC.