DragosPopse / mani

Lua exporter for odin, made in odin
24 stars 3 forks source link

'to_value` uses 'luaL.checkstring', which doesn't call the '__tostring' metamethod. #7

Open MineBill opened 7 months ago

MineBill commented 7 months ago

When calling an odin proc that expects a string, from lua, lua will not convert usertypes to string because luaL_checkstring doesn't attempt to call the __tostring metamethod, even if it is defined for the usertype. A possible solution is to use luaL_tolstring which will attempt to call __tostring for usertypes.

@(LuaExport)
print :: proc(what: string) {
    log.info(what)
}
@(LuaExport = {
    Name = "vec3",
    Type = {Full, Light},
    SwizzleTypes = {vec2, vec4},
    Fields = xyz,
    Metamethods = {
        __tostring = vec3_to_string,
    },
})
vec3 :: [3]f32

Using the above print function to print the vec3 will result in: bad argument #1 to 'print' (string expected, got vec3)

DragosPopse commented 7 months ago

i don't think it's an error, I need to think more about it. Calling the builtin print will do the correct thing, as that calls tostring(arg) itself. The correct usage in your case would be print(tostring(vec)) (if this one doesn't work either then it's totally a bug). I need to think what are the implications of directly using lua_tolstring, as that doesn't print a message for you.

MineBill commented 7 months ago

Assuming this is the correct function:

https://github.com/lua/lua/blob/88a50ffa715483e7187c0d7d6caaf708ebacf756/lbaselib.c#L29

You can see that the standard print also calls luaL_tolstring, which will call the __tostring metamethod.

I guess this is a question of which one mani should use. Both options can be valid and correct so ultimately i think this is something that needs to be configurable in some way:

DragosPopse commented 7 months ago

Yeah, you can change it on your side if it fits your usecase better, but i'm still a bit iffy about supporting it officially. Technically speaking, the "correct" definition (not that it will work right now) for a print alternative is print :: proc(arg: any), and then the conversion would be done on the print side. Thats why the builtin one works, because it accepts anything, not specifically a string. Having string as param and requiring an explicit tostring seems like the correct thing, and the solution to the real problem would be to have better conversion between the generic lua types and odin (probably via any)

I do see your point, i'll play a bit with accepting that option too and see what happens

MineBill commented 7 months ago

Hmm, i guess you are correct. A better solution would be to support any.