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.

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: make sure error causes of any type may be inspected #41097

Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 6, 2021

An error cause may be of any type. Handle all of them, no matter
if they are an error or not.

Fixes: #41096

Signed-off-by: Ruben Bridgewater ruben@bridgewater.de

@BridgeAR BridgeAR requested a review from targos December 6, 2021 13:34
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Dec 6, 2021
@targos
Copy link
Member

targos commented Dec 6, 2021

I also found that falsy causes are not inspected, but that can be fixed in a separate PR.

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2021

@targos how would you handle undefined and null? I would at least skip those?

@targos
Copy link
Member

targos commented Dec 6, 2021

I would always inspect if Object.hasOwn(err, 'cause') === true with no exception.

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2021

@targos I added another commit to also inspect falsy values besides undefined. Undefined is used in argument default values as "does not exist" and I would handle it here identically.

An error cause may be of any type. Handle all of them, no matter
if they are an error or not.

Fixes: nodejs#41096

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR force-pushed the fix-non-error-cause-inspection branch from 3f03997 to 38fc7f8 Compare December 6, 2021 14:34
@targos
Copy link
Member

targos commented Dec 6, 2021

@targos I added another commit to also inspect falsy values besides undefined. Undefined is used in argument default values as "does not exist" and I would handle it here identically.

Okay, I don't have a strong opinion, but new Error('message', { cause: undefined }) creates the property while just new Error('message') doesn't.

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 6, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 8, 2021

@nodejs/util PTAL. This should ideally land before the next release.

@BridgeAR BridgeAR added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 8, 2021
@nodejs-github-bot 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 Dec 8, 2021
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41097
✔  Done loading data for nodejs/node/pull/41097
----------------------------------- PR info ------------------------------------
Title      util: make sure error causes of any type may be inspected (#41097)
Author     Ruben Bridgewater  (@BridgeAR)
Branch     BridgeAR:fix-non-error-cause-inspection -> nodejs:master
Labels     util, author ready, needs-ci
Commits    2
 - util: make sure error causes of any type may be inspected
 - util: serialize falsy cause values while inspecting errors
Committers 1
 - Ruben Bridgewater 
PR-URL: https://github.com/nodejs/node/pull/41097
Fixes: https://github.com/nodejs/node/issues/41096
Reviewed-By: Michaël Zasso 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41097
Fixes: https://github.com/nodejs/node/issues/41096
Reviewed-By: Michaël Zasso 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 06 Dec 2021 13:34:46 GMT
   ✔  Approvals: 2
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/41097#pullrequestreview-824046654
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/41097#pullrequestreview-826822688
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2021-12-07T19:10:46Z: https://ci.nodejs.org/job/node-test-pull-request/41399/
- Querying data for job/node-test-pull-request/41399/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 41097
From https://github.com/nodejs/node
 * branch                  refs/pull/41097/merge -> FETCH_HEAD
✔  Fetched commits as 18ff5832501b..38fc7f86a0bf
--------------------------------------------------------------------------------
Auto-merging lib/internal/util/inspect.js
[master 12b79bd4f5] util: make sure error causes of any type may be inspected
 Author: Ruben Bridgewater 
 Date: Mon Dec 6 14:22:30 2021 +0100
 3 files changed, 39 insertions(+), 1 deletion(-)
Auto-merging lib/internal/util/inspect.js
Auto-merging test/parallel/test-util-inspect.js
[master 1272d9fe5a] util: serialize falsy cause values while inspecting errors
 Author: Ruben Bridgewater 
 Date: Mon Dec 6 15:25:42 2021 +0100
 2 files changed, 12 insertions(+), 1 deletion(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes
⚠ Found Fixes: #41096, skipping..
--------------------------------- New Message ----------------------------------
util: make sure error causes of any type may be inspected

An error cause may be of any type. Handle all of them, no matter
if they are an error or not.

Fixes: #41096

Signed-off-by: Ruben Bridgewater ruben@bridgewater.de

PR-URL: #41097
Reviewed-By: Michaël Zasso targos@protonmail.com
Reviewed-By: James M Snell jasnell@gmail.com

[detached HEAD b95b3f5eb4] util: make sure error causes of any type may be inspected
Author: Ruben Bridgewater ruben@bridgewater.de
Date: Mon Dec 6 14:22:30 2021 +0100
3 files changed, 39 insertions(+), 1 deletion(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
util: serialize falsy cause values while inspecting errors

Signed-off-by: Ruben Bridgewater ruben@bridgewater.de

PR-URL: #41097
Fixes: #41096
Reviewed-By: Michaël Zasso targos@protonmail.com
Reviewed-By: James M Snell jasnell@gmail.com

[detached HEAD 3d14b8d6bb] util: serialize falsy cause values while inspecting errors
Author: Ruben Bridgewater ruben@bridgewater.de
Date: Mon Dec 6 15:25:42 2021 +0100
2 files changed, 12 insertions(+), 1 deletion(-)

Successfully rebased and updated refs/heads/master.

ℹ Use --fixupAll option, squash the PR manually or land the PR from the command line.

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

@danielleadams
Copy link
Member

@BridgeAR this didn't land cleanly into the release, can you backport this to v17.x-staging?

@BridgeAR
Copy link
Member Author

@danielleadams this should land cleanly on top of #41002.

danielleadams pushed a commit that referenced this pull request Dec 14, 2021
An error cause may be of any type. Handle all of them, no matter
if they are an error or not.

Fixes: #41096

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41097
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41097
Fixes: #41096
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
@ljharb
Copy link
Member

ljharb commented Dec 16, 2021

@BridgeAR the spec for error cause very intentionally differentiates between an absent property, and a present undefined, so it's critically important that util.inspect differentiate between these two.

@BridgeAR
Copy link
Member Author

@ljharb I am fine if you like to also visualize an undefined cause. Please feel free to open another PR.

ljharb added a commit to ljharb/node that referenced this pull request Dec 19, 2021
ljharb added a commit to ljharb/node that referenced this pull request Dec 19, 2021
ljharb added a commit to ljharb/node that referenced this pull request Dec 20, 2021
nodejs-github-bot pushed a commit that referenced this pull request Dec 21, 2021
See #41097 (comment)

PR-URL: #41247
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2022
See #41097 (comment)

PR-URL: #41247
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
An error cause may be of any type. Handle all of them, no matter
if they are an error or not.

Fixes: #41096

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41097
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41097
Fixes: #41096
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
An error cause may be of any type. Handle all of them, no matter
if they are an error or not.

Fixes: #41096

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41097
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41097
Fixes: #41096
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
See #41097 (comment)

PR-URL: #41247
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
An error cause may be of any type. Handle all of them, no matter
if they are an error or not.

Fixes: nodejs#41096

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#41097
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#41097
Fixes: nodejs#41096
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
See nodejs#41097 (comment)

PR-URL: nodejs#41247
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
An error cause may be of any type. Handle all of them, no matter
if they are an error or not.

Fixes: #41096

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41097
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41097
Fixes: #41096
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
See #41097 (comment)

PR-URL: #41247
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inspection of error with cause fails for non-error causes
9 participants