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: harden util.inspect #21869

Closed
wants to merge 3 commits into from
Closed

Conversation

BridgeAR
Copy link
Member

This makes sure values without prototype will still be inspected
properly and do not cause errors. It restores the original
information if possible.

Besides that it fixes an issue with boxed symbols: extra keys were
not visualized so far.

The main focus here is correctness and performance. I am not always happy with the code but it is not always easy to "fix" these things. I could not find a fix for e.g. regular expressions and if someone has an idea, please let me know!

Fixes #19511

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This makes sure values without prototype will still be inspected
properly and do not cause errors. It restores the original
information if possible.

Besides that it fixes an issue with boxed symbols: extra keys were
not visualized so far.
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jul 18, 2018
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

might wanna use uncurryThis instead of .call

@BridgeAR
Copy link
Member Author

PTAL. It would be nice to get another LG.

@devsnek are you also fine with landing it as is?

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

lib/util.js Outdated
if (ctx.showHidden) {
formatter = formatWeakSet;
} else {
extra = '<items unknown>';
}
} else if (isWeakMap(value)) {
braces[0] = `${getPrefix(constructor, tag)}{`;
braces[0] = `${getPrefix(constructor, tag) || 'WeakMap '}{`;
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to make the || 'thing ' be a param of getPrefix to add the trailing space then?

@BridgeAR
Copy link
Member Author

I addressed both comments.

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

@BridgeAR
Copy link
Member Author

Landed in 10c850b 🎉

@BridgeAR BridgeAR closed this Jul 27, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jul 27, 2018
This makes sure values without prototype will still be inspected
properly and do not cause errors. It restores the original
information if possible.

Besides that it fixes an issue with boxed symbols: extra keys were
not visualized so far.

PR-URL: nodejs#21869
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
@targos
Copy link
Member

targos commented Jul 31, 2018

Should this be backported to v10.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Note there is another PR on util that wasn't backported yet: #20961

@targos
Copy link
Member

targos commented Aug 19, 2018

#20961 landed and I was able to cherry-pick this with just a trivial conflict to fix.

targos pushed a commit that referenced this pull request Aug 19, 2018
This makes sure values without prototype will still be inspected
properly and do not cause errors. It restores the original
information if possible.

Besides that it fixes an issue with boxed symbols: extra keys were
not visualized so far.

PR-URL: #21869
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
@BridgeAR
Copy link
Member Author

@targos seems like this did not yet land in any release. I would like to pull this out until #22437 landed. In that case both could land together. Otherwise the latter would be semver-major as I remove support for the Symbol.manipulation again. It is just difficult to deal with that properly all at once.

@targos
Copy link
Member

targos commented Aug 24, 2018

@BridgeAR I just landed #22437 on v10.x-staging. Is that OK?

@BridgeAR
Copy link
Member Author

@targos yes, awesome. Thanks a lot.

targos pushed a commit that referenced this pull request Sep 3, 2018
This makes sure values without prototype will still be inspected
properly and do not cause errors. It restores the original
information if possible.

Besides that it fixes an issue with boxed symbols: extra keys were
not visualized so far.

PR-URL: #21869
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnecessary TypeError from util.inspect(Object.setPrototypeOf(function() {}, null))
6 participants