-
Notifications
You must be signed in to change notification settings - Fork 723
Implement number operations per ECMAScript spec #208
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, though admittedly it may be easy to forget to do this in other places. One way to mitigate this would be if values that we intend to be JS numbers were forced to be a separate type.
On that note, we have a internal/stringutil/convert.go. After this PR, would it make more sense for those functions to move into the jsnum package?
I thought about doing that, but I wasn't sure if it would scale very well. Worth a try quick just to see.
Yes, I think that is a good idea, given that is also "number stuff that differs from JS to Go". |
|
Yeah, so |
|
Fixed rem/pow. I also have a change which switches all uses of |
The current Go port of the expression evaluator uses plain Go conversions; this turns out to be subtly incorrect for certain values as the conversion Go does is not the same one specified by ECMAScript. We didn't have this problem in JS because... we were already running in JS, so got its behavior for free.
This PR reimplements number operations following the ECMAScript spec, which involves a little more work than a conversion for out-of-range values, along with a load of tests which verify the implementation. Notably, it's possible to make a "fast" implementation that passes on
amd64, but then fail onarm64, so it's good that macOS is now anarm64platform.Sidenotes:
amd64conversion is more or less spec compliant, but the default behavior of the conversion inarm64is to useFCVTZS, which explicitly has a different behavior. This matters enough thatarm64addedFJCVTZS("Floating-point Javascript Convert to Signed fixed-point, rounding toward Zero"), which is used by all major JS engines, but is unavailable to us in Go.ToInt32/ToUint32, I found that esbuild does it similarly to what I did (modulo... the modulo part), butgoja's implementation is actually broken on arm64. Should report that.arm64) use bitwise operations to achieve the same thing, but are not super well documented as to why what they're doing works, which is extra annoying because they are all very different. Using a bitwise implementation would be faster, but I haven't been able to rationalize it. The speedup only actually benefits the "out of range" values, which we rarely evaluate anyway.ToInt32instead ofToUint32sinceFJCVTZSconverts to signed. This difference is not super important if you can't useFJCVTZS; the bits are identical so the name is just "what type I give to the result".This issue was more or less caught in #201, which detected the truncation of potentially large inputs and led me down the rabbit hole.