util: fix formatting of objects with built-in Symbol.toPrimitive #57832
+85
−14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
needs-ci
legendecas
added
request-ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57832 +/- ##
==========================================
- Coverage 90.24% 90.23% -0.02%
==========================================
Files 630 630
Lines 185197 185481 +284
Branches 36309 36367 +58
==========================================
+ Hits 167136 167372 +236
- Misses 10984 10996 +12
- Partials 7077 7113 +36
🚀 New features to boost your workflow:
|
BridgeAR
reviewed
Apr 11, 2025
There was a problem hiding this comment.
The rough implementation seems good!
What I would like to change to land this, is to improve the details (performance and potentially correctness edge cases).
Can you please add a few tests that also check for behavior on prototypes? We need a few test cases there. An object that inherits from a built-in that has either or method as property, both, and the same for an inheritance where it's inheriting from a user defined class.
BridgeAR
approved these changes
Apr 14, 2025
github-actions
bot
removed
the
request-ci
BridgeAR
added
commit-queue-squash
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/57832 ✔ Done loading data for nodejs/node/pull/57832 ----------------------------------- PR info ------------------------------------ Title util: fix formatting of objects with built-in Symbol.toPrimitive (#57832) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch islandryu:fix/format -> nodejs:main Labels util, author ready, needs-ci, commit-queue-squash Commits 6 - util: fix formatting of objects with built-in Symbol.toPrimitive - add symbol test - handle the case toPrimitive is defined in prototype - add test - add comment - fix to not call unnecessary hasOwnProperty Committers 1 - islandryu <[email protected]> PR-URL: https://github.com/nodejs/node/pull/57832 Fixes: https://github.com/nodejs/node/issues/57818 Reviewed-By: Ruben Bridgewater <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/57832 Fixes: https://github.com/nodejs/node/issues/57818 Reviewed-By: Ruben Bridgewater <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 11 Apr 2025 12:37:28 GMT ✔ Approvals: 1 ✔ - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/57832#pullrequestreview-2763991051 ✘ This PR needs to wait 70 more hours to land (or 0 hours if there is one more approval) ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-04-15T00:35:13Z: https://ci.nodejs.org/job/node-test-pull-request/66272/ - Querying data for job/node-test-pull-request/66272/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/14471235480 |
ljharb
reviewed
Apr 15, 2025
Comment on lines
+2198
to
+2200
| function returnFalse() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
is this faster than nulling out hasOwnToPrimitive and checking for that, avoiding the function call?
There was a problem hiding this comment.
That likely depends on a) the inlining and b) if it is more common to have to check for both methods (if that's the case, any additional check is more expensive).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #57818
Imo, built-in [Symbol.toPrimitive] can make use of inspect, just like built-in toString, because for built-in objects like Symbol and Date, their [Symbol.toPrimitive] and toString() return the same result.