JavaScript: Add flow tracking through nested properties.#90
Conversation
This eliminates a nest of `pos#` predicates resulting from ill-advised applications of magic.
asger-semmle
left a comment
asger-semmle
left a comment
There was a problem hiding this comment.
I have some initial comments and questions, but I'm not all the way through yet.
I was a little surprised to see loadLoadPair in there. It seems awfully expensive (see my comment below). I'd much rather have flow through access paths and avoid building the expensive all-pairs relation.
| * let base = root.innerProp; | ||
| * base.outerProp = rhs; | ||
| * let succ = root.innerProp; | ||
| * ``` |
There was a problem hiding this comment.
... such that the path from
rhstosuccis summarized bysummary.
This part was a bit confusing (in the otherwise light and enjoyable prose).
If I understand it correctly now, this predicate "defines" that there is an edge from rhs to succ, is that right? The "such that" seems to imply a condition that must be satisfied for the predicate to hold, but it's really the other way around; the edge is there because this predicate says so.
I think it would clarify things to talk separately about the edge defined by the predicate, and those it uses to generate it.
There was a problem hiding this comment.
You're right; this needs to be documented better.
| /** | ||
| * Holds if `load` is a read of some property `innerProp` from which we can reach a read `succ` of the same | ||
| * property `innerProp` under configuration `cfg`, and the concatenation of `oldSummary` with the summary | ||
| * of that path is `summary`. |
There was a problem hiding this comment.
This was quite confusing for the same reason as the above, because in the example nothing appears to be reachable from load.
| reachableFromLoadBase(innerProp, load, nd, cfg, newSummary) and | ||
| loadStep(nd, succ, innerProp) and | ||
| summary = oldSummary.append(newSummary) | ||
| ) |
There was a problem hiding this comment.
I don't understand why we have two PathSummary arguments. oldSummary is almost not bound at all; it's just anything that can be prefixed onto newSummary to yield summary.
It seems like the summary of this edge is a pair of input/output summaries, instead of just being a summary. I mean that's what PathSummary is for, right?
Apart from that, is it correctly understood that we would get N^2 load-load pairs for code like this?
elm.style.color = X;
elm.style.backgroundColor = X;
elm.style.border = X;
elm.style.padding = X;
...
There was a problem hiding this comment.
Yes, that's a bad case with the current implementation.
Adding oldSummary as an argument to this predicate is mostly done to enable a better join order in nestedPropFlow.
|
Thanks for your initial comments. As you rightly point out, the approach taken in this PR, while simple, is prone to blow-up in certain cases. I think we should try out the access path-based approach you outlined before for comparison. Anyway, I've marked this as WIP for now to prevent a premature merge. |
Kotlin: Add TrapWriter.writeComment
Add new sources and summary steps
…classes PS: Add more AST classes
2 participants
The technical meat is in the first commit, the second one is a (somewhat performance-motivated) refactoring, the third is a pure performance refactoring.
No new alerts on the default benchmarks, and only moderate performance cost, except on
esprima(11s = 6%),goojs(67s = 9%) andexpressCart(12s = 12.5%); full numbers here (internal link).There is no particular rush to get this in now (or even at all), I just wanted to put it up since I had a branch anyway and the performance more or less works out.