Skip to content

JS: Make SourceNode and PropRef.getBase non-recursive, again #5554

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 7 commits into from
Mar 30, 2021

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Mar 29, 2021

  • Makes SourceNode non-recursive (really, this time)
  • Makes PropRef.getBase non-recursive by fixing an accidental reference to SourceNode
  • Caches PropRef.getBase

Although #5499 was supposed to make SourceNode non-recursive, I had missed one recursive dependency going through SourceNode -> AngularJS -> Import -> NodeJS/createRequire -> SourceNode. My earlier attempt in #4772 fixed that by making all string literals SourceNode but that was a little too costly, so now I've instead made createRequire not depend on SourceNode.

Evaluation (internal link) looks good; an average 1% speed-up and a <1% increase is cache size.

@asgerf asgerf added the JS label Mar 29, 2021
@asgerf asgerf requested a review from a team as a code owner March 29, 2021 09:57
@asgerf asgerf added the JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis label Mar 29, 2021
@asgerf asgerf added no-change-note-required This PR does not need a change note and removed JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis labels Mar 29, 2021
@asgerf
Copy link
Contributor Author

asgerf commented Mar 29, 2021

@esbena I managed to make a non-recursion test that should work well in practice by requiring that SourceNode::Range cannot depend on SourceNode. In theory it's still possible for SourceNode::Range to be recursive by itself, but in practice any accidental recursion will go through SourceNode so we should have a good chance of catching it now.

esbena
esbena previously approved these changes Mar 29, 2021
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.

LGTM.
It is a shame that we can't easily add a failing recursion guard test to ensure that the compiler does not get smart enough that it bypasses the test one day.

@codeql-ci codeql-ci merged commit 25e26b9 into github:main Mar 30, 2021
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.

3 participants