Skip to content

JS: Support static accessor calls and fix some FP accessor-calls #8791

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

Merged
merged 9 commits into from
Apr 22, 2022

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Apr 21, 2022

Fixes two problems with our call graph for accessors, and adds support for calls to static accessors declared on a class.

This PR deals with two bugs:

  • ClassNode.getStaticMethod previously included static accessors. This lead to some FP call edges as a result from confusing accessors and methods.
  • A PropWrite node exists for accessors, representing the installation of an accessor. We accidentally saw these as potential calls to the setter.

It is addressed as follows:

  • Introduced ClassNode.getStaticMember, analogous to ClassNode.getInstanceMember, and made getStaticMethod only include methods.
  • Added accessor-calls to static accessors to the call graph, and made sure static method calls aren't seen as calls to an accessor.
  • Added PropWrite.isAccessorInstallation to distinguish between ordinary property assignments and accessor installation. Accessor installations are no longer seen as potential calls to a setter.

Evaluation on default slugs:

  • 3811 new call edges (calls to static accessors, and method calls whose receiver came from such a call)
  • 1687 removed FP call edges (from method calls and accessor installations being seen as accessor calls)
  • Performance looks OK, it's not without some cost, but acceptable given the nature of the fix

@asgerf asgerf added the JS label Apr 21, 2022
@asgerf asgerf requested a review from a team as a code owner April 21, 2022 06:33
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.

Nice 👍
LGTM.

And the performance evaluation looks to be within the margin of error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants