luau-lang / luau

A fast, small, safe, gradually typed embeddable scripting language derived from Lua
https://luau.org
MIT License
4k stars 373 forks source link

Writing common property to a type union errors #533

Open JohnnyMorganz opened 2 years ago

JohnnyMorganz commented 2 years ago

If you create a type union (e.g., through a type predicate) and then attempt to assign a property which is common to all types within the union, it fails with a type error. Example:

--!strict
local inst = Instance.new("SpotLight")

if inst:IsA("SpotLight") then
    inst.Enabled = true -- OK
end

if
    inst:IsA("SpotLight")
    or inst:IsA("SurfaceLight")
    or inst:IsA("BillboardGui")
    or inst:IsA("Beam")
then
    inst.Enabled = true -- TypeError: Expected type table, got 'Beam | BillboardGui | SpotLight | SurfaceLight' instead
end
JohnnyMorganz commented 1 year ago

This also fails for type intersections, potentially related to the same issue, but may be separate?

type Foo = {
    Bar: string,
} & { Baz: number }

local x: Foo = { Bar = "1", Baz = 2 }
local y = x["Bar"] -- TypeError: Expected type table, got '{| Bar: string |} & {| Baz: number |}' instead
JohnnyMorganz commented 1 year ago

I wanted to see if it was easy enough to patch in the current type checker. It seems the getIndexTypeFromType function already reduces a union of tables into its common properties. So I thought maybe we can just use that when the LValue is a Union type (i.e., just add get<UnionType>(lhs) to the following condition: https://github.com/Roblox/luau/blob/76bea81a7b798b3324f97a72a2a8faac29089eec/Analysis/src/TypeInfer.cpp#L3348-L3359

This does indeed work and fix the issue. However, it causes the "tagged_unions_immutable_tag" test case to (quite rightly) fail: https://github.com/Roblox/luau/blob/76bea81a7b798b3324f97a72a2a8faac29089eec/tests/TypeInfer.singletons.test.cpp#L210-L221

Now, this seems a bit trickier to resolve, and I am guessing where the meat of the issue lies.

In real-world code, I mostly see this error when using ClassTypes (typically Roblox Instances), which, if I am not mistaken, don't have any "tagged union" effect. Would it be feasible to half-fix this only for a union of ClassTypes? That would resolve most of the false positives I see

vegorov-rbx commented 1 year ago

This also fails for type intersections, potentially related to the same issue, but may be separate?

I want to note that this is fixed in the new typechecker, hope that it will be ready soon. Although there are still some issues in original example (it works as is, but there are hidden refinement issues).