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

util: inspect: do not crash on an Error with a regex name #56574

Merged
merged 1 commit into from
Jan 15, 2025
Merged

util: inspect: do not crash on an Error with a regex name #56574

merged 1 commit into from
Jan 15, 2025

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Jan 12, 2025

See #56570 - this avoids a crash with a regex set as the name property of an Error

@ljharb ljharb added the util Issues and PRs related to the built-in util module. label Jan 12, 2025
@ljharb ljharb requested review from jasnell and BridgeAR January 12, 2025 21:16
@ljharb ljharb force-pushed the inspect-regex-error-stack branch from 1f2a8f0 to d33853a Compare January 12, 2025 21:16
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 12, 2025
@ljharb ljharb mentioned this pull request Jan 12, 2025
Open
@ljharb ljharb changed the title util: inspect: do not crash on an Error insance with a regex name util: inspect: do not crash on an Error instance with a regex name Jan 12, 2025
@ljharb ljharb changed the title util: inspect: do not crash on an Error instance with a regex name util: inspect: do not crash on an Error with a regex name Jan 12, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 12, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.20%. Comparing base (2570f95) to head (a9638dc).
Report is 10 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #56574   +/-   ##
=======================================
  Coverage   89.20%   89.20%           
=======================================
  Files         662      662           
  Lines      191817   191828   +11     
  Branches    36922    36928    +6     
=======================================
+ Hits       171101   171113   +12     
- Misses      13556    13561    +5     
+ Partials     7160     7154    -6
Files with missing lines Coverage Δ
lib/internal/util/inspect.js 99.95% <100.00%> (+0.04%) ⬆️

... and 26 files with indirect coverage changes

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Jan 13, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
BridgeAR
BridgeAR reviewed Jan 13, 2025
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I think we could improve the result by logging even more information. It just requires a bit more code changes.

lib/internal/util/inspect.js Outdated Show resolved Hide resolved
@BridgeAR BridgeAR removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 13, 2025
@ljharb ljharb force-pushed the inspect-regex-error-stack branch 2 times, most recently from 0fcb903 to d842336 Compare January 13, 2025 20:08
BridgeAR
BridgeAR approved these changes Jan 13, 2025
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I very much like the improved output!
It might be possible to unify some paths in the improveStack() method, while that's not critical in my opinion.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 13, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@aduh95
Copy link
Contributor

aduh95 commented Jan 15, 2025

This needs a rebase

@ljharb ljharb force-pushed the inspect-regex-error-stack branch from d842336 to 73c0e96 Compare January 15, 2025 00:42
@ljharb ljharb force-pushed the inspect-regex-error-stack branch from 73c0e96 to a9638dc Compare January 15, 2025 00:47
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@BridgeAR BridgeAR added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Jan 15, 2025
BridgeAR
BridgeAR approved these changes Jan 15, 2025
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Still LGTM

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 15, 2025
@nodejs-github-bot nodejs-github-bot merged commit e96072a into nodejs:main Jan 15, 2025
56 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in e96072a

@ljharb ljharb deleted the inspect-regex-error-stack branch January 15, 2025 19:38
wongprom pushed a commit to wongprom/node that referenced this pull request Jan 15, 2025
@ljharb @wongprom
util: inspect: do not crash on an Error with a regex name
684cb33 
See nodejs#56570

PR-URL: nodejs#56574
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
aduh95 pushed a commit that referenced this pull request Jan 27, 2025
@ljharb @aduh95
util: inspect: do not crash on an Error with a regex name
5f79b63 
See #56570

PR-URL: #56574
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@github-actions github-actions bot mentioned this pull request Jan 28, 2025
Merged
aduh95 pushed a commit that referenced this pull request Jan 30, 2025
@ljharb @aduh95
util: inspect: do not crash on an Error with a regex name
6a16012 
See #56570

PR-URL: #56574
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
aduh95 pushed a commit that referenced this pull request Jan 31, 2025
@ljharb @aduh95
util: inspect: do not crash on an Error with a regex name
368c698 
See #56570

PR-URL: #56574
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KhafraDev KhafraDev KhafraDev approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ljharb @nodejs-github-bot @aduh95 @jasnell @lpinca @BridgeAR @marco-ippolito @KhafraDev