Skip to content

@std/assert/equals considers Object.create(null) and {} to be equal #6334

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

Open
lilnasy opened this issue Jan 6, 2025 · 4 comments · May be fixed by #6484
Open

@std/assert/equals considers Object.create(null) and {} to be equal #6334

lilnasy opened this issue Jan 6, 2025 · 4 comments · May be fixed by #6484
Labels
bug Something isn't working

Comments

@lilnasy
Copy link

lilnasy commented Jan 6, 2025

Describe the bug

  • Since fix(assert): check property equality up the prototype chain (#6151) #6153, assertEquals checks object prototypes. However, it makes certain exceptions to keep existing tests within the repo from failing.
  • Other assertion libraries have more correct checks: "node:assert/strict", "ava", and "uvu" all make a distinction between null-prototype object and plain objects. "node:assert" and "chai" don't.

Steps to Reproduce
The following assertion passes:

import { assertEquals } from "@std/assert"
assertEquals(Object.create(Object.prototype), Object.create(null))

Expected behavior
The assertion fails.

Environment
Not applicable.

@lilnasy lilnasy added bug Something isn't working needs triage labels Jan 6, 2025
@kt3k kt3k added feedback welcome We want community's feedback on this issue or PR and removed bug Something isn't working feedback welcome We want community's feedback on this issue or PR labels Jan 6, 2025
@kt3k
Copy link
Member

kt3k commented Jan 6, 2025

I don't get what this issue is about. The given example seems already failing.

@lilnasy
Copy link
Author

lilnasy commented Jan 6, 2025

You're right, my steps to reproduce were incorrect.

- assertEquals(Object.create(Object), Object.create(null))
+ assertEquals(Object.create(Object.prototype), Object.create(null))

@kt3k kt3k added bug Something isn't working and removed needs triage labels Jan 6, 2025
@WWRS
Copy link
Contributor

WWRS commented Mar 17, 2025

If both objects have no properties, it makes sense to distinguish between a prototype of Object.prototype and no prototype. But if the objects do have properties, should we consider those equal? Alternatively, we could have one function that only checks properties and one that also checks prototypes?

A note on the tests: Before #6153, equal checked constructors, not prototypes. That's why an undefined constructor used to equal Object's constructor. As far as I know, there are exactly two tests that could be affected by this change:

  • assert/equals_test.ts, assertEquals() compares objects structurally if one object's constructor is undefined and the other is Object: Compares objects that have properties and are the same aside from prototypes
  • msgpack/encode_test.ts, encode() handles maps: Uses assertEquals(decode(encode(mapNull)), mapNull) when actually decode produces {} here.

@lionel-rowe
Copy link
Contributor

lionel-rowe commented Mar 18, 2025

A note on the tests: Before #6153, equal checked constructors, not prototypes. That's why an undefined constructor used to equal Object's constructor.

Not exactly. Before the change, falsy constructors were already special-cased to count as equal to Object constructors. After the change, nullish prototypes are special-cased in the same way, which I just did for consistency with the assumed intention of the old behavior. There's no practical difference between falsy and nullish here, as the only falsy value possible for a prototype is null. constructor, meanwhile, is just a normal property name, so it could be overridden and return anything.

I'm not sure why falsy constructors were special-cased in the first place, but I assume it was intended as an ergonomic thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants