FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

assert,util: fix constructor lookup in deep equal comparison by BridgeAR · Pull Request #57876 · nodejs/node · GitHub

/ node Public

assert,util: fix constructor lookup in deep equal comparison - #57876

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
BridgeAR:fix-deep-strict-equal-constructor-lookup
Apr 19, 2025
ConversationCommits (3)ChecksFiles changed
Merged

assert,util: fix constructor lookup in deep equal comparison#57876
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
BridgeAR:fix-deep-strict-equal-constructor-lookup

Conversation

BridgeAR commented Apr 14, 2025

Copy link
Copy Markdown
Member

The latest performance deep equal optimization did not take into account that
an object may have a property called constructor. This is addressed
in this PR by adding a new fast path and using fallbacks.

On top of that, this adds a few performance optimizations for the constructor
name in util.inspect().

BridgeAR added assert Issues and PRs related to the assert subsystem. util Issues and PRs related to the built-in util module. labels Apr 14, 2025
nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 14, 2025
ljharb approved these changes Apr 14, 2025
View reviewed changes

ljharb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

i didn't thoroughly check that the list of constructors and prototypes is exhaustive, but LGTM

BridgeAR force-pushed the fix-deep-strict-equal-constructor-lookup branch from 8a6d0f1 to 3eda481 Compare April 14, 2025 18:21
jasnell reviewed Apr 14, 2025
View reviewed changes
Comment thread lib/internal/util/comparisons.js Outdated
jasnell reviewed Apr 14, 2025
View reviewed changes
Comment thread lib/internal/util/inspect.js Outdated

codecov Bot commented Apr 14, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (f89baf2) to head (8c38421).
Report is 29 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57876      +/-   ##
==========================================
- Coverage   90.24%   90.24%   -0.01%     
==========================================
  Files         630      630              
  Lines      185670   185784     +114     
  Branches    36401    36406       +5     
==========================================
+ Hits       167567   167664      +97     
- Misses      10992    10999       +7     
- Partials     7111     7121      +10
Files with missing lines Coverage Δ
lib/internal/util/comparisons.js 100.00% <100.00%> (ø)
lib/internal/util/inspect.js 99.88% <100.00%> (-0.04%) ⬇️

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

BridgeAR force-pushed the fix-deep-strict-equal-constructor-lookup branch from 1e6f108 to 36ebee6 Compare April 14, 2025 23:38
BridgeAR marked this pull request as draft April 15, 2025 11:06
BridgeAR added 2 commits April 15, 2025 14:29
assert,util: fix constructor lookup in deep equal comparison
e785a12 
The latest performance optimization did not take into account that
an object may have a property called constructor. This is addressed
in this PR by adding a new fast path and using fallbacks.
util: improve inspect constructor name lookup
016499f 
This adds known constructors to act as fast path when util.inspect()
checks those.
BridgeAR force-pushed the fix-deep-strict-equal-constructor-lookup branch from 36ebee6 to 016499f Compare April 15, 2025 12:29
BridgeAR marked this pull request as ready for review April 15, 2025 12:32

BridgeAR commented Apr 15, 2025

Copy link
Copy Markdown
Member Author

I accidentally already included Float16Array and that ended up with undefined as constructor and wrong behavior. I fixed that now and added an additional test case to also detect different null prototypes properly.

BridgeAR added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 15, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2025

nodejs-github-bot commented Apr 15, 2025

Copy link
Copy Markdown
Collaborator

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

aduh95 reviewed Apr 15, 2025
View reviewed changes
Comment thread lib/internal/util/comparisons.js Outdated
aduh95 reviewed Apr 15, 2025
View reviewed changes
Comment thread lib/internal/util/comparisons.js
ljharb mentioned this pull request Apr 16, 2025
Draft
aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2025

nodejs-github-bot commented Apr 16, 2025

Copy link
Copy Markdown
Collaborator

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

BridgeAR added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 19, 2025
BridgeAR added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. and removed commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Apr 19, 2025
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 19, 2025
nodejs-github-bot merged commit 733e0fc into nodejs:main Apr 19, 2025

nodejs-github-bot commented Apr 19, 2025

Copy link
Copy Markdown
Collaborator

Landed in 733e0fc

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
assert,util: fix constructor lookup in deep equal comparison
7cf6fbe 
The latest performance optimization did not take into account that
an object may have a property called constructor. This is addressed
in this PR by adding a new fast path and using fallbacks.

PR-URL: #57876
Reviewed-By: Jordan Harband <ljharb@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
assert,util: fix constructor lookup in deep equal comparison
d9e78c0 
The latest performance optimization did not take into account that
an object may have a property called constructor. This is addressed
in this PR by adding a new fast path and using fallbacks.

PR-URL: #57876
Reviewed-By: Jordan Harband <ljharb@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
assert,util: fix constructor lookup in deep equal comparison
c4f7c1e 
The latest performance optimization did not take into account that
an object may have a property called constructor. This is addressed
in this PR by adding a new fast path and using fallbacks.

PR-URL: #57876
Reviewed-By: Jordan Harband <ljharb@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
assert,util: fix constructor lookup in deep equal comparison
92dc55b 
The latest performance optimization did not take into account that
an object may have a property called constructor. This is addressed
in this PR by adding a new fast path and using fallbacks.

PR-URL: #57876
Reviewed-By: Jordan Harband <ljharb@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
assert,util: fix constructor lookup in deep equal comparison
1725692 
The latest performance optimization did not take into account that
an object may have a property called constructor. This is addressed
in this PR by adding a new fast path and using fallbacks.

PR-URL: #57876
Reviewed-By: Jordan Harband <ljharb@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
assert,util: fix constructor lookup in deep equal comparison
f95a394 
The latest performance optimization did not take into account that
an object may have a property called constructor. This is addressed
in this PR by adding a new fast path and using fallbacks.

PR-URL: #57876
Reviewed-By: Jordan Harband <ljharb@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 pushed a commit that referenced this pull request May 18, 2025
assert,util: fix constructor lookup in deep equal comparison
23e24f3 
The latest performance optimization did not take into account that
an object may have a property called constructor. This is addressed
in this PR by adding a new fast path and using fallbacks.

PR-URL: #57876
Reviewed-By: Jordan Harband <ljharb@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
github-actions Bot mentioned this pull request May 19, 2025
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
assert,util: fix constructor lookup in deep equal comparison
1c7396b 
The latest performance optimization did not take into account that
an object may have a property called constructor. This is addressed
in this PR by adding a new fast path and using fallbacks.

PR-URL: #57876
Reviewed-By: Jordan Harband <ljharb@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ghost mentioned this pull request Jun 8, 2025
Closed
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ljharb ljharb approved these changes
jasnell jasnell approved these changes
aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. 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.

5 participants

Back | FazBrowse Home | New Git URL