Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support maprecords #591

Open
uramer opened this issue Dec 5, 2022 · 5 comments
Open

Support maprecords #591

uramer opened this issue Dec 5, 2022 · 5 comments
Labels
feature request New feature or request

Comments

@uramer
Copy link

uramer commented Dec 5, 2022

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 hishamhm added the feature request New feature or request label Dec 20, 2022
hishamhm added a commit that referenced this issue Dec 26, 2022
This should make __index metamethod handling work for dot notation
as well (see #591).
hishamhm added a commit that referenced this issue Dec 26, 2022
See #591. This is not fully equivalent to a "maprecord"
because we don't yet implement support for __newindex.
But at least now the behavior of dot and index notation
are consistent.
@hishamhm
Copy link
Member

@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
Copy link
Author

uramer commented Dec 27, 2022

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
Copy link
Member

hishamhm commented Jan 3, 2023

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
Copy link
Author

uramer commented Jan 4, 2023

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants