Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Deep equality for sets fails in Node v20 #4444

Closed
WeepingClown13 opened this issue Oct 6, 2023 · 6 comments · Fixed by #6586
Closed

Deep equality for sets fails in Node v20 #4444

WeepingClown13 opened this issue Oct 6, 2023 · 6 comments · Fixed by #6586
Assignees
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler

Comments

@WeepingClown13
Copy link
Collaborator

WeepingClown13 commented Oct 6, 2023

I tried this:

let a = MutSet<Map<str>> {{"a" => "b", "c" => "d"}};
let b = a.copy();
assert(a == b);

let c = b.copyMut();
assert(a == c);

a.add({"e" => "f"});
c.add({"e" => "f"});
assert(a == {{"a" => "b", "c" => "d"}, {"e" => "f"}});
assert(c == {{"a" => "b", "c" => "d"}, {"e" => "f"}});
assert(a == c);

This happened:

Assertion failed with the following error (it is the final assert that is causing this):

ERROR: assertion failed: a == c

target/test/main.wsim.354620.tmp/.wing/preflight.js:17
       {((cond) => {if (!cond) throw new Error("assertion failed: a == {{\"a\" => \"b\", \"c\" => \"d\"}, {\"e\" => \"f\"}}")})((((a,b) => { try { return require('assert').deepStrictEqual(a,b) === undefined; } catch { return false; } })(a,new Set([({"a": "b","c": "d"}), ({"e": "f"})]))))};
       {((cond) => {if (!cond) throw new Error("assertion failed: c == {{\"a\" => \"b\", \"c\" => \"d\"}, {\"e\" => \"f\"}}")})((((a,b) => { try { return require('assert').deepStrictEqual(a,b) === undefined; } catch { return false; } })(c,new Set([({"a": "b","c": "d"}), ({"e": "f"})]))))};
>>     {((cond) => {if (!cond) throw new Error("assertion failed: a == c")})((((a,b) => { try { return require('assert').deepStrictEqual(a,b) === undefined; } catch { return false; } })(a,c)))};
     }
   }

I expected this:

For the assertions to pass without any errors.

Is there a workaround?

Run the tests by removing the final assertion, like so:

let a = MutSet<Map<str>> {{"a" => "b", "c" => "d"}};
let b = a.copy();
assert(a == b);

let c = b.copyMut();
assert(a == c);

a.add({"e" => "f"});
c.add({"e" => "f"});
assert(a == {{"a" => "b", "c" => "d"}, {"e" => "f"}});
assert(c == {{"a" => "b", "c" => "d"}, {"e" => "f"}});
// assert(a == c);

Which passes as expected: Tasks: 12 successful, 12 total

Component

Compiler

Wing Version

0.35.5

Node.js Version

v20.8.0

Platform(s)

Linux

Anything else?

Originally noticed this issue because of a CI failure for the PR #4432. This behavior only happens with node v20 as seen in the pipeline and upon further testing in local environment, it is confirmed that it still passes (the snippet I provided here, as well as the one that caused the ci failure) in node v18, but fails in node v20.

Community Notes

  • Please vote by adding a 👍 reaction to the issue to help us prioritize.
  • If you are interested to work on this issue, please leave a comment.
@WeepingClown13 WeepingClown13 added 🐛 bug Something isn't working 🛠️ compiler Compiler labels Oct 6, 2023
@monadabot monadabot added this to Wing Oct 6, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New - not properly defined in Wing Oct 6, 2023
@Chriscbr
Copy link
Contributor

Chriscbr commented Oct 6, 2023

it is confirmed that it still passes (the snippet I provided here, as well as the one that caused the ci failure) in node v18, but fails in node v20.

@WeepingClown13 To clarify the issue title, is it always failing on Node v20, or is it only sometimes failing (i.e. the behavior is nondeterministic)?

@WeepingClown13
Copy link
Collaborator Author

Is it always failing on Node v20, or is it only sometimes failing (i.e. the behavior is nondeterministic)?

It is always failing on Node v20, so it's consistent. The same always passes in Node v18.

@MarkMcCulloh MarkMcCulloh changed the title Deep equality for sets fails on random occassions Deep equality for sets fails in Node v20 Oct 6, 2023
@staycoolcall911
Copy link
Contributor

I can confirm this fails on node 20, but succeeds on node 18, very strange...

@staycoolcall911 staycoolcall911 moved this from 🆕 New - not properly defined to 🤝 Backlog - handoff to owners in Wing Oct 12, 2023
@staycoolcall911 staycoolcall911 added this to the KubeCon23 milestone Oct 12, 2023
@staycoolcall911 staycoolcall911 removed this from the Wing Cloud 1.0 milestone Nov 13, 2023
Copy link

Hi,

This issue hasn't seen activity in 60 days. Therefore, we are marking this issue as stale for now. It will be closed after 7 days.
Feel free to re-open this issue when there's an update or relevant information to be added.
Thanks!

@Chriscbr
Copy link
Contributor

Chriscbr commented Mar 5, 2024

I thought this could be related to running preflight code in a worker thread, but it turns out it's unrelated. This bug is basically due to how we're using deepStrictEqual here, which treats sets as equal only if they contain the same JavaScript values by reference. (nodejs/node#13347)

@tsuf239 tsuf239 self-assigned this May 16, 2024
@mergify mergify bot closed this as completed in #6586 Jun 2, 2024
mergify bot pushed a commit that referenced this issue Jun 2, 2024
Fixes #4444 
Replaced the native assert module import to an [external npm package](https://www.npmjs.com/package/assert) for the problematic file.

## Checklist

- [ ] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [ ] Description explains motivation and solution
- [ ] Tests added (always)
- [ ] Docs updated (only required for features)
- [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
@github-project-automation github-project-automation bot moved this from 🤝 Backlog - handoff to owners to ✅ Done in Wing Jun 2, 2024
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.74.32.

eladb pushed a commit that referenced this issue Jun 6, 2024
Fixes #4444 
Replaced the native assert module import to an [external npm package](https://www.npmjs.com/package/assert) for the problematic file.

## Checklist

- [ ] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [ ] Description explains motivation and solution
- [ ] Tests added (always)
- [ ] Docs updated (only required for features)
- [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants