Conversation
WalkthroughThe macro transpiler now routes unsupported macro values through a shared coercion error path, and blob serialization normalizes MIME types before deciding JSON or text inlining. Tests add coverage for unsupported platform objects and MIME values with parameters and case variation. ChangesMacro coercion and blob MIME classification
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, fix, expected behavior, documentation basis, and verification results. It does not use the exact template headings, but it includes the required information and is complete. Comment |
|
Reproduced on 1.4.1 before the fix: With this branch the build fails with The three new cases in Scope note: the first revision of this PR also fixed the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — thanks for folding in the case-insensitive essence match (and the extra tests for it).
Extended reasoning...
Overview
Two narrow fixes in src/js_parser_jsc/Macro.rs plus ~90 lines of new subprocess tests:
- The DOMWrapper fall-through in
Run::coercenow returns the standard "cannot coerce … to Bun's AST" build error instead of silently inlining""for unsupported platform objects (URL,Headers, …). The error-reporting logic was extracted intounsupported_value_error()and reused at all three fall-through sites, so the message format is now consistent (the bottom-of-coercepath gains the class name it previously lacked). expr_from_blobnow matches on the MIME essence — slice before;, ASCII-trimmed, lowercased via the existingstrings::copy_lowercase_if_neededhelper into a bump-arena scratch buffer — instead of byte-comparing the fullContent-Type. This restores the pre-port behavior whereapplication/json;charset=utf-8(the value real servers and theResponseconstructor produce) inlines the parsed object rather than a base64 data URL.
Since my earlier review on 36cc777, the author pushed 8998396 which addresses my case-sensitivity nit exactly as suggested (lowercase once, then compare) and added Application/JSON; charset=UTF-8 / TEXT/Plain test cases that exercise it.
Security risks
None. This is build-time macro return-value serialization; no auth, crypto, network, or filesystem surface is touched. No new unsafe blocks. The lowercasing buffer is sized exactly to essence.len() and copy_lowercase_if_needed writes at most that many bytes.
Level of scrutiny
Moderate — it's transpiler/bundler core, but the blast radius is confined to the macro return-value coercion path. The MIME change is strictly widening (every value that matched before still matches). The platform-object change is a deliberate behavior change from silently-wrong ("") to a build error, which matches the documented contract in docs/bundler/macros.mdx and the existing handling for every other unsupported type.
Other factors
- No CODEOWNERS entry covers
src/js_parser_jsc/or the test file. - The bug-hunting system found nothing on the latest revision.
copy_lowercase_if_neededandindex_of_charare established helpers already used the same way inresolver/fs.rsandhttp/h2_client/encode.rs.- The unconditional
bump.alloc_slice_fill_copy(essence.len(), 0)allocates ~20 bytes of arena scratch even when the essence is already lowercase — negligible, and the arena is per-MacroContext. - Tests are subprocess-based integration tests following the existing pattern in this file (
tempDir+Bun.spawn+ tail-match to tolerate the debug[macro] call …prefix), and the PR description confirms they fail on 1.4.0 and pass on this branch.
|
This PR needs a rebase onto #40059 once it lands, and half of it is covered there. Checked on a debug build of the #40059 branch with this PR's test cases:
On a rebase, keep the essence extraction in |
|
#40700 fixes the MIME parameter half of this PR on main. It routes the content type through |
…0700) ### Problem - A macro that returns `Response.json(...)`, a `fetch()` of a JSON endpoint, or a `Blob` typed `application/json; charset=utf-8` is inlined as a base64 `data:` URL string (`"data:application/json;charset=utf-8;base64,eyJhIjoxfQ=="`) instead of the parsed object. Same for a plain `application/json` header, because the Response to Blob step normalizes it to `application/json;charset=utf-8`. - `expr_from_blob` (`src/js_parser_jsc/Macro.rs:1058`) compares the raw content type with `== b"application/json"` and `starts_with(b"text/")`. Any parameter (`;charset=utf-8`) misses every branch and falls through to the data URL arm. The Zig code used `MimeType.init(...)` and `category.isTextLike()`. The Rust port replaced those with the byte compares. ### Fix - `expr_from_blob` calls `MimeType::init(content_type, false, None)` and branches on `mime_type.category`: `Json` parses the body, `is_text_like()` inlines a string, everything else stays a data URL. `MimeType::init` strips parameters. It is what `Body.rs` and `Blob.rs` already use to classify a blob type, so the macro now agrees with the runtime. - Adds `Category::is_text_like()` to `src/http_types/MimeType.rs`: javascript, html, text, css, json. This is the set the pre-port code used. `src/CLAUDE.md` already documents the method. - Adds `bun_http_types` to the `bun_js_parser_jsc` dependencies (Cargo.toml and Cargo.lock). - Verified: `test/bundler/transpiler/macro-test.test.ts` (new test, fails on 1.4.1 with the data URL output, passes with this change). Also ran the rest of that file, `transpiler.test.js`, the macro regression tests, and `test/js/web/fetch/blob.test.ts`. ### Background - A macro result is turned into an AST node by `Run::coerce`. A `Response` or `Request` is first reduced to its body `Blob`. `expr_from_blob` then picks the node shape from the blob's content type. - `bun_http_types::MimeType::init` parses `type/subtype;params` into a `MimeType` with a `Category`. Parameters are cut at the first `;`. `application/json` and `application/geo+json` map to `Category::Json`. `text/*` maps to `Text` (or `Css`, `Html`, `Javascript`, and `text/plain` to the `TEXT` constant). - `Response.json()` sets `application/json;charset=utf-8` (`MimeType::JSON`). A `Response` with a `content-type` header and no blob type gets its blob type from `MimeType::init` on that header (`src/runtime/webcore/Body.rs:2146`). <details><summary>Notes</summary> Repro on 1.4.1: ```ts // macro.ts export function j() { return Response.json({ a: 1 }); } // index.ts import { j } from "./macro.ts" with { type: "macro" }; console.log(j()); ``` `bun build index.ts` emits `console.log("data:application/json;charset=utf-8;base64,eyJhIjoxfQ==");`. With this change it emits `console.log({ a: 1 });`. Behavior that changes besides the parameter handling, because the classification now follows `MimeType::init`: - `application/javascript`, `application/x-javascript`, `application/ecmascript`, `application/xml` with no parameters were inlined as strings. They are `Category::Application` in `MimeType::init` and now become data URLs. With parameters they were data URLs before too. The pre-port code did the same. - `+json` suffixes other than `geo+json` (for example `application/ld+json`) with no parameters were parsed as JSON. They are `Category::Application` and now become data URLs. `text/json` is `Category::Text` and becomes a string. - The type is still case-sensitive, as it is everywhere else `MimeType::init` is used. A Blob built in JS is lowercased by the `Blob` constructor. A header value is used verbatim. The data URL arm keeps the raw content type in the URL (`data:<content type>;base64,...`), unchanged. Related: #32844 fixes the same parameter problem with string essence matching, plus a platform object error that #40059 covers. #40059 (`claude/macro-host`) also strips parameters inside `expr_from_blob` with string compares. This change conflicts with it only in the body of `expr_from_blob` and one Cargo.toml line. The `it.todo` cases in `transpiler.test.js` ("macros can return a Response body", "pass objects to macros") stay todo. They depend on `transformSync(code, ctx)`. That call appends `ctx` after the call arguments, so a macro called with no arguments receives `ctx` as its first parameter and the fixture's second parameter is `undefined`. That is separate from this change. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: BUILD FAILED (no junit output) $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/bundler/transpiler/macro-test.test.ts ninja: Entering directory `/workspace/bun/build/debug' [0/3] cargo bun_runtime → libbun_runtime.a FAILED: rust-target/x86_64-unknown-linux-gnu/debug/libbun_runtime.a /workspace/bun/build/release/bun /workspace/bun/scripts/build/stream.ts rust --console --cwd=/workspace/bun --env=CARGO_TERM_COLOR=always --env=BUN_CODEGEN_DIR=/workspace/bun/build/debug/codegen --env=CC=/usr/lib/llvm-21/bin/clang --env=CXX=/usr/lib/llvm-21/bin/clang++ --env=AR=/usr/lib/llvm-21/bin/llvm-ar --env=CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_LINKER=/usr/lib/llvm-21/bin/clang++ --env=CARGO_HOME=/root/.cargo --env=RUSTUP_HOME=/root/.rustup --env=RUSTUP_TOOLCHAIN=nightly-2026-07-20 --env=CARGO_PROFILE_RELEASE_LTO=off --env=CARGO_PROFILE_RELEASE_CODEGEN_UNITS=16 --env=CARGO_PROFILE_RELEASE_DEBUG_ASSERTIONS=true --env=CARGO_ENCODED_RUSTFLAGS='-Crelocation-model=static�-Cforce-frame-pointers=yes�-Zthreads=8�-Cllvm-args=-addrsig�-Ctarget-cpu=nehalem�--check-cfg=cfg(bun_asan)�-Zsanitizer=address�--cfg=bun_asan�--check-cfg=cfg(b ... (truncated) release without fix: 1 FAILED bun test v1.4.1-canary.1 (65362b5) test/bundler/transpiler/macro-test.test.ts: (pass) bun builtins can be used in macros [0.03ms] (pass) latin1 string (pass) ascii string (pass) type coercion [0.05ms] (pass) escaping [0.17ms] (pass) utf16 string [0.01ms] (pass) import aliases [0.02ms] (pass) default import (pass) namespace import [0.02ms] (pass) ireturnapromise [0.08ms] (pass) object argument with a sparse numeric key [15.56ms] (pass) object destructuring of a macro result keeps every bound property regardless of key order or repeated keys [16.09ms] 221 | stdout: "pipe", 222 | stderr: "pipe", 223 | }); 224 | const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); 225 | // Debug builds print "[macro] call <name>" to stdout before the script's own output. 226 | expect({ lastLine: stdout.trim().split("\n").pop(), stderr }).toEqual({ ^ error: expect(received).toEqual(expected) { - "lastLine": "[{"a":1,"b":[true,null,"x"]},{"b":2},{"from":"server"},"hello",{"c":3},"data:application/octet-stream;base64,AQID"]", + "lastLine": ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/bundler/transpiler/macro-test.test.ts bun test v1.4.1 (65362b5) test/bundler/transpiler/macro-test.test.ts: [macro] call escapeHTML [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call identity [macro] call escape [macro] call addStrings [macro] call addStrings [macro] call addStrings [macro] call addStrings [macro] call addStrings [macro] call addStrings [macro] call addStrings [macro] call addStrings [macro] call addStrings [macro] call addStrings [macro] call addStrings [macro] call addStrings [macro] c ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision a7583e5 features baseline 23 deps, 131 codegen, 1172 objects in 1405ms ninja: Entering directory `/workspace/bun/build/release' [1/1244] gen bindgenv2 [2/1244] fetch zlib [zlib] up to date [3/1244] fetch tinycc [tinycc] up to date [4/1243] fetch libjpeg-turbo [libjpeg-turbo] up to date [5/1216] install /workspace/bun bun install v1.4.1-canary.1 (65362b5) Checked 26 installs across 63 packages (no changes) [50.00ms] [6/1216] gen ErrorCode+*.h [7/1216] gen .bind.ts → GeneratedBindings.cpp [8/1216] gen ProcessBindingConstants.lut.h Generating /workspace/bun/build/release/codegen/ProcessBindingConstants.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingConstants.cpp [9/1216] install /workspace/bun/packages/bun-error bun install v1.4.1-canary.1 (65362b5) Checked 1 install across 2 packages (no changes) [3.00ms] [10/1216] gen JSBuffer.lut.h Generating /workspace/bun/build/release/codegen/JSBuffer.lut.h from /workspace/bun/src/jsc/bindings/JSBuffer.cpp [11/1216] gen b ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` Cargo.lock | 1 + src/http_types/MimeType.rs | 7 ++++ src/js_parser_jsc/Cargo.toml | 1 + src/js_parser_jsc/Macro.rs | 27 ++++++--------- test/bundler/transpiler/macro-test.test.ts | 55 ++++++++++++++++++++++++++++++ 5 files changed, 74 insertions(+), 17 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests Cargo.lock 0 0 0 src/http_types/MimeType.rs 1 1 0 src/js_parser_jsc/Cargo.toml 1 2 0 src/js_parser_jsc/Macro.rs 3 3 0 test/bundler/transpiler/macro-test.test.ts 1 2 0 ``` </details> <!-- robobun:evidence:end -->
8998396 to
8fc8265
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
A macro returning a platform object other than Response/Request/Blob (URL, Headers, FormData, ...) hit a fall-through in the DOMWrapper arm of Run::coerce that inlined an empty string at every call site, so the build succeeded with wrong constants baked in. Report the same build error every other unsupported return type gets.
8fc8265 to
1a5755b
Compare
robobun
changed the title
| expect({ stdout, stderr, exitCode }).toMatchObject({ | ||
| stderr: expect.stringContaining(`cannot coerce ${className}`), | ||
| exitCode: 1, | ||
| }); | ||
| // The bundle must not be emitted with the macro call replaced by an empty string. | ||
| expect(stdout).not.toContain('""'); |
There was a problem hiding this comment.
🟡 nit: expect(stdout).not.toContain('""') is a vacuous assertion that can never fail
Extended reasoning...
The preceding toMatchObject asserts exitCode: 1 and stderr contains the coercion error. When that assertion passes, bun build index.ts has failed and written nothing to stdout, so .not.toContain('""') is trivially true against an empty string. When it does not pass (the pre-fix behavior — build succeeds, exit 0, bundle on stdout containing ""), toMatchObject throws before this line is reached. Either way the stdout assertion never fires, so it does not guard the regression the comment above it describes; on the base branch the test fails on toMatchObject, not here. Per REVIEW.md ("Every assertion must be able to fail"), fold the stdout expectation into the same toMatchObject (e.g. stdout: expect.not.stringContaining('""')) so a single failure diff shows all three fields, or drop it.
Verification: nit — The assertion is vacuous as claimed. At test/bundler/transpiler/macro-test.test.ts (added block), the order is: expect({ stdout, stderr, exitCode }).toMatchObject({ stderr: expect.stringContaining(`cannot coerce ${className}`), exitCode: 1, }); // The bundle must not be emitted with the macro call replaced by an empty string. expect(stdout).not.toContain('""'); Two exhaustive…
|
Closing this in favor of #40700 and #40059. The MIME parameter half landed on main in #40700. The platform object half is covered by #40059, which reworks the same coercion path. Its |
1 participant
Problem
Response,Request, orBlob(Headers,FormData, ...) is inlined as""at every call site. The build succeeds with wrong constants baked in. On 1.4.1,bun buildof a file that callsexport function f() { return new Headers(); }emitsconsole.log(JSON.stringify(""))and exits 0.T::Privatearm ofRun::coerceinsrc/js_parser_jsc/Macro.rs. After theResponse/Request/Blobdowncasts miss, it returnsOk(Expr::init(E::EString::EMPTY, ..)). Every other unsupported return type (Symbol, Date, function,URLthrough its custom inspect hook) already reportscannot coerce <Class> (<JSType>) to Bun's AST. Please return a simpler type.Fix
Run::unsupported_value_error, and the two existing error sites call it too, so the bottom-of-coercepath gains the class name it lacked.docs/bundler/macros.mdx("Serializability") says instances of classes other than those three are not serializable. A build error is the documented contract. A silent placeholder is the worst outcome for a build-time inliner.test/bundler/transpiler/macro-test.test.tsgains three cases (Headers,FormData, an object with nestedHeaders). They fail on 1.4.1 with""inlined and pass with this change. The rest of that file,transpiler.test.js, and the macro regression tests also pass.Background
Run::coerceturns its JS return value into an AST node by the formatter tag of the value.T::Privateis the tag for aDOMWrappercell, that is, a native platform object.Response/Requestare reduced to their bodyBlob, and aBlobis inlined by its content type inexpr_from_blob. Those three are the only platform objects with an AST shape.Notes
The first version of this PR also fixed
expr_from_blob, which compared the wholeContent-Typeagainstapplication/jsonand so demotedapplication/json;charset=utf-8to a base64 data URL. #40700 landed that fix on main throughMimeType::init, so this PR now carries only the platform object change. #40700 keeps the type match case-sensitive on purpose, to agree with the runtime's blob classification, so the case-insensitive variant from an earlier revision of this PR is dropped too.#40059 rewrites the whole coercion path and makes the fall-through impossible as a side effect. This is the small fix that can land ahead of it.
Probe on 1.4.1 of what each platform object becomes when returned from a macro:
Related: #7115
[review] gate passed · iteration 6 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 6
evidence per changed file