JS: Cache some SourceNode getter methods differently

[asgerf]

We now cache the heavily-used getters in SourceNode (or rather, some helper predicates they are based on). Previously they relied on the optimizer getting creative with magic.

Apart from a speed-up here and now, I'm hoping this will lead to more stability when we make changes to the standard libraries, as an unrelated change won't affect how these central predicates are optimized.

One thing to note is that flowsTo/getALocalSource and getters like getAMethodCall are indexed in opposite directions: flowsTo/getALocalSource map a node backwards to its SourceNode, while getAMethodCall and friends map a SourceNode forwards to its use sites. It's worth keeping this in mind as the following two lines are not performance-equivalent:

x.flowsTo(call.getReceiver())
call = x.getAMethodCall()

^ @erik-krogh @esbena worth keeping in mind as rewrites like this may in theory cause/resolve performance issues.

Evaluations:

• Full/nightly shows a significant end-to-end speed-up
• XSS/nightly shows a speed-up even if only running one query
• XSS/big-apps looks good as well.

[esbena]

Very nice!! But it is also very sad that this is required. I think we should merge, but lets discuss some consequences of this PR first:

So now flowsTo may be misleading for performance reasons. Should we deprecate it and introduce flowsFrom instead? That is, the following are performance-equivalent:

call.getReceiver().flowsFrom(x)   // (new predicate, as an alternative to `flowsTo`)
call.getReceived.getALocalSource() = x

Should we adopt a code style flow that follows the evaluator flow for local data flow? That is:

call.getReceived.getALocalSource() = x
x.getAMethodCall() = call

is preferred to:

x = call.getReceived.getALocalSource()
call = x.getAMethodCall()

Since the left-to-right reading order matches the evaluator flow.

I am not advocating rewriting our existing code to match this pattern, but we could adopt it moving forward, or perhaps add a comment when it truly matters.

[asgerf]

So now flowsTo may be misleading for performance reasons. Should we deprecate it and introduce flowsFrom instead?

I think it's too soon to say if it should be deprecated, but I'll go over our existing uses of flowsTo and review. The issue I mentioned is theoretical and we'll need to see how much it matters in practice.

Should we adopt a code style flow that follows the evaluator flow for local data flow

I personally like it very much when the code has an obvious evaluation order, and would try to order conjuncts accordingly. Your suggested style sounds reasonable when chaining local data flow together (though I don't think expr = var should be generally preferred over var = expr).

[esbena]

but I'll go over our existing uses of flowsTo and review

Do you want to do that before or after the merge of this PR?

[asgerf]

Do you want to do that before or after the merge of this PR?

After. But first I have to fix these test failures that are making me nervous 😬

[esbena]

After. But first I have to fix these test failures that are making me nervous grimacing

Fair enough.

[asgerf]

The test failure was due to getAMethodCall originally including reflective calls whose callee is a PropAccess. I've added a commit to preserve the original behabvior and opened https://github.com/github/codeql-javascript-team/issues/101 to discuss how that ought to work.

An evaluation of XSS/smoke-test shows performance is still positive, so I'm willing to believe that this didn't impact performance, but I can run another evaluation if anyone thinks it's needed. For now, I'll wait for the original big-apps evaluation to finish.

[asgerf]

XSS/big-apps looks good as well