More fixes to uncalled function checks in && expressions

[andrewbranch]

Fixes #49192
Related: #42835, #49157

[gabritto]

So, given the new test, I can see why the old code is wrong, but I'm still having trouble understanding some things.
From what I understand, we need to find the parent to see if it is an if statement, get the if's body, and then we use that body to determine if the functions in the condition expression is called in the body, right?
So I have two questions:

• Why do we allow using an always truthy expression in a condition if it is later used in the if's body?
• Why are we now only skipping over parenthesis and && and || expressions to find the parent?

[andrewbranch]

Why do we allow using an always truthy expression in a condition if it is later used in the if's body?

This is the heuristic we’ve always employed to avoid erroring on defensive checks:

declare function doSomething(): boolean;

if (doSomething) { // Type indicates should be present, but clearly checked on purpose
  doSomething(); 
}

if (doSomething) { // Probably a mistake
  return;
}

Why are we now only skipping over parenthesis and && and || expressions to find the parent?

So, the feature is trying to detect places where the user checked the truthiness of a function in a conditional rather than the result of calling that function. So the relevant question is, if we have an AST like:

IfStatement
  condition:
    ???
      BinaryExpression (&&)
  then:
    Block
      ...

what are the possible structures of ??? such that we’d want to check the left and right of this && expression for uncalled functions? Previously, the answer was just “parenthesized expressions,” which presents a problem for this:

if (f1 && f2 && f3) {
  f1();
  f2();
  f3();
}

When we’re on the first && token, the parent is the higher && binary expression, which means we run the uncalled function check for f1 without sending the if body along to see if it’s used in there. In general, in a chain of &&s, only the last two ever got correctly checked since the last && token belongs to the highest-level binary expression such that its parent was the IfStatement.

On the other hand, the answer to what ??? can be clearly isn’t “everything” either, because you need to rule out stuff like

if (f1 !== doSomethingWith(f2 && f3)) {
  f2();
  f3();
}

Here, f2 and f3 are nested in a condition, but their truthiness is not actually being tested by the if, so it’s not relevant to this check.

It could be that there are more constructs we should care about, and that &&, ||, and parens is an incomplete list, but it’s more complete than before, and I was able to convince myself that any arbitrary chaining of those three kinds of expressions directly inside a condition is necessarily concerned with the truthiness of all the leaves.

[gabritto]

Like you said in your comment, even though there might be cases not covered yet, I agree that the new code seems more complete.