Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: improve inspects RegExp support #25192

Closed
wants to merge 2 commits into from
Closed

util: improve inspects RegExp support #25192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
daaf92e
util: make inspect aware of RegExp subclasses and null prototype
BridgeAR Dec 23, 2018
eabee50
test: add more inspect subclassing tests
BridgeAR Dec 23, 2018
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
7 changes: 5 additions & 2 deletions lib/internal/util/inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,9 +634,12 @@ function formatRaw(ctx, value, recurseTimes) {
base = `[${name}]`;
} else if (isRegExp(value)) {
// Make RegExps say that they are RegExps
base = regExpToString(constructor !== null ? value : new RegExp(value));
const prefix = getPrefix(constructor, tag, 'RegExp');
if (prefix !== 'RegExp ')
base = `${prefix}${base}`;
if (keys.length === 0 || recurseTimes < 0)
return ctx.stylize(regExpToString(value), 'regexp');
base = `${regExpToString(value)}`;
return ctx.stylize(base, 'regexp');
} else if (isDate(value)) {
// Make dates with properties first say the date
if (keys.length === 0) {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-assert-deep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ assert.throws(
{
code: 'ERR_ASSERTION',
message: `${defaultMsgStartFull}\n\n` +
"+ /test/\n- /test/ {\n- '0': '1'\n- }"
"+ /test/\n- MyRegExp /test/ {\n- '0': '1'\n- }"
}
);

Expand Down
46 changes: 36 additions & 10 deletions test/parallel/test-util-inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1580,15 +1580,7 @@ assert.strictEqual(util.inspect('"\''), '`"\'`');
// eslint-disable-next-line no-template-curly-in-string
assert.strictEqual(util.inspect('"\'${a}'), "'\"\\'${a}'");

{
assert.strictEqual(
util.inspect(Object.setPrototypeOf(/a/, null)),
'/undefined/undefined'
);
}

// Verify that throwing in valueOf and having no prototype still produces nice
// results.
// Verify that throwing in valueOf and toString still produces nice results.
[
[new String(55), "[String: '55']"],
[new Boolean(true), '[Boolean: true]'],
Expand All @@ -1609,6 +1601,7 @@ assert.strictEqual(util.inspect('"\'${a}'), "'\"\\'${a}'");
[new Promise((resolve) => setTimeout(resolve, 10)), 'Promise { <pending> }'],
[new WeakSet(), 'WeakSet { <items unknown> }'],
[new WeakMap(), 'WeakMap { <items unknown> }'],
[/foobar/g, '/foobar/g']
].forEach(([value, expected]) => {
Object.defineProperty(value, 'valueOf', {
get() {
Expand All @@ -1628,6 +1621,7 @@ assert.strictEqual(util.inspect('"\'${a}'), "'\"\\'${a}'");
assert.notStrictEqual(util.inspect(value), expected);
});

// Verify that having no prototype still produces nice results.
[
[[1, 3, 4], '[Array: null prototype] [ 1, 3, 4 ]'],
[new Set([1, 2]), '[Set: null prototype] { 1, 2 }'],
Expand All @@ -1652,7 +1646,8 @@ assert.strictEqual(util.inspect('"\'${a}'), "'\"\\'${a}'");
'[DataView: null prototype] {\n byteLength: undefined,\n ' +
'byteOffset: undefined,\n buffer: undefined }'],
[new SharedArrayBuffer(2), '[SharedArrayBuffer: null prototype] ' +
'{ byteLength: undefined }']
'{ byteLength: undefined }'],
[/foobar/, '[RegExp: null prototype] /foobar/']
].forEach(([value, expected]) => {
assert.strictEqual(
util.inspect(Object.setPrototypeOf(value, null)),
Expand All @@ -1665,6 +1660,37 @@ assert.strictEqual(util.inspect('"\'${a}'), "'\"\\'${a}'");
assert.notStrictEqual(util.inspect(value), expected);
});

// Verify that subclasses with and without prototype produce nice results.
[
[RegExp, ['foobar', 'g'], '/foobar/g'],
[WeakSet, [[{}]], '{ <items unknown> }'],
[WeakMap, [[[{}, {}]]], '{ <items unknown> }'],
[BigInt64Array, [10], '[ 0n, 0n, 0n, 0n, 0n, 0n, 0n, 0n, 0n, 0n ]']
].forEach(([base, input, rawExpected]) => {
class Foo extends base {}
const value = new Foo(...input);
const symbol = value[Symbol.toStringTag];
const expected = `Foo ${symbol ? `[${symbol}] ` : ''}${rawExpected}`;
const expectedWithoutProto = `[${base.name}: null prototype] ${rawExpected}`;
assert.strictEqual(util.inspect(value), expected);
value.foo = 'bar';
assert.notStrictEqual(util.inspect(value), expected);
Copy link
Contributor

Choose a reason for hiding this comment

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

One small suggestion: may be we want to pull out the test cases for properties in a separate block. Would be happy to see strictEqual rather than notStrictEqual(as it can silently fail on wrong op's).

Copy link
Member Author

Choose a reason for hiding this comment

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

We have lots of tests which verify that the properties are actually displayed correct and they always hit the same code path. Therefore verifying that it's a different result should be sufficient.
Below there's also another test to verify that at least the property is indeed displayed as expected.

The reason for not using the strict comparison here is that each data type has different ways of displaying some parts and it's difficult to generalize that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, it is sufficient as you said above but thought it could be better using strictEqual. Anyways this is not a blocker to land.

delete value.foo;
assert.strictEqual(
util.inspect(Object.setPrototypeOf(value, null)),
expectedWithoutProto
);
value.foo = 'bar';
let res = util.inspect(value);
assert.notStrictEqual(res, expectedWithoutProto);
assert(/foo: 'bar'/.test(res), res);
delete value.foo;
value[Symbol('foo')] = 'yeah';
res = util.inspect(value);
assert.notStrictEqual(res, expectedWithoutProto);
assert(/\[Symbol\(foo\)]: 'yeah'/.test(res), res);
});

assert.strictEqual(inspect(1n), '1n');
assert.strictEqual(inspect(Object(-1n)), '[BigInt: -1n]');
assert.strictEqual(inspect(Object(13n)), '[BigInt: 13n]');
Expand Down