yaml: map StringifyError to JsError in one place - #39116
Conversation
YAML.stringify matched on StringifyError twice, once after find_anchors_and_aliases and once after stringify, with identical arms. Move the mapping into StringifyError::to_js_error and call it from both sites, and drop the corresponding same_match_twice entry from mordant-baseline.toml.
|
Warning Review limit reached
Next review available in: 27 seconds 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 (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical dedup of two identical StringifyError match blocks into one helper.
What was reviewed:
- Verified
to_js_errormaps all three variants identically to the removed inline matches (OutOfMemory→JsError::OutOfMemory,JsError→JsError::Thrown,StackOverflow→global.throw_stack_overflow()). - Confirmed
throw_stack_overflow()returnsJsError, so.map_err(...)?preserves the exact control flow of the oldif let Err ... return match. - The removed
mordant-baseline.tomlentry matches the fixed lint (same_match_twice:src/runtime/api/YAMLObject.rs).
Extended reasoning...
Overview
This PR refactors YAML.stringify in src/runtime/api/YAMLObject.rs to eliminate a duplicated 3-arm match on StringifyError. The two identical inline matches (one after find_anchors_and_aliases, one after stringify) are replaced with a single StringifyError::to_js_error(self, global) -> JsError helper, called via .map_err(|err| err.to_js_error(global))?. The corresponding same_match_twice entry is removed from mordant-baseline.toml.
Security risks
None. This is a pure code-motion refactor of an internal error-mapping enum with no change to inputs, outputs, or observable behavior. No user-controlled data flows differently.
Level of scrutiny
Low. The transformation is mechanical and provably behavior-preserving: each of the three match arms in the new helper produces the same JsError value as before, and .map_err(f)? on Result<(), E> is semantically identical to if let Err(e) = ... { return Err(f(e)); }. I confirmed throw_stack_overflow() returns JsError directly (src/jsc/JSGlobalObject.rs:92), so no wrapping/unwrapping changed. The #[cold] hint is appropriate for an error-only path.
Other factors
- The PR description states existing tests (
throws on BigInt,handles Proxy that throws,handles stack overflow protection) exercise the mapped error variants and pass with the debug build. - This directly implements the REVIEW.md guidance ("the second time a multi-line block appears in your diff, extract a named helper") and tightens the mordant lint ratchet.
- No prior reviewer comments to address; no CODEOWNERS-sensitive paths touched.
### Problem - mordant's `reimplemented_helper` finding baselined for `src/jsc/webcore_types.rs`: `Blob::is_bun_file` and `Blob::needs_to_read_file` had the same signature and the same body (the store is `Some` and its data is `Store::File`), so a change to one would have silently missed the other. - The duplication is inherited, not a porting mistake: `Blob.zig` already had `isBunFile` and `needsToReadFile` with the same body, and neither side has changed since the port (#30412). Nothing relies on them differing. ### Fix - Delete `is_bun_file` and keep `needs_to_read_file`, which is what the ~40 other call sites (and the `blob_needs_to_read_file` hook that `bun_sql_jsc` goes through) already use. - Point the two `is_bun_file` callers at it: `Response.rs` (`getCompleteWebRequestOrResponseBodyValueAsArrayBuffer` returns `undefined` for a file-backed body) and `CryptoHasher.rs` (the synchronous hashers reject `Bun.file()` input). Same predicate, so no behavior change. - Remove the `reimplemented_helper:src/jsc/webcore_types.rs` entry from `mordant-baseline.toml`, and the two stale comments in `webcore/Blob.rs` that named the deleted method. The entry is removed by hand rather than by regenerating the file: a full `bun run rust:mordant:baseline` on Linux also drops two unrelated entries (`always_unwrapped_option` in `PackageInstall.rs`, `narrowed_two_ways` in `node_crypto_binding.rs`) that this PR has no business touching. - No new test: there is no observable behavior to pin (the two predicates were byte-for-byte the same), so no test can distinguish before from after. The `Bun.file()` rejection at the `CryptoHasher` call site is already covered by `bun-cryptohasher.test.ts` ("Bun.file in CryptoHasher is not supported yet"), and the mordant run below is the check for the finding itself. Same shape as #39124, #39116 and #39135 from this batch. - Verified: - `bun run rust:mordant` (full workspace) on this branch: clean, no `target/mordant/over-baseline.txt`. With main's `webcore_types.rs` restored on top of the trimmed baseline it reports `reimplemented_helper ... over the mordant baseline (0 recorded for src/jsc/webcore_types.rs)` and `1 finding(s) over the baseline in bun_jsc`, so the entry was this site and the lint no longer fires. - The `mordant` workflow also runs on this PR since the baseline changed; with the entry removed it fails if the finding is still reported. - `bun bd test test/js/bun/util/bun-cryptohasher.test.ts` (covers the `CryptoHasher` call site: `Bun.file()` input still throws): 402 pass. - `bun bd test` on `test/js/web/fetch/{blob,blob-write,body,blob-file-name-ownership,response}.test.ts`, `test/js/web/structured-clone-blob-file.test.ts` and `test/js/bun/io/bun-write.test.js` (`--timeout 60000`, since several of these spawn a debug+ASAN child and exceed the 5s default): everything passes except `blob.test.ts` "Bun.file(path).slice(start, end) streams only the slice", which fails identically on a clean checkout of main (88a6398) and is being handled separately; it is a `FileReader` streaming bug unrelated to this predicate. - Two open PRs add `is_bun_file()` calls in `Blob.rs` (#32434, #33659); whichever lands after this one needs those calls renamed to `needs_to_read_file()`. That is a compile error, not a silent change. ### Background - `Blob` (`src/jsc/webcore_types.rs`) is a view (offset + size) onto a refcounted `Store`, whose `data` is one of `Bytes` (in memory), `File` (a path or fd; what `Bun.file()` creates) or `S3`. `needs_to_read_file()` asks whether the blob is `File`-backed, i.e. whether its bytes have to be read off disk before anything in memory can look at them; synchronous consumers use it to bail out. - mordant is the advisory Rust lint pack the `rust-lints` workflow runs (`bun run rust:mordant`). `mordant-baseline.toml` holds the per-(lint, file) counts of findings that predate the job, so CI only fails when a count goes up; once a baselined finding is fixed its entry is deleted. Co-authored-by: Alistair Smith <hi@alistair.sh>
2 participants
Problem
YAML.stringify(src/runtime/api/YAMLObject.rs:42-56) matched onStringifyErrortwice, once afterfind_anchors_and_aliasesand once afterstringify, with identical arms. A change to one copy would miss the other.same_match_twice:src/runtime/api/YAMLObject.rsentry inmordant-baseline.toml.Fix
StringifyError::to_js_error(self, global) -> JsErrorholding the one mapping; both call sites become.map_err(|err| err.to_js_error(global))?.OutOfMemoryandJsErrorare passed through for the host-fn wrapper,StackOverflowthrows the range error), only the location moved.mordant-baseline.toml.bun bd test test/js/bun/yaml/yaml.test.ts test/regression/issue/23489.test.ts(644 pass, 18 pre-existing todo, 0 fail). The existing tests cover the thrown-exception and stack-overflow paths (throws on BigInt,handles Proxy that throws,handles stack overflow protection).Background
StringifyErroris the error type the YAML stringifier's recursive walk returns internally;JsErroris the runtime-wide two-state error (Thrown: an exception is already pending on the VM;OutOfMemory: thehost_fnwrapper throws the OOM error). OnlyStackOverflowneeds an actual throw when crossing back into aJsResult.bun run rust:mordant;mordant-baseline.tomlholds per-(lint, file) counts of pre-existing findings, and a fixed finding's entry is deleted so the ratchet tightens.