teal-language / tl

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

Functions that take {integer:string} parameters unexpectedly accept {string} #557

Open benbatt opened 2 years ago

benbatt commented 2 years ago

Not sure if this is intended behaviour or not, but in my project it caused issues with code similar to the following, where the array was being corrupted due to the wrong remove function being called.

local function removeValueFromMap<K, V>(map: {K:V}, value: V)
  for k,v in pairs(map) do
    if v == value then
      map[k] = nil
      return
    end
  end
end

local function removeValueFromIntegerStringMap(map: {integer:string}, value: string)
  for k,v in pairs(map) do
    if v == value then
      map[k] = nil
      return
    end
  end
end

local function removeValueFromArray<T>(array: {T}, value: T)
  for i,v in ipairs(array) do
    if v == value then
      table.remove(array, i)
      return
    end
  end
end

local stringIntegerMap: {string:integer} = {one = 1, two = 2, three = 3}
local integerStringMap: {integer:string} = {[2] = "two", [5] = "five", [7] = "seven"}
local array: {string} = {"one", "two", "three"}

removeValueFromMap(stringIntegerMap, 2)
removeValueFromIntegerStringMap(stringIntegerMap, 2) -- error: argument 1: in map key: got integer, expected string
removeValueFromArray(stringIntegerMap, 2) -- error: argument 1: in map key: got string, expected integer

removeValueFromMap(integerStringMap, "two")
removeValueFromIntegerStringMap(integerStringMap, "two")
removeValueFromArray(integerStringMap, "two") -- expected an error here

removeValueFromMap(array, "two") -- expected an error here
removeValueFromIntegerStringMap(array, "two") -- expected an error here
removeValueFromArray(array, "two")
hishamhm commented 2 years ago

Yeah, this behavior is not accidental, there is code in the type checker that explicitly accepts an array {T} as a {integer:T} map. My original rationale is that people coming from Lua would expect that compatibility given the structural similarity (arrays being tables with integer keys). Here is an example where another user passed an array to a map with integer keys, and here is another (I found both examples because they ended up in the Teal test suite as regression tests).

An interesting observation is that I found these two examples which rely on "{T} is a {integer:T}", but I found no examples that rely on "{integer:T} is a {T}" (and currently both are accepted in Teal).

There are also similar type rules for structurally comparing arrays and maps to records: if all entries of a record are of type T, then that record is accepted as a {string:T} map.

Whether these structural equivalences should be accepted or rejected, I personally don't lean strongly either way — I think no Teal code I wrote myself makes these assumptions — but I'd like to hear what others think, especially now since we've had both positive and negative feedback about them (positive in the form of existing examples which use it, negative in the form of this issue).

lenscas commented 2 years ago

I'm all for Teal wanting people to be more explicit when it comes to converting types, however then there should be an easy and safe way to convert them as well.

Safe, in this case meaning that the types are at least structurally compatible.

if this safe way to do conversions exists, then I would also like it if as gets renamed to both better reflect that it doesn't do any checks (and is thus "unsafe") and to make it more annoying to use to push people to use the safe conversion instead

tooolbox commented 2 years ago

if this safe way to do conversions exists, then I would also like it if as gets renamed to both better reflect that it doesn't do any checks (and is thus "unsafe") and to make it more annoying to use to push people to use the safe conversion instead

How about as! for the unsafe version?

Only partly joking.

lenscas commented 1 year ago

How about as! for the unsafe version?

What about transmute! ? Coming from Rust, as sounds pretty similar to transmute its just that in Rust's case, you get UB if you transmute something to another type it isn't compatible with so it is a bit more dangerous.

Then the new "safe" conversion can just be as which people already know from TS and also Rust.

I don't mind the ! for keywords that are "dangerous" so to speak. Makes them stand out just a bit more.

tooolbox commented 1 year ago

The as! is cribbed from Swift. (It has as, as? and as! which all behave a little differently.) transmute! is rather a mouthful.

lenscas commented 1 year ago

transmute! is rather a mouthful.

Considering the point of it is to totally sidestep the type system by doing what current day as is doing and have a new version of as that has some checks in place to make sure the conversion at least kinda makes sense, transmute! being a mouthful is kinda the point :P