teal-language / tl

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

Support maprecords #591

Open uramer opened 1 year ago

uramer commented 1 year ago

It would be more consistent to allow records to also be maps, not just arrays. It's a common Lua pattern to have a map which also has added (meta) fields on top.

In particular, something like this is very handy: a map, where some values are of known type, but it could have other arbitrary values too

local record Settings end
local record UI end
local record Package
  {string: any} -- any unknown field might exist, but could be any type or nil
  Settings: Settings
  UI: UI
  -- etc
end

Currently the best alternative I can think of is adding metamethod __index: function(self: Package, key: string): any.
That's not terrible, but doesn't support the . syntax (yet?), only the square brackets.

hishamhm commented 1 year ago

@uramer Thanks for the feedback! I've pushed a pair of commits that make dot-notation for __index work... that's not fully equivalent to maprecords, but it's a compiler improvement nonetheless!

uramer commented 1 year ago

Thanks, very cool. By the way, would maprecords be a reasonable first contribution to look into? And would such an MR be accepted, assuming the implementation is good?

hishamhm commented 1 year ago

I am a bit reluctant about the concept of maprecords for two reasons:

1. Conflicts between record fields and maps keys of {string:X}

Mixing records with maps-of-string-keys can be a bit of an antipattern: for example, in your code above, what happens if your package map needs to store an item that happens to be called "Settings"?

I've seen situations like these where people say "I'll never have a conflict"... until they do. And that could lead to hard-to-track bugs, because there would be no way to check at compile time that package[somevariable] = value is not overwriting a record field with an invalid value. (Dot-notation is less likely to cause problems, because it requires constant names.)

For that reason, if we were to have maprecords, I'd be tempted to support it for any key type but string at first (one could use enums instead of strings, if they know the finite universe of valid keys beforehand).

At least the __index metamethod makes the "fallback" behavior a bit more explicit, since Lua already defines that behavior and metamethods generally indicate "something potentially tricky is going on".

2. Arrayrecords should really be an intersection type

Arrayrecords were a very early addition to the language, and since we added union types it would be more consistent to support intersection types instead. Not just for arrayrecords and maprecords, but for all table types that could be safely intersected. For example:

local type MyArrayRecord = {string} & MyRecord
local type MyTupleRecord = {number, number, number} & MyRecord
local type MyArrayMap = {number} & {string:boolean}
local type MyMapMap = {string:number} & {number:string}
-- ...and so on!
-- as long as the keys don't conflict, we could intersect table types

Union types currently have a cautious restriction in which you can only add one table type per union (to avoid runtime issues when discriminating the concrete type of the value, which is done using Lua's type()). Likewise, intersection types could be implemented with a restriction that only tables whose keys don't conflict can be intersected.

uramer commented 1 year ago

arrayrecords stood out to me as weird and inconsistent as well. I guess I just went on the opposite route to making them consistent, I also had an enumrecord proposal in mind. I agree that intersection types would be significantly better though.

With your change, the __index metamethod is indeed an acceptable replacement. What do you think about deprecating arrayrecords? Since the same replacement is available for them. They don't have the name collision issue you mentioned, but their syntax is still awkward.

Btw, since you've mentioned unions. Teal really should allow arbitrary unions, even if the is operator can't distinguish the types. We just need a way to declare custom type guards, like in typescript. IMO the current restriction should only apply to the is operator, and not to union types themselves

lewis6991 commented 1 year ago

For that reason, if we were to have maprecords, I'd be tempted to support it for any key type but string at first (one could use enums instead of strings, if they know the finite universe of valid keys beforehand).

I like this idea a lot. I've tried to type a lot of existing Lua code and I think this would make that easier. I would especially like an integer key type (as {integer:T} !== {T}). Or on a less related note, allow records with integer named fields that essentially act like tuples but with the ability to add methods and other string fields.

local record R
 [1]: boolean
 [2]: string
 field: string
end