"CollapseClauseToAssert" lint/suggestion

[PhoenixWhitefire]

Inspired by 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
master

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 (change in behavior as assert does not have a level parameter)
• Error string must be constant (no string interpolation/concatenation) to avoid slowdowns:

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<AstExprConstantString>())
    return true;

• Body must only be an error call

For some reason, the string formatting wasn't working. I would have liked to have the message substitute the condition and error strings into the lint warning, but it's not too important.

Passed all tests w/ 0 errors, 0 skips (assuming I ran the tests correctly...)

Am I doing this right?

[PhoenixWhitefire]

😨Shocked, I was, when I saw... 2 OF 7 CHECKS FAILED ⛈️⛈️⚡⚡⚡, this being the first time I've failed a Unit Test (or actually done any Unit Tests at all), and... only on Mac OS? Oh no... this is bad... an Operating System-specific compile error?!?!

I furtively middle-clicked macos and macos-arm, and waited in fearful anticipation as GitHub scrolled to the bottom of the compile logs, and- and... and...

Analysis/src/Linter.cpp:2661:36: error: unused variable 'string' [-Werror,-Wunused-variable]
            AstExprConstantString* string = errmsg->as<AstExprConstantString>();
                                   ^

Why does only Mac have -Wunused-variable?

[PhoenixWhitefire]

Accidently closed this. Oh well. I don't know if there's any point re-opening it because I would assume the new type solver is the main focus right now.

[alexmccord]

The role of the linter is to detect patterns of code that are obviously incorrect, rather than to point out equivalency, and this seemed to be the latter. Automated code fixes would be able to give you the option to switch between assert or error versions, with no code warnings since there aren't any bugs in either version.

[PhoenixWhitefire]

@alexmccord I actually deleted the fork 3 weeks ago (it's listed as the last event before my message today) without knowing the consequences, I don't think anyone reviewed it, but thanks for the clarification on the role of the Linter :)

Also, if you don't mind me asking, what's the reasoning behind the warning for table.insert(t, #t+1, v) then? That was actually sort of my inspiration, the code for this lint is next to it because I was referencing it's implementation.