New Linter rule: MisleadingCondition

Analysis/src/Linter.cpp

@@ -3514,6 +3514,129 @@ struct LintRedundantNativeAttribute : AstVisitor
     }
 };
 
+class LintMisleadingCondition : AstVisitor
+{
+public:
+    LUAU_NOINLINE static void process(LintContext& context)
+    {
+        LintMisleadingCondition pass;
+        pass.context = &context;
+
+        context.root->visit(&pass);
+    }
+
+private:
+    LintContext* context;
+
+    bool checkCondition(AstExpr* cond, const char ** msg, bool * negated)
+    {
+        *negated = false;
+        if (const auto* unary = cond->as<AstExprUnary>())
+            if (unary-> op == AstExprUnary::Not)
+            {
+                *negated = true;
+                cond = unary->expr;
+            }
+
+        const std::optional<TypeId> type = context->getType(cond);
+        if (!type)
+            return true;
+        if (isBoolean(*type))
+            return true;  // boolean is a valid condition
+        if (isOptional(*type))
+            return true;  // anything that can be nil is a valid condition
+        // Several unit tests fail without this check, but only on the old solver:
+        // UnreachableCodeIfMerge, UnreachableCodeErrorReturnSilent, UnreachableCodeErrorReturnSilent,UnreachableCodeErrorReturnNonSilentBranch, UnreachableCodeErrorReturnPropagate
+        // They all test an untyped function argument
+        if (FFlag::LuauSolverV2 && get<GenericType>(follow(type)))
+            return true;
+        //auto id = get<TypeId>(follow(type));
+        //auto tid = type.value()->ty.getTypeId<>()
+        // Unions of X | boolean are technically falsifiable, but they seem wrong for other reasons, so I don't let them pass
+
+        if (isNumber(*type))
+        {
+            *msg = *negated ? "(not num) is always false; did you mean (num == 0)?" : "(num) is always true; did you mean (num ~= 0)?";
+            if (const auto* call = cond->as<AstExprCall>())
+                if (const auto* func = call->func->as<AstExprIndexName>())
+                    if (const auto* global = func->expr->as<AstExprGlobal>())
+                        if (strcmp(global->name.value, "bit32") == 0)
+                            if (strcmp(func->index.value, "band") == 0)
+                                *msg = *negated ? "(not bit32.band(X, Y)) is always false; did you mean (not bit32.btest(X, Y))?" :
+                                "(bit32.band(X, Y)) is always true; did you mean (bit32.btest(X, Y))?";
+            return false;
+        }
+        if (isString(*type))
+        {
+            *msg = *negated ? "(not str) is always false; did you mean (str == \"\")?" : "(str) is always true; did you mean (str ~= \"\")?";
+            return false;
+        }
+        if (getTableType(*type))
+        {
+            *msg = *negated ? "(not tbl) is always false; did you mean (next(tbl) == nil)?" : "(tbl) is always true; did you mean (next(tbl) ~= nil)?";
+            return false;  // just nil is weird enough that it's probably intentional. Also it would req
+        }
+
+        *msg = *negated ? "condition is always false" : "condition is always true";
+        return false;
+    }
+
+    bool visit(AstStatIf* node) override
+    {
+        const char * msg;
+        bool negated;
+        if (!checkCondition(node->condition, &msg, &negated))
+            emitWarning(*context, LintWarning::Code_MisleadingCondition, node->location, "%s", msg);
+
+        return true;
+    }
+
+    bool visit(AstExprIfElse* node) override
+    {
+        const char * msg;
+        bool negated;
+        if (!checkCondition(node->condition, &msg, &negated))
+            emitWarning(*context, LintWarning::Code_MisleadingCondition, node->location, "%s", msg);
+
+        return true;
+    }
+
+    bool visit(AstExprBinary* node) override
+    {
+        const char * msg;
+        bool negated;
+
+        if (node->op == AstExprBinary::Or)
+        {
+            if (!checkCondition(node->left, &msg, &negated))
+                emitWarning(
+                    *context,
+                    LintWarning::Code_MisleadingCondition,
+                    node->location,
+                    "The or expression %s the right side because %s",
+                    negated ? "always evaluates to" : "never evaluates",
+                    msg
+                );
+        }
+        else if (node->op == AstExprBinary::And)
+        {
+            if (!checkCondition(node->left, &msg, &negated))
+                if (!node->left->as<AstExprCall>()) // silence "func() and ..." due to it's idiomatic usage for side-effects
+                    emitWarning(
+                        *context,
+                        LintWarning::Code_MisleadingCondition,
+                        node->location,
+                        "The and expression %s the right side because %s",
+                        negated ? "never evaluates" : "always evaluates to",
+                        msg
+                    );
+        }
+
+        return true;
+    }
+};
+
+
 std::vector<LintWarning> lint(
     AstStat* root,
     const AstNameTable& names,
@@ -3612,6 +3735,9 @@ std::vector<LintWarning> lint(
             LintRedundantNativeAttribute::process(context);
     }
 
+    if (context.warningEnabled(LintWarning::Code_MisleadingCondition))
+        LintMisleadingCondition::process(context);
+
     std::sort(context.result.begin(), context.result.end(), WarningComparator());
 
     return context.result;

Config/include/Luau/LinterConfig.h

@@ -50,6 +50,7 @@ struct LintWarning
         Code_IntegerParsing = 27,
         Code_ComparisonPrecedence = 28,
         Code_RedundantNativeAttribute = 29,
+        Code_MisleadingCondition = 30,
 
         Code__Count
     };
@@ -117,6 +118,7 @@ inline constexpr const char* kWarningNames[] = {
     "IntegerParsing",
     "ComparisonPrecedence",
     "RedundantNativeAttribute",
+    "MisleadingCondition",
 };
 // clang-format on
 

tests/Linter.test.cpp

@@ -19,7 +19,7 @@ TEST_CASE_FIXTURE(Fixture, "CleanCode")
 {
     LintResult result = lint(R"(
 function fib(n)
-    return n < 2 and 1 or fib(n-1) + fib(n-2)
+    return n < 2 and true or fib(n-1) + fib(n-2)
 end
 
 )");
@@ -371,7 +371,7 @@ TEST_CASE_FIXTURE(Fixture, "LocalUnused")
 local arg = 6
 
 local function bar()
-    local arg = 5
+    local arg = true
     local blarg = 6
     if arg then
         blarg = 42
@@ -2240,7 +2240,7 @@ end
 
 TEST_CASE_FIXTURE(Fixture, "DuplicateConditions")
 {
-    LintResult result = lint(R"(
+    LintResult result = lint(R"(--!nolint MisleadingCondition
 if true then
 elseif false then
 elseif true then -- duplicate
@@ -2258,7 +2258,7 @@ _ = true or true
 _ = (true and false) and true
 _ = (true and true) and true
 _ = (true and true) or true
-_ = (true and false) and (42 and false)
+_ = (true and false) and (42 and false) -- also fails MisleadingCondition
 
 _ = true and true or false -- no warning since this is is a common pattern used as a ternary replacement
 
@@ -2320,10 +2320,11 @@ return foo, moo, a1, a2
 TEST_CASE_FIXTURE(Fixture, "MisleadingAndOr")
 {
     LintResult result = lint(R"(
+--!nolint MisleadingCondition
 _ = math.random() < 0.5 and true or 42
 _ = math.random() < 0.5 and false or 42 -- misleading
 _ = math.random() < 0.5 and nil or 42 -- misleading
-_ = math.random() < 0.5 and 0 or 42
+_ = math.random() < 0.5 and 0 or 42 -- fails MisleadingCondition
 _ = (math.random() < 0.5 and false) or 42 -- currently ignored
 )");
 
@@ -2379,14 +2380,15 @@ TEST_CASE_FIXTURE(Fixture, "WrongCommentMuteSelf")
 TEST_CASE_FIXTURE(Fixture, "DuplicateConditionsIfStatAndExpr")
 {
     LintResult result = lint(R"(
-if if 1 then 2 else 3 then
-elseif if 1 then 2 else 3 then
-elseif if 0 then 5 else 4 then
+a = true; b = true; c = false; d = true; e = false; f = true
+if if b then c else d then
+elseif if b then c else d then
+elseif if a then f else e then
 end
 )");
 
     REQUIRE(1 == result.warnings.size());
-    CHECK_EQ(result.warnings[0].text, "Condition has already been checked on line 2");
+    CHECK_EQ(result.warnings[0].text, "Condition has already been checked on line 3");
 }
 
 TEST_CASE_FIXTURE(Fixture, "WrongCommentOptimize")
@@ -2562,4 +2564,49 @@ a<<"hi">>("hi")
     REQUIRE(0 == result.warnings.size());
 }
 
+TEST_CASE_FIXTURE(Fixture, "MisleadingCondition")
+{
+    LintResult result = lint(R"(
+bit32 = { -- linter unit test does not load builtins
+    band = function(...number): number return 0; end
+}
+local function foo(): number return 5; end
+local str: string = ""
+local strNil: string? = ""
+local num: number = 0
+local numNil: number? = 0
+
+-- warn about these
+if num then end
+if not num then end
+local _ = if bit32.band(6, 3) then 1 else 0
+local _ = if not bit32.band(6, 3) then 1 else 0
+local _ = "" or true
+local _ = not str or true
+local _ = {} and true
+local _ = not {} and true
+local _ = foo() and true -- silent because it may be using foo() for side effects
+
+-- don't warn about these
+if true then end
+if not true then end
+local _ = if nil then 1 else 0
+local _ = if not nil then 1 else 0
+local _ = strNil or true
+local _ = not strNil or true
+local _ = numNil and true
+local _ = not numNil and true
+)");
+
+    REQUIRE(8 == result.warnings.size());
+    CHECK_EQ(result.warnings[0].text, R"((num) is always true; did you mean (num ~= 0)?)");
+    CHECK_EQ(result.warnings[1].text, R"((not num) is always false; did you mean (num == 0)?)");
+    CHECK_EQ(result.warnings[2].text, R"((bit32.band(X, Y)) is always true; did you mean (bit32.btest(X, Y))?)");
+    CHECK_EQ(result.warnings[3].text, R"((not bit32.band(X, Y)) is always false; did you mean (not bit32.btest(X, Y))?)");
+    CHECK_EQ(result.warnings[4].text, R"(The or expression never evaluates the right side because (str) is always true; did you mean (str ~= "")?)");
+    CHECK_EQ(result.warnings[5].text, R"(The or expression always evaluates to the right side because (not str) is always false; did you mean (str == "")?)");
+    CHECK_EQ(result.warnings[6].text, R"(The and expression always evaluates to the right side because (tbl) is always true; did you mean (next(tbl) ~= nil)?)");
+    CHECK_EQ(result.warnings[7].text, R"(The and expression never evaluates the right side because (not tbl) is always false; did you mean (next(tbl) == nil)?)");
+}
+
 TEST_SUITE_END();