assert,util: correct comparison when both contain same reference #53431

lemire · 2024-06-12T15:04:11Z

I would expect that given

const xarray = ['x'];

Then new Set([xarray, ['y']]) and new Set([xarray, ['y']]) are equal as per assert.deepStrictEqual but since Node 20, they are not.

This PR applies the fix proposed by @BridgeAR

Refs: #53423 reported by @chharvey

nodejs-github-bot · 2024-06-12T15:10:51Z

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

lemire · 2024-06-12T20:28:40Z

@BridgeAR @mcollina @anonrig

Many changes were introduced in #46593 comparison.js but it looks to me like the new code has bugs, as reported by @chharvey.

test/parallel/test-assert-deep.js

aduh95 · 2024-06-13T06:45:43Z

Can you fix the commit message? The subsystem should be test:, and it should be descriptive of what the change is – and the test should be added to known_issues given the test doesn't pass.

mcollina · 2024-06-13T08:09:36Z

One year has passed since #46593. Should we revert? @BridgeAR @lemire would you be able to work a fix?

Co-authored-by: Chris Harvey <1362083+chharvey@users.noreply.github.com>

lemire · 2024-06-13T15:46:27Z

@aduh95 Message reworded.

lemire · 2024-06-13T15:46:55Z

@mcollina In the issue, @BridgeAR proposed a fix which I have applied here.

lemire · 2024-06-13T16:34:43Z

lib/internal/util/comparisons.js

@@ -502,7 +502,7 @@ function setEquiv(a, b, strict, memo) {
    for (const val of b) {
      // Primitive values have already been handled above.
      if (typeof val === 'object' && val !== null) {
-        if (!setHasEqualElement(set, val, strict, memo))
+        if (!a.has(val) && !setHasEqualElement(set, val, strict, memo))
          return false;
      } else if (!strict &&


Which if are you matching with which else?

The else will be reached as it's the inner if. It could be optimized code wise (there is some duplication that can be prevented), but that could also be done in a later PR to check what implementation is best.

@lemire Oops, my bad, I was matching the else on line 507 with the wrong if, the one on line 505. Indentation is hard 😜

nodejs-github-bot · 2024-06-13T15:50:05Z

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

mcollina

lgtm

nodejs-github-bot · 2024-06-13T17:24:23Z

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

nodejs-github-bot · 2024-06-13T18:27:16Z

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

nodejs-github-bot · 2024-06-13T21:30:34Z

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

lib/internal/util/comparisons.js

Co-authored-by: James M Snell <jasnell@gmail.com>

nodejs-github-bot · 2024-06-14T19:51:00Z

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

nodejs-github-bot · 2024-06-17T07:20:25Z

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

nodejs-github-bot · 2024-06-17T08:10:36Z

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

nodejs-github-bot · 2024-06-17T13:08:39Z

Commit Queue failed

- Loading data for nodejs/node/pull/53431 ✔ Done loading data for nodejs/node/pull/53431 ----------------------------------- PR info ------------------------------------ Title assert,util: correct comparison when both contain same reference (#53431) Author Daniel Lemire (@lemire) Branch lemire:verify_53423 -> nodejs:main Labels test, needs-ci Commits 2 - assert,util: correct comparison when both contain same reference - Update lib/internal/util/comparisons.js Committers 2 - Daniel Lemire - GitHub PR-URL: https://github.com/nodejs/node/pull/53431 Refs: https://github.com/nodejs/node/issues/53423 Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater Reviewed-By: Moshe Atlow Reviewed-By: Yagiz Nizipli Reviewed-By: Luigi Pinca ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/53431 Refs: https://github.com/nodejs/node/issues/53423 Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater Reviewed-By: Moshe Atlow Reviewed-By: Yagiz Nizipli Reviewed-By: Luigi Pinca -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 12 Jun 2024 15:04:11 GMT ✔ Approvals: 6 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/53431#pullrequestreview-2116256581 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/53431#pullrequestreview-2116361323 ✔ - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/53431#pullrequestreview-2116501755 ✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/53431#pullrequestreview-2117085211 ✔ - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/53431#pullrequestreview-2117118245 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/53431#pullrequestreview-2119125467 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-06-17T08:10:36Z: https://ci.nodejs.org/job/node-test-pull-request/59827/ - Querying data for job/node-test-pull-request/59827/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 53431 From https://github.com/nodejs/node * branch refs/pull/53431/merge -> FETCH_HEAD ✔ Fetched commits as 2333573f3907..09449e7e728b -------------------------------------------------------------------------------- [main bf3d689d2c] assert,util: correct comparison when both contain same reference Author: Daniel Lemire Date: Wed Jun 12 11:00:46 2024 -0400 2 files changed, 6 insertions(+), 2 deletions(-) [main 2edda489f0] Update lib/internal/util/comparisons.js Author: Daniel Lemire Date: Fri Jun 14 09:57:10 2024 -0400 1 file changed, 2 insertions(+), 1 deletion(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
assert,util: correct comparison when both contain same reference

Co-authored-by: Chris Harvey 1362083+chharvey@users.noreply.github.com
PR-URL: #53431
Refs: #53423
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io
Reviewed-By: Luigi Pinca luigipinca@gmail.com

[detached HEAD c7b2ebd1fe] assert,util: correct comparison when both contain same reference
Author: Daniel Lemire daniel@lemire.me
Date: Wed Jun 12 11:00:46 2024 -0400
2 files changed, 6 insertions(+), 2 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update lib/internal/util/comparisons.js

Co-authored-by: James M Snell jasnell@gmail.com
PR-URL: #53431
Refs: #53423
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io
Reviewed-By: Luigi Pinca luigipinca@gmail.com

[detached HEAD d0c65e9521] Update lib/internal/util/comparisons.js
Author: Daniel Lemire daniel@lemire.me
Date: Fri Jun 14 09:57:10 2024 -0400
1 file changed, 2 insertions(+), 1 deletion(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/9548251230

nodejs-github-bot · 2024-06-17T13:30:25Z

Landed in ee5c6b6

Co-authored-by: Chris Harvey <1362083+chharvey@users.noreply.github.com> PR-URL: #53431 Refs: #53423 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Co-authored-by: Chris Harvey <1362083+chharvey@users.noreply.github.com> PR-URL: nodejs#53431 Refs: nodejs#53423 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

chharvey · 2024-06-30T23:16:24Z

@targos this seems like a critical bug fix… shouldn’t it go in an LTS version, v20?

Co-authored-by: Chris Harvey <1362083+chharvey@users.noreply.github.com> PR-URL: #53431 Refs: #53423 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

chharvey · 2024-08-04T00:09:04Z

responding to my own comment above … this is in LTS v20 as per 20.16 changelog

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jun 12, 2024

lemire added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 12, 2024

lemire mentioned this pull request Jun 12, 2024

deepStrictEqual fails for Sets that contain both object references and equivalent object values #53423

Closed

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 12, 2024

lemire requested a review from anonrig June 12, 2024 20:29

chharvey reviewed Jun 13, 2024

View reviewed changes

test/parallel/test-assert-deep.js Outdated Show resolved Hide resolved

lemire force-pushed the verify_53423 branch from 76677c0 to 40a807e Compare June 13, 2024 15:40

assert,util: correct comparison when both contain same reference

428b704

Co-authored-by: Chris Harvey <1362083+chharvey@users.noreply.github.com>

lemire force-pushed the verify_53423 branch from 40a807e to 428b704 Compare June 13, 2024 15:43

lemire changed the title ~~assert: verify issue 53423~~ assert,util: correct comparison when both contain same reference Jun 13, 2024

lemire added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2024

chharvey reviewed Jun 13, 2024

View reviewed changes

mcollina approved these changes Jun 13, 2024

View reviewed changes

jasnell approved these changes Jun 13, 2024

View reviewed changes

lemire linked an issue Jun 13, 2024 that may be closed by this pull request

deepStrictEqual fails for Sets that contain both object references and equivalent object values #53423

Closed

BridgeAR approved these changes Jun 13, 2024

View reviewed changes

MoLow approved these changes Jun 13, 2024

View reviewed changes

anonrig approved these changes Jun 13, 2024

View reviewed changes

jasnell reviewed Jun 14, 2024

View reviewed changes

lib/internal/util/comparisons.js Outdated Show resolved Hide resolved

Update lib/internal/util/comparisons.js

09449e7

Co-authored-by: James M Snell <jasnell@gmail.com>

lpinca approved these changes Jun 14, 2024

View reviewed changes

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2024

lemire added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 17, 2024

nodejs-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 Jun 17, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 17, 2024

nodejs-github-bot merged commit ee5c6b6 into nodejs:main Jun 17, 2024
66 checks passed

targos mentioned this pull request Jun 25, 2024

v22.4.0 release proposal #53583

Merged

Karliz24 mentioned this pull request Jul 9, 2024

## 2024-07-02, Version 22.4.0 (Current), @targos Karliz24/Marley-#16

Open

marco-ippolito mentioned this pull request Jul 19, 2024

v20.16.0 proposal #53945

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert,util: correct comparison when both contain same reference #53431

assert,util: correct comparison when both contain same reference #53431

lemire commented Jun 12, 2024 •

edited

Loading

nodejs-github-bot commented Jun 12, 2024

lemire commented Jun 12, 2024 •

edited

Loading

aduh95 commented Jun 13, 2024

mcollina commented Jun 13, 2024

lemire commented Jun 13, 2024

lemire commented Jun 13, 2024

This comment was marked as resolved.

lemire Jun 13, 2024

BridgeAR Jun 13, 2024

chharvey Jun 13, 2024 •

edited

Loading

nodejs-github-bot commented Jun 13, 2024

mcollina left a comment

nodejs-github-bot commented Jun 13, 2024

nodejs-github-bot commented Jun 13, 2024

nodejs-github-bot commented Jun 13, 2024

nodejs-github-bot commented Jun 14, 2024

nodejs-github-bot commented Jun 17, 2024

nodejs-github-bot commented Jun 17, 2024

nodejs-github-bot commented Jun 17, 2024

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Jun 17, 2024

chharvey commented Jun 30, 2024

chharvey commented Aug 4, 2024

assert,util: correct comparison when both contain same reference #53431

assert,util: correct comparison when both contain same reference #53431

Conversation

lemire commented Jun 12, 2024 • edited Loading

nodejs-github-bot commented Jun 12, 2024

lemire commented Jun 12, 2024 • edited Loading

aduh95 commented Jun 13, 2024

mcollina commented Jun 13, 2024

lemire commented Jun 13, 2024

lemire commented Jun 13, 2024

This comment was marked as resolved.

lemire Jun 13, 2024

Choose a reason for hiding this comment

BridgeAR Jun 13, 2024

Choose a reason for hiding this comment

chharvey Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 13, 2024

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 13, 2024

nodejs-github-bot commented Jun 13, 2024

nodejs-github-bot commented Jun 13, 2024

nodejs-github-bot commented Jun 14, 2024

nodejs-github-bot commented Jun 17, 2024

nodejs-github-bot commented Jun 17, 2024

nodejs-github-bot commented Jun 17, 2024

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Jun 17, 2024

chharvey commented Jun 30, 2024

chharvey commented Aug 4, 2024

lemire commented Jun 12, 2024 •

edited

Loading

lemire commented Jun 12, 2024 •

edited

Loading

chharvey Jun 13, 2024 •

edited

Loading