Skip to content

util: make sure error causes of any type may be inspected - #41097

Closed
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:fix-non-error-cause-inspection
Closed

util: make sure error causes of any type may be inspected#41097
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:fix-non-error-cause-inspection

Conversation

@BridgeAR

Copy link
Copy Markdown
Member

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 targosDecember 6, 2021 13:34
@nodejs-github-botnodejs-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
Copy Markdown
Member

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

@BridgeAR

Copy link
Copy Markdown
MemberAuthor

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

@targos

Copy link
Copy Markdown
Member

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

@BridgeAR

Copy link
Copy Markdown
MemberAuthor

@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
BridgeARforce-pushed the fix-non-error-cause-inspection branch from 3f03997 to 38fc7f8CompareDecember 6, 2021 14:34
@targos

Copy link
Copy Markdown
Member

@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.

@BridgeARBridgeAR 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-actionsgithub-actionsBot 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
Copy Markdown
Collaborator

@BridgeAR

Copy link
Copy Markdown
MemberAuthor

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

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

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

@BridgeAR

Copy link
Copy Markdown
MemberAuthor

@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: #41097Fixes: #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
Copy Markdown
Member

@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.

Comment threadtest/message/util-inspect-error-cause.out
@BridgeAR

Copy link
Copy Markdown
MemberAuthor

@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 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: #41097Fixes: #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: #41097Fixes: #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>
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: #41097Fixes: #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>
@danielleadamsdanielleadams mentioned this pull request Feb 1, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-rebaseAdd this label to allow the Commit Queue to land a PR in several commits.needs-ciPRs that need a full CI run.utilIssues 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

@BridgeAR@targos@nodejs-github-bot@Trott@danielleadams@ljharb@jasnell@antsmartian@tniessen