teal-language / tl

The compiler for Teal, a typed dialect of Lua
MIT License
2.11k stars 109 forks source link

Introduce 5.4 target with support for <close> #506

Closed euclidianAce closed 2 years ago

euclidianAce commented 2 years ago

All that is left is to add tests to verify that the above isn't a lie :)

github-actions[bot] commented 2 years ago

Teal Playground URL: https://506--teal-playground.netlify.app

hishamhm commented 2 years ago

Cross-referencing https://github.com/teal-language/tl/issues/252

The "Teal dialects" issue still crosses my mind... right now I'm wondering if it would make sense to have limited support for Lua versions < 5.4.

I'm not saying this is a requirement for this PR, but I'm going to jot some notes on how something like this could be done:

Essentially, that would make <close> work for LuaJIT and Lua < 5.4, except for error handling, where closing responsibilities would still be subject to the GC finalizer (as is the case for coroutines that are not resumed, even in 5.4).

euclidianAce commented 2 years ago

Just expanding on some stuff as I'm writing the code to validate this stuff:

emit explicit getmetatable(v).__close(v) calls for all <close> variables in scope before each break and return statement

And any goto that leaves the block ;) (and check for nil since local x <close> is a valid Lua 5.4 program)

Essentially, that would make work for LuaJIT and Lua < 5.4, except for error handling

To add to this, one big concern with errors is that __close metamethods are passed the error object in the case of an error. And errors in __close are protected against, making it very difficult to emulate.

But looking pragmatically, if you're relying on compatibility code being generated, you probably are just gonna copy your (hopefully idempotent) __gc method to __close as well? And destructors are such a nice feature to have I don't think people will mind that the behavior isn't 1-to-1 with 5.4. Just being able to do stuff like:

local function read_file(name: string): string, string
   local f <close>, err <const> = io.open(name, "r")
   if not f then return nil, err end
   return f:read("a")
end

(Writing that snippet made me finally realize why nil is a valid <close> value :laughing:)

Personally my take (as someone that deals with different versions of Lua in different projects) is that I am totally fine with locking features behind --gen-target flags, such as <close>, goto, etc. It could even help with the situation of "I'm running 5.3 right now, but want to target the subset of features that 5.1 has"

lenscas commented 2 years ago

Personally my take (as someone that deals with different versions of Lua in different projects) is that I am totally fine with locking features behind --gen-target flags, such as , goto, etc. It could even help with the situation of "I'm running 5.3 right now, but want to target the subset of features that 5.1 has"

that is how I feel about it as well, especially with luajit still being popular (which is pretty much lua 5.1). The compat library helps, but that is a C library if I remember correctly, which means that on most environments were I expect it to be needed it can't actually be used.

hishamhm commented 2 years ago

And any goto that leaves the block ;) (and check for nil since local x is a valid Lua 5.4 program)

Ah, all the flow typing analysis that is implemented already solemnly ignores goto... I didn't even bother to take it into consideration when doing a single-pass type checker. To do that properly, the right thing to do would really be to implement an IR with basic blocks, then goto would be supported for free along with while, for, if...

To add to this, one big concern with errors is that close metamethods are passed the error object in the case of an error. And errors in close are protected against, making it very difficult to emulate.

Good point! But it would still fall in the "except for error handling" caveat.

And destructors are such a nice feature to have I don't think people will mind that the behavior isn't 1-to-1 with 5.4.

Yeah, that's what made me do that whole thought exercise on how to implement it, and almost made me itchy to give it a try (perhaps after this PR is done?)

I am totally fine with locking features behind --gen-target flags, such as <close>, goto, etc. It could even help with the situation of "I'm running 5.3 right now, but want to target the subset of features that 5.1 has"

I totally get that feeling and LuaRocks is written like that, so it's probably over 10 years now that I've been walking on eggshells with that codebase to ensure everything works on every version. But my attempt with bundling compat tighly to Teal like that was really to try to get the Lua universe out of this 5.1-stasis. Especially since when we say "5.1" in 2022 we almost always mean "LuaJIT", and the "LuaJIT + Compat" combo is really much closer to 5.2+ than it is to vanilla 5.1.

that is how I feel about it as well, especially with luajit still being popular (which is pretty much lua 5.1). The compat library helps, but that is a C library if I remember correctly, which means that on most environments were I expect it to be needed it can't actually be used.

Compat is compatible with LuaJIT (which you could also call "sort of 5.1 but not really"), so if you're bundling a Lua VM written in C, you should be able to bundle it as well.

But if you're talking about things like scripting games that don't allow requiring additional modules, then yeah; but those often add their own restrictions to the standard library too (e.g. no os.execute()), so the Teal compiler just assumes a recent Lua stdlib available, and if you're compiling for an environment that doesn't have any parts of it, then it's up to you to avoid using missing functions (either because the game removed functions or because it's actually an older Lua version).

In short, I'd still rather keep the Lua-version-shenanigans away from Teal programmers as much as possible, but I'd like to find a good pragmatic middle-ground eventually.

Let's go with the approach @euclidianAce is taking in this PR now. At a later time, we could take a shot at a compat-<close> PR and see if it would be worth the effort.

hishamhm commented 2 years ago

@euclidianAce just checking back on this — since you added tests, what's missing for this to be ready for review/merge? I'm thinking of pulling in some code from #449 into master and it would be best to merge this first so that we don't end up with merge conflicts.

euclidianAce commented 2 years ago

@hishamhm From what I remember (as I haven't gotten the chance to look back at this) I wanted to basically do one final look through the code to make sure that errors were getting propagated correctly since this pr makes most if not every tl.* function fallible. So I wanted to make sure that any internal calls to these handle their errors nicely.

But after that I think it'd be good to merge

euclidianAce commented 2 years ago

After getting a chance to re-look this over, I think the places where errors are propagated make sense and think it's ready to merge (or at least be reviewed for merging :P)

hishamhm commented 2 years ago

I had already reviewed the major code changes before, so it already L'ed GTM before the tests, so if you're happy with the PR then I'm happy to merge :)