teal-language / tl

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

New variable attribute: <total> #612

Closed hishamhm closed 1 year ago

hishamhm commented 1 year ago

The <total> attribute declares a const variable with a table where the domain of its keys are totally declared.

This check can only be applied when a literal table is given at the time of variable initialization, and only for table types with well-known finite domains: maps with enum keys, maps with boolean keys, records.

Note that the requirement is that keys are declared: they may still be explicitly declared to be nil.

Adding <total> annotations to the tl source code caught 3 bugs, one of them #611 (which inspired this PR!).

github-actions[bot] commented 1 year ago

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

bjornbm commented 1 year ago

I think I get the idea from tl.tl, but don't forget the documentation! :)

hishamhm commented 1 year ago

@bjornbm Here are the docs! — let me know if there are clear enough!!

bjornbm commented 1 year ago

@bjornbm Here are the docs! — let me know if there are clear enough!!

Thanks, very good!

  1. Are there occasions when one would want a record to not be total? (If no, should all records be implicitly total, or should it be an error to “instantiate” them without <total>?)

  2. What is the story for arrayrecords? It appears attribute <total> only applies to maps and records (error message when applying <total> to an arrayrecord instance) and cannot be used for arrayrecords? With arrayrecords documented under the records section, this is a bit confusing.

lewis6991 commented 1 year ago

I do like the idea of <total> being opt-out as opposed to opt-in. Perhaps something like <partial> makes sense to have as opt-in.

Likewise (and unrelated), It would be good if <const> is also implicit, and instead you have an opt-out with something like <mut>.

lenscas commented 1 year ago

Seems like <total> does 2 things. The most easiest to grasp is when used on a record, where it will force you to fill in the whole record (though you can explicitly give it nil if you so desire).

I think I agree with the comments above that this behaviour probably should be the default, with maybe a <partial> to escape this behaviour. I would also expect that this behaviour pretty much becomes the default once/if the typesystem starts to care about nil.

The other thing it seems to do is basically make new record types on the fly using maps with an enum as a key (local degrees <total>: {Direction:number}) here, using total should probably stay opt-in rather than opt-out.

Something else that might be interesting is to move all this to the typesystem like TS did with its Partial<T> and friends, however that can always be done later and this is already a pretty good feature as is so probably best to leave that for later.

hishamhm commented 1 year ago

Thank you all for the feedback! On to the questions:

Are there occasions when one would want a record to not be total?

Whenever you want a non-const variable, for example: <total> implies <const> (this was the cautios choice to make, makes ttribute behaviors more similar, and allows one total variable to initialize another). Also, keep in mind this is about the variable (and its declaration), not the object (and its lifetime).

should all records be implicitly total

This would lead to very boilerplate-heavy code whenever records have many optional keys, which is a common scenario. Also, many records delegate many of its member values to metatables and thus the table literal does not describe the object totally. For this reason, records with metamethods cannot be marked <total> either.

But in short the reason is what @lewis6991 alluded to: total-by-default would be akin to const-by-default, which is something I won't do — it's too much of a break from Lua, it would hamper the migration path considerably.

Seems like does 2 things.

It's one thing; the examples in the documentation might have given a wrong impression. The original intended use case was indeed for maps-with-enum-keys; support for other suitable types was added for consistency where the behavior would be equivalent.

move all this to the typesystem like TS

Yeah, this is about the variable declaration, not the object lifetime, so that would look and behave very differently (and would be a whole other can of worms, semantics and implementation wise). Let's keep in mind that TypeScript has a large type system and language definition (and a huge development team!), and I want to keep Teal smallish.

hishamhm commented 1 year ago

Another bit of feedback I'd especially like from you all: does this PR break anything? It shouldn't, but lots of code moved around lately!

If you are able to give this branch a spin on your code base and let me know if anything looks off! Any positive feedback on that regard would be very reassuring!

(The laziest way to do it would be to simply download the tl.lua file from the Files section in this PR and plop it on top of your stable version — just back it up to revert :) )

hishamhm commented 1 year ago

Ah! Missed one question!

What is the story for arrayrecords?

Arrayrecords cannot ever be total because to have a total arrayrecord you would need to have math.maxinteger fields explicitly declared. :)

pigpigyyy commented 1 year ago

Tried total-attribute branch in my projects. Nothing seems to be breaking. Expecting merging.