Repository navigation
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 8 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
|
Updated 3:04 AM PT - Nov 8th, 2026
❌ @robobun, your commit 4828f58 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 48210That installs a local version of the PR into your bun-48210 --bun |
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, two more things were checked and ruled out: the ordered_properties / call_own_getters arms in print_object taking precedence over HiddenKeys::List is unreachable today, since show_hidden is set only by console.dir and the Buffer hook while ordered_properties comes from Bun.inspect({ sorted }) and call_own_getters from the test runner, so no caller sets both; and Bun.inspect(v, { showHidden: true }) still not listing hidden keys is a deliberate scope limit of this PR rather than a regression (its option parsing is untouched). The new test files were not run as part of this review.
Extended reasoning...
The change adds a showHidden switch to Bun's native console formatter across ConsoleObject.rs, JSValue.rs, bindings.cpp, UtilInspect.cpp and JSBuffer.cpp, with a new three-mode C++ property walk and a bitflags FFI parameter for the custom-inspect hook, plus tests and a docs line. It touches no auth, crypto or injection surface, but it does change a shared property-enumeration path and the DontEnum skip conditions in bindings.cpp. Four confirmed findings are already posted inline (Buffer cycle RangeError, stdio stream creation when walking console, constructor key omitted, URL context symbol not shown), so the inline comments already signal human review; this note only records the two candidate issues examined and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
8defc4c to
f2fee93
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…Hidden: true })` Since #44833 the native property walk skips own non-enumerable properties. That is right for `console.log(v)`. The global console then has no way to list them: `console.log("%o", v)` and `console.dir(v, { showHidden: true })` print `{ shown: 1 }` where Node.js prints `{ shown: 1, [hidden]: 2 }`. The formatter gets a switch for these keys. `%o` in the format string of the first argument and `showHidden` of `console.dir` turn it on. The walk then passes each non-enumerable property to a second callback, in its place among the other keys, and that callback prints the key in brackets. The inspect method of `Buffer` is told too, so `util.inspect(buffer, { showHidden: true })` lists the hidden keys of the extra values of a Buffer again.
f2fee93 to
8cbb3cf
Compare
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 nit, I also checked: the Buffer inspect hook running a user getter on an extra own property under showHidden (Node's buffer.inspect reads this[key] the same way, so not a divergence); own enumerable __esModule now printing from the structure fast path at bindings.cpp:6152 (the slow path and Node already print it, so the paths now agree); and the docs line on bracketed symbol keys (Node prints them bracketed too, so the sentence describes no divergence, but it is a wording nit only). Not run here: the test files and BUN_JSC_validateExceptionChecks=1 over the new ownCustomPropertyCell / iterHidden paths.
Extended reasoning...
The new commit adds the s_bufferBeingInspected chain in JSBuffer.cpp and the ownCustomPropertyCell branch in bindings.cpp, which address the two earlier red inline findings (self-referencing Buffer RangeError and console._stdout creation); the earlier constructor and URL showHidden notes remain as they were. The change touches the shared JSC property walk, the FFI callback signature and a per-thread global, with no security-sensitive surface; its size and the native-walk refactor are why a human look stays worthwhile rather than an approval.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
A getter of the Buffer that prints the Buffer starts a print of its own. It is no way back to the Buffer, so it must not print as [Circular].
There was a problem hiding this comment.
I re-reviewed the latest push and found no new bugs; the points raised on earlier versions (Buffer back-reference, console._stdout instantiation, re-entrant Buffer print, constructor divergence) now have code or docs behind them in the diff. Because this rewires the shared property walk in bindings.cpp that every console.log/Bun.inspect goes through, a human look would still be worthwhile.
What was reviewed:
- Both walks in
JSC__JSValue__forEachPropertyImpl: inEnumerable/CallingOwnGettersmode ownDontEnumkeys stillreturn/continuebefore delivery, so plain prints are unchanged;ownCustomPropertyCellis only reachable for an ownDontEnumkey, i.e.WithHiddenmode. - JSBuffer.cpp: the
SetForScopeon the thread-local chain is RAII, so it unwinds on theRETURN_IF_EXCEPTIONpath too;thisBufferoutlives every use. A human may still want to weigh a thread-local chain against keeping it on the VM, though it is scoped to one synchronous call. - UtilInspect.cpp: the Buffer options object is null-prototype, so
Object.prototype.showHidden = trueno longer leaks into plainconsole.log(buf)(covered by the new test). - Not run here: the new subprocess tests and
BUN_JSC_validateExceptionChecks=1; the verdict rests on reading only.
Extended reasoning...
The change adds a showHidden switch to Bun's native console formatter: a new WithHidden mode in the shared C++ property walk (bindings.cpp), a HiddenKeys state and KeyKind in ConsoleObject.rs, a flags byte on the custom-inspect FFI call, a new Bun__inspect_singleline_show_hidden export used by Buffer's native inspect with a thread-local cycle guard, one common string, a docs sentence, and about 530 lines of tests. It touches no auth, crypto or injection surface; the sensitive part is memory/exception safety in the property walk and the Buffer re-entrancy guard, which read as correct (snapshot plus MarkedArgumentBuffer in the fast walk, RAII restore in JSBuffer.cpp). The bug hunt ran dry on this push and prior inline findings are addressed in the two commits, but the diff is roughly 850 lines across the hot shared enumeration path used by every console print, so it is not a candidate for approval without a human.
Related to #1713
Problem
console.log("%o", v)andconsole.dir(v, { showHidden: true })print{ shown: 1 }. Node.js prints{ shown: 1, [hidden]: 2 }.src/jsc/bindings/bindings.cpp:6145,:6278). The formatter has noshowHidden.util.inspect(v, { showHidden: true })works on main. One docs sentence can replace this PR (Notes).Fix
%oin the first argument's format string, andshowHiddenofconsole.dir, turn on aFormatterswitch. The walk then passes each non-enumerable property to a second callback, which prints[key]: value.Buffer's inspect method gets the switch, and prints a Buffer that it meets again as[Circular].%ohalf is separable.test/js/web/console/console-log.test.ts(11 of 13 new tests fail on main),buffer.test.js,inspect.test.js, 4 AOT programs, 21 related test files.Background
Object.keysskips a non-enumerable property. Node'sshowHiddenprints its key as[key].%oimplies it.console.logandBun.inspectshare one native formatter. A C++ walk hands it each property.util.inspectfor%o: it changes the layout of every%oprint.Downsides
%oprints the methods of a class instance as[method]..textgrows 2,560 bytes.Notes
The choice
The report that started this asks for one of two things: the hidden key in the
%oandshowHiddenrows, or a docs line that says that the console takes neither. This PR is the first. The second is this sentence fordocs/runtime/nodejs-compat.mdx, which is true on main:If the sentence is the pick, close this PR. One row then still needs a decision: on main
util.inspect(buffer, { showHidden: true })prints<Buffer 61 62, prop: { shown: 1 }, hiddenProp: 7>, where Node.js printsprop: { shown: 1, [hidden]: 2 }. Only code can repair that row. The%oarm and theconsole.diroption are separate in the diff (format2and the%oarm against the option read inmessage_with_type_and_level_).Not in this PR: a
showHiddenoption forBun.inspect. Nobody asked for it.test/js/bun/globals.test.js:408passes the option, andBun.inspectdrops it on main and here. It is a small follow-up if a maintainer wants it.What prints what
vis{ shown: 1 }with a non-enumerablehidden: 2.console.log(v),console.dir(v),console.log("%O", v)shown,hiddenshownshownshownconsole.log("%o", v)shown,hiddenshownshown,[hidden]shown,[hidden]console.dir(v, { showHidden: true })shown,hiddenshownshown,[hidden]shown,[hidden]util.inspect(buffer, { showHidden: true }), a value of the Buffershown,hiddenshownshown,[hidden]shown,[hidden]No release had an effect of
%oorshowHiddenof its own. #1713 asks for the bracketed key underconsole.dir(obj, { showHidden: true })in its last paragraph. Its main case (an inherited enumerable key is printed) is not changed here.How it works
HiddenKeyson theFormatterhas three states:Skip,InPercentO(a%oof the format string that is being printed lists the keys for its value) andList.format2setsInPercentOonly when the first argument is a primitive string, and clears it when that argument is printed. Bun also fills the%directives of later string arguments (Fixconsole.login Lambda runtime #4826, open console: only a primitive-string first argument is a format string #45571). A%oin such a string lists no hidden key.ForEachPropertyMode::WithHiddenis a third mode ofJSC__JSValue__forEachPropertyImpl. Both loops of the walk pick the callback (deliver) from theDontEnumattribute. A hidden property passes the same private-name check as every other property.for_each_hidden, prints the key through the same writer withKeyKind::Hidden.Bufferis native. It prints each extra value withBun__inspect_singleline. A flags byte tells it the switch. Other[util.inspect.custom]functions get the options that they get on main.[Circular]. The read of the property is outside that mark: a getter that prints its Buffer starts a print of its own. On main,buf.holder = { back: buf }; console.log(buf)throwsRangeError: Maximum call stack size exceeded. With the switch passed on, a non-enumerable such value would throw too.[Getter],[Setter]or[Getter/Setter]. The inspect method ofBufferis different. It reads each own property of the Buffer, as the one of Node.js does. So a non-enumerable getter of a Buffer itself is called under%oandshowHidden, and its value is printed:<Buffer 61, g: 1>.Bufferhave no prototype. WithObject.prototype.showHidden = true, main lists the own non-enumerable keys of a Buffer in a plainconsole.log(buffer). This branch lists none there, as Node.js.console._stdoutprints as[Getter]and the stream is not created, andprocess.envprints[TZ]: [Getter/Setter]. One without a cell prints its value, as in a plain print:DataView.prototypelists[buffer]: undefined.Limits, with their trackers
console.logprints the keys of a value: plain objects, class instances and arrays that are not empty. AMap,Set, function,Date,RegExp, typed array,Promiseand boxed primitive print no own key in any mode. Bun.SQL: inserts and updates dont console.log properties if the result is an empty array, will only print[]#22569 and open console: print the named properties of an empty array #45564 cover an empty array, CJS interop: function exports with attached properties are not preserved in import (graphql-tag mismatch vs Node.js) #32226 covers functions. Nothing tracks the other kinds.Errorprints its enumerable own keys through another enumerator (JSPropertyIterator.cpp). It lists no hidden key.[method]. Node.js lists its accessors only.[length], and no object lists aconstructorkey.%okeeps the configured depth (Node.js uses 4) and prints aProxyas its target. The two remaining TODO lines are in the%oarm.console.dirreadsshowHidden,depthandcolorsonly.node:vmcontext lists none (open inspect: print a node:vm context global from its own property slots, sorted or not #47217 changes that walk).Proxyamong the prototypes prints without brackets, and its getter runs, as on main.Bufferget noshowHidden. A function that hands its options on toinspectwould reach throws in the JSutil.inspect(open util.inspect: do not throw when an error's stack, name, message or errors getter throws #45254, util.inspect: read Symbol.toStringTag in a try, as Node.js does #46414).util.inspect.defaultOptions.showHiddenis not read by the global console (Add Global Configuration Options for console.log Output Formatting #12762, open console: honor util.inspect.defaultOptions on the global console #35061).console.table("%o", v)with a first argument that is no object prints asconsole.logdoes, so it lists the keys. Open console.table: log only the first argument when it is not an object #46313 changes that fallback.URLis native and readsshowHiddentoo. It is not told, so%oof a URL prints no[Symbol(context)], as on main.Measurements
Release builds of the merge base 2596ac5 and of this branch at f2fee93, linux-x64. The later commits change comments and the inspect method of
Bufferonly. Instructions are user-mode instructions per call: (I(400,000 calls) - I(200,000 calls)) / 200,000,BUN_JSC_useConcurrentJIT=0, 5 interleaved rounds, minimum.Bun.inspect({ a: 1 })Bun.inspectof a 32-key objectBun.inspect([1, 2, 3])Bun.inspectof a class instanceBun.inspectof an 8-key object with a getterBun.inspectof a Bufferconsole.log(v),v = { a: 1, b: "x", c: [1, { d: 2 }] }console.log("%o", v)console.dir(v, { depth: 2 })write_property_key(it gets a three-state key kind in place of a bool), 3 infor_eachand 2 in the walk. A const generic twin of the key writer costs no instruction, but 4.9 KB of code and new symbol names for two hot functions. It was measured 4% to 10% slower on the links with an order file below, so the runtime kind is what is here..text: 108,257,833 to 108,260,393 bytes. File: 138,119,440 to 138,123,536 bytes.size_of::<Formatter>(): 152 to 160 bytes, on the stack, one per print.FormatOptionsstays at 12.Bun.inspectprints beforeRangeError: debug+ASAN 615 to 596 (the test inbun-inspect.test.tsneeds 512), release 2,912 to 2,886.Time of
Bun.inspect, nanoseconds per call, best of 9, 6 interleaved runs, one pinned core (AMD EPYC, 32 KiB L1i):{ a: 1 }scripts/orderfile/generate.ts). The rows with an order file use one that this script wrote locally, and were measured one commit earlier on this branch (the walk and the formatter are the same). The artifact of CI could not be downloaded here.Open pull requests that touch the same code
console.logformatter #46729 and inspect: print a node:vm context global from its own property slots, sorted or not #47217 merge with this branch without a conflict (git merge-tree).Bun__inspect_singlelineand a flags byte to the hook call.SHOW_HIDDENis bit 2, because console.table, Bun.inspect compact: carry single-line mode to inspect.custom hooks and native printers #47593 uses bit 1.Review concerns that were rejected
Of the two self-reviews:
console.tablefallback from listing the keys." The fallback isconsole.logwith every argument on main, and the table of built-ins says so. console.table: log only the first argument when it is not an object #46313 owns that change.console.logformatter #46729 which lands first." This branch keepsFormatter.call_own_gettersand adds its field and type away from the lines that Print Bun's snapshot format with theconsole.logformatter #46729 changes. The two merge without a conflict.Tests
console-log.test.ts: 13 new tests. The 2 that pass on main pin that a later string or a non-string first argument lists no key, without colors and in color.buffer.test.js: 2 new tests. The one for a Buffer that leads back to itself fails on main with theRangeError.inspect.test.js: an object that a hook changes while its hidden keys are listed. It runs on every build. An ASAN build also reports a read of a freed property table.test/aot/parallel/hidden-members: 2 new programs and 2 corrected ones. Without"showHidden"inconsole.builtins.ts,console-dir-reads-show-hidden-of-its-options.jsfails:'showHidden' was not compiled into this executable.test/js/web/console,test/js/bun/console,test/js/node/console,bun-inspect.test.ts,inspect-error.test.js,custom-inspect.test.js,util.test.js,globals.test.js,jest-each.test.ts,dom-nodes.test.ts,url.test.ts,inspect-strings-parsed-from-json.test.ts,vitest-flavor.test.ts, and the 23test-console-*,test-util-inspect-*andtest-buffer-*inspect*scripts oftest/js/node/test/parallel.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/node/buffer.test.js, test/js/bun/util/inspect.test.js