Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS: Fix jump steps generated by IIFEs and exception flow #18043

Merged

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Nov 20, 2024

This PR fixes a few semi-related issues that caused performance and precision problems:

Immediately-invoked function expressions (IIFEs)

Removes jump steps generated by local flow into and out of immediately-invoked function expressions (IIFEs), and fixes some bugs so that the same flow is now handled by regular flow rules. IIFEs are special-cased in the local flow relation, which benefits things like type tracking and type inference, but is unhelpful for the data flow library.

Exceptions

Removes jump steps resulting from exception-propagating flow steps involving callbacks. Exceptions from callbacks passed to a library function are now handled as follows:

  • When a call has no call targets, but it seems to take a callback, it is assigned the "exception propagator" as a call target. This summary propagates Argument[0..].ReturnValue[exception] to ReturnValue[exception].
  • When a flow summary doesn't mention ReturnValue[exception], the summary is assumed to propagate the exceptions from each callback mentioned in the summary. (This isn't equivalent to adding the exception propagator as an additional target, because flow-through from a parameter to the exceptional return wouldn't work in that case).

Block flow into test cases

  • js/insecure-randomness now blocks flow through test cases. Perhaps more queries ought to do this, but it seems particularly problematic for this query. Also broadens our classifications of test files a bit.

Evaluation:

  • 71% speedup on vscode
  • 48 fixed FPs, mainly due to the change to js/insecure-randomness
  • 2 gained FPs, due to imprecise higher-order function flow combined with more exception propagation.

Evaluation against main shows that we're down to a median 30% slowdown, with a 141% worst-case slowdown.

We generate local flow steps into and out of IIFEs, but these come jump steps automatically, resulting in FPs.
Bailing out can be more expensive as the resulting jump steps themselves
cause perf issues. The limit of 100 variables per scope has also been
added in the interim, which handles the cases that this needed to cover.
@github-actions github-actions bot added the JS label Nov 20, 2024
@asgerf asgerf added the no-change-note-required This PR does not need a change note label Nov 22, 2024
@asgerf asgerf marked this pull request as ready for review November 22, 2024 09:42
@asgerf asgerf requested a review from a team as a code owner November 22, 2024 09:42
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

LGTM 👍

/**
* Holds if `node1 -> node2` should be removed as a jump step.
*
* Currently this is done as a workaround for the local steps generated from IIFEs.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Currently"?
Is that hinting towards plans for a another solution in the future?

Copy link
Contributor Author

@asgerf asgerf Nov 25, 2024

Choose a reason for hiding this comment

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

In general this predicate can populated with jump steps that should be excluded, and currently the only use-case for this is the workaround mentioned. So it was meant to imply that other things could get added to the predicate as well.

@asgerf asgerf merged commit c2e9dca into github:js/shared-dataflow-branch Nov 26, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants