Faulty Script Analysis Warning, "Condition has already been checked"

Bug Reports Studio Bugs
#1

Paste this into a new script in a blank place:

local value = true and 5
print(value and value >= 0 and value)

(The original code had variable expressions instead of true and 5 and was returning the value instead of printing it; the point is that value could be a falsy value or a numeric one.)

The Script Analysis window will display the warning -- W024: (2,32) Condition has already been checked on column 7

This warning is incorrect because just saying value and value >= 0 returns a boolean (or nil) whereas value and value >= 0 and value has the potential to return a number as well. The initial value and check is required in case value is nil, since nil >= 0 is an error.

Similar code that doesn’t emit the warning:

local value = true and 5
if value and value >= 0 then
	print(value)
end
print(false)

This warning is present regardless of which Betas you’re in.

1 Like
#3

While this is technically true, it’s not clear how we can change this analysis to be silent on this case. I’d recommend making the code a bit more explicit and clear like this:

print(value ~= nil and value >= 0 and value)
2 Likes
#4

The code you proposed will produce a run-time error if this pattern is used when value could be either false or nil or a table/Instance when you need to access something in value:

local value = false -- or an arbitrary Instance
print(value and value.Parent and value) -- works
print(value ~= nil and value.Parent and value) -- attempt to index boolean with 'Parent'

You could do:
print(value ~= nil and value ~= false and value.Parent and value),
but if I came across code like that I’d immediately think "why wasn’t it written value and?

I recommend:

  1. Whenever an expression expr is used in a conditional (such as in expr and otherExpr or if/while/repeat conditions), change the analysis to interpret expr as meaning (expr ~= nil and expr ~= false).

    • Note: a common pattern is to check to make sure that an argument was provided, and strict mode currently doesn’t warn if you say if not var then (for var:string), but does warn if you say if var == nil then or if var == false then - I would suggest that this change should not produce a warning (perhaps not for any of those cases, at least until the statement after where its type is checked - I say “statement” because one might use an elseif to check different possible values for some given input - but just not warning on if not var then would suffice for most).

    • Note 2: Whenever you’re in a conditional (whether that’s in a larger expression or in an if/while/repeat condition), you can perform an extra analysis step in keeping with this point: such as in if expr and expr then, (where expr is any expression [except see #2]), which can be interpreted this way:

      • First sub-expression is the same as (expr ~= nil and expr ~= false)
      • The result of the entire expression is either the second sub-expression, expr, or false
      • If it’s expr, Lua must now check if expr ~= nil and expr ~= false then, which is a duplicate check

      In other words, any sub-expression that can be used as the result of a conditional has an implied ~= nil and ~= false check.

  2. Never warn when a function call is part of the repeated expression, since those can have side effects:

    local function tryAdvance()
	-- Try to perform some action.
	-- If successful, return true; otherwise return false
end
local function tryAdvanceTwice()
	return tryAdvance() and tryAdvance()
end

    (Technically table accesses could invoke functions via metatables but I’m pretty sure that’s considered bad practice and supporting that possibility would mean being unable to check for if self.var and self.var then)
    (Also technically, I suppose that any function call could have side-effects such that any variable’s value may have changed, such as in return value or assignToValue() and value)
    (I suppose it would be neat if, in strict mode, one could declare functions as “pure” (without side-effects), in which case calling such functions would not need to be ignored.)

  3. If an expression is known to be a boolean (and doesn’t contain a function call), performing expr and expr is always pointless (so is expr or expr); this could theoretically give a warning.

Result of #1:

print(value and value ~= nil and "Yes" or "No")

would trigger the “Condition has already been checked”
warning (ideally

print(value ~= nil and value and "Yes" or "No")

would also, though the warning would have to say value ~= nil is redundant to be clear)

1 Like