Type definitions of Window.setTimeout and setInterval allow insidious errors [js]

[jcward]

I'm referring to the extern definitions of js.html.Window.setTimeout and setInterval, found here:

https://github.com/HaxeFoundation/haxe/blob/development/std/js/html/Window.hx#L572-L576

These type definitions are technically correct, but allow for an insidious, silent bug to creep in. If you (accidentally) pass a Float as the timeout, Haxe will generate a call to setTimeout(func, null, my_float). This does not fail at compile time, nor throw at runtime -- it's perfectly valid JavaScript. It simply executes the callback with 0ms timeout and passes the float to the callback function. But it's almost definitely not what the user intended, and it's caused by Haxe's handling of the combination of default / optional timeout and the rest parameter.

For example, can you spot the bug in this code (that would work perfectly well in JavaScript):

js.html.Window.setInterval(function() {
  // Update my status
}, 10000 + Math.random()*2000);

That's right -- the Math.random() I added for a little randomization changed the type from Int to Float, and so I silently introduced a bug where my interval runs every 0ms - ouch!

Proposed solution:

I propose we change the timeout parameter to Float in both setTimeout and setInterval. The underlying JS doesn't care - it's just a number, and for Haxe's sake, Float will nicely catch either Float or Int. Try haxe sample: https://try.haxe.org/#dCd83

Another possible solution is to make timeout:Int required (remove ? and / or = 0), but that forces me to use a Math.floor or Std.int when the underlying JS doesn't care.

CC / FYI some JS Haxians: @kevinresol @back2dos @elsassph

[elsassph]

Woah this is bad indeed... I agree with you fix suggestion (both seem ok but I prefer the non-optional argument) but ideally the compiler shouldn't fall into this case anyway.

[nadako]

@haxiomic could you look into this, please?

[haxiomic]

Should be good to go in PR #8224 👍

[jcward]

Thank you!