New Linter rule: MisleadingCondition

[tapple]

• Closes Linter should check for non-booleans in condition expressions #2183
• Matching Documentation PR: Document MisleadingCondition warning in lint.md site#83

The linter now complains about condition expressions that are not one of:

1. a boolean
2. a supertype of nil

The linter considers a condition expression to be one of:

1. The condition expression of an if statement
2. The condition expression of an if expression
3. The left expression of an and operator
4. The left expression of an or operator

See the unit test for error messages

[jLn0n]

this should be documented on a rfc and approved first before creating a new linter rule i think

[WolfGangS]

this should be documented on a rfc and approved first before creating a new linter rule i think

Contribute suggests rfcs only for language changes, this wouldn't do that, and an issue has been created, though maybe some more time should have passed between issue and pr.

On the rule in general, I think it's extremely useful for devs unfamiliar with lua. Nearly every other language has at least 0 as false, and many have a concept of falsey for things like empty strings or tables.

As lua and by extension luau doesn't have a broad falsey concept, this lint would catch a very common footgun for newer users.

[tapple]

I believe the 2 remaining unit test failures are due to a typechecker bug:

• [New Solver] Error typing local a = ... #2188

[JohnnyMorganz]

Can't comment on all the cases, but I like the idea. I think this is also valuable for a case where you have a function foo that returns a boolean, and you accidentally write if foo then instead of if foo() then, which will always be true. This has caused prod incidents before

[aatxe]

this should be documented on a rfc and approved first before creating a new linter rule i think

The linter is not part of the language itself, and frankly will probably be sunset in favor of the linter we're writing in lute since that allows programmers to author their own lints in luau instead of writing C++ to lint luau. Even if it continues to exist as well, it's not something we've treated as requiring an RFC.

[tapple]

TODO: also check the first argument to assert()

[tapple]

Or maybe don't. An assert evaluating false is usually understood to be a bug. And the typechecker should often be able to catch the same bugs (at leasts the asserts that assert not nil). So nearly every time this linter rule flagged an assert, it would be a false warning.

also, asserts are pretty specialized. this linter rule is meant to catch common, stupid errors. And I don't think the linter even could distinguish between "asserts that meant to check a boolean but forgot a comparison operator" and "asserts that are checking for non-nil and the type-checker agrees". Maybe checking for intersection types, but either way, it's different enough that it should be a different linter rule

[aatxe]

An assert evaluating false is usually understood to be a bug.

I don't think this is true at all. We use things like assert(false) or assert("this message to read about an unreachable assumption" && false) all the time to mark branches that we expect to be unreachable.

[tapple]

using this on my own code, I've only found one pattern so far where I can't rewrite the code to shut this linter rule up and the linter rule seems wrong:

local list: {number} = {1, 2, 3}
local n5 = list[5] or 5 -- Misleading Condition

direct indexing can return nil if the list is too short, and the typechecker can't detect that

I think I should handle this case specifically