teal-language / tl

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

Metamethod type not checked in metatable definition #633

Open bjornbm opened 1 year ago

bjornbm commented 1 year ago

Hi, is the below code supposed to pass the typechecker? (The type of __add in rec_mt does not have the correct type for metamethod __add in Rec.)

local type Rec = record
   metamethod __add: function(Rec, Rec): Rec
end

local rec_mt: metatable<Rec>
rec_mt = {
   __add = function(_a: Rec, _b: number): number  -- Type error??
      return 1
   end,
}

local r: Rec = setmetatable({ }, rec_mt)
print(r + r)      -- prints 1

What about this one? (__add is not defined in rec_mt)

local type Rec = record
   metamethod __add: function(Rec, Rec): Rec
end

local rec_mt: metatable<Rec> = {}  -- __add is not defined

local r: Rec = setmetatable({ }, rec_mt)
hishamhm commented 1 year ago

The second case (__add is not defined in rec_mt) is unlikely to be supported soon (the compiler currently does not do nil checks in general), but the first one sounds like it should have been checked — (note to self: this may require some adding custom code for checking setmetatable arguments, similarly to how we handle pcall, etc.).

bjornbm commented 1 year ago

OK, so my understanding for the second case: the record definition defines what keys/values are valid, but not that they are defined (or more generally perhaps the values may be nil, since nil is a valid value of every type). What is checked is that values for the defined keys have the right type, and that no other keys are added to the record.

bjornbm commented 1 year ago

For the first case, I guess the safest place to check is at setmetatable.

local r: Rec = setmetatable({ }, rec_mt)

I assume the inferred type of { } here is Rec, thanks to the assignment to r: Rec? And thus rec_mt must be metatable<Rec>, which in turn should satisfy the metamethods of Rec?

You could (should?) also check at the assignment to rec_mt that it satisfies the metamethods of Rec, since rec_mt is declared as a metatable<Rec>.

Currently it seems the metatable<Rec> type does nothing at all:

  1. The contents of rec_mt are not checked versus Recs metamethods.
  2. setmetatable does not check that rec_mt is a metatable<Rec> (it will accept any table, including of type metatable<SomeOtherType>).
svermeulen commented 1 month ago

I keep getting a similar problem:

local record Foo
   metamethod __add: function(Foo, Foo): Foo
end

local record Bar
end

local foo:Foo
local bar:Bar

-- Teal allows this
local result:any = foo + bar

print(result)

This only results in runtime errors but would be great if it was a compiler error instead. I would expect the add method to not be allowed since Foo explicitly requires Foo type for it

hishamhm commented 1 month ago

@svermeulen This looks like a different issue. Could you open a separate ticket? Thanks!!

Edit: also, I tried your example above in v0.15.3, master and next, and all three branches reported

========================================
1 error:
633.tl:12:26: argument 2: Bar is not a Foo

...so I assume you typed this from memory. But if you could come up with a minimal repro that triggers the problem you're facing at operator evaluation, that would help a lot! But let's do it in a separate issue to avoid burying the original topic of this one. Thanks!!

svermeulen commented 1 month ago

@hishamhm I upgraded from 0.15.2+dev to 0.15.3+dev and it is now being detected properly. Thank you!