JS: Make DataFlow::SourceNode non-recursive

[asgerf]

DataFlow::SourceNode was part of a weird recursive SCC involving AMD, Angular, lazy-cache, HTTP, and LodashUnderscore.

• The contributions from HTTP and LodashUnderscore were no-ops as the charpred requires that this is a SourceNode. These have been removed.
• AngularJS contributed to SourceNode in a way that depended on angular() whose return type is SourceNode. Rewritten to use Import instead.
• AMD and lazy-cache both contributed to Import in a way that depended on SourceNode, and the above changes to Angular means SourceNode depends on Import. Rewritten to rely on AST or the local data flow relation (which does not depend on SourceNode).

Full evaluation on smoke-test and security suite on nightly look good after finally giving up and using a pragma[noopt] in lazy-cache.

[asgerf]

@esbena do you remember why the AngularJS model needs string literals to be SourceNodes? mayHaveStringValue already tracks string constants locally so I was wondering if we could just use that instead, but I couldn't immediately see where it was used.

[esbena]

I think @max-schaefer is the author of this particular feature, perhaps he remembers some subtlety we are missing?

Hisorically, I think the feature is a legacy thing from when we introduced SourceNode, and as a result initially lost the default-flow of string literals.

[asgerf]

Re-run of the slowest slugs without DPMs look good as well.

[asgerf]

@esbena could I ask you for a review?

[esbena]

LGTM. That is some nice detective work.

It seems easy to accidentally re-introduce these costly cycles again, AFAIU you are not breaking any negative cycles in these refactorings - or are you?

I have two proposals:

• document the places where it is tempting to undo your current improvements (see inline comments)
• construct a sneaky test that fails if we reintroduce a bad SourceNode/Import dependency - if possible. (The test could either change analysis behaviour or cause a compilation error due to non-monotonic recursion.)

---

We are still waiting for Max' reply to "perhaps he remembers some subtlety we are missing?" btw.

[esbena]

Do we want to add a warning here as well?

[max-schaefer]

perhaps he remembers some subtlety we are missing?

I believe @esbena's answer is correct: this was done to make up for lacking string-literal tracking. Fine to remove it now.

[asgerf]

Excellent idea with the test case. I've added a test that fails if the domain of SourceNode depends on SourceNode.flowsTo which is the root cause of the Import/SourceNode dependency being unwanted.

I'll look into removing the string literal source node from the Angular model, but given the good evaluations so far I think this is safe to land.