Repository navigation
Conversation
…tWarning The nextTick warning handler only printed to stderr when no 'warning' listener was installed, so adding any listener silenced the on-terminal deprecation trail. When it did print it used the error inspector instead of Node's one-line "(node:PID) [CODE] Name: msg" form, and the Error constructed for a string warning kept the internal util/deprecate frames on its stack, so --throw-deprecation surfaced a snippet of builtin source (with raw JSC intrinsic syntax) in the uncaught-error report. Always emit the 'warning' event and, unless NODE_NO_WARNINGS, call a new internal onWarning printer that writes Node's format via process.stderr. When a ctor is passed to process.emitWarning, re-capture the warning's stack with it as the caller so only user frames remain. The Error.captureStackTrace body is factored into captureStackTraceForError for reuse.
|
Warning Review limit reached
Next review available in: 11 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Updated 8:22 AM PT - Jul 18th, 2026
❌ @robobun, your commit 5f701eb has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34608That installs a local version of the PR into your bun-34608 --bun |
…tack The hint line now points at a flag that actually does something: when either --trace-* flag is present in execArgv, onWarning prints the warning's stack (already trimmed to the user call site when a ctor was supplied) and skips the hint. Also replace the exact-empty stderr assertion in the NODE_NO_WARNINGS test with a negative match on the report text.
Node's ErrorCaptureStackTrace(warning, ctor) runs inside createWarningObject,
which is only reached when the warning argument is a string. A caller-supplied
Error is emitted as-is. Moving the captureStackTraceForError call inside the
string branch restores that, so process.emitWarning(err, {ctor}) / the 4-arg
form no longer overwrite err.stack.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/bindings/BunProcess.cpp:2018-2021— Once this moves inside thewarning.isString()branch (per the earlier comment on this block), also fall back toprocess.emitWarningas the filter whenctoris not callable — Node callsErrorCaptureStackTrace(warning, ctor || process.emitWarning)unconditionally for string warnings. As-is, the common no-ctor formprocess.emitWarning('msg', 'Type', 'CODE')skips the re-capture entirely and leaves anat emitWarning (native)frame at the top ofwarning.stackunder--trace-warnings. The removed TODO comment quoted exactly this fallback.Extended reasoning...
What the bug is
Node's
createWarningObjectinlib/internal/process/warning.jscallsErrorCaptureStackTrace(warning, ctor || process.emitWarning)unconditionally for every string warning — when the caller passes noctor, Node falls back toprocess.emitWarningas the filter, so theemitWarningframe itself is always stripped. The removed TODO comment in this diff literally quoted that line:// TODO: ErrorCaptureStackTrace(warning, ctor || process.emitWarning);.The new code at
BunProcess.cpp:2018gates the whole re-capture onif (ctor.isCallable())with no fallback:if (ctor.isCallable()) { Bun::captureStackTraceForError(globalObject, errorInstance, ctor); RETURN_IF_EXCEPTION(scope, {}); }
So the common forms
process.emitWarning("msg")andprocess.emitWarning("msg", "Type", "CODE")— wherectorarrives asjsUndefined()— skip the re-capture entirely and keep the stack thatcreateError()collected insideProcess_emitWarning.Process_emitWarningis registered in the static hash table at line ~4463 as a plain public host function (defaultImplementationVisibility::Public), so JSC'sErrorInstance::finishCreation→Interpreter::getStackTracecaptures its frame at the top.Step-by-step proof
- User runs
bun --trace-warnings -e 'process.emitWarning("trace me", "CustomWarning", "CODE10")'. Process_emitWarningis invoked withwarning="trace me",type="CustomWarning",code="CODE10",ctor=undefined.- Line 2003:
warning.isString()→errorInstance = createError(globalObject, "trace me").createErrorcaptures a stack fromvm.topCallFrame— the top frame is theProcess_emitWarninghost call. - Line 2018:
ctor.isCallable()is false (undefined) → nocaptureStackTraceForErrorcall. ThecreateErrorstack is kept as-is. onWarningruns withtrace = trueand printswarning.stack. Output containsat emitWarning (native)above the user's[eval]frame.
Node's output for the same command shows only the user's call site. The new test at
process-warning-output.test.ts:99-106only asserts/^\s+at /mand does not check for absence of theemitWarningframe, so it passes either way.Why existing code doesn't prevent it
Before this PR the entire block was a commented-out TODO, so the no-ctor path never re-captured — this isn't a regression, but the PR's title is "hide internal frames in emitWarning" and the no-ctor form is by far the most common call shape. Per REVIEW.md's "Fix the whole class in the same PR", the sibling no-ctor path should get the same treatment.
Relationship to the earlier comment on this block
This is not a duplicate of the earlier comment on lines 2018-2021 — that one is about over-application (running the re-capture on user-supplied Error instances). This is about under-application (skipping the re-capture for string warnings when
ctoris omitted). Both fixes land at the same location and can be done together.How to fix
Once moved inside the
if (warning.isString())branch per the earlier comment, add the|| process.emitWarningfallback:if (warning.isString()) { auto s = warning.getString(globalObject); errorInstance = createError(globalObject, !s.isEmpty() ? s : "Warning"_s); errorInstance->putDirect(vm, vm.propertyNames->name, type, JSC::PropertyAttribute::DontEnum | 0); JSValue filter = ctor.isCallable() ? ctor : process->get(globalObject, Identifier::fromString(vm, "emitWarning"_s)); RETURN_IF_EXCEPTION(scope, {}); Bun::captureStackTraceForError(globalObject, errorInstance, filter); RETURN_IF_EXCEPTION(scope, {}); }
Optionally tighten the
--trace-warningstest to assertexpect(stderr).not.toMatch(/at emitWarning/).Impact
One extra
at emitWarning (native)line at the top of the printed stack under--trace-warnings/--trace-deprecation, and inwarning.stackas read by any'warning'listener. Purely cosmetic Node-compat gap — nothing breaks — hence nit, but it's a one-liner squarely in this PR's stated scope. - User runs
- Re-capture the synthesized warning's stack unconditionally (not gated on
ctor), so the enclosing host-function frame is dropped even for
process.emitWarning('msg') with no ctor and --trace-warnings starts at
the user's call site.
- Honor runtime process.traceProcessWarnings / process.traceDeprecation in
addition to the execArgv flags.
- Strip a trailing .exe from the hint-line argv0 on Windows.
|
Re: the no-ctor fallback note in the additional-findings comment: done in fe65d4c. The synthesized warning now re-captures unconditionally; when |
There was a problem hiding this comment.
Beyond the inline nit: e6a0c1d addresses the last two open items (NODE_NO_WARNINGS pin, re-capture comment wording). Also checked that the captureStackTraceForError extraction passing nullptr for callFrame is behavior-preserving — getFramesForCaller marks it UNUSED_PARAM (ErrorStackTrace.cpp:118), so Error.captureStackTrace semantics are unchanged. The candidate "detail appended in trace mode diverges from Node" was ruled out.
Extended reasoning...
The most recent commit e6a0c1d closes out the two nits from my previous round; every prior thread on this PR is now resolved. The one remaining inline finding this run is pre-existing (.code/.detail putDirect on user-supplied Errors) and same-class-adjacent to the .stack fix already made — worth noting but not a regression. The FormatStackTraceForJS.cpp refactor's only non-mechanical change is dropping the callFrame argument to getFramesForCaller; I verified that parameter is UNUSED_PARAM in the callee, so the JS-facing Error.captureStackTrace path is byte-for-byte equivalent. Given the C++ JSC-bindings surface (emit dispatch rewrite + captureStackTrace body extraction) and the number of iterations, leaving the final call to a human.
Node's createWarningObject sets name/code/detail and captures the stack only when the warning argument is a string; a caller-supplied Error is emitted unmodified. The stack re-capture was already scoped to the string branch; move the code/detail putDirect calls there too.
|
CI on 5f701eb (build #75422) is green for this change: 284/286 jobs passed, and The two red jobs are unrelated to this diff:
Ready for review. |
|
Status against current main (165dc9f), now that #31831 has landed and closed #9867.
Already on main via #31831: the Still failing on main. Node v26.3.0 behaves the way the tests expect in each case:
So the stack re-capture in |
|
Checked this PR against main after #31831 merged. Method: debug build of main at 731aa92, then this PR's Landed in #31831 (the 4 passing tests):
Still open on main (the 5 failing tests):
The branch conflicts with main in Per-test result on main |
|
#48213 replaces the part of this PR that is about the stack of a warning. It is based on current main, and it captures the stack through the body of The printer of this PR is on main since #31831. What is left here and in no other PR is the hint under a deprecation: it names |
|
Status on main at 2596ac5, from debug builds. This PR's
#48213 covers the two tests for the printed stack ( The one test that is left is the hint under a deprecation. Main names The hint change is written and tested, but it is not a PR yet. #48120 rewrites the lines that build the hint: |
|
Closing. Each part of this PR now lives in another place.
This PR's One behaviour of this PR is in none of them, on purpose. Its printer used |
Fixes three presentation problems in the
process.emitWarning/util.deprecatewarning path. Semantics (once-law,[code]dedup,this/args/return passthrough,'warning'event payload,--no-deprecation,--throw-deprecationexit 1) were already Node-correct; only the stderr report and thrown-error stack were off.Fixes #9867. Supersedes #32433.
Repro
Cause
jsFunction_emitWarning(the nextTick handler) emitted the'warning'event or printed via the console error inspector, never both:so installing any listener silenced the on-terminal deprecation trail, and when it did print it used the error inspector instead of Node's one-line form. Separately,
Process::emitWarningbuilt the warningErrorwithcreateError()but never re-captured its stack past the suppliedctor, leaving the internalutil/deprecateemitter frames on top; under--throw-deprecationthe uncaught-error reporter then showed a source snippet from the bundled builtin (complete with@undefinedintrinsic syntax).Fix
'warning', then (unlessNODE_NO_WARNINGS) call a newinternal/process/warningonWarningthat writes Node's(name:pid) [CODE] Name: msgline,detail, and the one-time--trace-*hint viaprocess.stderr.write. A throwingwarning.toString()falls back toError.prototype.toStringso the printer itself never escalates a diagnostic into an uncaught exception.Error.captureStackTraceintocaptureStackTraceForErrorand call it fromProcess::emitWarningwithctorso the warning's stack starts at the user's call site.Verification
Node parallel tests
test-process-warning.js,test-process-emitwarning.js,test-process-no-deprecation.js,test-child-process-no-deprecation.js,test-util-deprecate.js,test-common-expect-warning.js, andtest/js/node/v8/capture-stack-trace.test.jsall pass.[review] gate passed · iteration 2 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 2
evidence per changed file