codeql
codeql copied to clipboard
[Feature branch] JS: Migrate to shared dataflow library
Targeting a feature branch
This PR is targeting a branch named js/shared-dataflow-branch, not main as we normally would.
- Merging into
mainwill happen at a later point, and will require that- we have a good-looking DCA report (currently we have too many performance regressions), and
- possibly more evaluations on different suites and/or a QA trial run, and
- TODO comments have been resolved
- The criteria for merging to
js/shared-dataflow-branchshould be that the code has been reviewed, and is considered good enough to go intomainonce the above criteria are met. - This means things like readability should be reviewed as if we were targeting
main. We don't want to have to go back a re-review the code for readability later.
Review order
Commits have been organised into a reading order. The large number of commits is there to make it easier to the review.
Note that not every commit can compile in isolation. I split some commits into smaller chunks for easy of readability, but you may sometimes see odd artifacts like see a trailing and/or, an empty predicate body, or the occasional mention of a class that doesn't exist yet.
Otherwise the overarching order of commits are:
- The first commit is from this PR, which should be reviewed in its own PR.
- Core data flow changes: instantiating shared libraries and related work to enable that
- Library modeling changes: mainly rewriting collection models as flow summaries
- Query changes: porting to
ConfigSig-style and updating tests - Library tests: port test configurations to
ConfigSigstyle, generic test output changes - Add TODOs and further attempts at fixing some of the performance and precision issues.
Steps across function boundaries
The most difficult change for JS is that steps across function boundaries are no longer permitted, except for jump steps which can result in FPs and performance issues.
This means the following type of steps need to be rewritten:
- Steps into or out of callbacks. This applies equally to value-preversing steps, taint steps, read steps, and store steps.
- Store steps that target
getALocalSource(), because that can cross function boundaries. - Steps that use the combined
loadStoreStepfeature, which is no longer supported.
The current status of this migration is:
- Ported the core models for
Array,Map,Set,Promise, and generators. - The above took care of all our existing uses of
loadStoreStep. String#replacehas been converted, as it is the only string-related step that needed to be ported.- Value steps and taint steps from other models are implicitly converted to jump steps when they cross a function boundary.
- Read/store steps from other models are just retained as they are for now, which in some case breaks the invariant by crossing a function boundary, as there is no combined jump-read or jump-store step we can map to.
Related reading:
- Details of the
Awaitedtoken: https://github.com/github/codeql-javascript-team/issues/423
Evaluation
The latest evaluation can be found here. As mentioned there are too many performance regressions at the moment, and alerts need to be triaged.
Thanks for the review @erik-krogh! I have (finally) gotten through the comments and pushed a bunch of fixes.
It seems the tests won't work on CI due to changes in the build system that we'll need to include via a merge/rebase. I have a merge resolution ready but maybe you want to look at the new commits separately first.
The new commits look good 👍 Feel free to merge/rebase.
@erik-krogh some post-merge fixups have appeared after the commit called Merge branch 'main' into js/shared-dataflow-merged (they appear prior to our previous comments due to having an earlier commit date).
@erik-krogh after keeping up the CI changes and breaking changes in the data flow library, this PR is finally green again (apart from the QLDoc check which complains about shared libraries).
I'd like to merge it to the feature branch as it is now, so we can start working with smaller PRs. The first unreviewed commit so far is https://github.com/github/codeql/pull/14412/commits/102ca77acfa2afe84446abcb1e3214f7c5a98f11.
Merging to the feature branch (not main)