nodejs/node

Negative zero broken on ≥ v10.4.0

Closed this issue · 11 comments

TimothyGu commented
  • Version: ≥ v10.4.0 < v12.0.0
  • Platform: Linux 4.9.0-8-amd64 SMP Debian 4.9.130-2 (2018-10-27) x86_64 GNU/Linux
  • Subsystem: v8

V8 versions between 6.7 and 7.0 (inclusive) have a bug where

[...[]];
console.log(Object.is(-0, 0));

prints true rather than false. This is reproducible with all Node.js versions after 10.4.0, though it is fixed on master which uses V8 7.1. We should find the V8 commit that fixes this and backport it to LTS at the very least.

/cc @devsnek, who helped triage this bug
/cc @nodejs/v8

devsnek commented

to be clear, the issue is that -0 evaluates to 0, it's not a bug with Object.is or console.log.

bnoordhuis commented

If it helps, this bug seems to persist after the function is optimized by TurboFan and therefore it's likely not an Ignition bug.

ryzokuken commented

@TimothyGu this seems to be fixed on v11.5.0 as well:

> $ node -v
v11.5.0

> $ node -e "console.log(Object.is(-0, 0))"
false
TimothyGu commented

@ryzokuken You have to use the spread operator prior to the evaluation of -0 to make the bug surface.

ryzokuken commented

@TimothyGu I'd already tried that:

I see. Will try to investigate.

> $ node 
> [...[]]; console.log(Object.is(-0, 0));
true
undefined
targos commented

This was fixed as a side-effect in v8/v8@1c48d52.

That's a big change that we probably won't be able to backport.

  • 😕2
BridgeAR commented

@targos there do not seem to be a lot of conflicts in that commit when backprorting it to v11.

@nodejs/v8 @psmarshall @bmeurer @hashseed would you be so kind and check what is required to backport the necessary commit?

ryzokuken commented

@BridgeAR I think what @targos means is that since this isn't an isolated fix but a huge change, it won't be great idea to backport the whole commit. Had it been a smaller commit, I guess it would have been simpler.

mikermcneil commented

Edit: whoops nevermind, see the comment from @BridgeAR below

————

I think it would be good to add this to the node 10 release notes, since it's not currently mentioned there or in v8 6.6's release notes.

@BridgeAR @ryzokuken @TimothyGu @bnoordhuis @devsnek @nodejs/v8 @psmarshall @bmeurer @hashseed If there's agreement, what's the best way to make the change and deploy it to the website?

You can see this reproduced in failing tests here:
image
https://travis-ci.org/node-machine/rttc/builds/527074481

image
https://travis-ci.org/node-machine/rttc/jobs/527074484

Thank you!

BridgeAR commented

@mikermcneil this seems to be an independent issue. assert.strictEqual verifies the input stricter than before. That seems to be the reason your test fails (you convert '-0' to the number -0 but your expectation is 0). You can fix the test by writing it as: assert(rttc.parseHuman('-0') === 0);. I personally recommend to check for -0 instead: assert.strictEqual(rttc.parseHuman('-0'), -0);.

  • 👍1
jasnell commented

This appears to be fixed in 10.x. Closing