JS: Add PreCallGraphStep and use some array steps in type tracking#3641
Merged
Conversation
erik-krogh
previously approved these changes
Jun 8, 2020
Contributor
erik-krogh
left a comment
There was a problem hiding this comment.
LGTM.
I think we should refactor PromiseTypeTracking to use the same pattern post-sprint.
Contributor
Author
|
The evaluation on security/nightly looks fairly safe. |
erik-krogh
reviewed
Jun 16, 2020
Contributor
erik-krogh
left a comment
There was a problem hiding this comment.
Now that #3639 is closed, the name PreCallGraphStep might not be the most descriptive, as the steps are not used in the callgraph. (There is also a few doc-strings that might need updating).
Contributor
Author
|
Type tracking is still mutually recursive with call graph construction, since classes and instances are type-tracked, and they affect the call graph. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This makes the array steps for
.map,.forEach, andfor-ofloops work for type tracking by default, which, in combination with #3639 is needed to get call edges in https://github.com/github/codeql-javascript-CVE-coverage/issues/721.Due to negative recursion, it's not possible to use
AdditionalFlowStepin the core type tracking library, since some contributions to that class depend on negatively on type tracking (and positive dependencies would likely cause performance issues).This therefore introduces an internal class,
PreCallGraphStep, whose steps are contributed to bothAdditionalFlowStepandAdditionalTypeTrackingStep. This third class is needed since neither class of steps can contribute their contents to the other:AdditionalFlowSteps can't be included inAdditionalTypeTrackingStepdue to negative recursion.AdditionalTypeTrackingStepshould in principle not be included inAdditionalFlowSteps since type tracking steps are generally allowed to be more imprecise.The new class uses a unit type as base, but since this is an internal class we are free to change this later. For pure data flow steps, there isn't generally a need to categorize the steps so it seems independent of how we handle #3603.
Evaluations:
TODO
JS: Track functions through props and returns #3639JS: Support calls to returned functions #3715