ceifa / wasmoon

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

[BUG] : cant handle multi return values #84

Open ParadiseFallen opened 1 year ago

ParadiseFallen commented 1 year ago

so basically code is

const factory = new LuaFactory()
const engine = await factory.createEngine();

const fn= await engine.doString(`
      return function ()
        return '22', '33', {};          
      end;
      `);

const result = fn(); // result is '22' while expected ['22','33', {}]
// same there
const result2 = await engine.doString(`
      return '11','22',{};
      `);
// result  is '11' while expected ['11','22',{}]
ParadiseFallen commented 1 year ago

after some time i figure out that it always return ONLY one result for functions (which is wrong. bc .call() returns multiresult)

this is fix for functions

engine.global.typeExtensions[4].extension.__proto__.getValue = (thread: Thread, index: number) => {
        thread.lua.lua_pushvalue(thread.address, index)
        const func = thread.lua.luaL_ref(thread.address, LUA_REGISTRYINDEX)

        const jsFunc = (...args: any[]): any => {
          if (thread.isClosed()) {
            console.warn('Tried to call a function after closing lua state')
            return
          }

          const internalType = thread.lua.lua_rawgeti(thread.address, LUA_REGISTRYINDEX, func)
          if (internalType !== LuaType.Function) {
            const callMetafieldType = thread.lua.luaL_getmetafield(thread.address, -1, '__call')
            thread.pop()
            if (callMetafieldType !== LuaType.Function) {
              throw new Error(`A value of type '${internalType}' was pushed but it is not callable`)
            }
          }

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

          const base = thread.getTop();
          const status = thread.lua.lua_pcallk(thread.address, args.length, LUA_MULTRET, 0, 0, null)
          if (status === LuaReturn.Yield) {
            throw new Error('cannot yield in callbacks from javascript')
          }
          thread.assertOk(status)
          const multiReturn = thread.getStackValues(thread.getTop() - base - args.length);

          // const result = thread.getValue(-1)

          thread.pop()
          return multiReturn;
        }

        ext.functionRegistry?.register(jsFunc, func)

        return jsFunc
      };
  And this is fix for just return
      engine.doString = async (code : string) => {
        const thread = engine.global.newThread()
        const threadIndex = engine.global.getTop()
        try {
            thread.loadString(code);
            const results = await thread.run(0)
            if (results.length > 0) {
              engine.cmodule.lua_xmove(thread.address, engine.global.address, results.length)
            }
            return results;
        } finally {
            // Pop the read on success or failure
            engine.global.remove(threadIndex)
        }
      };
ParadiseFallen commented 1 year ago
const [result] = fn();

Is looks much predicteble when we always mean that lua function returns multiresult. Or we can use kinda helper method that

const result = fn().singleResult();

where

array.prototype.singleResult = function()
{
  return this.at(0);
}
ParadiseFallen commented 1 year ago

https://github.com/ParadiseFallen/wasmoon/blob/main/test/engine.test.js#L268

So this test is always failed. seems like top calculated not correctly

https://github.com/ParadiseFallen/wasmoon/blob/main/src/type-extensions/function.ts#L187

there was expeted 8 but i got always only 3. so where is 5 more missing?

ParadiseFallen commented 1 year ago

https://github.com/ceifa/wasmoon/issues/84

Fix is right there

ParadiseFallen commented 1 year ago

@ceifa i can change apis to always return LuaReturn or just array. It will be breaking change. I want this feature

ceifa commented 1 year ago

@ceifa i can change apis to always return LuaReturn or just array. It will be breaking change. I want this feature

@ParadiseFallen this is not actually a bug, it was designed to work that way. We need to understand now which design would be better for readability/functionality/DX.

What is your use case?

ParadiseFallen commented 1 year ago

@ceifa use function from lua with multireturn. So for example if you want to call require in js side from lua it must return module & optionally path to module. It will also good to use with other methods.

I suggest

Always return an array expecting mulret

It looks like

let [module,path] = luaRequire('abc');

And if it still

function ()
  return 123;
end

on js will looks like

let [result] = luaMethod();