jsc: name the line:column flag at source_url_formatter call sites - #39152
Conversation
source_url_formatter took two bools (exclude_line_column, enable_color)
and two of its callers passed literals for both, so a call read as
source_url_formatter(.., true, false) with nothing saying which was
which. The line:column flag is now a two-variant enum, LineColumn, so
each call names it. enable_color stays a bool: most callers forward a
runtime allow_ansi_colors, and name_formatter beside it takes the same
bool. Same shape as Colon::{IncludeColon, ExcludeColon} in
ConsoleObject.rs.
Output is unchanged. Drops the matching bare_bool_args entry from
mordant-baseline.toml.
|
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 (3)
Comment |
|
Updated 12:05 PM PT - Aug 15th, 2026
❌ @robobun, your commit f63cad7 has some failures in 🧪 To try this PR locally: bunx bun-pr 39152That installs a local version of the PR into your bun-39152 --bun |
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with no behavior change.
What was reviewed:
- All 5
source_url_formattercall sites map to the correctLineColumnvariant (polarity of the oldexclude_line_columnbool preserved); no other callers or directSourceURLFormatterconstructions exist in the tree. - The two
Displayconditions flip!self.exclude_line_column→== LineColumn::Include, which is equivalent. crate::zig_stack_frame::LineColumnresolves via thepub mod zig_stack_frameinsrc/jsc/lib.rs, andpub(crate)visibility is sufficient for theVirtualMachine.rscallers.
Extended reasoning...
Overview
This PR replaces the exclude_line_column: bool parameter of ZigStackFrame::source_url_formatter with a two-variant LineColumn enum (Include / Exclude), updates the corresponding SourceURLFormatter struct field, flips the two conditions in its Display impl, updates all five call sites (four in VirtualMachine.rs, one in ZigStackFrame.rs), and removes the corresponding bare_bool_args entry from mordant-baseline.toml. The remaining enable_color: bool parameter is left as-is since it's forwarded from a runtime value and no longer sits next to another bool literal.
Security risks
None. This is a type-level refactor of an internal formatting helper for stack-trace output; no parsing, no I/O, no external input handling changes.
Level of scrutiny
Low. The change is small, mechanical, and type-checked by the compiler — a missed call site or wrong argument type would fail to compile. The only thing to verify by hand is polarity, and each of the five sites maps correctly: the four callers that previously passed false (don't exclude) now pass LineColumn::Include, and the one snapshot() caller that passed true now passes LineColumn::Exclude. The two Display conditions !self.exclude_line_column become self.line_column == LineColumn::Include, which is semantically identical.
Other factors
Grep confirms there are no other references to source_url_formatter, SourceURLFormatter, or exclude_line_column in the tree, so nothing was missed. The use crate::zig_stack_frame::LineColumn import path is valid (pub mod zig_stack_frame at src/jsc/lib.rs:460) and the pub(crate) visibility on the enum is sufficient for both callers, which are in the same crate. The PR description documents byte-identical output verification against the release binary and passing tests for each caller (dev error page, GitHub annotation, reportError). The baseline removal is scoped to exactly the entry being fixed.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
The overlap with #37180 is this one call site. That PR retypes the flag (and |
Problem
ZigStackFrame::source_url_formatter(src/jsc/ZigStackFrame.rs) takes two bools,exclude_line_columnandenable_color. Two callers pass literals for both:source_url_formatter(root_path, origin, true, false)insnapshot()and(file, origin, false, false)inprint_github_annotation. Nothing at those calls says which flag is which, and the swapped call compiles."bare_bool_args:src/jsc/ZigStackFrame.rs"entry inmordant-baseline.toml.Fix
exclude_line_column: boolbecomesline_column: LineColumn, a two-variant enum (Include/Exclude) next toSourceURLFormatter, so every call names it. The twoDisplayconditions that read the flag are the only other code change.enable_colorstays a bool: theprint_stack_tracecallers forward a runtimeallow_ansi_colors, andname_formatteron the same lines takes the same bool. With one bool left in the signature there is nothing to confuse it with, which is also what makes the lint quiet. Same shape asErrorDisplayLevel::formatter(name, enable_colors: bool, colon: Colon)inConsoleObject.rs.cargo dylint --all -- -p bun_jscon main's source reports exactly this finding (bun_jsc 1intarget/mordant/over-baseline.txt); on this branch it reports nothing. Regenerating in write mode scoped tobun_jscand the crates it pulls in reproduces the file byte for byte apart from this line and one unrelated entry that is already stale on main (always_unwrapped_option:src/install/PackageInstall.rs), which this PR leaves alone.bare_bool_argsfinding in it from now on.bun bd test test/js/bun/http/serve.test.ts -t "dev error page"(theExcludecaller: the page'sframes[].filemust not carry:line:col),test/cli/test/bun-test.test.ts -t "Github Actions"(the annotation caller),test/js/bun/util/reportError.test.ts,error-name-preservation.test.ts,test/regression/issue/{23649,11793,12782}.test.ts: pass.bungives byte-identical frame lines withNO_COLOR, withFORCE_COLOR=1(the dimmed cwd prefix path) and as aGITHUB_ACTIONSannotation.test/js/bun/util/inspect-error.test.jshas two snapshot failures under a debug build that reproduce identically on an unmodified checkout (a debug-onlyat require (51:24)frame its filter does not strip); tracked separately.Background
SourceURLFormatteris theDisplaytype behind everyat name (file:line:col)stack frame line Bun prints: terminal error output, the::errorannotation under GitHub Actions, and theBun.servedevelopment error page. The error page stores the position separately and wants only the file, which is the oneExcludeuse.mordantjob inrust-lints.yml.mordant-baseline.tomlrecords, per lint and file, how many findings predate the job; the job fails only on findings above the recorded count, so fixing one means deleting its line so the count cannot creep back.