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

Jump to bottom

errors: extract type detection & use in ERR_INVALID_RETURN_VALUE #43558

Merged
merged 7 commits into from
Jul 1, 2022
Merged

errors: extract type detection & use in ERR_INVALID_RETURN_VALUE #43558

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
74be6fe
errors: extract type detection & use in `ERR_INVALID_RETURN_VALUE`
JakobJingleheimer Jun 24, 2022
572c162
fixup! errors: extract type detection & use in `ERR_INVALID_RETURN_VA…
JakobJingleheimer Jun 26, 2022
4a4204e
fixup! fixup! errors: extract type detection & use in `ERR_INVALID_RE…
JakobJingleheimer Jun 26, 2022
b757821
fixup! fixup! fixup! errors: extract type detection & use in `ERR_INV…
JakobJingleheimer Jun 26, 2022
4871b85
fixup! fixup! fixup! fixup! errors: extract type detection & use in `…
JakobJingleheimer Jun 30, 2022
96dbd0e
fixup! fixup! fixup! fixup! fixup! errors: extract type detection & u…
JakobJingleheimer Jul 1, 2022
d2eded5
fixup! fixup! fixup! fixup! fixup! fixup! errors: extract type detect…
JakobJingleheimer Jul 1, 2022
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
68 changes: 43 additions & 25 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,44 @@ const genericNodeError = hideStackFrames(function genericNodeError(message, erro
return err;
});

/**
* Determine the specific type of a value for type-mismatch errors.
* @param {*} value
* @returns {string}
*/
function determineSpecificType(value) {
let type = '';

if (value == null) {
type += value;
} else if (typeof value === 'function' && value.name) {
type = `function ${value.name}`;
} else if (typeof value === 'object') {
if (value.constructor?.name) {
type = `an instance of ${value.constructor.name}`;
} else {
const inspected = lazyInternalUtilInspect()
.inspect(value, { depth: -1 });

if (StringPrototypeIncludes(inspected, '[Object: null prototype]')) {
type = 'an instance of Object';
} else {
type = inspected;
}
}
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved
} else {
let inspected = lazyInternalUtilInspect()
.inspect(value, { colors: false });
if (inspected.length > 25) {
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved
inspected = `${StringPrototypeSlice(inspected, 0, 25)}...`;
}

type = `type ${typeof value} (${inspected})`;
}

return type;
}

module.exports = {
AbortError,
aggregateTwoErrors,
Expand All @@ -866,6 +904,7 @@ module.exports = {
connResetException,
dnsException,
// This is exported only to facilitate testing.
determineSpecificType,
E,
errnoException,
exceptionWithHostPort,
Expand Down Expand Up @@ -1237,25 +1276,8 @@ E('ERR_INVALID_ARG_TYPE',
}
}

if (actual == null) {
msg += `. Received ${actual}`;
} else if (typeof actual === 'function' && actual.name) {
msg += `. Received function ${actual.name}`;
} else if (typeof actual === 'object') {
if (actual.constructor?.name) {
msg += `. Received an instance of ${actual.constructor.name}`;
} else {
const inspected = lazyInternalUtilInspect()
.inspect(actual, { depth: -1 });
msg += `. Received ${inspected}`;
}
} else {
let inspected = lazyInternalUtilInspect()
.inspect(actual, { colors: false });
if (inspected.length > 25)
inspected = `${StringPrototypeSlice(inspected, 0, 25)}...`;
msg += `. Received type ${typeof actual} (${inspected})`;
}
msg += `. Received ${determineSpecificType(actual)}`;

return msg;
}, TypeError);
E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => {
Expand Down Expand Up @@ -1335,12 +1357,8 @@ E('ERR_INVALID_RETURN_PROPERTY_VALUE', (input, name, prop, value) => {
` "${name}" function but got ${type}.`;
}, TypeError);
E('ERR_INVALID_RETURN_VALUE', (input, name, value) => {
let type;
if (value?.constructor?.name) {
type = `instance of ${value.constructor.name}`;
} else {
type = `type ${typeof value}`;
}
const type = determineSpecificType(value);

return `Expected ${input} to be returned from the "${name}"` +
` function but got ${type}.`;
}, TypeError, RangeError);
Expand Down
149 changes: 149 additions & 0 deletions test/errors/test-error-value-type-detection.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
// Flags: --expose-internals

import '../common/index.mjs';
import { strictEqual } from 'node:assert';
import errorsModule from 'internal/errors';


const { determineSpecificType } = errorsModule;

strictEqual(
determineSpecificType(1n),
'type bigint (1n)',
);

strictEqual(
determineSpecificType(false),
'type boolean (false)',
);

strictEqual(
determineSpecificType(2),
'type number (2)',
);

strictEqual(
determineSpecificType(NaN),
'type number (NaN)',
);

strictEqual(
determineSpecificType(Infinity),
'type number (Infinity)',
);

strictEqual(
determineSpecificType(''),
"type string ('')",
);

strictEqual(
determineSpecificType(Symbol('foo')),
'type symbol (Symbol(foo))',
);

strictEqual(
determineSpecificType(function foo() {}),
'function foo',
);

strictEqual(
determineSpecificType(null),
'null',
);

strictEqual(
determineSpecificType(undefined),
'undefined',
);

strictEqual(
determineSpecificType([]),
'an instance of Array',
);

strictEqual(
determineSpecificType(new Array()),
'an instance of Array',
);
strictEqual(
determineSpecificType(new BigInt64Array()),
'an instance of BigInt64Array',
);
strictEqual(
determineSpecificType(new BigUint64Array()),
'an instance of BigUint64Array',
);
strictEqual(
determineSpecificType(new Int8Array()),
'an instance of Int8Array',
);
strictEqual(
determineSpecificType(new Int16Array()),
'an instance of Int16Array',
);
strictEqual(
determineSpecificType(new Int32Array()),
'an instance of Int32Array',
);
strictEqual(
determineSpecificType(new Float32Array()),
'an instance of Float32Array',
);
strictEqual(
determineSpecificType(new Float64Array()),
'an instance of Float64Array',
);
strictEqual(
determineSpecificType(new Uint8Array()),
'an instance of Uint8Array',
);
strictEqual(
determineSpecificType(new Uint8ClampedArray()),
'an instance of Uint8ClampedArray',
);
strictEqual(
determineSpecificType(new Uint16Array()),
'an instance of Uint16Array',
);
strictEqual(
determineSpecificType(new Uint32Array()),
'an instance of Uint32Array',
);

strictEqual(
determineSpecificType(new Date()),
'an instance of Date',
);

strictEqual(
determineSpecificType(new Map()),
'an instance of Map',
);
strictEqual(
determineSpecificType(new WeakMap()),
'an instance of WeakMap',
);

strictEqual(
determineSpecificType(Object.create(null)),
'an instance of Object',
Copy link
Contributor

Choose a reason for hiding this comment

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

Well that's just untrue x) Object.create(null) instanceof Object === false

Copy link
Contributor Author

@JakobJingleheimer JakobJingleheimer Jun 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Technically untrue, yes, but I think being helpful and a tiny bit wrong is better.

I had originally put 'an instance of Object (null Prototype)', but decided that was TMI; if I put that in, would it address your concern? (Or perhaps 'type object (null Prototype)' or any other similar flavour)

Otherwise you end up with a error message like Expected an instance of Map, but instead got [Object: null prototype] {} (which looks ugly and kind of confusing)

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically untrue, yes, but I think being helpful and a tiny bit wrong is better.

I very much disagree here, I feel quite strongly that error messages must always be technically correct first, to me I believe one can't be helpful if they are misleading.

I think it should not be special cased, type object ([Object: null prototype] {}) is good enough to me. What is it that you don't like about it exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is it that you don't like about it exactly?

It looks bizarre before your suggested change below—it didn't have type (I think it's better now with your suggestion), it was just got [Object: null prototype] {}); type object (null Prototype) seems more human-friendly to me, and similar to other type outputs (ex type number (2)), but I can live with type object ([Object: null prototype] ...).

Note that the actual output with your suggestion below is type object ([Object: null prototype] ...)

Copy link
Member

Choose a reason for hiding this comment

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

I definitely prefer the former output: limiting the string was only done for primitives and functions. Objects have not been inspected closer and just the most outer layer was inspected. That's not the case anymore and would possibly hide important information. The reason not to add type object is that it should be obvious that it's an object as everything is that's not a primitive (or function). It was therefore a way to limit the output to the necessary information.

Please change that back.

);
strictEqual(
determineSpecificType({}),
'an instance of Object',
);

strictEqual(
determineSpecificType(Promise.resolve('foo')),
'an instance of Promise',
);

strictEqual(
determineSpecificType(new Set()),
'an instance of Set',
);
strictEqual(
determineSpecificType(new WeakSet()),
'an instance of WeakSet',
);
5 changes: 3 additions & 2 deletions test/parallel/test-assert-async.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,9 @@ const invalidThenableFunc = () => {
promises.push(assert.rejects(promise, {
name: 'TypeError',
code: 'ERR_INVALID_RETURN_VALUE',
// FIXME(@JakobJingleheimer): This should match on key words, like /Promise/ and /undefined/
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved
message: 'Expected instance of Promise to be returned ' +
'from the "promiseFn" function but got type undefined.'
'from the "promiseFn" function but got undefined.'
}));

promise = assert.rejects(Promise.resolve(), common.mustNotCall());
Expand Down Expand Up @@ -162,7 +163,7 @@ promises.push(assert.rejects(
let promise = assert.doesNotReject(() => new Map(), common.mustNotCall());
promises.push(assert.rejects(promise, {
message: 'Expected instance of Promise to be returned ' +
'from the "promiseFn" function but got instance of Map.',
'from the "promiseFn" function but got an instance of Map.',
code: 'ERR_INVALID_RETURN_VALUE',
name: 'TypeError'
}));
Expand Down