equal and deepEqual use different equality semantics for primitive types

[mourner]

Consider the following assertion:

t.equal(0, -0);

It passes because JavaScript defines strict equality between signed zeroes (0 === -0). However, the following scenario unexpectedly fails:

t.deepEqual(0, -0);

not ok 1 should be equivalent
  ---
    operator: deepEqual
    expected: |-
      -0
    actual: |-
      0

Discovered this while switching from tap to tape — the latter had a failing case where tap passed because of this inconsistency. cc @ljharb

[ljharb]

tap and tape aren’t precisely equivalent, so some differences are expected.

t.same, in tape, is an alias for deepEqual, not equal.

Separately, tape v4 uses deep-equal v1; tape@5 uses deep-equal v2 which more closely matches node’s assert. If you install tape@next, what happens?

[mourner]

t.same, in tape, is an alias for deepEqual, not equal.

Yes. However, it is expected that deepEqual/same compares leaf values with the same semantics as equal. Node's assert behaves correctly in this regard:

const assert = require('assert');
assert.deepEqual([0], [-0]); // passes

Upgrading to tape@next doesn't resolve the issue.

It looks like the problem stems from using strict: true for deepEqual across the code (e.g. here). Is there any reason for using that option? It was added in this ancient commit, but there's no explanation for it.

In deep-equal, the only difference when passing strict: true is using Object.is instead of ===, which means 0 and -0 are different, and so are NaN and Number.NaN.

I think this is a bug — deepEqual should semantically be consistent with equal for basic values.

[ljharb]

deepEqual should match assert; tape should as well.

“same” is just an alias, a convenience; it has no direct expectations.

I very recently updated deep-equal and tape, and that involved changes to -0 handling, so I’m quite convinced there’s not a bug here, but if you’d like to submit a PR with failing test cases, I’d be happy to look into it.

[mourner]

@ljharb I think there's a misunderstanding there — I know same is an alias for deepEqual, and this bug concerns both.

deepEqual should match assert; tape should as well.

That's exactly what I'm saying — it should, but currently tape does not match assert because of {strict: true}, which seems to be a mistake.

An additional argument (beyond incompatibility with Node's assert) is that it doesn't make sense for the following two lines to have different results:

t.equal(0, -0); // ok
t.deepEqual(0, -0); // fail

[ljharb]

They often have different results; try 3 and '3', for example.

There’s a bug if assert.equal doesn’t match t.equal, and assert.deepEqual doesn’t match t.deepEqual. The strict option should match assert.deepStrictEqual.

[mourner]

Ah, yeah, you're right, although the fact equal and deepEqual use different equality semantics (=== vs Object.is) feels very counter-intuitive, inconsistent, and also doesn't match the docs. In summary:

• Tape's equal and strictEqual use === , but deepEqual uses Object.is, not ===.
• Node's strictEqual and deepStrictEqual both use Object.is.

Edit: updated the issue title/description to describe the issue more precisely.

[ljharb]

So, if tape and deep-equal are both consistent with node's assert - then the place to file an issue requesting a change is in node itself. If they're not consistent, I will absolutely fix them to be so.

[mourner]

@ljharb sorry if I'm not expressing myself clearly enough — see my comment above, which shows how tape itself is inconsistent, while Node assert and deep-equal are fine. The way to fix this would be to change === to Object.is for strict equality checks in tape.

[ljharb]

Here's what I see on latest master (tape@next)

matches	method	0, -0	NaN, NaN	3, '3'
	assert.equal	true	false	true
❌	t.equal	✅ true	✅ false	❌ false
--	--	--	--	--
	assert.strictEqual	false	true	false
❌	t.strictEqual	❌ true	❌ false	✅ false
--	--	--	--	--
	assert.deepEqual	true	false	true
✅	t.deepLooseEqual	true	false	true
--	--	--	--	--
	assert.deepStrictEqual	false	true	false
✅	t.deepEqual	false	true	false

Which shows that there is indeed a mismatch between equal/strictEqual/deepStrictEqual on t vs assert. This is something I can definitely fix in v5, which is still in prerelease.

[mourner]

@ljharb exactly! Thank you very much!

[mourner]

@ljharb thanks a lot for addressing this! Thinking about this further, if equal semantics are changing to be non-strict, is there any reason not to fully match assert behavior in deepEqual too? I mean, now the behavior is:

tape	assert	type
equal	equal	loose
looseDeepEqual	deepEqual	loose
strictEqual	strictEqual	strict
deepEqual	strictDeepEqual	strict

As a part of semver-major change which you're doing anyway, would it makes sense to do:

tape	assert	type
equal	equal	loose
deepEqual / looseDeepEqual	deepEqual	loose
strictEqual	strictEqual	strict
strictDeepEqual	strictDeepEqual	strict

[ljharb]

I think that it's generally an improvement for tape to default to strictDeepEqual, and make "loose" opt-in. If anything, it'd be reasonable to swap .equal and .strictEqual, but that adds a lot of churn for arguable value.

[ljharb]

Added 24240e2 as well, to ensure all the inverse functions were consistent.

[mourner]

@ljharb it still seems weird to me that equal defaults to "loose" and deepEqual to "strict" — is there a reason not to either 1) go all the way with consistency with assert (equal and deepEqual are loose), or 2) go all the way with defaulting to strict comparisons (equal and deepEqual use is)?

[ljharb]

I don't know the history; I think this is just the way it's been for awhile - but basically this is a question of tradeoffs: is the consistency worth the breakage and churn? I'm not convinced it is.

[mourner]

@ljharb if we're worried about breakage and churn, why should we introduce the big change of equals === to ==? If we're doing it now before v5, we could do the deepEquals change as well.

On the other hand, if we want to keep things mostly as they were before but improve consistency, why not revert the equals loosening and instead switch it to is, while keeping deepEquals as it was? This will be both consistent and almost unnoticeable to users.

[ljharb]

That won't break anything - currently passing tests will continue to pass.

That's also a reasonable alternative, true - making equal and deepEqual both strict, and looseEqual and looseDeepEqual non-strict.

[ljharb]

The more I think about that suggestion, the more I like it:

• for both deep and non-deep, strict will be the default
• if in v4 you used looseEqual, it will become loose in v5 (no new failures, but potentially things will pass that failed before)
• if in v4 you used equal/is/strictEqual, it will still be strict in v5
• tape's API will match assert's, except that "strict is the default", which will now be consistently applied in v5

[mourner]

@ljharb 🎉 excellent! I like this way much more too. Thanks a lot for your patience bearing through all my comments, and generally really great work maintaining this awesome project.