text formats: name the input flags passed to with_text_format_source - #39144
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 27 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 74 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. 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 (9)
Comment |
There was a problem hiding this comment.
LGTM — mechanical bool-to-enum refactor with no behavior change.
Checked that each enum variant maps to the same branch as the bool it replaces (Bytes↔true, Throw↔true, AsIs↔true) at all three if sites in with_text_format_source_encoded.
Verified all five callers (TOML/JSONC/JSON5/YAML/XML) plus the with_text_format_source wrapper pass the variant matching their previous literal — JSONC keeps ToString for buffers, YAML keeps ToString for nullish, XML keeps AsIs for strings, wrapper forces Utf8.
Confirmed no other callers of either helper exist in the tree, and the removed mordant-baseline.toml line matches the two sites eliminated.
Extended reasoning...
Overview
Replaces three positional bool parameters of the shared Bun.{TOML,JSONC,JSON5,YAML,XML}.parse scaffold (with_text_format_source / with_text_format_source_encoded in src/runtime/api.rs) with three private two-variant enums (BlobOrBufferInput, NullishInput, StringInput), updates all five call sites plus the wrapper, updates two doc comments referencing the old parameter name, and removes the corresponding bare_bool_args:src/runtime/api.rs = 2 line from mordant-baseline.toml.
Security risks
None. This is a type-level refactor of internal function signatures; no new code paths, no changed input handling, no changed error handling. The three condition checks in the helper body are semantically identical to the bools they replace.
Level of scrutiny
Low. The change is mechanical and the compiler enforces exhaustiveness — a swapped or missed argument would fail to compile. I traced each old literal to its new variant and each condition to confirm polarity is preserved: accept_blob_or_buffer=true → BlobOrBufferInput::Bytes (checked with == Bytes), reject_nullish=true → NullishInput::Throw (checked with == Throw), string_passthrough=true → StringInput::AsIs (checked with == AsIs), and the wrapper's hardcoded false → StringInput::Utf8. Every caller's before/after pair matches.
Other factors
A repo-wide grep confirms the two helpers have no callers outside the five modified files, so nothing is missed. The enums are crate-private and derive only Clone, Copy, PartialEq, Eq — no public surface change. The PR description includes a 46-case before/after probe with byte-identical output and passing test suites for all five formats. No prior reviewer comments to address and no CODEOWNERS entries for the touched paths.
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with no behavior change.
Checked that each enum variant maps to the same branch as the bool it replaces (Bytes↔true, Throw↔true, AsIs↔true; wrapper's Utf8↔false), and that all five call sites (TOML/JSONC/JSON5/YAML/XML) pass the variant equivalent to their previous literal. The three if tests in with_text_format_source_encoded compare against the same-polarity variant. New tests cover the two odd-one-out call sites (JSONC's ToString for Blob, YAML's ToString for nullish) that had no prior coverage.
Extended reasoning...
Overview
Replaces three positional bool parameters on with_text_format_source / with_text_format_source_encoded in src/runtime/api.rs with three two-variant enums (BlobOrBufferInput, NullishInput, StringInput), updates the five callers (TOML/JSONC/JSON5/YAML/XML parse), removes the corresponding bare_bool_args line from mordant-baseline.toml, and adds two small test blocks in jsonc.test.ts and yaml.test.ts covering the input-handling variants that differ from the majority.
Security risks
None. This is a type-level rename with no change to input validation, allocation, or control flow. The enum comparisons preserve the exact polarity of the previous bool checks.
Level of scrutiny
Low. The diff is mechanical and verifiable by inspection: each old true/false maps to a named variant, and each if <bool> becomes if <enum> == <variant-for-true>. I traced every call site's old literals to its new variants — TOML (true,true)→(Bytes,Throw), JSONC (false,true)→(ToString,Throw), JSON5 (true,true)→(Bytes,Throw), YAML (true,false)→(Bytes,ToString), XML (true,true,true)→(Bytes,Throw,AsIs), wrapper's hardcoded false→Utf8 — and the helper body's three conditionals to confirm no polarity was flipped.
Other factors
The PR description includes a 46-case before/after probe showing byte-identical output on main vs. this branch, and the author ran the full text-format test suite plus mordant. The added tests are simple, deterministic, and follow harness conventions (no sleeps, no network, placed in existing test files). No outstanding reviewer comments.
|
Status: rebased onto main as 57028f5 (the mordant sweep landing on main deleted the baseline lines next to this PR's line, which made it conflict; the resolution is still exactly the one removed line). The rebase also folds two doc comments down to one line each; the code is otherwise the one reviewed above. Rebuilt and re-ran the TOML/JSONC/JSON5/YAML/XML suites and the 46-case probe on the rebased branch: unchanged. Previous head fe762d8, build 98432: 177 of 179 jobs passed. The other two were the |
…ffold with_text_format_source and with_text_format_source_encoded took two and three bool parameters, and every caller passed bare true/false, so a call like with_text_format_source(.., true, true, ..) did not say which flag was which and a swapped call would still compile. Each flag is now a two-variant enum (BlobOrBufferInput, NullishInput, StringInput) that the call sites spell out. No behavior change. Removes the matching bare_bool_args entry from mordant-baseline.toml.
YAML.parse is the one caller that stringifies undefined/null instead of throwing, and JSONC.parse the one that stringifies a Blob or Buffer instead of reading its bytes; neither had a test.
fe762d8 to
57028f5
Compare
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with no behavior change.
What was reviewed:
- Verified each enum comparison in
with_text_format_source_encodedmaps to the same polarity as the bool it replaces (Throw↔true,Bytes↔true,AsIs↔true), and the wrapper's hardcodedStringInput::Utf8matches the oldfalse. - Checked all five call sites (TOML/JSONC/JSON5/YAML/XML) pass the variant matching their prior bool literal.
- New tests in
jsonc.test.tsandyaml.test.tscover the two divergent call sites (JSONC'sToStringblob handling, YAML'sToStringnullish handling); assertions are specific and non-flaky.
Extended reasoning...
Overview
This PR replaces three positional bool parameters on with_text_format_source / with_text_format_source_encoded in src/runtime/api.rs with three private two-variant enums (BlobOrBufferInput, NullishInput, StringInput), updates the five call sites in {TOML,JSONC,JSON5,YAML,XML}Object.rs to spell out the variant they pass, removes the corresponding bare_bool_args:src/runtime/api.rs = 2 line from mordant-baseline.toml, and adds two small test blocks covering the call sites whose flag values differ from the majority.
Security risks
None. This is a type-level refactor of an internal helper's parameter list; no user-facing behavior, parsing logic, allocation, or input validation changes. The three if conditions in the helper body are rewritten from if bool_flag to if enum_param == Variant with the same truth value at every call site.
Level of scrutiny
Low. The change is mechanical and self-verifying: swapping the enum variants at a call site would now be a visible semantic choice rather than a silent true, true → true, false typo, which is the point of the lint being cleared. I traced each of the five call sites' old bool literals against the new enum variant and confirmed the mapping is exact (TOML/JSON5: true,true→Bytes,Throw; JSONC: false,true→ToString,Throw; YAML: true,false→Bytes,ToString; XML: true,true,true→Bytes,Throw,AsIs; wrapper's false→Utf8). The SourceEncoding doc comments are updated to reference the new enum name.
Other factors
The PR description includes a 46-case before/after probe showing byte-identical output, and CI (build 98432) passed 177/179 with the two non-passing jobs being darwin agent-unavailable cancellations unrelated to the diff. The mordant job passed with the baseline line removed, confirming the lint no longer fires. The new tests follow harness conventions (specific error messages via toThrow("..."), toBe/toEqual on exact values, no sleeps, no network). No outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with no behavior change.
What was reviewed:
- Verified each enum variant maps to the same branch as the old bool:
Bytes↔true,Throw↔true,AsIs↔true, and the wrapper's hard-codedfalse→StringInput::Utf8. - Checked all five call sites (TOML/JSONC/JSON5/YAML/XML) pass the variant matching their previous literal.
- New tests cover the two divergent call sites (YAML nullish→ToString, JSONC Blob→ToString) with specific error classes/messages.
Extended reasoning...
Overview
Replaces three positional bool parameters on with_text_format_source / with_text_format_source_encoded in src/runtime/api.rs with three two-variant enums (BlobOrBufferInput, NullishInput, StringInput), updates the five call sites in {TOML,JSONC,JSON5,YAML,XML}Object.rs, removes the corresponding bare_bool_args line from mordant-baseline.toml, and adds regression tests to jsonc.test.ts and yaml.test.ts for the two call sites whose flag values differ from the rest.
Security risks
None. This is a type-level refactor of internal helper signatures; no user-facing input handling, validation, or control flow changes. The three if conditions in the helper body compare against the enum variant that corresponds exactly to the old true value, so every branch is taken under the same conditions as before.
Level of scrutiny
Low. This fits the ongoing mordant lint-sweep pattern visible in recent commits on main (#39152, #39166, #39147, #39143 — each replacing bare bools with named enums and dropping a baseline line). The change is mechanical and the type system enforces that every call site was updated. I traced each old literal to its new variant and confirmed the helper body's three comparisons preserve polarity: accept_blob_or_buffer (true→Bytes), reject_nullish (true→Throw), string_passthrough (true→AsIs, wrapper's false→Utf8).
Other factors
The PR description includes a 46-case before/after probe showing byte-identical output across all five formats and both values of every flag. The new tests assert specific error messages ("Expected a string to parse") and error classes (SyntaxError) rather than bare toThrow(), and are placed in the existing test files for their modules. The removed baseline line means CI's mordant job will fail if a bare bool is reintroduced here.
Problem
with_text_format_source(src/runtime/api.rs) takes two positional bools,accept_blob_or_bufferandreject_nullish, and all four callers (Bun.TOML.parse,Bun.JSONC.parse,Bun.JSON5.parse,Bun.YAML.parse) pass bare literals for both. Atwith_text_format_source(.., b"input.toml", true, true, ..)nothing says which flag is which, and swapping them compiles.with_text_format_source_encodedadds a third bool,string_passthrough, andBun.XML.parsecalls it withtrue, true, true.bare_bool_argsfindings mordant has baselined forsrc/runtime/api.rs.Fix
api.rs, and every call site spells out what it sets:BlobOrBufferInput::{Bytes, ToString}replacesaccept_blob_or_buffer(JSONC is the oneToStringcaller).NullishInput::{Throw, ToString}replacesreject_nullish(YAML is the oneToStringcaller).StringInput::{Utf8, AsIs}replacesstring_passthrough(XML is the oneAsIscaller; thewith_text_format_sourcewrapper passesUtf8).StringInput::Utf8itself, and a separate parameter keeps that from being settable through the wrapper.iftests in the helper body compare against the enum instead of reading the bool; the values every caller passes are unchanged, so there is no behavior change."bare_bool_args:src/runtime/api.rs" = 2line is removed frommordant-baseline.toml.yaml.test.ts:YAML.parsestringifiesundefined,nulland a missing argument instead of throwing (NullishInput::ToString).jsonc.test.ts:JSONC.parsethrows on nullish input, and stringifies everything else (aBlobincluded) rather than reading bytes (BlobOrBufferInput::ToString).Buffer/Blobtests intoml.test.ts,json5.test.ts,xml.test.ts; thexml.test.tsencodings block forStringInput::AsIs; Latin-1 range strings intoml.test.tsfor the wrapper'sUtf8.mordantCI job fails ifbare_bool_argsfires inapi.rsagain.bun bd testontest/js/bun/{toml,jsonc,json5,yaml,xml}/*.test.ts,test/js/bun/resolve/toml/toml-parse.test.ts,test/js/bun/resolve/jsonc.test.tsand the YAML regression tests: 4718 pass. Three stringify/leak-loop tests hit the 5s timeout under the local debug+ASAN build; two of them time out identically on an unmodified build and the third passes when run alone, and all three pass on main's CI (build 98374).parsewith a string,Buffer,Blob,undefined,null, no argument, a number and atoStringobject, plus XML with Latin-1 and UTF-16 strings and declared-encoding buffers (46 cases, covering both values of all three flags), produces byte-identical output before and after (see below).rustfmt --checkon the touched files.bun run rust:mordantwith this baseline: nothing over the baseline, locally and in themordantjob on this PR. As a check that the local run was live, deleting thetypes.rsbare_bool_argsline as well makes it report exactly those 4 findings and still nothing forapi.rs.bun run rust:mordant:baselinereproduces the committed file (the only other lines it dropped at the time have since been removed on main by their own PRs).Background
Bun.TOML.parse,Bun.JSONC.parse,Bun.JSON5.parse,Bun.YAML.parseandBun.XML.parseshare one scaffold,with_text_format_source: it takes argument 0 of the call, turns it into bytes, and hands the bytes to a per-format closure that runs the parser and converts the result to JS. Outside the closure, the formats differ only in how argument 0 is treated, which is what these flags select.BlobOrBufferInput: whether aBlob,ArrayBuffer, typed array orDataViewargument is parsed as bytes. JSONC mirrorsJSON.parseand instead runs every non-string argument through JSToString(so aBufferstill parses viaBuffer.prototype.toString, while aBlobbecomes"[object Blob]"and fails to parse).NullishInput: whetherundefined/null(including a missing argument) throwsExpected a string to parse. YAML instead stringifies them, soBun.YAML.parse()returns the string"undefined"andBun.YAML.parse(null)returnsnull.StringInput: whether a JS string is re-encoded to UTF-8 before parsing or handed over as its own Latin-1 bytes or UTF-16 code units along with aSourceEncodingtag. Only XML wants the latter, because a string is already-decoded text whoseencoding="..."declaration must not be applied to it.bun run rust:mordant(pinned inCargo.toml);mordant-baseline.tomlrecords the pre-existing findings per lint and file, so fixing a site means deleting its line.Before/after probe
Output of the probe script below is byte-identical on main (88a6398) and with this change:
Probe script: