espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.79k stars 747 forks source link

Extra undefined element added in array.map() #2543

Closed theauk closed 1 month ago

theauk commented 2 months ago

https://github.com/espruino/Espruino/commit/ff487e008472deff8113a32ebd3217dbd606405c

Test case

>var a = [1]; var b = 1; console.log((a.push(1) / a.map(b => a.unshift(2)).length));
=0.66666666666

In steps:

>var a = [1]; var b = 1;
=1
>var x = a.push(1)
=2
>var y = a.map(b => a.unshift(2))
=[ 3, undefined, 4 ]
>x/y.length
=0.66666666666

Expected behavior I would expect y to become an array of length 2, so 1 would be logged at the end (from 2 / 2). It seems like an unexpected undefined is placed into the array, which makes its length 3, resulting in the logged 0.66. Running the test case on other JavaScript engines (Node, Hermes, and QuickJS), I get the expected 1.

gfwilliams commented 2 months ago

Thanks for these reports! I'll take a look at them when I get a bit of free time.

In the mean time if you or anyone else wants to contribute fixes that'd be awesome!

gfwilliams commented 1 month ago

Ok, looking into this, modifying an array you're iterating over is something that IMO non-broken code should not do, and because we're trying to target very constrained systems we don't have the flash/ram space or cycles to try and circumvent this, so I've just added comments to all the iterator functions pointing out that you might get non-spec-compliant behaviour.

Another similar issue was mentioned recently at https://github.com/espruino/Espruino/issues/2553 as well - was this a colleague?

theauk commented 1 month ago

Makes sense. And yes, https://github.com/espruino/Espruino/issues/2553 is from a colleague.