Skip to content

sourcemap: make ParseResult a Result alias (fixes clippy on main) #38272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dylan-conway wants to merge 1 commit into from
Closed

sourcemap: make ParseResult a Result alias (fixes clippy on main) #38272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/runtime/bake/dev_server/source_map_store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,11 +682,11 @@ impl SourceMapStore {
0, // unused
Default::default(),
) {
source_map::ParseResult::Fail(fail) => {
Err(fail) => {
bun_core::debug_warn!("Failed to re-parse source map: {}", fail.err.message());
None
}
source_map::ParseResult::Success(mut psm) => Some(GetResult {
Ok(mut psm) => Some(GetResult {
mappings: core::mem::take(&mut psm.mappings),
file_paths: &entry.paths,
entry_files: &entry.files,
Expand Down
26 changes: 13 additions & 13 deletions src/sourcemap/Mapping.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ pub fn parse(

if let Some(count) = estimated_mapping_count {
if mapping.ensure_total_capacity(count).is_err() {
return ParseResult::Fail(ParseResultFail {
return Err(ParseResultFail {
err: crate::Error::Alloc(bun_alloc::AllocError),
loc: Loc::default(),
});
Expand Down Expand Up @@ -508,7 +508,7 @@ pub fn parse(
);
}
SimdResult::OutOfMemory => {
return ParseResult::Fail(ParseResultFail {
return Err(ParseResultFail {
err: crate::Error::Alloc(bun_alloc::AllocError),
loc: Loc::default(),
});
Expand Down Expand Up @@ -539,7 +539,7 @@ pub fn parse(
let generated_column_delta = decode_vlq(remain, 0);

if generated_column_delta.start == 0 {
return ParseResult::Fail(ParseResultFail {
return Err(ParseResultFail {
err: crate::Error::MissingGeneratedColumnValue,
loc: Loc {
start: i32::try_from(bytes.len() - remain.len()).unwrap_or(i32::MAX),
Expand All @@ -554,7 +554,7 @@ pub fn parse(
.zero_based()
.wrapping_add(generated_column_delta.value);
if generated_column < 0 {
return ParseResult::Fail(ParseResultFail {
return Err(ParseResultFail {
err: crate::Error::InvalidGeneratedColumnValue,
loc: Loc {
start: i32::try_from(bytes.len() - remain.len()).unwrap_or(i32::MAX),
Expand Down Expand Up @@ -587,7 +587,7 @@ pub fn parse(
// Read the original source
let source_index_delta = decode_vlq(remain, 0);
if source_index_delta.start == 0 {
return ParseResult::Fail(ParseResultFail {
return Err(ParseResultFail {
err: crate::Error::InvalidSourceIndexDelta,
loc: Loc {
start: i32::try_from(bytes.len() - remain.len()).unwrap_or(i32::MAX),
Expand All @@ -597,7 +597,7 @@ pub fn parse(
source_index = source_index.wrapping_add(source_index_delta.value);

if source_index < 0 || source_index >= sources_count {
return ParseResult::Fail(ParseResultFail {
return Err(ParseResultFail {
err: crate::Error::InvalidSourceIndexValue,
loc: Loc {
start: i32::try_from(bytes.len() - remain.len()).unwrap_or(i32::MAX),
Expand All @@ -609,7 +609,7 @@ pub fn parse(
// Read the original line
let original_line_delta = decode_vlq(remain, 0);
if original_line_delta.start == 0 {
return ParseResult::Fail(ParseResultFail {
return Err(ParseResultFail {
err: crate::Error::MissingOriginalLine,
loc: Loc {
start: i32::try_from(bytes.len() - remain.len()).unwrap_or(i32::MAX),
Expand All @@ -622,7 +622,7 @@ pub fn parse(
.zero_based()
.wrapping_add(original_line_delta.value);
if original_line < 0 {
return ParseResult::Fail(ParseResultFail {
return Err(ParseResultFail {
err: crate::Error::InvalidOriginalLineValue,
loc: Loc {
start: i32::try_from(bytes.len() - remain.len()).unwrap_or(i32::MAX),
Expand All @@ -635,7 +635,7 @@ pub fn parse(
// Read the original column
let original_column_delta = decode_vlq(remain, 0);
if original_column_delta.start == 0 {
return ParseResult::Fail(ParseResultFail {
return Err(ParseResultFail {
err: crate::Error::MissingOriginalColumnValue,
loc: Loc {
start: i32::try_from(bytes.len() - remain.len()).unwrap_or(i32::MAX),
Expand All @@ -648,7 +648,7 @@ pub fn parse(
.zero_based()
.wrapping_add(original_column_delta.value);
if original_column < 0 {
return ParseResult::Fail(ParseResultFail {
return Err(ParseResultFail {
err: crate::Error::InvalidOriginalColumnValue,
loc: Loc {
start: i32::try_from(bytes.len() - remain.len()).unwrap_or(i32::MAX),
Expand All @@ -672,7 +672,7 @@ pub fn parse(
// Read the name index
let name_index_delta = decode_vlq(remain, 0);
if name_index_delta.start == 0 {
return ParseResult::Fail(ParseResultFail {
return Err(ParseResultFail {
err: crate::Error::InvalidNameIndexDelta,
loc: Loc {
start: i32::try_from(bytes.len() - remain.len())
Expand All @@ -686,7 +686,7 @@ pub fn parse(
name_index = name_index.wrapping_add(name_index_delta.value);
if !has_names {
if mapping.ensure_with_names().is_err() {
return ParseResult::Fail(ParseResultFail {
return Err(ParseResultFail {
err: crate::Error::Alloc(bun_alloc::AllocError),
loc: Loc {
start: i32::try_from(bytes.len() - remain.len())
Expand Down Expand Up @@ -737,7 +737,7 @@ pub fn parse(
let mut psm = ParsedSourceMap::default();
psm.mappings = mapping;
psm.input_line_count = input_line_count;
ParseResult::Success(psm)
Ok(psm)
}

enum SimdResult {
Expand Down
2 changes: 1 addition & 1 deletion src/sourcemap/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
}

impl Error {
/// What `ParseResult::Fail` reports to the user.
/// What `ParseResultFail` reports to the user.

Check warning on line 40 in src/sourcemap/error.rs

View check run for this annotation

Claude / Claude Code Review

Stale ParseResult::Fail references missed in sibling comments 

This doc-comment update from `ParseResult::Fail` → `ParseResultFail` missed three sibling references: `src/jsc/bindings/highway_sourcemap.cpp:918` and `test/js/node/module/sourcemap-simd.test.ts:447,462` still say `ParseResult::Fail`. Comment-only, no behavior impact — but per REVIEW.md "grep for every sibling site sharing the pattern", these should be updated in the same PR.

@claude claude Bot Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 This doc-comment update from ParseResult::Fail → ParseResultFail missed three sibling references: src/jsc/bindings/highway_sourcemap.cpp:918 and test/js/node/module/sourcemap-simd.test.ts:447,462 still say ParseResult::Fail. Comment-only, no behavior impact — but per REVIEW.md "grep for every sibling site sharing the pattern", these should be updated in the same PR.

Extended reasoning...

What the bug is

This PR replaces enum ParseResult { Fail(ParseResultFail), Success(ParsedSourceMap) } with pub type ParseResult = Result<ParsedSourceMap, ParseResultFail>, so the ParseResult::Fail variant no longer exists. The PR correctly updated the doc comment at src/sourcemap/error.rs:40 from ParseResult::Fail to ParseResultFail to reflect this, but missed three other comments in the repo that still reference the deleted variant.

The specific stale sites

A repo-wide grep for ParseResult::Fail after this PR shows:

  • src/jsc/bindings/highway_sourcemap.cpp:918 — // and reports the exact same ParseResult::Fail as before.
  • test/js/node/module/sourcemap-simd.test.ts:447 — // Scalar caps at 8 bytes and returns no-progress -> ParseResult::Fail;
  • test/js/node/module/sourcemap-simd.test.ts:462 — test("out-of-range source index: identical ParseResult::Fail", ...)

These are the only remaining hits; the PR updated all Rust match arms and the error.rs doc comment but did not sweep C++ comments or test descriptions.

Why existing checks don't catch this

These are all inside comments (or a test title string), so neither rustc, clang, nor clippy sees them. The PR's stated verification (bun run rust:clippy + runtime smoke tests) exercises none of them.

Why this is worth fixing

REVIEW.md, under Correctness: the bug class, not the bug → "Fix the whole class in the same PR — grep for every sibling site sharing the pattern" and "Signature changes and renames → grep the whole repo". The PR explicitly recognized this class by editing error.rs:40, so the three remaining sites are same-class stragglers. Left as-is, a future reader grepping for ParseResult::Fail (from these comments) will find no such symbol.

Step-by-step proof

  1. Before this PR, ParseResult::Fail is a real enum variant (src/sourcemap/lib.rs:108 in the base).
  2. This PR deletes it: -pub enum ParseResult { Fail(...), Success(...) } / +pub type ParseResult = Result<...>.
  3. This PR updates one comment referencing it: error.rs:40 ParseResult::Fail → ParseResultFail.
  4. rg 'ParseResult::Fail' on the post-PR tree still returns 3 hits (listed above) — none of them in Rust, so they were missed by the mechanical Fail( → Err( / Success( → Ok( sweep.

How to fix

Update the three comments to say ParseResultFail (or Err(ParseResultFail) where the phrasing describes the return value), matching the change already made at error.rs:40. Zero behavior impact.

pub fn message(self) -> &'static str {
match self {
Self::MissingGeneratedColumnValue => "Missing generated column value",
Expand Down
13 changes: 4 additions & 9 deletions src/sourcemap/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,7 @@ pub struct ParseUrl {
pub source_contents: Option<Box<[u8]>>,
}

pub enum ParseResult {
Fail(ParseResultFail),
Success(ParsedSourceMap),
}
pub type ParseResult = Result<ParsedSourceMap, ParseResultFail>;

pub struct ParseResultFail {
pub loc: bun_ast::Loc,
Expand Down Expand Up @@ -953,7 +950,7 @@ pub(crate) fn parse_json(source: &[u8], hint: ParseUrlResultHint) -> crate::Resu
};

let map: Option<Arc<ParsedSourceMap>> = if !source_only {
let mut map_data = match mapping::parse(
let mut map_data = mapping::parse(
mappings_vlq,
None,
i32::MAX,
Expand All @@ -968,10 +965,8 @@ pub(crate) fn parse_json(source: &[u8], hint: ParseUrlResultHint) -> crate::Resu
),
sort: true,
},
) {
ParseResult::Success(x) => x,
ParseResult::Fail(fail) => return Err(fail.err),
};
)
.map_err(|fail| fail.err)?;

if let ParseUrlResultHint::All {
include_names: true,
Expand Down
6 changes: 3 additions & 3 deletions src/sourcemap_jsc/JSSourceMap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use bstr::BStr;

use bun_core::{self as bstring, strings};
use bun_jsc::{CallFrame, JSGlobalObject, JSValue, JsResult, StringJsc as _, bun_string_jsc};
use bun_sourcemap::{Mapping, Ordinal, ParseResult, ParsedSourceMap, mapping};
use bun_sourcemap::{Mapping, Ordinal, ParsedSourceMap, mapping};

// generate-classes.ts does not emit Rust accessors yet, so the
// `to_js`/cached-setter helpers below forward to the codegen-emitted C++
Expand Down Expand Up @@ -157,8 +157,8 @@ impl JSSourceMap {
);

let mapping_list = match parse_result {
ParseResult::Success(parsed) => parsed,
ParseResult::Fail(fail) => {
Ok(parsed) => parsed,
Err(fail) => {
if let Some(loc) = fail.loc.to_nullable() {
return Err(global.throw_value(global.create_syntax_error_instance(
format_args!("{} at {}", fail.err.message(), loc.start),
Expand Down