errors: extract type detection & use in `ERR_INVALID_RETURN_VALUE` #43558

JakobJingleheimer · 2022-06-24T10:16:18Z

Address issues in error messaging like when value is null (error message previously says something to the effect of expected an object […] but got an object).

BridgeAR

LGTM

Is there no test that requires an update?

aduh95

LGTM. We tend not to test for error messages because we want to be able to change them in semver-patch commits, it’s not surprising that there are no tests to update.

nodejs-github-bot · 2022-06-24T11:31:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44830/

JakobJingleheimer · 2022-06-24T13:41:47Z

We tend not to test for error messages because we want to be able to change them in semver-patch commits

Comparison {
5044
    code: 'ERR_INVALID_RETURN_VALUE',
5045
+   message: 'Expected instance of Promise to be returned from the "promiseFn" function but got undefined.',
5046
-   message: 'Expected instance of Promise to be returned from the "promiseFn" function but got type undefined.',
5047
    name: 'TypeError'
5048
  }

What were you saying @aduh95? 😆 I'll update that test tomorrow.

test/parallel/test-assert-async.js

test/errors/test-error-value-type-detection.mjs

aduh95 · 2022-06-26T10:56:18Z

test/errors/test-error-value-type-detection.mjs

+  determineSpecificType(Object.create(null)),
+  'an instance of Object',


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

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)

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?

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] ...)

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.

lib/internal/errors.js

…LUE` `new Array(0)` et al → new Array(); tag self in FIXME

test/parallel/test-assert-async.js

lib/internal/errors.js

…TURN_VALUE` type object (…)

…ALID_RETURN_VALUE` de-lint

JakobJingleheimer · 2022-06-26T15:34:19Z

@aduh95 @BridgeAR looks like there's a test helper in common that duplicates the functionality of the new determineSpecificType().

node/test/common/index.js

Lines 732 to 749 in 42ad967

    
           function invalidArgTypeHelper(input) { 
        
             if (input == null) { 
        
               return ` Received ${input}`; 
        
             } 
        
             if (typeof input === 'function' && input.name) { 
        
               return ` Received function ${input.name}`; 
        
             } 
        
             if (typeof input === 'object') { 
        
               if (input.constructor && input.constructor.name) { 
        
                 return ` Received an instance of ${input.constructor.name}`; 
        
               } 
        
               return ` Received ${inspect(input, { depth: -1 })}`; 
        
             } 
        
             let inspected = inspect(input, { colors: false }); 
        
             if (inspected.length > 25) 
        
               inspected = `${inspected.slice(0, 25)}...`; 
        
             return ` Received type ${typeof input} (${inspected})`; 
        
           }

I think it should be de-duplicated. Would it be okay to import the new helper into test/common and re-export it?

aduh95 · 2022-06-26T16:15:14Z

I think it should be de-duplicated. Would it be okay to import the new helper into test/common and re-export it?

What about removing it? If we have tests for determineSpecificType, we probably don't need it at all, from what I can see it's only used to validate exact error messages, which we don't need to do.

JakobJingleheimer · 2022-06-26T17:14:44Z

What about removing it? If we have tests for determineSpecificType, we probably don't need it at all, from what I can see it's only used to validate exact error messages, which we don't need to do.

I think that's right thing to do in the end, but it's used in 104 places, most of which are using things like assert.throws that strict equals error.message under the hood. This would be a fairly significant refactor (and I presume we can't change assert.throws, asserts.rejects, etc), and we would basically have to stop using those assertions.

aduh95 · 2022-06-26T19:19:04Z

What about removing it? If we have tests for determineSpecificType, we probably don't need it at all, from what I can see it's only used to validate exact error messages, which we don't need to do.

I think that's right thing to do in the end, but it's used in 104 places, most of which are using things like assert.throws that strict equals error.message under the hood. This would be a fairly significant refactor (and I presume we can't change assert.throws, asserts.rejects, etc), and we would basically have to stop using those assertions.

I agree it’s not something to do in this PR, so I guess my stance on it is we should leave it as is for now.
FWIW assert.throws supports using regex to check for a pattern rather than an exact string equality (e.g. assert.throws(()=>{throw new Error("this is a test")},{message:/test$/})).

JakobJingleheimer · 2022-06-26T19:25:55Z

Oow! That's good to know, but I think that isn't sufficient because it would do only 1 match, and there are multiple key words that need to match (the type it should've been and the type it received).

I might have to update a whole lotta tests' message checks—test parallel is drip-feeding me the failures.

BridgeAR · 2022-06-26T20:37:32Z

I suggest to keep everything as is. The point to test internal error messages even for concrete errors is actually useful in a couple of cases where it's possible to pass through individual error message content. Yes, it's possible to use a regexp and that would mostly suffice while it would not always detect all issues. We do not have a downside by keeping it as is, so why do that?

JakobJingleheimer · 2022-06-26T22:20:51Z

There is a downside to keeping it as is: it makes the tests rigid and brittle, and creates make-work for inconsequential changes. The error messages are the same aside from the keys words that matter and can change, so checking for those are all the actual matters downstream (especially when upstream tests already guarantee that they're the same).

nodejs-github-bot · 2022-07-01T07:07:35Z

Commit Queue failed

- Loading data for nodejs/node/pull/43558
✔  Done loading data for nodejs/node/pull/43558
----------------------------------- PR info ------------------------------------
Title      errors: extract type detection & use in `ERR_INVALID_RETURN_VALUE` (#43558)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     JakobJingleheimer:errors/account-for-specific-types-in-return-value -> nodejs:main
Labels     errors, needs-ci, commit-queue-squash
Commits    5
 - errors: extract type detection & use in `ERR_INVALID_RETURN_VALUE`
 - fixup! errors: extract type detection & use in `ERR_INVALID_RETURN_VA…
 - fixup! fixup! errors: extract type detection & use in `ERR_INVALID_RE…
 - fixup! fixup! fixup! errors: extract type detection & use in `ERR_INV…
 - fixup! fixup! fixup! fixup! errors: extract type detection & use in `…
Committers 1
 - Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
PR-URL: https://github.com/nodejs/node/pull/43558
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Antoine du Hamel 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43558
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Antoine du Hamel 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup! fixup! fixup! fixup! errors: extract type detection & use in `…
   ℹ  This PR was created on Fri, 24 Jun 2022 10:16:18 GMT
   ✔  Approvals: 3
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/43558#pullrequestreview-1018314632
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/43558#pullrequestreview-1018326931
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/43558#pullrequestreview-1020506108
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-06-24T11:31:50Z: https://ci.nodejs.org/job/node-test-pull-request/44830/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - errors: extract type detection & use in `ERR_INVALID_RETURN_VALUE`
   ⚠  - fixup! errors: extract type detection & use in `ERR_INVALID_RETURN_VA…
   ⚠  - fixup! fixup! errors: extract type detection & use in `ERR_INVALID_RE…
   ⚠  - fixup! fixup! fixup! errors: extract type detection & use in `ERR_INV…
   ⚠  - fixup! fixup! fixup! fixup! errors: extract type detection & use in `…
- Querying data for job/node-test-pull-request/44830/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2594944073

BridgeAR

Please do not change the message for objects without constructor (name).

aduh95 · 2022-07-01T12:21:14Z

@BridgeAR can you clarify why? Is it because you worry it might be too breaking or because you think the new message is worse?

BridgeAR · 2022-07-01T12:22:08Z

@aduh95 the latter.

JakobJingleheimer · 2022-07-01T12:39:33Z

@BridgeAR sorry! I didn't mean to sneak something through. Antoine was online, so I DM'd him to ask how to unblock this PR (which is blocking an esm bugfix), and it seemed I had misunderstood your objection (but apparently not).

I mentioned your opposition a few times in the above comments, and the dialogue spanned a couple days without you chiming in (and you had approved and didn't change it), so I thought I'd misinterpreted your comments.

I'll revert the change to objects without a constructor name.

…se in `ERR_INVALID_RETURN_VALUE` revert type determination output for objects without a constructor

BridgeAR

Thanks!

BridgeAR · 2022-07-01T13:12:49Z

test/common/index.js

@@ -737,14 +737,15 @@ function invalidArgTypeHelper(input) {
    return ` Received function ${input.name}`;
  }
  if (typeof input === 'object') {
-    if (input.constructor && input.constructor.name) {
+    if (input?.constructor?.name) {


Suggested change

if (input?.constructor?.name) {

if (input.constructor?.name) {

We know for certain that input is an object of some kind (null is already checked above).

BridgeAR · 2022-07-01T13:13:52Z

lib/internal/errors.js

+    return `function ${value.name}`;
+  }
+  if (typeof value === 'object') {
+    if (value?.constructor?.name) {


Suggested change

if (value?.constructor?.name) {

if (value.constructor?.name) {

…ion & use in `ERR_INVALID_RETURN_VALUE` remove superfluous optional property access on constructor

JakobJingleheimer · 2022-07-01T16:46:51Z

@aduh95 or @BridgeAR could one of you approve again? I added Ruben's suggested tweak after he just approved, so I think the commit queue will reject.

nodejs-github-bot · 2022-07-01T16:58:22Z

Commit Queue failed

- Loading data for nodejs/node/pull/43558
✔  Done loading data for nodejs/node/pull/43558
----------------------------------- PR info ------------------------------------
Title      errors: extract type detection & use in `ERR_INVALID_RETURN_VALUE` (#43558)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     JakobJingleheimer:errors/account-for-specific-types-in-return-value -> nodejs:main
Labels     errors, needs-ci, commit-queue-squash
Commits    7
 - errors: extract type detection & use in `ERR_INVALID_RETURN_VALUE`
 - fixup! errors: extract type detection & use in `ERR_INVALID_RETURN_VA…
 - fixup! fixup! errors: extract type detection & use in `ERR_INVALID_RE…
 - fixup! fixup! fixup! errors: extract type detection & use in `ERR_INV…
 - fixup! fixup! fixup! fixup! errors: extract type detection & use in `…
 - fixup! fixup! fixup! fixup! fixup! errors: extract type detection & u…
 - fixup! fixup! fixup! fixup! fixup! fixup! errors: extract type detect…
Committers 1
 - Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
PR-URL: https://github.com/nodejs/node/pull/43558
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Antoine du Hamel 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43558
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Antoine du Hamel 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup! fixup! fixup! fixup! fixup! fixup! errors: extract type detect…
   ℹ  This PR was created on Fri, 24 Jun 2022 10:16:18 GMT
   ✔  Approvals: 3
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/43558#pullrequestreview-1026202846
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/43558#pullrequestreview-1018326931
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/43558#pullrequestreview-1020506108
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-07-01T07:07:35Z: https://ci.nodejs.org/job/node-test-pull-request/44830/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - fixup! fixup! fixup! fixup! fixup! errors: extract type detection & u…
   ⚠  - fixup! fixup! fixup! fixup! fixup! fixup! errors: extract type detect…
- Querying data for job/node-test-pull-request/44830/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2598011811

nodejs-github-bot · 2022-07-01T18:08:10Z

Landed in 5a18304

PR-URL: #43558 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs/node#43558 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

JakobJingleheimer requested a review from aduh95 June 24, 2022 10:16

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. labels Jun 24, 2022

BridgeAR approved these changes Jun 24, 2022

View reviewed changes

aduh95 approved these changes Jun 24, 2022

View reviewed changes

BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2022

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 24, 2022

github-actions bot mentioned this pull request Jun 25, 2022

CI Reliability 2022-06-25 nodejs/reliability#311

Open

32 tasks

BridgeAR reviewed Jun 25, 2022

View reviewed changes

test/parallel/test-assert-async.js Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Jun 26, 2022

CI Reliability 2022-06-26 nodejs/reliability#312

Open

32 tasks

errors: extract type detection & use in ERR_INVALID_RETURN_VALUE

74be6fe

JakobJingleheimer force-pushed the errors/account-for-specific-types-in-return-value branch from c0fae42 to 74be6fe Compare June 26, 2022 10:28

aduh95 reviewed Jun 26, 2022

View reviewed changes

test/errors/test-error-value-type-detection.mjs Outdated Show resolved Hide resolved

aduh95 reviewed Jun 26, 2022

View reviewed changes

lib/internal/errors.js Outdated Show resolved Hide resolved

fixup! errors: extract type detection & use in `ERR_INVALID_RETURN_VA…

572c162

…LUE` `new Array(0)` et al → new Array(); tag self in FIXME

aduh95 reviewed Jun 26, 2022

View reviewed changes

test/parallel/test-assert-async.js Outdated Show resolved Hide resolved

aduh95 reviewed Jun 26, 2022

View reviewed changes

lib/internal/errors.js Outdated Show resolved Hide resolved

JakobJingleheimer added 2 commits June 26, 2022 13:53

fixup! fixup! errors: extract type detection & use in `ERR_INVALID_RE…

4a4204e

…TURN_VALUE` type object (…)

fixup! fixup! fixup! errors: extract type detection & use in `ERR_INV…

b757821

…ALID_RETURN_VALUE` de-lint

JakobJingleheimer added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jul 1, 2022

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 1, 2022

BridgeAR requested changes Jul 1, 2022

View reviewed changes

fixup! fixup! fixup! fixup! fixup! errors: extract type detection & u…

96dbd0e

…se in `ERR_INVALID_RETURN_VALUE` revert type determination output for objects without a constructor

BridgeAR approved these changes Jul 1, 2022

View reviewed changes

BridgeAR reviewed Jul 1, 2022

View reviewed changes

fixup! fixup! fixup! fixup! fixup! fixup! errors: extract type detect…

d2eded5

…ion & use in `ERR_INVALID_RETURN_VALUE` remove superfluous optional property access on constructor

JakobJingleheimer added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jul 1, 2022

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 1, 2022

BridgeAR approved these changes Jul 1, 2022

View reviewed changes

JakobJingleheimer added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jul 1, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 1, 2022

nodejs-github-bot merged commit 5a18304 into nodejs:main Jul 1, 2022

targos mentioned this pull request Jul 12, 2022

v18.6.0 release proposal #43789

Merged

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: extract type detection & use in `ERR_INVALID_RETURN_VALUE` #43558

errors: extract type detection & use in `ERR_INVALID_RETURN_VALUE` #43558

JakobJingleheimer commented Jun 24, 2022

BridgeAR left a comment

aduh95 left a comment

nodejs-github-bot commented Jun 24, 2022

JakobJingleheimer commented Jun 24, 2022

aduh95 Jun 26, 2022

JakobJingleheimer Jun 26, 2022 •

edited

Loading

aduh95 Jun 26, 2022

JakobJingleheimer Jun 26, 2022

BridgeAR Jun 26, 2022

JakobJingleheimer commented Jun 26, 2022

aduh95 commented Jun 26, 2022

JakobJingleheimer commented Jun 26, 2022

aduh95 commented Jun 26, 2022

JakobJingleheimer commented Jun 26, 2022

BridgeAR commented Jun 26, 2022

JakobJingleheimer commented Jun 26, 2022

nodejs-github-bot commented Jul 1, 2022

BridgeAR left a comment

aduh95 commented Jul 1, 2022

BridgeAR commented Jul 1, 2022

JakobJingleheimer commented Jul 1, 2022 •

edited

Loading

BridgeAR left a comment

BridgeAR Jul 1, 2022

BridgeAR Jul 1, 2022

JakobJingleheimer commented Jul 1, 2022

nodejs-github-bot commented Jul 1, 2022

nodejs-github-bot commented Jul 1, 2022

		determineSpecificType(Object.create(null)),
		'an instance of Object',

	if (input?.constructor?.name) {
	if (input.constructor?.name) {

	if (value?.constructor?.name) {
	if (value.constructor?.name) {

errors: extract type detection & use in ERR_INVALID_RETURN_VALUE #43558

errors: extract type detection & use in ERR_INVALID_RETURN_VALUE #43558

Conversation

JakobJingleheimer commented Jun 24, 2022

BridgeAR left a comment

Choose a reason for hiding this comment

aduh95 left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 24, 2022

JakobJingleheimer commented Jun 24, 2022

aduh95 Jun 26, 2022

Choose a reason for hiding this comment

JakobJingleheimer Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

aduh95 Jun 26, 2022

Choose a reason for hiding this comment

JakobJingleheimer Jun 26, 2022

Choose a reason for hiding this comment

BridgeAR Jun 26, 2022

Choose a reason for hiding this comment

JakobJingleheimer commented Jun 26, 2022

aduh95 commented Jun 26, 2022

JakobJingleheimer commented Jun 26, 2022

aduh95 commented Jun 26, 2022

JakobJingleheimer commented Jun 26, 2022

BridgeAR commented Jun 26, 2022

JakobJingleheimer commented Jun 26, 2022

nodejs-github-bot commented Jul 1, 2022

BridgeAR left a comment

Choose a reason for hiding this comment

aduh95 commented Jul 1, 2022

BridgeAR commented Jul 1, 2022

JakobJingleheimer commented Jul 1, 2022 • edited Loading

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Jul 1, 2022

Choose a reason for hiding this comment

BridgeAR Jul 1, 2022

Choose a reason for hiding this comment

JakobJingleheimer commented Jul 1, 2022

nodejs-github-bot commented Jul 1, 2022

nodejs-github-bot commented Jul 1, 2022

errors: extract type detection & use in `ERR_INVALID_RETURN_VALUE` #43558

errors: extract type detection & use in `ERR_INVALID_RETURN_VALUE` #43558

JakobJingleheimer Jun 26, 2022 •

edited

Loading

JakobJingleheimer commented Jul 1, 2022 •

edited

Loading