diff --git a/Analysis/src/Linter.cpp b/Analysis/src/Linter.cpp index 328ae14f3..29184b587 100644 --- a/Analysis/src/Linter.cpp +++ b/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()) + if (unary-> op == AstExprUnary::Not) + { + *negated = true; + cond = unary->expr; + } + + const std::optional 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(follow(type))) + return true; + //auto id = get(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()) + if (const auto* func = call->func->as()) + if (const auto* global = func->expr->as()) + 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()) // 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 lint( AstStat* root, const AstNameTable& names, @@ -3612,6 +3735,9 @@ std::vector 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; diff --git a/Config/include/Luau/LinterConfig.h b/Config/include/Luau/LinterConfig.h index 6926ef2e0..10e5e00e1 100644 --- a/Config/include/Luau/LinterConfig.h +++ b/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 diff --git a/tests/Linter.test.cpp b/tests/Linter.test.cpp index df509909d..6ac078735 100644 --- a/tests/Linter.test.cpp +++ b/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();