Skip to content

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Feb 17, 2020

Extends our call graph to deal with calls to bound functions, that is, functions created by a partial invocation such as fn.bind(x, y). For example:

function foo(x, y) {}
let fn = foo.bind(this, 0);
fn('hey'); // 'hey' flows to parameter 'y'

It is not enough to simply add these to getACallee and FlowSteps::calls() since that wouldn't take into account that the argument-to-parameter mapping is not 1:1.

So "bound calls" are handled as a first-class concept in the data flow libraries. We could generalize ordinary calls to be seen as "bound calls with zero bound arguments and unbound receiver", but since the overwhelming majority of calls are plain calls, it seemed wasteful.

We already type-track the result of partial invocations, for use in library models that require aggressive tracking into callbacks (getABoundCallbackParameter). We now use the same type-tracking to create the call graph for bound calls.

To avoid spurious flow, we distinguish cases where a bound function was tracked into a call from those that weren't. If it wasn't tracked into a call, we add a call edge that's assumed to hold in any context. If it was tracked into a call, we use our existing handling of callbacks to track the flow.

Evaluation looks fine.

@asgerf asgerf added the JS label Feb 17, 2020
@asgerf asgerf requested a review from a team as a code owner February 17, 2020 13:10
@esbena
Copy link
Contributor

esbena commented Feb 18, 2020

This seems fine, but before I check the details, I would like to confirm that the following is intentional:

We do not use type tracking for callees in the callgraph, we only use ordinary dataflow and then type tracking for an eventual class/instance. But this change will permit type tracking for the bound function callees - right?
Semantically, it is fine, but it is inconsistent architecturally IMO. As a consequence we will get better results for f = bind((x) => sink(x)) than for f = (x) => sink(x).

@asgerf
Copy link
Contributor Author

asgerf commented Feb 18, 2020

but it is inconsistent architecturally IMO

Indeed. I've been meaning to track functions as well, but this will require a separate evaluation.

Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM, but I found a few nits.
Do we want a generic change note for "improved callgraph"?

@esbena
Copy link
Contributor

esbena commented Feb 21, 2020

Implementation LGTM then.

Do we want a generic change note for "improved callgraph"?

@asgerf asgerf force-pushed the js/returned-partial-call branch from 5b27c49 to 1ee112a Compare February 21, 2020 13:55
@asgerf asgerf added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Feb 21, 2020
@asgerf
Copy link
Contributor Author

asgerf commented Feb 21, 2020

Rebased to fix a conflict and added change note.

I'd like to run a another short evaluation since the other PR involving partial invocations has landed.

@asgerf
Copy link
Contributor Author

asgerf commented Feb 24, 2020

Evaluation still looks good.

@esbena I rephrased the change note a bit, could you take a quick look?

esbena
esbena previously approved these changes Feb 24, 2020
Co-Authored-By: Esben Sparre Andreasen <esbena@github.com>
@asgerf asgerf removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Feb 24, 2020
@semmle-qlci semmle-qlci merged commit aadb148 into github:master Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants