Add MultiReturn support #87

ParadiseFallen · 2023-08-08T19:35:04Z

It kinda with breaking changes (in filesystem tests) but i tryed my best to bring multret support as softly as i can.

ParadiseFallen · 2023-08-08T19:36:05Z

Linked issue is #84

ParadiseFallen · 2023-08-11T18:58:36Z

code that failed wasn't changed. so it looks like skip this one for me. What do you think, @ceifa?

ceifa · 2023-08-11T19:02:00Z

code that failed wasn't changed. so it looks like skip this one for me. What do you think, @ceifa?

can you run npm run lint? I think it will fix the build

ParadiseFallen · 2023-08-11T19:44:26Z

@ceifa done. check now

ceifa · 2023-08-12T13:57:24Z

src/type-extensions/function.ts

+            const oldTop = thread.lua.lua_gettop(thread.address)
+
            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)


Replace the thread.lua.lua_gettop with wrapper thread.getTop

ceifa · 2023-08-12T14:06:10Z

Im not confortable about this change.
Its a little bit weird that the return type changes if you are returning a single value or a multi value, if we understand that we should support it, we should stick with always returning the array, but this causes other problems, like we cant write a function in lua and just translate it to JS, since one will return a value, and the other an array, example:

function sum(x, y) -- return an array in JS
    return x + y
end

function sum(x, y) { // return a number in lua
    return x + y
}

@timstableford do you have any thoughts?

ParadiseFallen · 2023-08-12T14:35:42Z

@ceifa there is soft support of mulret. so basically your first code will return one and only one return. and only if you wrap add , x it will return mulret. So anyone can check return type (which is fine) and do what they need to.

ParadiseFallen · 2023-08-12T14:41:07Z

@ceifa i was thinking about this issue too. but mine impl wont bring any breaking changes to all exsisting projects. So if we make it always return an array it will cause many breaking changes.

For now we can use kinda generics

const [a,b,c] = lua.doString<MulRet>(`code`); // return all returns
const a = lua.doString<any>(`code`); // return first one return

timstableford · 2023-08-12T16:15:44Z

Im not confortable about this change. Its a little bit weird that the return type changes if you are returning a single value or a multi value, if we understand that we should support it, we should stick with always returning the array, but this causes other problems, like we cant write a function in lua and just translate it to JS, since one will return a value, and the other an array, example:
function sum(x, y) -- return an array in JS
    return x + y
end
function sum(x, y) { // return a number in lua
    return x + y
}
@timstableford do you have any thoughts?

That's a tough one. I think there's two cases though that could be reasonably kept consistent. When calling into a Lua program from JS as an entrypoint like with doString I'd personally have it always return an array. Then for functions that are passed back to JS from Lua have it only use the single return value, like when calling JSON.stringify from Lua and passing a replacer function.

Perhaps I'm just struggling to see a use case for that half of it though? @ParadiseFallen out of curiosity is this a just for the fun of it PR or did you have a specific use in mind?

ParadiseFallen · 2023-08-12T17:03:44Z

there is no any special use cases but it would be nice to have that feture. if lua uses mulret we can support it softly.

For now there is some built in functions that i want to call and can't bc there is no mulret.

ParadiseFallen · 2023-08-12T20:27:05Z

@timstableford why we won't use generics? its seems exatly what we want. so by default it will be just single return and if you pass expected return as <MulRet>() it will wrap it in mullret. even if there is only one return

timstableford · 2023-08-12T20:36:50Z

I'm not against using generics but it'd be necessary to link it to something runtime. As far as I understand TypeScript that could be an argument to the function to say which return mode to use or an alternate function. I'm having a bit of trouble imagining it at the mo but tl;dr I'm sure you can go that route if you work through quirks

ParadiseFallen · 2023-08-12T21:05:44Z

@timstableford well. if you want use it in js then we can made kinda

function doString(code: string, returnStategy: IReturnStrategy = SingleReturn)
{
  // just as normal code
  return returnStrategy.makeReturn(thread);
}

but need to look for how to hookup it when you want mulret (bc we need start top)

ParadiseFallen · 2023-08-12T21:14:13Z

or even just use enum for that

export enum ReturnType{
  SingleValue = 0,
  MultipleValues = 1,
}

ceifa · 2023-08-17T00:12:56Z

That's a tough one. I think there's two cases though that could be reasonably kept consistent. When calling into a Lua program from JS as an entrypoint like with doString I'd personally have it always return an array. Then for functions that are passed back to JS from Lua have it only use the single return value, like when calling JSON.stringify from Lua and passing a replacer function.

You are right, it would break implementations like that, which is very common.

@ceifa there is soft support of mulret. so basically your first code will return one and only one return. and only if you wrap add , x it will return mulret. So anyone can check return type (which is fine) and do what they need to.

I'm not a fan of this approach, returning always the same type would be better for maintainability.

I will let the PR open for now, and think better about it later

ParadiseFallen · 2023-10-11T11:30:32Z

@ceifa any updates? i suggest move to always returning an array expecting multiparams

tims-bsquare · 2023-11-30T10:14:45Z

test/engine.test.js

@@ -168,13 +168,14 @@ describe('Engine', () => {
        expect(sum(10, 50)).to.be.equal(60)
    })

+    // TEST OFTEN BREAKS


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

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

tims-bsquare · 2023-11-30T10:19:37Z

@ParadiseFallen I've come up with something that might make this more palatable (to me, I can't speak for @ceifa who will be merging it).

What you could do is overload the function signatures like this https://stackoverflow.com/a/13212871
For instance with doFile

public doFile(filename: string, returnMode: 'multi'): Promise<MultiReturn>;
public doFile(filename: string, returnMode?: 'single' | 'multi'): Promise<any> {
 // Implementation here
}

That would mean the current function signatures would remain compatible and return a single item but if return mode was set to multi the return type would be MultiReturn instead

ParadiseFallen · 2023-11-30T10:44:42Z

Автор

@ParadiseFallen I've come up with something that might make this more palatable (to me, I can't speak for @ceifa who will be merging it).

What you could do is overload the function signatures like this https://stackoverflow.com/a/13212871 For instance with doFile
public doFile(filename: string, returnMode: 'multi'): Promise<MultiReturn>;
public doFile(filename: string, returnMode?: 'single' | 'multi'): Promise<any> {
 // Implementation here
}
That would mean the current function signatures would remain compatible and return a single item but if return mode was set to multi the return type would be MultiReturn instead

I already proposed that. we dont even need overload for that.

tims-bsquare · 2023-11-30T11:01:10Z

You're right that you proposed a return mode, which I generally liked but the explicit return type of multireturn I think will make it easier.

I've just been reading through the comments again after your prompt and the other part that I think was tripping us up was that functions returned from Lua to JS would need to be single return for JS compatibility. I'd be okay with you passing it in as an option to the engine whether to do single returns or multi-returns in that circumstance.

It's been a while since I've put any thought into this, so sorry for rehashing a bit.

ParadiseFallen · 2023-11-30T11:56:43Z

@tims-bsquare why just single return?
its easilly supported with just mulret

const mulret = call();
const [r1, r2, r3] = call();

tims-bsquare · 2023-11-30T12:24:24Z

For JS interoperability unless I'm misunderstanding you.
Consider exposing the JSON.stringify function to Lua as an example. So Lua does

JSON.stringify({}, function (replacer) return something end)

stringify expects a function that return a single argument, by not keeping single as default the above breaks

ParadiseFallen · 2023-11-30T12:46:28Z

@tims-bsquare there many pain points on example like that. also it looke like

-- lua
-- return generic builder
someJsFunc(function (startIndex) 
-- some wrap for example.
return function ()
  local x = startIndex;
  -- indexer
  return function ()
    x = x + 1;
    return x;
  end
end

end)

this example will breaks. same for js side.

tims-bsquare · 2023-11-30T13:16:50Z

Sorry @ParadiseFallen I'm not following the problem you're describing. Currently the example I showed works and I'm not sure what your function is showing

ParadiseFallen · 2023-11-30T13:25:58Z

@tims-bsquare it's showing another problem, that not mentioned there. sometimes wrapper breaks

ceifa · 2023-11-30T14:43:04Z

@tims-bsquare it's showing another problem, that not mentioned there. sometimes wrapper breaks

I'm a little bit confused too, can you elaborate the problem you are showing?

ParadiseFallen · 2023-11-30T17:58:13Z

@ceifa will do it later and on another issue

ParadiseFallen · 2023-11-30T18:07:22Z

@ceifa @tims-bsquare
#98

gudzpoz · 2023-12-03T09:05:51Z

My two cents as a user:

In Lua, a = (function() return "a", "truncated" end)() does not assign a table to a but simply truncates the stack (i.e. a = "a"). And I think it is only natural to keep this as default.

Alternatively, if multiple return values are wanted, I can always wrap them in a table like return { "a", "b" } and can comfortably achieve anything I want without necessitating this MULTI_RET change.

Some more justifications:

The Array when >= 2 (JS) approach:

In Lua, changing from return 1 to return 1, "extra" is usually not a breaking change due to how the stack truncation works. However, with the current PR (return 1 => (JS) 1, return 1, 2 => (JS) [1, 2]), it will. So either we stick with our previous truncation approach or we always return an array.
The Always array approach:

Always returning an array has its own problem: it poses a limit on the library's capability - Lua functions will no longer be able to simulate any JS function that can return anything.

The following matrix outlines whether the user will be able to do something with code change in only one language:

	Get a single value	Return multiple values
Previous (JS side)	/	Function wrapper
Previous (Lua side)	/	Wrap in tables
Always array (JS)	Function wrapper	/
Always array (Lua)	You can't	/
Array when >= 2 (JS)	Sometimes / wrapper	Sometimes / wrapper
Array when >= 2 (Lua)	Sometimes / wrapper	Sometimes / wrapper

Therefore, speaking as a user, I am against this breaking change. (The overloading approach is non-breaking and feels better.)

ParadiseFallen · 2024-03-19T15:57:31Z

@ceifa so any updates on this issue?

ParadiseFallen · 2024-03-19T16:45:13Z

lets take this as example

-- IdGenerator
return function (start,step)
  local current = start
  return function ()
    current = current + step
    return current
  end
end

-- main
local IdGenerator = require 'IdGenerator'

-- this method will called from js
function tryCreateGenerator(start)
  
  if start < 0 then
    return false
  end
  
  return true, IdGenerator(start, 1) 
end

return tryCreateGenerator

Then lets call it from js

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

const tryCreateGenerator = engine.doString(/*code goes here*/);
// we can also get `tryCreateGenerator` just by engine.global.get('name'). 

// and now we facing few issues
// first one is mulret. it won't work
// second one is returning of lua function. it just breakes
const [created, generator] = tryCreateGenerator(2) // won't work

ParadiseFallen · 2024-03-19T17:11:00Z

also it shows one more issue. if generator() will return 2 values
ie

function createGenerator(start, step)
  local current = start
  -- generator itself
  return function()
    current = current + step
    if current > 100 then
      return false, current - step
    end
    return true, current
  end
end

it won't work. bc internal function will called on js side. and it must return mulret. but we can't specify which value we expect. mulret or not. So this option muse be specified a bit higher that just doString or doFile

ParadiseFallen · 2024-03-19T20:34:14Z

i tryed this again after updating to latest verion. oh yeah. issue with func seems to be fixed now

But lets keep talking about mulret.
Trimming of stack as done by now isn't what i expect. if it won't trim i could at least get that values by low level calls.

It could be option in createEngine to make this behaviour engine wide.
I don't like idea of overload bc of sample above with generator

also it shows one more issue. if generator() will return 2 values

We could utilize
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof
so current behaviour will be kept ie

const singleValue = engine.doString(`
return 1;
`); // typeof number
const array = engine.doString(`
return {"a","b","c"};
`); // instance of array
cosnt object = engine.doString(`
return { a = 1, b = 2, c = 3 }
`); // typeof object && ! instanceof MulRet
// new behaviour
const mulret = engine.doString(`
return 1,2,3
`); // typeof object && instance of MulRet

It won;t breaks any existing code unleast there smth strange goes at lua side
example

function badFunction()
  return true, "ommited"
end

bc now it will translates to mulret. wich is exatly what on lua side.

jmptbl · 2024-03-28T15:21:17Z

I tried getting MultiRet working today, and I couldn't figure it out. What I've done for now is to add this shim function to my Lua code:

function multiret (fname, ...)
	local f = _G[fname]
	if f == nil then return nil end
	return table.pack(f(...))
end

Maybe it's not pretty, but I missed how the code in this PR's commit is supposed to work, so this shim seemed reasonable. Either way I don't think there's a clean way of handling MultiRet. Obviously the two languages are different, so the programmer will have to step in to fill this gap. I suggest keeping the default case of returning only one value as is, and supplying a JS-side shim to convert multiret into an array return so that programmers don't have to write their own shim.

ParadiseFallen · 2024-03-28T17:48:08Z

@jmptbl
mulret is

return a, b, c

And not what you wrote there.
So on js it will transform to

const result = await engine.doString(`...`);
if(result instanceof  MulRet)
{
  ...
}
else
{
  throw new Error("mulret expected");
}

jmptbl · 2024-03-29T10:01:41Z

@ParadiseFallen Thank you. For some reason I thought multiret support had already been committed to wasmoon. I was trying to get it working with the wasmoon 1.16.0 release, but ended up writing the lua shim that I shared in my previous comment.

RodrigoDornelles · 2024-07-10T12:07:11Z

how about a simple solution like:

// multi return in lua
// single return custom object in js
function javascript() {
 return someWasmoonLib.unpack([1, 2])
}

-- multi return in lua
-- single return array in js
function lua()
 return 1, 2
end

I think it shouldn't be something complicated that changes existing interfaces, or something that is difficult to explain beyond an example.

// javascript has destructors, this is already a bit of a multi return, keep it simple.
const [a, b] = lua();

I think it should only be work on the side: javascript -> lua
as that's where I felt a deficiency, but the opposite is easily solvable.

what exists, must work as is

function lua()
 return 'hello'
end

const message = lua();

@ParadiseFallen @timstableford @tims-bsquare @gudzpoz @jmptbl @ceifa

RodrigoDornelles · 2024-07-10T12:09:18Z

test/filesystem.test.js

@@ -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")')


unnecessary changes interface

RodrigoDornelles · 2024-07-10T12:10:11Z

test/engine.test.js

-            setInterval(function()
+            setInterval(function ()


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

RodrigoDornelles · 2024-07-10T12:10:49Z

package.json

-        "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",


unnecessary change, and dont related with PR

ParadiseFallen added 4 commits August 5, 2023 18:34

fix build on windows

d536b64

[WIP] : fixed multireturn from dostring

e3f0ef7

soft mulret fix

eb09314

add comments

0feffda

fix bug with args. add tests

d8494e5

run 'npm run lint' command

6d816a0

ceifa reviewed Aug 12, 2023

View reviewed changes

ParadiseFallen added 2 commits August 12, 2023 17:36

change to wrapped getTop()

84601e8

run npm run lint

3186b18

tims-bsquare reviewed Nov 30, 2023

View reviewed changes

RodrigoDornelles suggested changes Jul 10, 2024

View reviewed changes

Add MultiReturn support #87

Are you sure you want to change the base?

Add MultiReturn support #87

Conversation

ParadiseFallen commented Aug 8, 2023

ParadiseFallen commented Aug 8, 2023

ParadiseFallen commented Aug 11, 2023

ceifa commented Aug 11, 2023

ParadiseFallen commented Aug 11, 2023

ceifa Aug 12, 2023

Choose a reason for hiding this comment

ceifa commented Aug 12, 2023

ParadiseFallen commented Aug 12, 2023

ParadiseFallen commented Aug 12, 2023

timstableford commented Aug 12, 2023

ParadiseFallen commented Aug 12, 2023

ParadiseFallen commented Aug 12, 2023

timstableford commented Aug 12, 2023

ParadiseFallen commented Aug 12, 2023

ParadiseFallen commented Aug 12, 2023 • edited Loading

ceifa commented Aug 17, 2023

ParadiseFallen commented Oct 11, 2023

tims-bsquare Nov 30, 2023

Choose a reason for hiding this comment

ParadiseFallen Nov 30, 2023

Choose a reason for hiding this comment

tims-bsquare commented Nov 30, 2023

ParadiseFallen commented Nov 30, 2023

tims-bsquare commented Nov 30, 2023

ParadiseFallen commented Nov 30, 2023

tims-bsquare commented Nov 30, 2023

ParadiseFallen commented Nov 30, 2023

tims-bsquare commented Nov 30, 2023 • edited Loading

ParadiseFallen commented Nov 30, 2023

ceifa commented Nov 30, 2023

ParadiseFallen commented Nov 30, 2023

ParadiseFallen commented Nov 30, 2023

gudzpoz commented Dec 3, 2023

ParadiseFallen commented Mar 19, 2024

ParadiseFallen commented Mar 19, 2024 • edited Loading

ParadiseFallen commented Mar 19, 2024 • edited Loading

ParadiseFallen commented Mar 19, 2024 • edited Loading

jmptbl commented Mar 28, 2024

ParadiseFallen commented Mar 28, 2024 • edited Loading

jmptbl commented Mar 29, 2024

RodrigoDornelles commented Jul 10, 2024 • edited Loading

what exists, must work as is

RodrigoDornelles Jul 10, 2024

Choose a reason for hiding this comment

RodrigoDornelles Jul 10, 2024

Choose a reason for hiding this comment

RodrigoDornelles Jul 10, 2024

Choose a reason for hiding this comment

ParadiseFallen commented Aug 12, 2023 •

edited

Loading

tims-bsquare commented Nov 30, 2023 •

edited

Loading

ParadiseFallen commented Mar 19, 2024 •

edited

Loading

ParadiseFallen commented Mar 19, 2024 •

edited

Loading

ParadiseFallen commented Mar 19, 2024 •

edited

Loading

ParadiseFallen commented Mar 28, 2024 •

edited

Loading

RodrigoDornelles commented Jul 10, 2024 •

edited

Loading