Skip to content

Commit

Permalink
util: fix .format() not always calling toString when it should be
Browse files Browse the repository at this point in the history
This makes sure that `util.format('%s', object)` will always call
a user defined `toString` function. It was formerly not the case
when the object had the function declared on the super class.

At the same time this also makes sure that getters won't be
triggered accessing the `constructor` property.

PR-URL: #30343
Fixes: #30333
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
  • Loading branch information
BridgeAR authored and addaleax committed Nov 30, 2019
1 parent f0181d9 commit be30911
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 22 deletions.
57 changes: 35 additions & 22 deletions lib/internal/util/inspect.js
Original file line number Diff line number Diff line change
Expand Up @@ -1572,6 +1572,31 @@ function reduceToSingleString(
return `${braces[0]}${ln}${join(output, `,\n${indentation} `)} ${braces[1]}`;
}

function hasBuiltInToString(value) {
// Count objects that have no `toString` function as built-in.
if (typeof value.toString !== 'function') {
return true;
}

// The object has a own `toString` property. Thus it's not not a built-in one.
if (ObjectPrototypeHasOwnProperty(value, 'toString')) {
return false;
}

// Find the object that has the `toString` property as own property in the
// prototype chain.
let pointer = value;
do {
pointer = ObjectGetPrototypeOf(pointer);
} while (!ObjectPrototypeHasOwnProperty(pointer, 'toString'));

// Check closer if the object is a built-in.
const descriptor = ObjectGetOwnPropertyDescriptor(pointer, 'constructor');
return descriptor !== undefined &&
typeof descriptor.value === 'function' &&
builtInObjects.has(descriptor.value.name);

This comment has been minimized.

Copy link
@cpcallen

cpcallen Dec 2, 2019

I'm not especially familiar with the node.js codebase, but in isolation it is surprising to me that you would check to see if an object was a built-in by looking at the value of its .constructor property, since its value is arbitrary and not generally used (e.g., in the ES5 spec, no specification operation ever reads the value of this property of an object, and even in ES6+ it is only consulted in certain fairly arcane circumstances).

I would have thought it would be better to have a set containing the builtin objects themselves (i.e., the ones referred to by the spec as %ObjectPrototype%, %ArrayPrototype%, etc.) and test to see if pointer is itself in that set.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Dec 2, 2019

Author Member

The problem are realms. There might be different prototype instances depending on where the object was created in.

}

const firstErrorLine = (error) => error.message.split('\n')[0];
let CIRCULAR_ERROR_MESSAGE;
function tryStringify(arg) {
Expand Down Expand Up @@ -1630,29 +1655,17 @@ function formatWithOptionsInternal(inspectOptions, ...args) {
tempStr = formatNumber(stylizeNoColor, tempArg);
} else if (typeof tempArg === 'bigint') {
tempStr = `${tempArg}n`;
} else if (typeof tempArg !== 'object' ||
tempArg === null ||
!hasBuiltInToString(tempArg)) {
tempStr = String(tempArg);
} else {
let constr;
if (typeof tempArg !== 'object' ||
tempArg === null ||
(typeof tempArg.toString === 'function' &&
// A direct own property.
(ObjectPrototypeHasOwnProperty(tempArg, 'toString') ||
// A direct own property on the constructor prototype in
// case the constructor is not an built-in object.
((constr = tempArg.constructor) &&
!builtInObjects.has(constr.name) &&
constr.prototype &&
ObjectPrototypeHasOwnProperty(constr.prototype,
'toString'))))) {
tempStr = String(tempArg);
} else {
tempStr = inspect(tempArg, {
...inspectOptions,
compact: 3,
colors: false,
depth: 0
});
}
tempStr = inspect(tempArg, {
...inspectOptions,
compact: 3,
colors: false,
depth: 0
});
}
break;
case 106: // 'j'
Expand Down
60 changes: 60 additions & 0 deletions test/parallel/test-util-format.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,66 @@ assert.strictEqual(util.format('%s', () => 5), '() => 5');
util.format('%s', new Foobar(5)),
'Foobar [ <5 empty items>, aaa: true ]'
);

// Subclassing:
class B extends Foo {}

function C() {}
C.prototype.toString = function() {
return 'Custom';
};

function D() {
C.call(this);
}
D.prototype = Object.create(C.prototype);

assert.strictEqual(
util.format('%s', new B()),
'Bar'
);
assert.strictEqual(
util.format('%s', new C()),
'Custom'
);
assert.strictEqual(
util.format('%s', new D()),
'Custom'
);

D.prototype.constructor = D;
assert.strictEqual(
util.format('%s', new D()),
'Custom'
);

D.prototype.constructor = null;
assert.strictEqual(
util.format('%s', new D()),
'Custom'
);

D.prototype.constructor = { name: 'Foobar' };
assert.strictEqual(
util.format('%s', new D()),
'Custom'
);

Object.defineProperty(D.prototype, 'constructor', {
get() {
throw new Error();
},
configurable: true
});
assert.strictEqual(
util.format('%s', new D()),
'Custom'
);

assert.strictEqual(
util.format('%s', Object.create(null)),
'[Object: null prototype] {}'
);
}

// JSON format specifier
Expand Down

0 comments on commit be30911

Please sign in to comment.