assert.deepStrictEqual() and the `vm` module are incompatible

[Qix-]

Version

v18.8.0

Platform

Linux DESKTOP-TBGODF1 4.4.0-19041-Microsoft #1237-Microsoft Sat Sep 11 14:32:00 PST 2021 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

import vm from 'node:vm';
import assert from 'node:assert/strict';
const g = {exports: {}};
const result = vm.runInNewContext(`exports.result = {foo:'bar'};`, g);
assert.deepStrictEqual(g.exports.result, {foo: 'bar'});

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

These two objects are structurally identical and have the Object prototype. The assert library should accept them as deep-equal.

What do you see instead?

Since the VM script's context creates its own Object for use in prototypes, therefore the prototype check introduced to the assert library in 02b66b5 (#24974, ref #24917) fails since they're not equal (even though they represent the same underlying 'facility' in Javascript).

This caused some confusing output:

AssertionError [ERR_ASSERTION]: Values have same structure but are not reference-equal:

{
  foo: 'bar'
}

Further, of course they're not reference equal - that would defeat the entire purpose of a deepEqual check, would it not? 🙃 I had to google to figure out it was referring to prototype checking. Perhaps the error message could be improved here?

Additional information

It'd be great if we could get a variant of the node:assert library that is 1) strict and 2) ignores prototypes for cases like this. I don't see how this could be fixed otherwise, given how v8's contexts work.

[Qix-]

cc @BridgeAR (hi!) since you committed the aforementioned changes.

[joyeecheung]

At this point it's probably more sensible to try to enable creation of new built-ins in a new vm context which is also what the ShadowRealms integration needs to unblock - not all of the built-ins are easy to duplicate, but assert should be one of the low-hanging fruits.

[BridgeAR]

I agree that the prototype check should be optional. It would be great to change assert to a class with options. Passing through options to the current assert calls is probably not possible.

Improving the error message would also be great. We would have to track a lot of things during comparison though and that might slow the algorithm down or we do a second round in case of errors that are unclear that indicate the exact problem.

[bnoordhuis]

I disagree that the prototype check should be optional. It's why assert.deepEqual() exists, right? Don't ask for strictness if you don't want strictness.

These two objects are structurally identical and have the Object prototype. The assert library should accept them as deep-equal

Bad expectation, IMO. JS being the language that it is, it's trivial to create similar-looking but distinct objects. The vm module is just a special case of that general principle.

[BridgeAR]

I believe it depends on the use case. The only reason I found that people wanted to use deepEqual() instead of strictDeepEqual() was about the prototype check. The strict equality does a lot more than just also checking for the prototype. The biggest difference being the abstract equality being used.
Thus, having an option to deactivate the prototype check seems reasonable to me.

[BridgeAR]

With the new assert class we are adding an option to deactivate the prototype check.

#59762