[RESOLVED] Major regressions with most recent type system update

[DataBrain (Amber)]

EDIT: Looks like this change just got reverted, thanks to Apakovtac

I re-opened Team Create after a crash, only to find 108 warnings in Script Analysis for… completely benign code that works around previous limitations of luau’s type system, which now can’t even be worked around in a good way at all now. I’m mostly going to address this most annoying Narrowing for falst values will always evaluate to false warning, although there are also other warnings that are breaking otherwise benign code. This change should be reverted overall.

Now allow me to write a dissertation on why this “Narrowing for falsy values” is not only unsound, unproductive, and is halting me from getting lots of work done today (and potentially multiple days if this doesn’t get addressed soon).

Why is narrowing for (statically) falsy values useful?

Here are some very practical and valid reasons to narrow for falsy values:

Record types

In Luau, you can’t type a record type with optionally-typed values. Why not? For one thing, it’s bad and inconsistent with a lot of the internal typings for roblox types. An example of this is instance:GetAttributes() which, although currently untyped, would usually return a value typed as {[string]: any} (Or {[string]: any?} if we used optionally-typed values)

-- Both are valid ways to type this
local attrs = workspace:GetAttributes() :: {[string]: any}`
local attrs = workspace:GetAttributes() :: {[string]: any?}`

The problem with optionally-typed values here is application-dependent. However, one very practical case that breaks here is iteration through such a record with optionally-typed values:

local attrs = workspace:GetAttributes() :: {[string]: any?}
for attrName, value in pairs(attrs) do
    print(attrName, value)
end

For the sake of example, let’s say we have an object, which we know a-priori that all of the attribute values will be numbers (and there’s enough abstraction/encapsulation to guarantee this). Then, we can get a sum of attributes as follows:

local sumHolder = workspace :: Instance -- Could be any object
local attrs = sumHolder:GetAttributes() :: {[string]: number}
local sum = 0
for attrName, value in pairs(attrs) do
    sum = sum + value
end

So far so good… Until we type the values in this record as having optional values:

local sumHolder = workspace :: Instance -- Could be any object
local attrs = sumHolder:GetAttributes() :: {[string]: number?}
local sum = 0
for attrName, value in pairs(attrs) do
    sum = sum + value
end

Now I know this is an issue that can be fixed in the engine, but more to the point… right now my entire codebase is working around this limitation by never using optional types in records. For example:

local playerDataMap = {} :: {[Player]: {Coins = 1}}

In this case, we have a hash map from player instances to player data… This might not always be guaranteed to exist, so we check for truthiness of their data as the simplest way to see if their data has loaded in. This is especially going to matter if SignalBehavior is set to Deferred and race conditions can happen.

local playerDataMap = {} :: {[Player]: {Coins: number}}

--...

local function PlayerCanPurchaseItem(player: Player, cost: number)
    local playerData = playerDataMap[player]
    if not playerData then return false end
    
    return playerData.Coins >= cost
end

So right now I’m left with a catch 22 situation. I can’t type my values as optional because of the iteration issue. And I can’t type my values as non-optional because of this stupid opinionated warning.

But wait, it gets worse

Assuming you don’t iterate, it’s not even sound to do a falsiness check here according to the script analysis!

But wait, can’t you just do a truthiness check in the loop

I mean yeah, if you want to sacrifice an indentation level and performance at each step in the loop at runtime for no reason, I guess you could.

local playerDataMap = {} :: {[Player]: {Coins: number}?}

for player, data in pairs(playerDataMap) do
	if data then
		print(player, data.Coins)
	end
end

But truthiness checks are not sound for boolean values.

local playerIsSubscribedMap = {} :: {[Player]: boolean?}

for player, isSubscribed in pairs(playerIsSubscribedMap) do
	if isSubscribed then
		print(player, isSubscribed)
	end
end

This will always print true, since isSubscribed catches the case of both nil and false. So we would have to refine with an '~= nil` statement… which doesn’t work according to script analysis

local playerIsSubscribedMap = {} :: {[Player]: boolean?}

for player, isSubscribed in pairs(playerIsSubscribedMap) do
	if isSubscribed ~= nil then
		local x: boolean = isSubscribed
	end
end

Please don’t enforce opinions like this. I understand that I run the risk of having redundant falsiness/truthiness checks. At the end of the day, the runtime code matters more than the types, and roblox is just not built for type guards as it stands. Right now, I can’t even work around it.

Other regressions that came with this update:

Types do get refined through this falsiness check sometimes, but then assignment messes up

local couldNotExist: string? = 'Foo'

if not couldNotExist then
    couldNotExist = 'Fighters'
end

Even though the type gets refined to “nil” in the if not couldNotExist then block, that doesn’t mean the whole type of the variable should change when assigning a value to it. This kind of pattern is actually a very valid and common pattern.

This update needs to be reverted and thought through a little bit more.

[zeuxcg]

Thanks! We’ve reverted this and will fix the problems you’ve raised before reenabling this - this update came with a complete rework of our refinements in the type system which is needed to make many conditions that type checker currently doesn’t recognize work, but since it’s a complete rework it’s difficult to make sure that all prior code is compatible - this feedback helps us identify cases we’ve previously missed.