Python: Add module boundary flow steps #4514
Merged
Conversation
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
Based on https://www.python.org/dev/peps/pep-0328/#guido-s-decision Original "code" is in the Public Domain.
This is the quick-and-dirty solution, as discussed. An even quicker-and-dirtier solution would have used `ModuleValue::attr` and take the `getOrigin` of that as the source of the jump step. However, this turns out to be a bad choice, since `attr` might fail to have a value for the given attribute (for a variety of reasons). Thus, we instead appeal to a helper predicate that keeps track of which names are defined by which right-hand-sides in a given module. (Observe that type tracking works correctly for `x` in `mymodule.py`, even though `x` is never assigned a value in the eyes of the Value API.) This means that points-to is only used to actually figure out if the object we're looking an attribute up on is a module or not. This is the next thing to replace in order to eliminate the dependence on points-to, but this will require some care to ensure that all module lookups are handled correctly. Only two test files needed to be changed for the tests to pass. The first was the fixed false negative in the type tracker, and the other was a bunch of missing flow in the regression test. I have manually removed the `# Flow not found` annotations to make them consistent with the output. Pay particular attention to the annotation on line 117 -- I believe it was misplaced and should have been on line 106 instead (where, indeed, we now have flow where none appeared before).
tausbn
changed the title
yoff
previously approved these changes
Oct 19, 2020
Contributor
yoff
left a comment
yoff
left a comment
There was a problem hiding this comment.
One simple typo, otherwise looks good. It does look like you are correct about the test annotations. It will be glorious once all test annotations are verified :-)
python/ql/src/experimental/dataflow/internal/DataFlowPrivate.qll
Outdated
Show resolved
Hide resolved
Co-authored-by: yoff <lerchedahl@gmail.com>
RasmusWL
approved these changes
Oct 20, 2020
Member
RasmusWL
left a comment
RasmusWL
left a comment
There was a problem hiding this comment.
LGTM!
I'll just run this on a snapshot locally, and if everything looks good, merge it as well 👍
Local testing shows that the `getDefinition` result for this is a `SSA filter definition`, and not an `AssignmentDefinition`.
…efinition Python: Add test for tricky module member for type-tracking
I'm slightly suspicious of this fix -- it seems to work, but it makes me wonder if we're potentially missing other kinds of flow, by not handling other kinds of definitions. Also, I feel like this should really be attached to an appropriate post-update node of the given argument. As it is written now, the flow will go from the argument _before_ the call, which obviously misses a step if the argument is modified by the call. In practice, I would expect this to be rather rare.
yoff
approved these changes
Oct 20, 2020
Contributor
yoff
left a comment
yoff
left a comment
There was a problem hiding this comment.
Interesting, it seems we might do this in a more principled way later, by referring to ModuleExports, perhaps. But this is fine as bandaid for now.
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 is the quick-and-dirty solution, as discussed.
An even quicker-and-dirtier solution would have used
ModuleValue::attrand take thegetOriginof that as the source ofthe jump step. However, this turns out to be a bad choice, since
attrmight fail to have a value for the given attribute (for avariety of reasons). Thus, we instead appeal to a helper predicate
that keeps track of which names are defined by which right-hand-sides
in a given module. (Observe that type tracking works correctly for
xin
mymodule.py, even thoughxis never assigned a value in theeyes of the Value API.)
This means that points-to is only used to actually figure out if the
object we're looking an attribute up on is a module or not. This is
the next thing to replace in order to eliminate the dependence on
points-to, but this will require some care to ensure that all module
lookups are handled correctly.
Only two test files needed to be changed for the tests to pass. The
first was the fixed false negative in the type tracker, and the other
was a bunch of missing flow in the regression test. I have manually
removed the
# Flow not foundannotations to make them consistentwith the output. Pay particular attention to the annotation on line
117 -- I believe it was misplaced and should have been on line 106
instead (where, indeed, we now have flow where none appeared before).
Finally, this PR adds a bunch of test files based on PEP-328 in anticipation
of the import resolution handling that will be needed eventually. The actual
test will come at a later point. (Probably not this PR.)