[JS] Null Sensitive Context (new library)

[BekaValentine]

This PR adds a new library to the JS libs to describe (local) evaluation contexts in which the values of expressions in that context cannot be null or undefined, and where using those values will cause an error to be thrown or cause other undesirable behavior.

For example, in the expression fac(5), the expression fac cannot compute to null or undefined (nor replaced with them), without the code producing an error (in this case, a runtime TypeError).

The library has one abstract class, NullSensitiveContext, which has a large number of concrete classes for different contexts. By convention, the names of the context classes are <NameOfExprClass> + <NameOfPrincipalArgument>. So for example, in the CallExpr fac(5), because the fac part is the callee (getCallee), the context that it inhabits within that expression as its principal argument is CallExprCallee.

[xiemaisi]

Looks useful, but can be simplified considerably. Also, please add some tests.

[ghost]

@mc-semmle: this PR does not require a doc review.
@psygnisfive Generally, the doc team only reviews our qhelp files.

This bit seems to duplicate the crashing cases of this PR:

https://github.com/Semmle/ql/blob/86040575b1533b28adf284ecd1af7a9220d015cc/javascript/ql/src/semmle/javascript/DefensiveProgramming.qll#L193-L198

Once this PR lands, perhaps we should try to use your changes to make https://github.com/Semmle/ql/blob/86040575b1533b28adf284ecd1af7a9220d015cc/javascript/ql/src/Expressions/UnneededDefensiveProgramming.ql flag more cases.

[mchammer01]

Thanks @esben-semmle - no worries.

[xiemaisi]

On second thought, I'd suggest replacing all of these abstract classes with a simple predicate on Expr. It's just as extensible (since you can override that predicate when needed), and if you combine the individual cases as suggested in the review comments it won't even be that complicated.

[BekaValentine]

On second thought, I'd suggest replacing all of these abstract classes with a simple predicate on Expr. It's just as extensible (since you can override that predicate when needed), and if you combine the individual cases as suggested in the review comments it won't even be that complicated.

Would this make it possible to include the for-of statement iteration domain? Or should we omit that in preference of a method predicate?

[xiemaisi]

Would this make it possible to include the for-of statement iteration domain?

You can include that in either style, but I think just implementing the whole thing as a member predicate of Expr is less verbose and makes for a simpler API surface.

[BekaValentine]

Would this make it possible to include the for-of statement iteration domain?

You can include that in either style, but I think just implementing the whole thing as a member predicate of Expr is less verbose and makes for a simpler API surface.

[EDIT] Ok, in talking w/ Aditya, the solution resolved itself into being a predicate that is either isInSensitiveContext or getSensitiveContext (which can then be quantified to turn it into the is version). This would make it possible to have this exclusively on Expr while providing the same functionality.

[xiemaisi]

Yes, this is the approach I had in mind. A few minor comments, but overall lgtm.

[asger-semmle]

I'd suggest whitelisting the following cases as not "null sensitive":

• +x
• x | 0 and 0 | x
• x >> 0
• x >>> 0

These explicitly serve the purpose of converting a value to a number, so I wouldn't say it's indicative of "undesired behaviour" to get null or undefined in there.

[xiemaisi]

You could also consider adding some recursive cases: e is null-sensitive if any of the following are

• (e)
• d, e
• d && e
• d || e
• c ? d : e
• c ? e : d

[xiemaisi]

Getting close! Just a few minor things left to address.

[xiemaisi]

Tests are failing.