Skip to content

Conversation

erik-krogh
Copy link
Contributor

Removes the Deferred model from UseOfReturnlessFunction.ql, and replaces it with an extremely simple heuristic that looks for methods named resolve.

Promises were not used for much when the UseOfReturnlessFunction query was created.
And adding a Promise model into a single query was therefore not a big deal.

Promises were later used to create AdditionalFlowSteps, and now we are looking at adding Promises as type-tracking steps. (#3036).

Adding these type-tracking steps caused non-monotonic recursion as the Deferred model uses type-tracking.
I therefore suggest removing the model from UseOfReturnlessFunction.

The performance benefit from this change is negligible, as the UseOfReturnlessFunction query does not use a data-flow or taint-tracking configuration.

@erik-krogh erik-krogh added the JS label Mar 12, 2020
@erik-krogh erik-krogh requested a review from asgerf March 12, 2020 18:45
@erik-krogh erik-krogh requested a review from a team as a code owner March 12, 2020 18:45
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

LGTM 👍

If you still have the verbose.log file from that run, could you see if it contains any mention of Recomputed cached stages? We ought to have checks for this sort of thing.

@erik-krogh
Copy link
Contributor Author

If you still have the verbose.log file from that run, could you see if it contains any mention of Recomputed cached stages? We ought to have checks for this sort of thing.

I just did a clean rerun on underscore. I found no such mention in the verbose.log.

The UseOfReturnlessFunction query doesn't use a DataFlow::Configuration, and that is probably why we didn't see a big performance penalty.

@semmle-qlci semmle-qlci merged commit 20cae30 into github:master Mar 14, 2020
@asgerf
Copy link
Contributor

asgerf commented Mar 14, 2020

Ah of course, good point. Since the query doesn't depend on AdditionalFlowStep it doesn't get re-evaluated. Nice to have that cleared up. I'm still happy with this change though, as it's a bit unexpected for a general concept like PromiseDefinition to change between queries.

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