luau-lang / luau

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

Incorrect type widening with multiple arguments to generic `T` with new solver #1293

Open Ketasaja opened 3 months ago

Ketasaja commented 3 months ago

Tested on v0.629, with no errors in strict or non-strict mode:

--!strict
local t: {string} = {}
table.insert(t, true)
aatxe commented 3 months ago

I'm not sure that I'd necessarily call this a bug. There does exist a type to provide for t such that this typechecks: number | boolean.

Ketasaja commented 3 months ago

I'm not sure that I'd necessarily call this a bug. There does exist a type to provide for t such that this typechecks: number | boolean.

You're right, I don't have label changing permissions though.

My usecase is that the ECS library I use has world:set(id, Position, Vector3.new(0, 0, 0)), where Position is ecr.component() :: Vector3.

The advantage over function overloads is that there's no need to define components upfront, which makes it easy for other libraries to define them.

Maybe if other users have similar usecases it could be worth considering delaying this inference change in strict mode if generic constraints are a long way away?

Ketasaja commented 3 months ago

More common example:

--!strict
local t: {string} = {}
table.insert(t, 1)
aatxe commented 3 months ago

I don't think there's an immediately obvious way for us to "delay" this change, but if the type of one of the fields is already required to be some particular type (e.g. in that table.insert example, or if your Position is cast to Vector3 or annotated Vector3), then I don't think we should be widening the type to include additional components. It's really only in the case of examples like your original one where I think the behavior is expected. So, there is probably still a bug here, but one more specific than your initial report.