Conversation
The four stringifiers decided "is this an array" with JSValue::is_array, which checks the cell type. A Proxy is never an array cell, so a Proxy whose target is an array was written as an object with index keys (YAML, JSON5, TOML) or rejected with "'0' is not a valid XML element name" (XML). Array.isArray and JSON.stringify use ECMA-262 IsArray, which looks through a Proxy. Add JSValue::is_array_including_proxy (JSC::isArray) and use it at every array check in the four stringifiers. It throws a TypeError for a revoked Proxy. A value that is not a Proxy does not cross FFI. JSArrayIterator needs no change. For a value that is not a JSArray it already reads "length" and each index with [[Get]]. xml.test.ts: the deep values test uses depth 100,000 on a debug build, where it otherwise runs longer than the 5 s timeout.
|
Status Reproduced on bun 1.4.3-canary (b993710), release build: const p = new Proxy([1, 2, [3]], {});
JSON.stringify(p); // [1,2,[3]]
Array.isArray(p); // true
Bun.YAML.stringify(p); // {"0": 1,"1": 2,"2": [3]}
Bun.JSON5.stringify(p); // {'0':1,'1':2,'2':[3]}
Bun.TOML.stringify({ a: p }); // [a]\n0 = 1\n1 = 2\n2 = [3]\n
const q = new Proxy(["1", "2"], {});
Bun.XML.stringify({ r: { a: q } }); // throws: '0' is not a valid XML element nameWith Fix: this PR, #42650. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughThe change adds ECMAScript ChangesProxy-aware array serialization
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No merge-blocking risk is identified for the proxy-array serialization change. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 1:20 PM PT - Sep 13th, 2026
✅ @robobun, your commit ff06a8c9af022f65c4ee007d735a190fae667274 passed in 🧪 To try this PR locally: bunx bun-pr 42650That installs a local version of the PR into your bun-42650 --bun |
The doc on is_array_including_proxy, the next method, already states how the two differ.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This branch has not been deployed
1 participant
Problem
p = new Proxy([1, 2, [3]], {}),Bun.YAML.stringify(p)returns{"0": 1,"1": 2,"2": [3]}. JSON5 and TOML also write index keys.Bun.XML.stringify({ r: { a: new Proxy(["1"], {}) } })throws'0' is not a valid XML element name.JSON.stringify(p)returns[1,2,[3]].JSValue::is_array()(src/jsc/JSValue.rs:284). It tests the cell type, and a Proxy is never an array cell.Fix
JSValue::is_array_including_proxy(JSC::isArray, the ECMA-262IsArray). It replaces all 19is_array()calls in the four stringifiers. A non-Proxy value makes no FFI call. A revoked Proxy throws aTypeError, as before.JSArrayIteratoralready readslengthand each index of a non-JSArrayvalue with[[Get]].a Proxy of an arrayblocks intest/js/bun/{yaml,json5,toml,xml}/*.test.ts. Release 1.4.3-canary fails 14 of the 18 tests. Also the four conformance suites andexpect.test.js.Background
IsArray(ECMA-262 7.2.2) is true for an Array and for a Proxy whose target is an array.Array.isArrayandJSON.stringifyuse it.JSValue::is_array()reads the type byte of the cell. A Proxy has theProxyObjecttype, whatever its target is.JSON.stringifyskips it (IsCallable). The stringifiers write{}(is_function(), 19 sites). That is a separate change.Notes
[1,2,[3]], yaml 2.8.1 and js-yaml 4.1.0- 1\n- 2\n- - 3\n, smol-toml 1.4.2a = [ 1, 2, [ 3 ] ], fast-xml-parser 5.2.5<r><a>1</a><a>2</a></r>.TypeError(YAML, JSON5, TOML), and a Proxy array that contains itself is a circular structure in JSON5. In XML a revoked Proxy aschildrenthrew a plainErrorbefore.IsArraymessage of JSC,Array.isArray cannot be called on a Proxy that has been revoked. Before, theownKeysstep threwProxy has already been revoked. No more operations are allowed to be performed on it. Both areTypeErrors.ownKeys,getOwnPropertyDescriptorper key, thengetper key. This branch, per pass over the array:has length, get length, get 0, get 1.JSON.stringify:get toJSON, get length, get 0, get 1. The extrahas lengthcomes fromJSValue::get_length, which usesgetIfPropertyExists.JSArrayIteratoralso truncates alengthaboveu32::MAX, whereJSON.stringifythrowsRangeError: Out of memory. Both belong to the shared iterator (93 call sites) and are not changed here.xml.test.tsdepth change, as a duplicate of XML.stringify: throw for a stringspacethat is not XML whitespace #42531. Without it that file times out on a debug build, so it stays. (2) Add doc lines onJSValue::is_arrayandJSType::is_array. The comment check of this repository flagged them, so they are removed.JSArrayIterator::initto read the length of a Proxy withLengthOfArrayLike. That changesBun.spawn, theexpectmatchers and other callers with no test there, so it is out of this PR.RedisClient.hset(key, proxy)writes fields0and1,new Bun.CookieMap(proxy)is empty, the$template joins the items into one argument,node:http2joins header values into one string. Each API has its own contract. This PR does not change them.mainbut not on top of this branch: it conflicts inJSValue.rs,bindings.cpp,TOMLObject.rsandYAMLObject.rs. The helper hunks sit at the same place on purpose, so the second PR to land gets a textual conflict and not a duplicate definition. Its rebase must keepis_array_including_proxyat the value checks.is_array_or_proxied_arrayforbun:testmatchers. It returnsfalsefor a revoked Proxy and does not throw, so it is a different predicate.xml.test.ts: the "deep values are a catchable error" test builds 2 million nested objects. On a debug ASAN build that takes 13 s against the 5 s timeout, with no XML code involved. It now uses depth 100,000 on a debug build. XML.stringify: throw for a stringspacethat is not XML whitespace #42531 makes the identical change, so the two merge cleanly in either order.BUN_JSC_validateExceptionChecks=1: they pass.[human-review] gate passed · iteration 0 · 10 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file