JS: Use guard nodes in useless-conditionals

[asger-semmle]

This changes useless-conditional to use ConditionGuardNode as the basis to determine if something is a condition. This has a few consequences:

• It recognises loop conditions.
• It recognises z in if (x && z) as a conditional. Previously only x and x && z were recognised.
• For !p it will report that the variable p is always true/false, rather than pointing to the whole !p expression.
• The whitelisting rules apply to the "inner" condition, such p instead of !p. This means trivial expressions like !true and true && true will not be reported anymore, since true is already whitelisted.

The two latter ones are debatable, but for now I've just kept it like this for simplicity.

Moving to ConditionGuardNode is also necessary in order to report range analysis results with this query. Putting this up separately so the results won't interfere with the evaluation of range analysis.

[ghost]

LGTM, except for bullet 3.

For !p it will report that the variable p is always true/false, rather than pointing to the whole !p expression

I fear that this will make the query results too easy to mis-interpret.

The believe the query already confuse users of the LGTM interface that fail to check exactly what region the alert message is for. This will make it worse for complex expressions since we no longer report the actual value of a guard.

Consider:

if (!(complex1(complex2())) {
//    ^^^^^^^^^^^^^^^^^^^^ // this expression always evaluate to false
} else {
// so this code is dead
}

[asger-semmle]

That alert could indeed be confusing. Unfortunately, I can't write a test case that reproduces the example since return-type inference is not in yet. At the moment it will always highlight a variable or a trivially truthy expression like new E(), and conditions like !(new E()) are unlikely to occur in practice.

I believe we got a report from a user about this once. I can't seem to find it now, but I believe this change would actually have fixed it. It would have mentioned the variable itself in the alert message instead of highlighting something like !p. So highlighting the "root" condition isn't always the answer.

I propose to keep this change, but once return-type inference lands, add another special case for function invocations, similar to what we have for variables.

[ghost]

return-type inference is not in yet

It is, if the callee is a LocalFunction (provably only invoked at a single location):

function complex1(v){
    return v
}
function complex2(){
   return true;
}
if (!(complex1(complex2())) {
//    ^^^^^^^^^^^^^^^^^^^^ // this expression always evaluate to false
} else {
// so this code is dead
}

[asger-semmle]

Okay so the isConstantBooleanReturnValue heuristic is interfering. On master, it whitelists any call or a negation of a call unless it's wrapped in a spurious parenthesis. With this change, it makes no difference if the parenthesis is there. I don't think it's worth going out of our way to preserve that behaviour.

[asger-semmle]

I've rebased to avoid conflicts, although now the test for if ((x, true)) is no longer flagged due to the whitelisting of true. I can see why we'd want to bring back this one, as it can happen as a result of poorly balanced parentheses in a call.

[ghost]

Approved. I have an idea for making the alert less confusing, I will open up a separate PR for that.

[asger-semmle]

Oh, sorry, that was a bit quick. I was also trying out some ideas but nothing quite worked out so far.

I started an evaluation last night: https://git.semmle.com/asger/dist-compare-reports/tree/domitian.ti.semmle.com_1540941089548

One of the results looks a bit fishy, the one about event being true in WebKarma: https://github.com/usc-isi-i2/Web-Karma/blob/c1a718ee44f0754aba45a9413cd6e44393cf7aa8/karma-web/src/main/webapp/uiLibs/jquery/js/jquery.qtip.js#L1248

I think the problem is that there is an outgoing guard node from the rightmost event in that line, belonging to the enclosing || event. But the target CFG node must have multiple guard nodes targeting it, so it's not dead just because one guard is always true/false.

This is objectively a false positive, so we should probably revert this and rethink.

[ghost]

Sorry about that, I read the rebase comment as if you were done. Will you open the revert?

[asger-semmle]

I'll revert when I get in.

[asger-semmle]

I'll put up another PR that just addresses the first two bullet points after the revert has merged.