Repository navigation
Conversation
|
Updated 5:36 PM PT - Nov 10th, 2026
✅ @robobun, your commit 851defdcd981be2bf0c4346bce2918a9b3154744 passed in 🧪 To try this PR locally: bunx bun-pr 45564That installs a local version of the PR into your bun-45564 --bun |
|
Status: CI is green on Reproduced on 1.4.3 and on main This branch prints The 9 new tests in The design point from the first review is settled on main. Since #44833 the property walk skips own non-enumerable keys. An array prints no non-enumerable key, alone or beside an enumerable one, as in Node. A new test entry holds that for an empty array. No known output differs from main outside the bug. A review found one that did: an empty array whose only key is A review also found that the dev server console formatter called the enumerable getters of an empty array. Since #47563 is open on the same statement in An earlier version of this comment said that this PR does not close #22569. That was based on a draft of #45304. The merged #45304 keeps the four result properties in |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughArray inspection now displays enumerable named properties on empty arrays. The change updates native and bake-client formatting and adds regression tests for inspection and console output, including rowless SQLite results. ChangesArray inspection
Possibly related PRs
Suggested reviewers: Severity of issue fixed: Low Merge Risk:
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/jsc/ConsoleObject.rs— nit, pre-existing: callers ofBun.inspect(arr, { sorted: true })get the named keys of an array in insertion order, and this PR's new test certifies that. ConsoleObject.rs:4542 always usesfor_each_property_non_indexed, never the ordered walk thatprint_objectpicks at ConsoleObject.rs:5468 whenordered_propertiesis set. The change routes empty keyed arrays through the same walk, and inspect.test.js:1160 records"[\n total: 7001, b: 2\n]"under thesortedkey. Fix: whenordered_propertiesis set, sort the named keys of arrays (elements first, then sorted keys, as Node does) for empty and non-empty arrays alike, and update thesortedandsortedWithOneElementexpectations instead of pinning the unsorted output.Why this was flagged
A user calls
Bun.inspect(Object.assign([], { total: 7001, b: 2 }), { sorted: true });sortedis documented in packages/bun-types/bun.d.ts:4972-4974 as sorting the properties and setsordered_propertiesat ConsoleObject.rs:1278.print_arrayreaches the named-property walk at ConsoleObject.rs:4542 and callsfor_each_property_non_indexedunconditionally, whileprint_objectat ConsoleObject.rs:5467-5472 callsfor_each_property_orderedfor the same option. The output is[\n total: 7001, b: 2\n]withtotalbeforeb. The base branch printed[]for this input and had the same unsorted order forObject.assign([1], {...}), so this is pre-existing for non-empty arrays and newly visible for empty ones. The new expectation at test/js/bun/util/inspect.test.js:1160 (andsortedWithOneElementat the next line) records the unsorted order, so the test now locks in the behaviour rather than checking it.Verification: pre-existing. Trigger: any caller passing
{ sorted: true }to Bun.inspect on an array with named (non-index) keys.print_arrayat ConsoleObject.rs:4542 callsvalue.for_each_property_non_indexed(...)unconditionally, with no sort step. The base commit has the same unconditional call. The new test at test/js/bun/util/inspect.test.js pinssorted: "[\n total: 7001, b: 2\n]".
console.log and Bun.inspect printed `[]` for an array that has no elements and has an own named property. print_array returned on `len == 0` before its named-property pass. print_array now reads the length in one call that runs no JS and that also says whether an array with no elements has an enumerable named property (Bun__JSArray__lengthAndNamedKeys). Such an array continues into the named-property pass, and the first property that prints writes the opening bracket. Past the depth limit it prints the `[Array ...]` marker, like an array with elements. The dev server console formatter (bake/client/inspect.ts) had the same early return. It prints the keys now. The console-log.js cases are from #23324. Co-authored-by: Pranav Joglekar <pranav2000joglekar@gmail.com>
d213a1d to
a4cc20b
Compare
|
Two review points that are not on a diff line.
Linked issue #22569. The first version of this PR did not close it, because a draft of #45304 made the |
There was a problem hiding this comment.
I re-reviewed the rewritten revision (a4cc20b, 91bfe1c) and found no new bugs; because this changes user-visible console.log/Bun.inspect output and adds a deferred-bracket state to the array printer, a human look is still worthwhile.
What was reviewed:
- The new
ArrayOpenflow inprint_array/for_each_prelude: thePending→[]early return,Multilineforcing the closing bracket onto its own line,istarting at 0 andalways_newlineforced off for the zero-length case — line-length accounting matches the array-with-elements path. Bun__JSArray__lengthAndNamedKeys: type is validated beforeuncheckedDowncast, no call enters JS, and theconstructorexclusion now keeps{ x: Object.assign([], { constructor: 5 }) }atx: []past the depth limit (covered by the new test).- One thing I could not settle from the checkout: the helper accepts only
ArrayType, whileinherits<JSArray>elsewhere also coversDerivedArrayType. Thesubclassandsqlite-sql.test.tscases assert subclass instances print their keys, so CI settles whetherclass X extends Arrayinstances areArrayTypehere.
Extended reasoning...
The change touches the Rust console formatter (src/jsc/ConsoleObject.rs), a new FFI helper in src/jsc/JSValue.rs and src/jsc/bindings/UtilInspect.cpp, the bake dev-server client inspector, and adds tests in four suites. It touches no security-sensitive surface: no auth, injection or data-exposure paths, and the native helper runs no user JS. The prior run's inline finding about constructor-only arrays past the depth limit is addressed by the helper's constructor exclusion with a test; the non-enumerable-key behavior is unchanged and is a design call the description defends as matching arrays with elements. Deferring rather than approving because the output change is user-visible (Bun.SQL results, matcher messages) and the ArrayType-vs-DerivedArrayType question for subclass instances can only be confirmed by running the new tests, which I could not do here. No CODEOWNERS entry covers the changed files and no outstanding third-party objection is visible in the timeline.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
|
An answer to the open question in the last review: an instance of
|
|
The
|
There was a problem hiding this comment.
I re-reviewed the current head (merge of main plus the besideNonEnumerable test) and found no bugs; since this changes user-visible console.log/Bun.inspect/matcher output on a hot formatter path and adds an FFI probe, a human look at the format decision is still worthwhile.
What was reviewed:
- After the merge,
for_each_property_non_indexedskips own DontEnum keys (bindings.cpp:6154), so the helper's predicate (enumerable, non-private, notconstructor) now matches what the walk prints; the new test pins that. The PR text "a non-enumerable key prints beside an enumerable one" is stale against the code. - Ruled out
class extends Arrayinstances falling to the non-ArrayTypefallback: JS subclasses keepArrayType(onlyArray.prototype/C++ subclasses areDerivedArrayType), and theRowssubclass and SQLite zero-row tests cover it. - Traced
ArrayOpen:Pending->[]returns inside the scoped depth/indent/quote restores;i >= 1for every non-empty array sohandle_first_propertyis never reached fromprint_array;always_newline/was_good_timeonly differ forlen == 0. - The u64 packing cannot collide with the
u64::MAXsentinel (bit 32 is only set when length is 0); the probe runs no JS, so no exception check is needed.
Extended reasoning...
The change touches the native console formatter (src/jsc/ConsoleObject.rs print_array and the property-iterator callback), adds a no-JS FFI probe in src/jsc/bindings/UtilInspect.cpp with its Rust wrapper in src/jsc/JSValue.rs, and mirrors the fix in the bake client inspector, with tests across inspect, console-log expected output, sqlite results and the dev server. It touches no security-sensitive surface. Both of my earlier inline concerns are now covered: the helper excludes constructor, and main's merged walk skips own non-enumerable keys so the helper and the walk agree. I chose defer rather than approve because the PR changes user-visible output (including expect() Received: lines) on a hot path, the bracket-deferral state machine is non-trivial, the PR was bot-authored, and the description is partly stale relative to the code.
The property walk prints no key whose text is `constructor`, a string or
a symbol. The check compared the string key by pointer, so an array with
no elements whose only key is Symbol("constructor") printed `[Array ...]`
past the depth limit, where it prints `[]` at any other depth.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding on the dev-server formatter, I checked the native side after the last two commits and found nothing further: the helper's key filter in Bun__JSArray__lengthAndNamedKeys (own DontEnum, private names, constructor compared by text for string and symbol keys) now matches what the property walk actually prints (bindings.cpp skips own DontEnum keys for the iterated object; for_each_prelude drops any key whose text is constructor), so the depth-limit marker and the Pending -> [] fallback agree with top-level output for those cases. I also confirmed open_array is only reachable for a zero-length array, since a non-empty array (even one that is all holes) hands the property iterator an index of at least 1, so the object-style { prefix in handle_first_property cannot fire for arrays.
Extended reasoning...
The change touches the console/Bun.inspect array printer in Rust, a new C++ structure-scan helper, the bake client formatter, and tests; it touches no auth, crypto or input-parsing surface. One inline finding remains on the browser-side formatter, and the native helper/walk consistency and the lazy-bracket control flow were examined and ruled out this run.
The console formatter of the dev server read each named property of an array with `value[key]`. Since an array with no elements reaches that loop, `console.log` in a page called its enumerable getters, and a getter that throws made `console.log` throw. The loop reads the descriptor first, as the object formatter does. An accessor prints `[Getter]`, `[Setter]` or `[Getter/Setter]`. A property that throws prints `undefined`.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The latest commit addresses the getter concern in the bake inspector (accessors print as [Getter]/[Setter] without being invoked, and the new esm.test.ts case pins it). Beyond the inline nit, I also checked that the C++ helper's key filter (DontEnum, private names, constructor text) matches what the walk in bindings.cpp actually prints for own keys of an array, so "does anything print" and "what prints" agree, and that when every key is skipped (hidden value, failed tag) the Pending state still falls back to [] and exceptions propagate through ?. The remaining user-visible change (empty SQL results now print their fields) is a product call worth a maintainer's glance.
Extended reasoning...
The change replaces the len == 0 early return in print_array (src/jsc/ConsoleObject.rs) with a C++ structure probe (Bun__JSArray__lengthAndNamedKeys in src/jsc/bindings/UtilInspect.cpp) and a deferred-bracket state machine (ArrayOpen) in the property iterator, plus the parallel fix in the bake client inspector; it touches no security-sensitive surface. All three concerns raised in earlier runs (DontEnum consistency, depth marker for constructor-only keys, bake getters) are now addressed in code with tests. I did not approve because the formatter is a hot, widely-observed path with a new two-place rule for which keys count, and the output change for empty Bun.SQL results is a product decision a maintainer should confirm.
… empty array) A snapshot prints no named property of an array, so it prints an array with no elements as before.
…t-named-keys-of-empty-array
Fixes #22569
Problem
console.logprints[]for an empty array with a named property:const a = []; a.total = 7001. With one element Bun prints[ 5, total: 7001 ]. An emptyBun.SQLresult hidescountandcommand.print_arrayreturns onlen == 0before its named-property pass (src/jsc/ConsoleObject.rs:4268).Fix
bake/client/inspect.ts) had the same return. It does not call a named accessor: it prints[Getter].test/js/bun/util/inspect.test.js(9 new tests fail on main),sqlite-sql.test.ts,console-log.test.ts,bake/dev/esm.test.ts.constructorrule (output changes), a note on One value formatter for console, Bun.inspect, the error printer and bun:test #44268 (closed).Background
result.count). JSC stores it in the array's Structure.[]) and a second probe. One read makes[]cheaper: 144 to 126 instructions.Downsides
console.log,Bun.inspect, matcherReceived:lines,test.eachtitles with%p. Stored snapshots do not.{a:1,b:2}: 5252 to 5262), an arguments object 26 more. Release text: +1,536 bytes.[]: an empty array with only non-enumerable named properties (an empty postgres.jsResult), as in Node.Notes
Output
The format is the one an array with elements has.
util.inspectwas correct before and does not use this code.Bun.SQL and #22569
count,command,lastInsertRowidandaffectedRowsare enumerable fields ofSQLResultArray(src/js/internal/sql/shared.ts). sql: consolidate the open Bun.sql, bun:sqlite and node:sqlite pull requests #45304 decided thatconsole.log(rows)keeps showing them.[]on main. That is the report in Bun.SQL: inserts and updates dont console.log properties if the result is an empty array, will only print[]#22569.sqlite-sql.test.tsholds that, and it fails on main.[]#22569 and keeps that test. The self-review asked for the opposite, because the draft of sql: consolidate the open Bun.sql, bun:sqlite and node:sqlite pull requests #45304 made these fields non-enumerable. The merged sql: consolidate the open Bun.sql, bun:sqlite and node:sqlite pull requests #45304 keeps them enumerable.Which keys print
console.loghas different output for pg #13946) is open. It adds the class name that Node prints before the bracket.[]: no named property, a deleted one, only non-enumerable ones (an empty postgres.jsResult, and Node hides those keys too), only a private field, onlyconstructor,Array.prototype, an empty arguments object.constructor: the string key, and a symbol with that description (for_each_preludecompares the text). The helper skips the same keys by text. The self-review asked to delete that rule from the helper, because Print an object's ownconstructorkey in console.log, Bun.inspect and bun:test #38937 proposes to change it in the walk. Without the rule an empty array with only such a key prints[Array ...]past the depth limit, where main prints[]. The rule stays, and tests fail without it. A test also pins that an array with elements hides these keys, so a change to the walk fails beside the cases of the helper.7177d5ead8the helper compared theconstructorkey by pointer, and an empty array whose only key isSymbol("constructor")printed[Array ...]past the depth limit. A review found it.6172b53f81compares the text.inspect.customthrows now throws from the print. Before, the value was never formatted.{ sorted: true }does not sort the named keys of an array, with or without elements. That is the behaviour on main for arrays with elements. The new tests use keys that are already in order.The read
Bun__JSArray__lengthAndNamedKeys(UtilInspect.cpp) returns the length in the low 32 bits. Bit 32 is set for an array with no elements whose Structure has an enumerable key that is not a private name and whose text is notconstructor. A Structure with no property slots is answered with no scan. The helper takesArrayTypecells. An instance of anArraysubclass isArrayType. Arguments objects andArray.prototypekeep the old length read.Measurements
Release builds of main
017b2439a7and of this branch at91bfe1c7cc. The branch was merged with main675ee5b97bafter these runs. The merge changes no added or removed line of the source diff, and the runs were not repeated.6172b53f81then changed one line of the helper: theconstructorcomparison, which runs only for an empty array that has named property slots. Tools: gdb 16.3 (breakpoint hit counts,stepi),size,nm,objdump. perf, valgrind, bloaty and strace are not installed in the container. Wall-clock is not reported: in an earlier run the same binary against a copy of itself differed by up to 34%.Engine calls per 1000 prints: property walk / length read /
PropertyNameArraybuilt / new helper.[][0][[], [], []]Object.assign([5], { total: 1 })Object.assign([], { total: 1 }){}Instructions from the entry of
print_arrayto its return (gdbstepi, colors off, two runs agree).[]inside{ rows: [] }[0][1, 2, 3][[], [], []]Object.assign([5], { total: 1 })print_objectof{ a: 1, b: 2 }size).print_array4109/4189 to 4389/4501 bytes (colors off/on).for_each_prelude1265/1295 to 1252/1346. New symbols 1,187 bytes.print_array: release 216/216 to 216/216 bytes. Debug with ASAN 4416/4448 to 4544/4576.PropertyIteratorCtxstays 48 bytes (const assert).6172b53f81, and a test holds it. The corpus was not printed again.Bun.inspect(nest(N), { depth: Infinity }): the smallest N that throwsRangeErroris 13,471 on both release builds.Object.keyscalls per empty array 0 to 1. OneObject.getOwnPropertyDescriptorcall per named key of an array, 0 before.The walk skip that is not here
A draft skipped the property walk for an array with elements and no named property. That saved one walk and one allocation per print of
[0]. The walk also holds JSC's stack check. Input: nested arrays where aninspect.customobject prints after a deeper array. On debug builds of the same tree, the draft aborts (exit 134) from 1,000 to 1,550 levels. Without the skip the abort is at 1,000 only andRangeErrorcomes from 1,040. On release builds, main and this branch are equal for that input: abort at 13,300 and 13,450 levels,RangeErrorfrom 13,480. Aninspect.customat the bottom of the nest aborted on main017b2439a7and on this branch from 13,300 to 21,400 levels. #39382 had the fix for that abort, and main has it since #46021. All numbers in this paragraph are from before the merge with main.Each clause is held by a test
Ten clauses of the diff were removed in three mutant builds, before the merge with main. Each removal failed at least one of the new tests.
One of the ten is now a guard: the
[]thatprint_arraywrites when the helper reports a key and the walk prints none. The symbol case reached it. Since6172b53f81no known input does, and a build that writes[?]there passes the four suites. Without the guard, a disagreement between the helper and the walk would print]with no opening bracket.The text comparison of
6172b53f81is held by theconstructorSymbolcase past the depth limit. That case fails with the pointer comparison.Merge with main
The branch is merged with main
89452d2ac1(2026-11-10), and before that with main675ee5b97b(2026-11-03). Neither merge had a conflict, and neither changes an added or removed line of the source diff. Main has #44833, which changed the property walk andprint_array. It also has #47893 and #47942, which changed the signature of the property callback and the walk. After the merge the suites below pass. On the source of main89452d2ac1the new tests fail as before: 9 ininspect.test.js, and 1 each inconsole-log.test.ts,sqlite-sql.test.tsandesm.test.ts.One test in
console-table.test.tsruns 64 full GCs. On the debug build in my container it takes 4.4 to 6.4 s with and without this change, so it meets the 5 s default timeout by chance on both.Other open pull requests
console.loghas different output for pg #13946) replaces the same length statement inprint_arraywithBun__JSArray__lengthAndFlags. That call has the shape of the call here:length | bit 32, andUINT64_MAXfor a value that is not anArrayTypecell. Its bit 32 means a prototype other than the realm'sArray.prototype. The two PRs conflict inConsoleObject.rs,JSValue.rs,UtilInspect.cppandinspect.test.js.Option<(u32, bool)>, so a hand merge can read one flag as the other.Arraysubclass printsName(n)before the bracket. The tests here that print a subclass instance or aBun.SQLresult then expect that prefix:Rows(0) [ total: 7001 ], as in Node. console: print the class name of Array subclass instances #47563 calls its helper only forJSType::Array. The fold keeps that test, so an arguments object makes no helper call.console.logformatter #46729 makesprint_arraythe snapshot printer, with a mode that skips the named-property pass. With both merged, the flag of this PR must stay off in that mode. The snapshot test here pins[]. One value formatter for console, Bun.inspect, the error printer and bun:test #44268 is closed.constructorkey in console.log, Bun.inspect and bun:test #38937, inspect: don't probe every index of an arguments object with a huge length #42247, inspect: count a hole run before the 101st array element once #42328, console: print an empty object past the depth as {} #43193 and console.dir: honor maxArrayLength in the native formatter #31809 editprint_arrayor the property walk.Dev server console formatter
formatArrayreturned[]for every array with no elements. It now returns[]when neither the element loop nor the named-key loop prints.value[key]. With the first revisions of this PR an empty array reached that loop, soconsole.login a page called its enumerable getters, and a getter that throws madeconsole.logthrow. A review found it.a77bbf2e98the loop reads the descriptor, asformatPlainObjectdoes. An accessor prints[Getter],[Setter]or[Getter/Setter]. A property that throws printsundefined.6172b53f81with a count of 2.Not in this PR
inspect.custom) goes out of the page'sconsole.log. Both are on main.pretty_format.rs) prints no named property of an array. A new test pins that a snapshot of such an array is[].Suites
bun bd testontest/js/bun/util/inspect.test.js,test/js/web/console/console-log.test.ts,test/js/sql/sqlite-sql.test.ts,test/bake/dev/esm.test.ts,test/js/bun/console/bun-inspect-table.test.ts,console-table.test.ts,console-iterator.test.ts,test/js/node/util/bun-inspect.test.ts,custom-inspect.test.js,test/js/bun/util/inspect-error.test.js,test/js/bun/test/jest-each.test.ts. All ran again after the merge with main.The
console-log.jscases are from #23324 by @Pranav2612000.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/sql/sqlite-sql.test.ts, test/js/bun/util/inspect.test.js, test/bake/dev/esm.test.ts