Warns on error-prone and unnecessary negations

[arichard4]

Closes #43

[arichard4]

One false positive here: sometimes metatables override the various boolean operators, and end up implementing them in terms of each other.

[alerque]

Thanks for the contribution. It looks pretty good to me. I do want to put it through a couple paces before merging, but as far as I'm concerned this is on the right track.

[alerque]

Having not actually run this bot looking at the code, then looking at use cases from my own code, wouldn't this be a false positive for a situation like this:

if not (t and #t >= 1 and t[#t].prop) then ... end

?

[UncombedCoconut]

Having not actually run this bot looking at the code, then looking at use cases from my own code, wouldn't this be a false positive for a situation like this:

if not (t and #t >= 1 and t[#t].prop) then ... end

?

Nope, that's still clean. Neither rule has anything to say about the negation of a conjunction.
There is a known false positive mentioned here, though: #43 (comment) -- fortunately a niche use case compared to the above.

[arichard4]

I'll update the docs to talk specifically about 'relational operators' rather than 'binary boolean operators'; that's the correct term, and it's clearer that conjunctions aren't included.

[arichard4]

Oh, one more false-positive case: if one side is NaN, the proposed replacement would fail. (Because for any concrete number, NaN isn't greater than, equal to, or less than that number.)

[arichard4]

@alerque For context- I have a local test suite of 13 large open source projects that use luacheck already; the only false positive, so far as I can tell, was luarocks, line 63 here: https://github.com/luarocks/luarocks/blob/master/src/luarocks/core/vers.lua

It seems like no projects actually cared about the NaN case, but in theory that could matter too. I've clarified messaging for both cases.

[alerque]

I think my only remaining objection is the false positives this generates in my own projects, but the cases are all metatable related and convoluted that the reason why they use an unconventional order really should be annotated anyway, so having to adding a lint suppression seems fair enough.

[alerque]

@arichard4 It might be a good idea to submit a fix with a lint suppressor for that Luarocks metatable trickery (or flip it and add a :__ge method to compensate) so that the fix can land before this lint issue pops up.

[alerque]

Any idea what's up with the Luajit test segfault? Doesn't seem to be related to this PR.

[arichard4]

Both the luajit test jobs seems to frequently suffer segfaults. Some things I've noticed:

(1) They're both building against Luajit 2.1-beta3, which is way out of date (from 2017); plausibly they should try to build against a more recent commit instead. The build target seems to come from https://github.com/leafo/gh-actions-lua/blob/master/.github/workflows/publish.yml
(2) As of 2017, there were a bunch of issues reported in busted/luassert involving busted reloading requires, and the previous required modules getting GC'd, causing luajit-specific segfaults: e.g. lunarmodules/busted#496 Luacheck has a lot of tests, if (1) doesn't fix the tests plausibly the luajit tests should be dropped.