Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MultiReturn support #87

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ module.exports = {
child_process: false,
crypto: false,
url: false,
module: false
module: false,
},
},
}
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
"scripts": {
"build:wasm:dev": "./build.sh dev",
"build:wasm": "./build.sh",
"build:wasm:docker:dev": "docker run --rm -v $(pwd):/wasmoon emscripten/emsdk /wasmoon/build.sh dev",
"build:wasm:docker": "docker run --rm -v $(pwd):/wasmoon emscripten/emsdk /wasmoon/build.sh",
"build:wasm:docker:dev": "docker run --rm -v .:/wasmoon emscripten/emsdk /wasmoon/build.sh dev",
"build:wasm:docker": "docker run --rm -v .:/wasmoon emscripten/emsdk /wasmoon/build.sh",
Comment on lines -9 to +10

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary change, and dont related with PR

"start": "rollup -c -w",
"test": "mocha --parallel --require ./test/boot.js test/*.test.js",
"luatests": "node --experimental-import-meta-resolve test/luatests.mjs",
Expand Down
23 changes: 17 additions & 6 deletions src/engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,11 @@ export default class LuaEngine {
}

public doStringSync(script: string): any {
this.global.loadString(script)
const result = this.global.runSync()
return result[0]
return this.callByteCodeSync((global) => global.loadString(script))
}

public doFileSync(filename: string): any {
this.global.loadFile(filename)
const result = this.global.runSync()
return result[0]
return this.callByteCodeSync((global) => global.loadFile(filename))
}

// WARNING: It will not wait for open handles and can potentially cause bugs if JS code tries to reference Lua after executed
Expand All @@ -72,10 +68,25 @@ export default class LuaEngine {
if (result.length > 0) {
this.cmodule.lua_xmove(thread.address, this.global.address, result.length)
}

if (result.length > 1) {
return result // support for multi return values
}

return result[0]
} finally {
// Pop the read on success or failure
this.global.remove(threadIndex)
}
}

private callByteCodeSync(loader: (global: Global) => void): any {
const global = this.global
loader(global)
const result = global.runSync()
if (result.length > 1) {
return result // support for multi return values
}
return result[0]
}
}
21 changes: 14 additions & 7 deletions src/type-extensions/function.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { BaseDecorationOptions, Decoration } from '../decoration'
import { LUA_REGISTRYINDEX, LuaReturn, LuaState, LuaType, PointerSize } from '../types'
import { LUA_MULTRET, LUA_REGISTRYINDEX, LuaReturn, LuaState, LuaType, PointerSize } from '../types'
import Global from '../global'
import MultiReturn from '../multireturn'
import RawResult from '../raw-result'
Expand Down Expand Up @@ -158,7 +158,7 @@ class FunctionTypeExtension extends TypeExtension<FunctionType, FunctionDecorati
thread.lua.lua_pushvalue(thread.address, index)
const func = thread.lua.luaL_ref(thread.address, LUA_REGISTRYINDEX)

const jsFunc = (...args: any[]): any => {
const jsFunc = (...args: any[]): MultiReturn | any => {
if (thread.isClosed()) {
console.warn('Tried to call a function after closing lua state')
return
Expand All @@ -173,19 +173,26 @@ class FunctionTypeExtension extends TypeExtension<FunctionType, FunctionDecorati
}
}

const oldTop = thread.getTop()

for (const arg of args) {
thread.pushValue(arg)
}

const status = thread.lua.lua_pcallk(thread.address, args.length, 1, 0, 0, null)
const status = thread.lua.lua_pcallk(thread.address, args.length, LUA_MULTRET, 0, 0, null)
const newTop = thread.lua.lua_gettop(thread.address)
if (status === LuaReturn.Yield) {
throw new Error('cannot yield in callbacks from javascript')
}
thread.assertOk(status)

const result = thread.getValue(-1)

thread.pop()
const deltaTop = newTop - (oldTop - 1)
let result = null
if (deltaTop > 1) {
result = thread.getStackValues(oldTop - 1)
} else {
result = thread.getValue(-1)
}
thread.pop(deltaTop)
return result
}

Expand Down
51 changes: 50 additions & 1 deletion test/engine.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,13 +168,14 @@ describe('Engine', () => {
expect(sum(10, 50)).to.be.equal(60)
})

// TEST OFTEN BREAKS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just having a think about this PR and I've never seen this test fail before this change

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure that PR breake this test. it breaks from time to time. sometimes if more or less then expected.

it('scheduled lua calls should succeed', async () => {
const engine = await getEngine()
engine.global.set('setInterval', setInterval)

await engine.doString(`
test = ""
setInterval(function()
setInterval(function ()
Comment on lines -177 to +178

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's not related to the PR, don't change it

test = test .. "i"
end, 1)
`)
Expand Down Expand Up @@ -238,6 +239,54 @@ describe('Engine', () => {
expect(returns.at(-1)).to.be.a('function')
})

it('doString with multiple returns should succeed', async () => {
const engine = await getEngine()

const returns = await engine.doString(`
return 1, x, y, "Hello World", {}, function() end, {1,2,3}, {a = 1, b = 2, c = 3};
`)

expect(returns).to.be.a('array')
expect(returns).to.have.length(8)
expect(returns[5]).to.be.a('function')
expect(returns[6]).to.be.a('array')
expect(returns[7]).to.be.a('object')
})

it('call lua function with multiple returns should succeed', async () => {
const engine = await getEngine()

const fn = await engine.doString(`
return function (x, y)
return 1, x, y, function () end, {1,2,3}, {a = 1, b = 2};
end
`)

expect(fn).to.be.a('function')

const returns = fn(4, 5)
const [func, arr, obj] = returns.slice(3)
expect(returns).to.have.length(6)
expect(func).to.be.a('function')
expect(arr).to.be.a('array')
expect(obj).to.be.a('object')
})

it('call lua function with single returns array should succeed', async () => {
const engine = await getEngine()

const fn = await engine.doString(`
return function (a, b, c)
return {a, b, c};
end
`)

expect(fn).to.be.a('function')
const array = fn(3, 4, 5)
expect(array).to.be.an('array')
expect(array).to.have.length(3)
})

it('get a lua thread should succeed', async () => {
const engine = await getEngine()

Expand Down
6 changes: 4 additions & 2 deletions test/filesystem.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ describe('Filesystem', () => {
await factory.mountFile('yolo/sofancy/test.lua', 'return 42')
const engine = await factory.createEngine()

const value = await engine.doString('return require("yolo/sofancy/test")')
// second parameter is path to module
const [value] = await engine.doString('return require("yolo/sofancy/test")')

expect(value).to.be.equal(42)
})
Expand All @@ -27,7 +28,8 @@ describe('Filesystem', () => {
await factory.mountFile('hello/init.lua', 'return 42')
const engine = await factory.createEngine()

const value = await engine.doString('return require("hello")')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary changes interface

// second parameter is path to module
const [value] = await engine.doString('return require("hello")')

expect(value).to.be.equal(42)
})
Expand Down