ceifa / wasmoon

A real lua 5.4 VM with JS bindings made with webassembly
MIT License
495 stars 32 forks source link

Undesired tostring()/format() results for large numbers #99

Closed Sapu94 closed 11 months ago

Sapu94 commented 11 months ago

While #97 by @tims-bsquare did fix handling of large integers for the most part, it introduced a few bits of undesired behavior with how Lua converts large numbers to strings that would be great to continue to improve if possible.

tostring()

I missed this while reviewing that PR, but suffixing large integer values with ".0" did end up breaking my application. This is exemplified by the test case:

        const res = await engine.doString(`return tostring(value)`)

        // Since it's a float in Lua it will be prefixed with .0 from tostring()
        expect(res).to.be.equal(`${String(value)}.0`)

The following patch can be made to lua's tostring as a workaround for this:

    const tostringOrig = lua.global.get('tostring');
    lua.global.set('tostring', (val: any) => {
      const str: string = tostringOrig(val);
      if (typeof val === 'number' && Number.isInteger(val) && str.endsWith('.0')) {
        return str.slice(0, -2);
      }
      return str;
    });

Is this something wasmoon could integrate in some way?

string.format() / concatenation

This one is a bit trickier. Here is some test Lua code:

local val = 10000000000
print("TEST1 "..val) -- > TEST1 10000000000.0
print(("TEST2 %s"):format(val)) -- > TEST2 10000000000.0
print(("TEST3 %d"):format(val)) -- Error: test.lua:74: bad argument #1 to 'format' (number has no integer representation)

The first two prints behave as I would expect given the current implementation. It would be awesome if they would match the stock lua implementation without the ".0" suffix, as above.

The last print crashing is definitely not ideal though. For what it's worth, the stock lua implementation results in "TEST3 10000000000" for this one as well.

tims-bsquare commented 11 months ago

I think this one might not be sensible, or possible to fix, though I'm open to suggestions.

I'm not keen on overriding the tostring behaviour because that just hides that larger numbers are represented as doubles rather than integers so I think I'd like to keep it in line with Lua's expected print behaviour.

WASM/Emscripten only supports up to 32 bit integers but does support 64 bit doubles whereas when Lua runs on your host machine that supports both 64 bit integers and doubles so for your case that large number can be represented as an integer but in WASM it can't. During my last PR I updated the README.md to document the number behaviour https://github.com/ceifa/wasmoon#numbers

The new behaviour now matches fengari at least, an even more mature project than this one, if they don't have a better solution yet I'm not sure I'll think of one. https://github.com/fengari-lua/fengari#integers

Sapu94 commented 11 months ago

Dug into this and was able to resolve these issues and also remove the need for building lua with 32-bit integers using WASM_BIGINT. PR up here: https://github.com/ceifa/wasmoon/pull/100

tims-bsquare commented 11 months ago

@ceifa you can close this one too now thanks to @Sapu94 's research and PR :)