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
Tests #11024
Draft
asgerf
wants to merge
28
commits into
github:main
Choose a base branch
from
asgerf:rb/data-flow-layer-capture2
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Tests #11024
+1,792
−415
Conversation
This file contains 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
This is the inverse of getALocalSource()
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
99c8eba
to
be1d931
Compare
be1d931
to
04a5c91
Compare
These had the same name and ended up being unified
04a5c91
to
c2e33a2
Compare
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Adds some classes to do basic stuff with
DataFlownodes without having to map down to CFG and AST nodes, and a few other tidbits I found myself missing.To ensure some degree of battle-hardening I (mostly) ported three library models to use the new nodes where it makes sense. The affected models are
Rails.qll,Railties.qll, andActionController.qll.Migrating from AST nodes to DataFlow nodes revealed a couple of pre-existing issues, some of which have now been fixed or fixed enough to avoid a regression:
Some other issues which were too hard to fix prior to opening this PR:
self, but of course that's not always the case. There is currently no way to tell the analysis what the value ofselfis, so we have to work around it for now. This means using things likegetParent+()and treating the moduleselfas if it's an instance of the module.getConst() and what about API graphs?
One of the changes that may look odd is the introduction of
DataFlow::getConst()andConstRef. It provides convenient access to constants that may resolve to a given qualified name. For example, the incantationgives us all descendents of
ActionController::Base.This was probably the biggest use case for API graphs in Ruby, as you can get to (almost) the same thing with
API::getTopLevelMember("ActionController").getMember("Base"). So why addgetConst()?getConst()doesn't rely on the call graph, which means it can be used for things likegetACallSimple()and other pre-call graph customizations (we have quite a few of those in JS and I expect we'll find the need in Ruby as well).getConst()handles the rules of constant lookup more precisely than API graphs, in particular when related toinclude, nested modules, and modules with more than one declaration. I think API graphs could be extended to make this work, but it's not really a natural fit for its use/def tracking, it would need to be a bunch of new predicates specific to constant lookup, which might as well exist earlier in the pipeline.I replaced some uses of API graphs with
getConst, not because it matters in practice, I just had to ensuregetConst()is being used enough to test it in the wild.I hope the name
getConst()makes more sense in the context of Ruby. I think the name in API graphs,getMember, was used mostly because that's what JS called it.Evaluation
Evaluation shows
flowsToThe new sources and sinks are mainly due to resolving methods upwards in the hierarchy in
ActionController. The combinationgetADescendentModule().getAnInstanceMethod()takes "sideways" method resolution into account without having to really think about it, so I like that.