Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/jsc/ConsoleObject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2203,13 +2203,22 @@ pub mod formatter {
use jsc::JSType as T;
let tag = match js_type {
T::ErrorInstance => TagPayload::Error,
// JSC makes these three prototypes boxed values (#8627). `cell` selects string colouring.
T::DerivedStringObject | T::NumberObject | T::BooleanObject
if crate::cpp::Bun__isStringNumberOrBooleanPrototype(value) =>
{
return Ok(TagResult {
tag: TagPayload::Object,
cell: T::Object,
});
}
T::NumberObject => TagPayload::Double,
Comment thread
robobun marked this conversation as resolved.
Resolved
T::DerivedArray
| T::Array
| T::DirectArguments
| T::ScopedArguments
| T::ClonedArguments => TagPayload::Array,
T::DerivedStringObject | T::String | T::StringObject => TagPayload::String,
T::String | T::StringObject => TagPayload::String,
T::RegExpObject => TagPayload::String,
T::Symbol => TagPayload::Symbol,
T::BooleanObject => TagPayload::Boolean,
Expand Down Expand Up @@ -3166,6 +3175,10 @@ pub mod formatter {
) -> JsResult<Option<bun_core::String>> {
let name_str = value.get_class_name(global_this)?;
if !name_str.eq_ascii(b"Object") {
// Node prints `String.prototype`, `Number.prototype` and `Boolean.prototype` as `{}`.
if crate::cpp::Bun__isStringNumberOrBooleanPrototype(value) {
return Ok(None);
}
return Ok(Some(name_str));
} else if value.get_prototype(global_this)?.eql_value(JSValue::NULL) {
return Ok(Some(bun_core::String::static_("[Object: null prototype]")));
Expand Down Expand Up @@ -5366,7 +5379,8 @@ pub mod formatter {
i: 0,
};

if ordered_properties {
// Only the other walk leaves out the built-in methods of `String.prototype` and its two siblings.
if ordered_properties && !crate::cpp::Bun__isStringNumberOrBooleanPrototype(value) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): callers passing { sorted: true } get insertion-ordered keys for String.prototype, Number.prototype and Boolean.prototype, while every other object is sorted. The new guard at src/jsc/ConsoleObject.rs:5383 routes those three objects to the unsorted walk whenever they are the value being printed. The new test at test/js/bun/util/inspect.test.js:1641 pins the unsorted b: 1, a: 2 output. Fix: keep the sorted walk for these objects and have it list own enumerable keys only, for example by passing DontEnumPropertiesMode::Exclude in JSC__JSValue__forEachPropertyOrdered when Bun::isStringNumberOrBooleanPrototype(object) holds, so sorted is honoured for every object.

Why this was flagged

A program adds enumerable keys to Number.prototype in non-alphabetical order (test/js/bun/util/inspect.test.js:1627-1630 does b then a) and prints it with Bun.inspect(Number.prototype, { sorted: true }) or util.inspect(..., { sorted: true }). src/jsc/ConsoleObject.rs:5383 skips for_each_property_ordered for the three prototypes and calls for_each_property instead, which emits keys in creation order. The output is { b: 1, a: 2 } where every other object prints { a: 2, b: 1 }; the test at test/js/bun/util/inspect.test.js:1641 asserts exactly that unsorted text. On the base branch these values went to the number/string/boolean printers, so the option was never consulted; the diff is what makes sorted reachable and then drops it. The PR description lists sorting under { sorted: true } as not done; the ordered walk at src/jsc/bindings/bindings.cpp:5942 already lists own properties only, so the only change it needs is the DontEnum mode for these objects.

Verification: Acknowledged in diff: test/js/bun/util/inspect.test.js:1645 pins { b: 1, a: 2 } for Bun.inspect(Number.prototype, { sorted: true }). src/jsc/ConsoleObject.rs:5383 routes these three objects to the unsorted walk at :5390, so the sorted option is silently ignored for exactly them. On base these objects never listed keys at all, so this is a new, narrow inconsistency, not a regression.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not change this. The reasons:

  • JSC__JSValue__forEachPropertyOrdered is also the walk of the snapshot printer (src/runtime/test_runner/pretty_format.rs:2307). With DontEnumPropertiesMode::Exclude for the three prototypes, expect(Number.prototype).toMatchSnapshot() prints Number {} where it prints six methods today, so stored snapshots change. snapshot.test.ts pins today's text.
  • console: print built-in prototypes as {} instead of boxed value / method dump #36660, an earlier attempt at this issue, added a check to that function. After that, test/js/bun/test/pretty-format-overflow.test.ts failed on the x64-asan lane of that PR. This PR leaves the function as it is: 0 of its 1709 instructions differ in a release build. The x64-asan lane passed on this PR at 20f194e (20 of 20 shards).
  • The case is narrow. A program adds two or more enumerable properties to one of the three prototypes, and prints that prototype with Bun.inspect(value, { sorted: true }). console.log() never sorts.

To honour sorted here and keep the snapshot text, the caller has to tell the ordered walk what to list (a new parameter, which pretty_format.rs does not set). I can do that in a follow-up if a maintainer wants it. The PR body has this case under "Not changed, for later".

value.for_each_property_ordered(
global_this,
(&raw mut iter).cast::<c_void>(),
Expand Down
1 change: 1 addition & 0 deletions src/jsc/JSType.rs
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,7 @@ impl JSType {
/// ```
pub const StringObject: JSType = JSType(100);

/// `String.prototype` of any realm, and nothing else. An instance of a subclass is a `StringObject`.
pub const DerivedStringObject: JSType = JSType(101);

pub const Event: JSType = JSType(0b11101111);
Expand Down
6 changes: 6 additions & 0 deletions src/jsc/bindings/UtilInspect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "JavaScriptCore/PropertyNameArray.h"
#include "JavaScriptCore/JSArray.h"
#include "JavaScriptCore/IdentifierInlines.h"
#include "UtilInspect.h"

namespace Bun {

Expand Down Expand Up @@ -73,6 +74,11 @@ extern "C" JSC::EncodedJSValue JSC__JSValue__callCustomInspectFunction(
RELEASE_AND_RETURN(scope, JSValue::encode(inspectRet));
}

extern "C" [[ZIG_EXPORT(nothrow)]] bool Bun__isStringNumberOrBooleanPrototype(JSC::EncodedJSValue encodedValue)
{
return isStringNumberOrBooleanPrototype(JSValue::decode(encodedValue));
}

// Port of V8's `internalBinding('util').getOwnNonIndexProperties(object, filter)` used by
// util.inspect and the REPL completer: own keys minus array indices, without materializing
// a name (or descriptor) per element the way Object.getOwnPropertyNames() on an array would.
Expand Down
25 changes: 25 additions & 0 deletions src/jsc/bindings/UtilInspect.h
Original file line number Diff line number Diff line change
@@ -1,9 +1,34 @@
#pragma once

#include "root.h"
#include "JavaScriptCore/BooleanPrototype.h"
#include "JavaScriptCore/NumberPrototype.h"

namespace Bun {

// String.prototype, Number.prototype or Boolean.prototype of any realm. JSC makes them boxed values of "", 0 and false.
ALWAYS_INLINE bool isStringNumberOrBooleanPrototype(JSC::JSCell* cell)
{
// Each of the three has a static property table, and few printed objects do: most values pay for this test only.
if (!JSC::TypeInfo::hasStaticPropertyTable(cell->inlineTypeFlags()))
return false;
switch (cell->type()) {
case JSC::DerivedStringObjectType:
return true;
case JSC::NumberObjectType:
return cell->classInfo() == JSC::NumberPrototype::info();
case JSC::BooleanObjectType:
return cell->classInfo() == JSC::BooleanPrototype::info();
default:
return false;
}
}

ALWAYS_INLINE bool isStringNumberOrBooleanPrototype(JSC::JSValue value)
{
return value.isCell() && isStringNumberOrBooleanPrototype(value.asCell());
}

JSC::Structure* createUtilInspectOptionsStructure(JSC::VM& vm, JSC::JSGlobalObject* globalObject);

// internalBinding('util').getOwnNonIndexProperties(object, filter)
Expand Down
32 changes: 27 additions & 5 deletions src/jsc/bindings/bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@
#include "JavaScriptCore/TimeZoneICUBridge.h"

#include "JavaScriptCore/FunctionPrototype.h"
#include "UtilInspect.h"
#include "JSFetchHeaders.h"
#include "FetchHeaders.h"
#include "BunCompiledObjectEquality.h"
Expand Down Expand Up @@ -5553,6 +5554,22 @@ extern "C" void JSGlobalObject__throwStackOverflow(JSC::JSGlobalObject* globalOb
throwStackOverflowError(globalObject, scope);
}

// A boxed String, Number or Boolean. String.prototype, Number.prototype and Boolean.prototype are boxed values too.
static ALWAYS_INLINE bool isBoxedStringNumberOrBoolean(JSC::JSValue value)
{
if (!value.isCell())
return false;
switch (value.asCell()->type()) {
case StringObjectType:
case DerivedStringObjectType:
case NumberObjectType:
case BooleanObjectType:
return true;
default:
return false;
}
}

template<bool nonIndexedOnly>
static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* globalObject, void* arg2, void (*iter)(JSC::JSGlobalObject* arg0, void* ctx, EncodedSlice* arg2, JSC::EncodedJSValue JSValue3, bool isSymbol, bool isPrivateSymbol))
{
Expand Down Expand Up @@ -5614,13 +5631,18 @@ static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC:
bool hasNoOwnProperty = structure->outOfLineSize() == 0 && structure->inlineSize() == 0;
#endif

// These three prototypes show what a program put on them. The quick walk lists each method of a reified one.
const bool listsOwnEnumerableOnly = Bun::isStringNumberOrBooleanPrototype(object);
if (listsOwnEnumerableOnly)
fast = false;

if (fast) {
if (hasNoOwnProperty) {
fast = false;

JSValue proto = object->getPrototype(globalObject);
RETURN_IF_EXCEPTION(scope, );
if (proto) {
if (proto && !isBoxedStringNumberOrBoolean(proto)) {
if ((structure = proto.structureOrNull())) {
prototypeObject = proto;
fast = canWalkQuickly(asObject(proto));
Expand Down Expand Up @@ -5728,7 +5750,7 @@ static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC:
JSValue proto = prototypeObject.getPrototype(globalObject);
RETURN_IF_EXCEPTION(scope, );
if (proto) {
if (!(proto == globalObject->objectPrototype() || proto == globalObject->functionPrototype() || (proto.inherits<JSGlobalProxy>() && uncheckedDowncast<JSGlobalProxy>(proto)->target() != globalObject))) {
if (!(proto == globalObject->objectPrototype() || proto == globalObject->functionPrototype() || isBoxedStringNumberOrBoolean(proto) || (proto.inherits<JSGlobalProxy>() && uncheckedDowncast<JSGlobalProxy>(proto)->target() != globalObject))) {
if ((structure = proto.structureOrNull())) {
prototypeObject = proto;
fast = canWalkQuickly(asObject(proto));
Expand All @@ -5750,11 +5772,11 @@ static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC:
auto* globalProxy = dynamicDowncast<JSGlobalProxy>(object);
const bool isOtherGlobal = globalProxy && globalProxy->target() != globalObject;

while (iterating && !(iterating == globalObject->objectPrototype() || iterating == globalObject->functionPrototype() || (iterating != object && iterating->inherits<JSGlobalProxy>() && uncheckedDowncast<JSGlobalProxy>(iterating)->target() != globalObject)) && prototypeCount++ < 5) {
while (iterating && !(iterating == globalObject->objectPrototype() || iterating == globalObject->functionPrototype() || (iterating != object && (isBoxedStringNumberOrBoolean(iterating) || (iterating->inherits<JSGlobalProxy>() && uncheckedDowncast<JSGlobalProxy>(iterating)->target() != globalObject)))) && prototypeCount++ < 5) {
if constexpr (nonIndexedOnly) {
iterating->getOwnNonIndexPropertyNames(globalObject, properties, DontEnumPropertiesMode::Include);
} else {
iterating->methodTable()->getOwnPropertyNames(iterating, globalObject, properties, isOtherGlobal ? DontEnumPropertiesMode::Exclude : DontEnumPropertiesMode::Include);
iterating->methodTable()->getOwnPropertyNames(iterating, globalObject, properties, (isOtherGlobal || listsOwnEnumerableOnly) ? DontEnumPropertiesMode::Exclude : DontEnumPropertiesMode::Include);
}

RETURN_IF_EXCEPTION(scope, void());
Expand Down Expand Up @@ -5848,7 +5870,7 @@ static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC:
properties.data()->propertyNameVector().shrink(0);
if (iterating->isCallable())
break;
if (iterating == globalObject || isOtherGlobal)
if (iterating == globalObject || isOtherGlobal || listsOwnEnumerableOnly)
break;
JSValue prototype = iterating->getPrototype(globalObject);
RETURN_IF_EXCEPTION(scope, );
Expand Down
6 changes: 6 additions & 0 deletions test/bundler/transpiler/macro-test.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import defaultMacro, {
identity as identity1,
identity as identity2,
ireturnapromise,
stringNumberAndBooleanPrototypes,
symbolKeys,
} from "./macro.ts" with { type: "macro" };

Expand Down Expand Up @@ -47,6 +48,11 @@ test("object with Symbol keys", () => {
expect(Reflect.ownKeys(value)).toEqual(["v"]);
});

// In JSC these prototypes are boxed values of "", 0 and false. They inline as the objects they are.
test("String.prototype, Number.prototype and Boolean.prototype", () => {
expect(stringNumberAndBooleanPrototypes()).toEqual([{}, {}, {}]);
});

test("escaping", () => {
expect(identity("\\")).toBe("\\");
expect(identity("\f")).toBe("\f");
Expand Down
4 changes: 4 additions & 0 deletions test/bundler/transpiler/macro.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,7 @@ export async function ireturnapromise() {
export function symbolKeys() {
return { v: 2, [Symbol.for("s")]: 1, [Symbol("d")]: 3, [Symbol()]: 4 };
}

export function stringNumberAndBooleanPrototypes() {
return [String.prototype, Number.prototype, Boolean.prototype];
}
45 changes: 45 additions & 0 deletions test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -939,6 +939,51 @@ test("error inline snapshots", () => {
}).toThrowErrorMatchingInlineSnapshot(`"😊"`);
});

// The snapshot printer has a property walk of its own. console.log prints the three prototypes as `{}` (#8627).
test("String.prototype, Number.prototype, Boolean.prototype and boxed values", () => {
class N extends Number {}
const withKeys = (value: object) =>
Object.defineProperty(Object.assign(value, { b: 1, a: 2 }), "hidden", { value: 3 });
expect(String.prototype).toMatchInlineSnapshot(`String {}`);
expect(Number.prototype).toMatchInlineSnapshot(`
Number {
"toExponential": [Function: toExponential],
"toFixed": [Function: toFixed],
"toLocaleString": [Function: toLocaleString],
"toPrecision": [Function: toPrecision],
"toString": [Function: toString],
"valueOf": [Function: valueOf],
}
`);
expect(Boolean.prototype).toMatchInlineSnapshot(`
Boolean {
"toString": [Function: toString],
"valueOf": [Function: valueOf],
}
`);
expect(withKeys(new Number(1))).toMatchInlineSnapshot(`
Number {
"a": 2,
"b": 1,
"hidden": 3,
}
`);
expect(withKeys(new Boolean(true))).toMatchInlineSnapshot(`
Boolean {
"a": 2,
"b": 1,
"hidden": 3,
}
`);
expect(withKeys(new N(1))).toMatchInlineSnapshot(`
N {
"a": 2,
"b": 1,
"hidden": 3,
}
`);
Comment on lines +947 to +984

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): maintainers get inline snapshots that certify the snapshot printer's divergence from Jest and that pass unchanged on the base branch. The block at test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts:947-984 pins Number.prototype as Number { "toExponential": ... } and withKeys(new Number(1)) with the non-enumerable "hidden": 3, while the same PR makes console.log print {} for the prototype; Jest prints Number {} for both. Nothing here runs code this PR changes, so the test cannot fail under USE_SYSTEM_BUN=1. Fix: either drop the block or pin only the behaviour the PR changes, and leave the snapshot printer's own-key listing for the PR that fixes it rather than snapshotting it as correct.

Why this was flagged

A later change makes the snapshot printer in src/runtime/test_runner/pretty_format.rs:2307 list only own enumerable keys, matching Jest's pretty-format and the console output this PR introduces. The inline snapshots at test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts:948-957 and :964-984 then fail, because they pin the built-in DontEnum methods of Number.prototype and the non-enumerable hidden key that JSC__JSValue__forEachPropertyOrdered collects with DontEnumPropertiesMode::Include at src/jsc/bindings/bindings.cpp:5942. The snapshot printer's Tag::get at src/runtime/test_runner/pretty_format.rs:494-499 and its walk are not edited by this diff, so every assertion in this block already passes on the base branch; the comment at :942 says as much. REVIEW.md asks that snapshots never be captured from buggy output and that each new test fail without the fix; this block does neither, so it adds maintenance cost without covering the change.

Verification: nit. The new test block runs against the snapshot printer, which this PR does not touch, so the inline snapshots pin pre-existing behaviour rather than anything the PR changes. JSC__JSValue__forEachPropertyOrdered (src/jsc/bindings/bindings.cpp:5924) is unchanged, so every assertion in the new block passes identically on the base commit.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept this block. It is there for #46729, not for the code that this PR changes.

  • Print Bun's snapshot format with the console.log formatter #46729 deletes pretty_format.rs and prints snapshots with the console.log formatter. Its description says: "Snapshot text is kept as it was, old bugs included, because people have those files committed."
  • After that PR, the arm that this PR adds to Tag::get_advanced also decides snapshot text. A merge build of Print Bun's snapshot format with the console.log formatter #46729 with an earlier version of this PR showed the effect. Number.prototype and Boolean.prototype lost their method lists, and a boxed Number with keys lost its sort order and its non-enumerable key. Both test suites stayed green.
  • The pins make that visible. The boxed rows hold this PR's exact test of the class in place: an instance is not a prototype. The prototype rows fail when the two PRs meet, so the second one to merge has to gate the arm for snapshots or change the text on purpose.

You are right that each assertion passes on the base branch, and that the text is not what Jest prints. That is the text that #46729 promises to keep. The same contract is why I did not change the ordered walk in the { sorted: true } thread. The PR body has this under "The snapshot printer".

If a maintainer prefers to keep that contract in #46729 only, I will drop the block.

});

test("snapshot numbering", () => {
function fails() {
throw new Error("snap");
Expand Down
Loading
Loading