Merge pull request #97 from tims-bsquare/bugfix/misbehaving-integers

Fix #54 misbehaving integers

README.md

@@ -181,3 +181,90 @@ npm run build:wasm:dev # build lua
 npm run build # build the js code/bridge
 npm test # ensure everything it's working fine
 ```
+
+## Edge Cases
+
+### Numbers
+
+WASM does not support 64 bit integers only 32 bit integers and 64 bit doubles. If a number is an integer and will fit within a Lua integer then it will be pushed as a Lua native integer type. However, if it exceeds that size even if it is still an integer it will be pushed as a 64 bit double. This is unlikely to effect inteoperability with JS since JS treats all numbers the same but should allow some optimisation within Lua for smaller numbers.
+
+### Null
+
+`null` is not exposed to Lua and it has no awareness of it which can cause some issues when using it a table. `nil` is equivalent to `undefined`. Issue #39 tracks this and a workaround until `null` is added into Wasmoon.
+
+### Promises
+
+Promises can be await'd from Lua with some caveats detailed in the below section. To await a Promise call `:await()` on it which will yield the Lua execution until the promise completes.
+
+```js
+const { LuaFactory } = require('wasmoon')
+const factory = new LuaFactory()
+const lua = await factory.createEngine()
+
+try {
+    lua.global.set('sleep', (length) => new Promise((resolve) => setTimeout(resolve, length)))
+    await lua.doString(`
+        sleep(1000):await()
+    `)
+} finally {
+    lua.global.close()
+}
+```
+
+### Async/Await
+
+It's not possible to await in a callback from JS into Lua. This is a limitation of Lua but there are some workarounds. It can also be encountered when yielding at the top-level of a file. An example where you might encounter this is a snippet like this:
+
+```js
+local res = sleep(1):next(function ()
+    sleep(10):await()
+    return 15
+end)
+print("res", res:await())
+```
+
+Which will throw an error like this:
+
+```
+Error: Lua Error(ErrorRun/2): cannot resume dead coroutine
+    at Thread.assertOk (/home/tstableford/projects/wasmoon/dist/index.js:409:23)
+    at Thread.<anonymous> (/home/tstableford/projects/wasmoon/dist/index.js:142:22)
+    at Generator.throw (<anonymous>)
+    at rejected (/home/tstableford/projects/wasmoon/dist/index.js:26:69)
+```
+
+Or like this:
+
+```
+attempt to yield across a C-call boundary
+```
+
+You can workaround this by doing something like below:
+
+```lua
+function async(callback)
+    return function(...)
+        local co = coroutine.create(callback)
+        local safe, result = coroutine.resume(co, ...)
+
+        return Promise.create(function(resolve, reject)
+            local function step()
+                if coroutine.status(co) == "dead" then
+                    local send = safe and resolve or reject
+                    return send(result)
+                end
+
+                safe, result = coroutine.resume(co)
+
+                if safe and result == Promise.resolve(result) then
+                    result:finally(step)
+                else
+                    step()
+                end
+            end
+
+            result:finally(step)
+        end)
+    end
+end
+```

build.sh

@@ -7,15 +7,19 @@ LUA_SRC=$(ls ./lua/*.c | grep -v "luac.c" | grep -v "lua.c" | tr "\n" " ")
 extension=""
 if [ "$1" == "dev" ];
 then
-    extension="$extension -O0 -g3 -s ASSERTIONS=1 -s SAFE_HEAP=1 -s STACK_OVERFLOW_CHECK=2"
+    extension="-O0 -g3 -s ASSERTIONS=1 -s SAFE_HEAP=1 -s STACK_OVERFLOW_CHECK=2"
 else
-    extension="$extension -O3 --closure 1"
+    # TODO: This appears to be a bug in emscripten. Disable assertions when that bug is resolved or a workaround found.
+    # ASSERTIONS=1 required with optimisations and strict mode. https://github.com/emscripten-core/emscripten/issues/20721
+    extension="-O3 --closure 1 -s ASSERTIONS=1"
 fi
 
+# Instead of telling Lua to be 32 bit for both floats and ints override the default
+# int type to 32 bit and leave the float as 64 bits.
 if [[ "$OSTYPE" == "darwin"* ]]; then
-    sed -i '' "s/^#define LUA_32BITS\t0$/#define LUA_32BITS\t1/" ./lua/luaconf.h
+    sed -E -i '' "s/#define LUA_INT_DEFAULT\s+LUA_INT_LONGLONG/#define LUA_INT_DEFAULT \tLUA_INT_INT/" ./lua/luaconf.h
 else
-    sed -i "s/^#define LUA_32BITS\t0$/#define LUA_32BITS\t1/" ./lua/luaconf.h
+    sed -E -i "s/#define LUA_INT_DEFAULT\s+LUA_INT_LONGLONG/#define LUA_INT_DEFAULT \tLUA_INT_INT/" ./lua/luaconf.h
 fi
 
 emcc \
@@ -44,7 +48,7 @@ emcc \
     -s STRICT=1 \
     -s EXPORT_ES6=1 \
     -s NODEJS_CATCH_EXIT=0 \
-	-s NODEJS_CATCH_REJECTION=0 \
+    -s NODEJS_CATCH_REJECTION=0 \
     -s MALLOC=emmalloc \
     -s STACK_SIZE=1MB \
     -s EXPORTED_FUNCTIONS="[

src/engine.ts

@@ -70,9 +70,15 @@ export default class LuaEngine {
             loader(thread)
             const result = await thread.run(0)
             if (result.length > 0) {
+                // Move all stack results to the global state to avoid referencing the thread values
+                // which will be cleaned up in the finally below.
                 this.cmodule.lua_xmove(thread.address, this.global.address, result.length)
+                // The shenanigans here are to return the first reuslt value on the stack.
+                // Say there's 2 values at stack indexes 1 and 2. Then top is 2, result.length is 2.
+                // That's why there's a + 1 sitting at the end.
+                return this.global.getValue(this.global.getTop() - result.length + 1)
             }
-            return result[0]
+            return undefined
         } finally {
             // Pop the read on success or failure
             this.global.remove(threadIndex)

src/thread.ts

@@ -23,6 +23,9 @@ export interface OrderedExtension {
 
 // When the debug count hook is set, call it every X instructions.
 const INSTRUCTION_HOOK_COUNT = 1000
+// These reflect math.maxinteger and math.mininteger with 32 bit ints in Lua.
+const MAX_SAFE_INTEGER = Math.pow(2, 31) - 1
+const MIN_SAFE_INTEGER = -Math.pow(2, 31)
 
 export default class Thread {
     public readonly address: LuaState
@@ -204,7 +207,9 @@ export default class Thread {
                 this.lua.lua_pushnil(this.address)
                 break
             case 'number':
-                if (Number.isInteger(target)) {
+                // Only push it as an integer if it fits within 32 bits, otherwise treat it as a double.
+                // This is because wasm doesn't support 32 bit ints but does support 64 bit floats.
+                if (Number.isInteger(target) && target <= MAX_SAFE_INTEGER && target >= MIN_SAFE_INTEGER) {
                     this.lua.lua_pushinteger(this.address, target)
                 } else {
                     this.lua.lua_pushnumber(this.address, target)

test/engine.test.js

@@ -660,4 +660,55 @@ describe('Engine', () => {
 
         expect(res).to.be.equal(str)
     })
+
+    it('negative integers should be pushed and retrieved as string', async () => {
+        const engine = await getEngine()
+        engine.global.set('value', -1)
+
+        const res = await engine.doString(`return tostring(value)`)
+
+        expect(res).to.be.equal('-1')
+    })
+
+    it('negative integers should be pushed and retrieved as number', async () => {
+        const engine = await getEngine()
+        engine.global.set('value', -1)
+
+        const res = await engine.doString(`return value`)
+
+        expect(res).to.be.equal(-1)
+    })
+
+    it('number greater than 32 bit int should be pushed and retrieved as string', async () => {
+        const engine = await getEngine()
+        const value = 1689031554550
+        engine.global.set('value', value)
+
+        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`)
+    })
+
+    it('number greater than 32 bit int should be pushed and retrieved as number', async () => {
+        const engine = await getEngine()
+        const value = 1689031554550
+        engine.global.set('value', value)
+
+        const res = await engine.doString(`return value`)
+
+        expect(res).to.be.equal(value)
+    })
+
+    it('print max integer as 32 bits', async () => {
+        const engine = await getEngine()
+        const res = await engine.doString(`return math.maxinteger`)
+        expect(res).to.be.equal(2147483647)
+    })
+
+    it('print min integer as 32 bits', async () => {
+        const engine = await getEngine()
+        const res = await engine.doString(`return math.mininteger`)
+        expect(res).to.be.equal(-2147483648)
+    })
 })