JS: add query: js/useless-defensive-code

[ghost]

This PR introduces js/useless-defensive-code as a recommendation query. The query is based on syntactic patterns, and type inference for a variable in each pattern.

Defensive code is typically a condition that prevents null or undefined values from causing a crash later on. Defensive code is useless when the condition evaluates to a single value for all execution paths. In that case, the condition might as well be removed. I welcome a discussion of the patterns.

The end result is that:

1. Some alerts from js/trivial-conditional and js/comparison-between-incompatible-types are whitelisted
2. The above whitelisted alerts are flagged by js/useless-defensive-code
3. Additional, more advanced, unnecessary defensive code variants are also flagged by js/useless-defensive-code

The most important consequence of this is that the remaining alerts from js/trivial-conditional and js/comparison-between-incompatible-types now are much more severe on average. A secondary consequence is that alerts caused by type inference improvements will be easier to judge the value of.

Please note that the query deliberately treats cases like o === undefined || o.p and o === undefined && o.p identically, for the sake of simplicity. It is the responsibility of another query to flag the definite error in the latter case.

Bullet 1 and 2 above may cause a special kind of wobbliness: if we gain precision because of a simpler program or because of an improved analysis implementation, then additional alerts may be flagged by js/useless-defensive-code, while a more complex program may change those alerts to js/trivial-conditional. C.f. another evaluation.

An oldish evaluation shows that performance is fine and an interesting amount of result changes, with a net additional 69 alerts that the programmer will be presented for by default. A smaller, but more recent, evaluation on top of the current master shows that this still scales.

[asger-semmle]

While we're on the subject, am I the only one who thinks the word "useless" sounds rude? I liked the previously suggested "unnecessary defensive programming".

Do I understand it correctly that this will not introduce any new alerts, but merely reclassify existing ones into this less severe query? It's not quite clear from bullet 3 if this means new alerts.

[ghost]

The phrasing is inherited from js/trivial-conditional aka UselessConditional.ql which has @name Useless conditional. We should change both or none.

---

Brand new alerts will be added because of some additional constant folding steps in this query. Consider the simple case of:

    if (u !== undefined) {
        u.p;
    }

This is obviously not flagged by js/heterogeneous-comparison because the comparison is homogeneous, and it is not flagged by js/trivial-conditional because we do not have a type inference step for the result of a comparison.
Another example is:

    typeof f === "function" && f();

Maybe we should let js/trivial-conditional know that null coerces to false, but that is the only low hanging fruit I have seen.

[xiemaisi]

Ping @Semmle/doc on whether "useless" is considered rude; as a non-native speaker I don't feel qualified to judge.

[xiemaisi]

Wow, that's a lot of new code! A few comments and suggestions.

Overall, the large number of syntactic checks seems quite cumbersome and I'm not confident I fully understand all of the patterns being checked for. This is the sort of thing we managed to avoid in the type inference by using a more precise lattice for analysing refinement conditions, but such an approach may well not apply here. If it helps us avoid false positives that's good.

[ghost]

Overall, the large number of syntactic checks seems quite cumbersome.

Agreed, but I believe we must use syntactic checks to identify the relevant programming patterns.

This could be simplified a bit if we added constant folding for boolean results to the type inference, but we decided that it may not be worth it for just this query and js/trivial-conditional. Such a constant folding would also mean that js/trivial-conditional needed a whitelist for the js/heterogenous-comparison.

and I'm not confident I fully understand all of the patterns being checked for.

I have added source code examples to the docstrings.

[ghost]

Rebased on current master to make the tests for js/trivial-conditional work.

[semmledocs-ac]

I've commented on the use of "useless" but haven't done a review as such.

[mchammer01]

Asked @semmledocs-ac for help as I'm not a native speaker either. Please let me know when this is ready for a doc review. Thanks.

[ghost]

@mc-semmle I have addressed the "is useless" comments, and this PR is ready for doc review.

[mchammer01]

Are we happy with the use of the word "Useless" in the query name?

[mchammer01]

Sorry but I don't understand what you mean in the description. Would it be possible to rephrase so it's clearer?

[mchammer01]

Remove "it"?
...then the defensive code serves no purpose and can safely be removed.

[mchammer01]

Same here - remove "it"?
so the v == null guard serves no purpose, and can be removed:

[mchammer01]

@esben-semmle - doc review completed. This LGTM, I have just made a few minor comments / suggestions. Feel free to ignore them if you don't agree.

[ghost]

Thank you @mc-semmle

The query description is now "Defensive code that guards against a situation that never happens is not needed." And the query name is "Unneeded defensive code"

[mchammer01]

Thank you for the updates @esben-semmle - LGTM 👍

[mchammer01]

Good to go from my perspective. Thanks for the last amendments.

[xiemaisi]

Conflicts.

[ghost]

Conflicts resolved.