[wip] test next-compat-api branch for tl
#46
Conversation
|
This does pass CI but it's not there yet! I'm still fixing up this branch locally to make |
Should we do this, or read it from the config? How to do that from this function?
...so that Cyan can type-check itself
hishamhm
force-pushed
the
test-teal-next
branch
from
c2b5308
to
71b7d32
Compare
|
@euclidianAce Please give this branch a look! Using this and |
|
(for clarity: this is marked as draft because this is not intended to merge as-is, but it is ready for review) |
|
Also pinging @vlaaad from https://github.com/defold/extension-teal since that also uses Cyan — using branches |
There was a problem hiding this comment.
Looks good to me 👍. I have a local branch with some similar (albeit messy) changes to get cyan running with the next branch. I've left some general comments more as reminders for myself and questions rather than things that need to be fixed. Thanks for taking the time to do this!
| local type Node = tl.Node | ||
| local type Token = tl.Token |
There was a problem hiding this comment.
Are these types going to be exposed? That will definitely help since some of this module's purpose is to essentially launder the types from tl into something usable
There was a problem hiding this comment.
Now that we have interfaces I tried something different, which worked: I exposed Node as an empty interface, which I then specialized within the tl module. In a structural type system, an empty interface like interface{} in Go means "accept anything". In a nominal type system, an empty interface like that means essentially an abstract data type. This allows you to declare Node variables and pass them around, without having to expose the internals of the structure.
At first I did that for both Node and Token, but after doing a bit of cleanup in the Token type, I decided keep Node abstract but expose the Token internals, since you were using them and those have been stable for literally years now.
There was a problem hiding this comment.
Ah, it's important to note that using these exposed types is an API change, so if we want to make Cyan next-ready but not yet next-mandatory for users, we might not want to have this bit in just yet.
Since you ship the generated .lua, even if Cyan depends on next to build itself (e.g. using arity=on and ? annotations), you could keep the tool itself master-compatible as long as you don't depend on API changes -- I tried to make it possible for one to run next with 0.15.x-compatible APIs (but I don't know if I succeeded fully).
There was a problem hiding this comment.
Since you ship the generated .lua, even if Cyan depends on next to build itself (e.g. using arity=on and ? annotations), you could keep the tool itself master-compatible as long as you don't depend on API changes -- I tried to make it possible for one to run next with 0.15.x-compatible APIs (but I don't know if I succeeded fully).
I've been thinking about this and I think I was overcomplicating things. It would probably be easiest if we just sync up so that we release a new Teal and a new Cyan roughly at the same time, so you could just have a next-compatible branch which would become the next Cyan once the next Teal is out (I'm targeting the end of the month, but can postpone if you need more time -- having Cyan ready would be especially important this time around because tl build will be gone for good). How does that sound?
| @@ -21,15 +21,16 @@ jobs: | |||
| - uses: actions/checkout@main | |||
|
|
|||
| - name: Install Lua | |||
| uses: leafo/gh-actions-lua@v10.0.0 | |||
| uses: hishamhm/gh-actions-lua@master | |||
There was a problem hiding this comment.
Your fork has some amount of windows support right? I've also been working on porting some of the test utilities to teal and with that attempting to make them windows compatible. Getting them running on CI would be nice.
There was a problem hiding this comment.
Your fork has some amount of windows support right?
Yes, but it's slightly tricky atm. For PUC Lua, it builds using MSVC (and needs an extra action to be setup to load the toolchain); for LuaJIT it builds using GCC (and the MSVC action must not be present otherwise LuaRocks get confused).
Some examples:
- Teal repo - runs Windows with LuaJIT only
- hishamhm/gh-actions-luarocks repo - note the conditional in the
ilammy/msvc-dev-cmdaction to disable it for LuaJIT
| @@ -0,0 +1,111 @@ | |||
|
|
|||
There was a problem hiding this comment.
Probably should vendor and/or auto-fetch these definition files.
I have a script laying around to fetch from teal-types that I can add to the pre-build
There was a problem hiding this comment.
I've been thinking that in an ideal world we should have some sort of automated teal-types integration somewhere... I just don't know if it should live in cyan or in luarocks! Any thoughts?
There was a problem hiding this comment.
To me this is essentially fetching a dependency, which leans towards luarocks. But I think having a simple feature in cyan¹ would be a good compromise until luarocks gets more teal friendly. Ideally, I think it would be wise for cyan to depend on luarocks in the future so the base functionality of fetching type definitions could be done in luarocks, then cyan could build on that primitive.
¹: something like cyan build --fetch-types to just enumerate any unresolved requires in the build and query against some sort of manifest that tells what teal-types has.
There was a problem hiding this comment.
Ideally, I think it would be wise for cyan to depend on luarocks in the future so the base functionality of fetching type definitions could be done in luarocks, then cyan could build on that primitive.
That makes sense. I think there's a case for a dependency both ways: both what you described, and https://github.com/luarocks/luarocks-build-cyan for the opposite direction.
|
|
||
| warning_error = { "unused", "redeclaration" }, | ||
|
|
||
| gen_compat = "required", | ||
| feat_arity = "off", |
There was a problem hiding this comment.
I was able to get cyan building with arity on with little changes. I'll have to clean them up and merge with these.
There was a problem hiding this comment.
I wanted to test feat_arity = "off" as a test case, because pretty much all existing Teal code out there will need it on day 1 at least once next ships.
I was able to get cyan building with arity on with little changes.
That's encouraging to hear, and I thought that would be the case! But we just need to make sure we support both modes in the tooling.
Perhaps we should add some way for cyan to ask Teal what are the supported feat flags? (Thinking that I'll probably want to add more as time goes by) -- any suggestions on what would be a good API for that?
|
@euclidianAce Thank you for going through this! Your feedback gave me confidence to merge |
|
I've pushed my local branch |
No description provided.