Use Math.imul(a, b) instead of a * b | 0

[no-longer-on-githu-b]

Math.imul correctly handles arithmetic overflow, whereas | 0 just discards the most significant bits. This affects purescript-prelude as well as the inliner. Using Math.imul would also make Semiring Int lawful.

[garyb]

It'd be better if we can auto-polyfill this or something, since imul does not have solid browser support. It's not even ES5, nevermind the currently-supported ES3. 😕

[hdgarrood]

There's already an issue for this, isn't there?

[hdgarrood]

Ok, there's #2921, but it was closed, so maybe let's just use this one.

MDN's Math.imul page suggests the following polyfill:

Math.imul = Math.imul || function(a, b) {
  var ah = (a >>> 16) & 0xffff;
  var al = a & 0xffff;
  var bh = (b >>> 16) & 0xffff;
  var bl = b & 0xffff;
  // the shift by 0 fixes the sign on the high part
  // the final |0 converts the unsigned value into a signed value
  return ((al * bl) + (((ah * bl + al * bh) << 16) >>> 0)|0);
};

[hdgarrood]

Come to think of it, we shouldn't mutate Math, and we can't exactly inline all that every time someone uses Int's *. Without a stdlib it's hard to see how to achieve this without either giving up inlining or dropping support for platforms which don't provide Math.imul.

[hdgarrood]

If we had a general inliner i.e. one that didn't only use a set of special cases built in to the compiler, this would probably be doable, though.

[michaelficarra]

@hdgarrood Do we have an official stance on JS engine support? It'd be nice to make a requirement that can shift over time like "browsers with >0.1% market share (by page loads) globally" or "browsers with >0.5% market share (by installations) in every individual market" or "browsers released in the last 5 years". A target of "just ES3" is problematic both because it doesn't actually represent meaningful browser support, and it doesn't allow us to improve/adapt as time passes and things get better.

[natefaubion]

I think it's reasonable to say that PureScript does not require a runtime, but relies on some set of easily polyfilled features, such as Math.imul or Object.keys.

[hdgarrood]

I don't think we really have an official stance. The compiler generates ES3, most of the Node libraries assume node >= 4, and I guess from @natefaubion's comment that maps assumes Object.keys, so there's a mix. I think what you say makes sense though. I guess we haven't really needed to develop a clear stance as I don't think there are that many situations where we can't reasonably accomplish what we want using just ES3.

Would we be able to justify using Math.imul with any of those stances you gave, incidentally?

[Pauan]

@hdgarrood I personally don't see a problem with mutating Math

The polyfill will only be added if Math.imul doesn't already exist, and the polyfill should have the correct behavior (it's just slower than native support), so I don't think the polyfill will cause any problems.

@michaelficarra Math.imul is supported in every browser except IE11.

This is because Math.imul is super easy for browsers to support (it's just standard C multiplication), and it was necessary for asm.js support, so all the browsers added it in really fast.

IE11 has ~4% market share, so I don't think it's viable to drop support for it right now.

[paf31]

Is there anything wrong with providing this as a library function in math for now?

[garyb]

Only that it doesn't fix (*) for integers.