From fba48ae3e433db16dce6916a4cdcff5bc62938d4 Mon Sep 17 00:00:00 2001 From: PhoenixWhitefire <86601049+PhoenixWhitefire@users.noreply.github.com> Date: Sun, 11 Aug 2024 16:00:44 +0530 Subject: [PATCH 1/3] Add CollapseClauseToAssert lint Adds the `CollapseClauseToAssert` lint, which triggers in the following code: ``` -- `if`-clause can be collapsed to an `assert` if X then error("SomeErrorMessage") end ``` * Any number of conditions for the `if` * No `else` block * `error` cannot be given a `level` argument * Error string must be constant (no string interpolation/concatenation) to avoid slowdowns * Body must _only_ be an `error` call Inspired due to the following code on luau-lang.org: https://luau-lang.org/2023/03/31/luau-recap-march-2023.html#:~:text=if%20not%20x%20then%20error(%27first%20argument%20is%20nil%27)%20end --- Analysis/src/Linter.cpp | 88 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/Analysis/src/Linter.cpp b/Analysis/src/Linter.cpp index 23457f4c..7a3d917c 100644 --- a/Analysis/src/Linter.cpp +++ b/Analysis/src/Linter.cpp @@ -2587,6 +2587,91 @@ private: } }; +// Recommend collapsing `if/if not COND then error(STR) end` to `assert(COND, STR)` +class LintCollapseClauseToAssert : AstVisitor +{ +public: + LUAU_NOINLINE static void process(LintContext& context) + { + LintCollapseClauseToAssert pass{&context}; + context.root->visit(&pass); + } + +private: + LintContext* context; + + LintCollapseClauseToAssert(LintContext* context) + : context(context) + { + } + + bool visit(AstStatIf* stat) override + { + if (stat->elsebody) + return true; + + if (!stat->thenbody) + return true; + + // Clause contains more than just an `error()` + if (stat->thenbody->body.size > 1) + return true; + + Luau::AstArray& contents = stat->thenbody->body; + + // Avoid nullptr-dereference if there's a syntax error + if (contents.size < 1) + return true; + + Luau::AstStat* body = contents.data[0]; + + if (!body->is()) + return true; + + AstStatExpr* expr = body->as(); + + if (!expr->expr->is()) + return true; + + AstExprCall* call = expr->expr->as(); + + if (!call->func->is()) + return true; + + AstExprGlobal* global = call->func->as(); + + if (global->name == "error") + { + // `assert` does not have a 'level' parameter + + // `error` requires at-least 1 argument + if (call->args.size > 1 || call->args.size == 0) + return true; + + AstExpr* errmsg = call->args.data[0]; + + // The `if`-clause allows the VM to occasionally (and, in this case, almost always) + // avoid resolving any string operations, but `assert` (like all functions) + // forces it to resolve it's arguments every time they are called, regardless + // of if the condition actually fails. + // https://devforum.roblox.com/t/1289175 + // Don't give the suggestion in this case, we don't want to slow people's code down. + if (!errmsg->is()) + return true; + + AstExprConstantString* string = errmsg->as(); + + emitWarning( + *context, + LintWarning::Code_CollapseClauseToAssert, + stat->location, + "`if`-clause can be collapsed to an `assert`" + ); + } + + return true; + } +}; + class LintDuplicateCondition : AstVisitor { public: @@ -3389,6 +3474,9 @@ std::vector lint( if (context.warningEnabled(LintWarning::Code_ComparisonPrecedence)) LintComparisonPrecedence::process(context); + if (context.warningEnabled(LintWarning::Code_CollapseClauseToAssert)) + LintCollapseClauseToAssert::process(context); + if (FFlag::LuauNativeAttribute && FFlag::LintRedundantNativeAttribute && context.warningEnabled(LintWarning::Code_RedundantNativeAttribute)) { if (hasNativeCommentDirective(hotcomments)) From 83b2f7b29ffd1e9b236b12607f1309713c6f6d3b Mon Sep 17 00:00:00 2001 From: PhoenixWhitefire <86601049+PhoenixWhitefire@users.noreply.github.com> Date: Sun, 11 Aug 2024 16:05:21 +0530 Subject: [PATCH 2/3] Add CollapseClauseToAssert lint string Add `CollapseClauseToAssert` as a member of the `kWarningNames` list to go along with the new linting itself. --- Config/include/Luau/LinterConfig.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Config/include/Luau/LinterConfig.h b/Config/include/Luau/LinterConfig.h index 3a68c0d7..42ce4ece 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_CollapseClauseToAssert = 30, Code__Count }; @@ -117,6 +118,7 @@ static const char* kWarningNames[] = { "IntegerParsing", "ComparisonPrecedence", "RedundantNativeAttribute", + "CollapseClauseToAssert" }; // clang-format on From cde9fb0f0ed602da5dab1232ae60844fc76e7e99 Mon Sep 17 00:00:00 2001 From: PhoenixWhitefire <86601049+PhoenixWhitefire@users.noreply.github.com> Date: Sun, 11 Aug 2024 16:22:17 +0530 Subject: [PATCH 3/3] Remove unused variable My first failing Unit Test caused by an unused variable --- Analysis/src/Linter.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Analysis/src/Linter.cpp b/Analysis/src/Linter.cpp index 7a3d917c..0a9427b7 100644 --- a/Analysis/src/Linter.cpp +++ b/Analysis/src/Linter.cpp @@ -2657,9 +2657,7 @@ private: // Don't give the suggestion in this case, we don't want to slow people's code down. if (!errmsg->is()) return true; - - AstExprConstantString* string = errmsg->as(); - + emitWarning( *context, LintWarning::Code_CollapseClauseToAssert,