Conversation
to take into account the split between import time and runtime.
tausbn
left a comment
There was a problem hiding this comment.
A few comments and suggestions, otherwise I think this looks good! 👍
I found the tests quite difficult to read because of the verbosity, though, so i'm not 100% sure that I would have spotted any mistakes.
| | examples.py:0:0:0:0 | GSSA Variable SOURCE | examples.py:27:15:27:20 | ControlFlowNode for SOURCE | | ||
| | examples.py:0:0:0:0 | GSSA Variable object | examples.py:6:13:6:18 | ControlFlowNode for object | | ||
| | examples.py:6:1:6:20 | ControlFlowNode for ClassExpr | examples.py:6:7:6:11 | GSSA Variable MyObj | | ||
| | examples.py:6:7:6:11 | ControlFlowNode for MyObj | examples.py:0:0:0:0 | ModuleVariableNode for Global Variable MyObj in Module examples | |
There was a problem hiding this comment.
Wow, these ModuleVariableNode toStrings are really wordy. I wonder if we should just change the implementation to, say,
override string toString() {
result = "ModuleVariableNode for " + mod.getName() + "." + var.getId()
}to save a bit of space.
There was a problem hiding this comment.
That might be nicer, is it OK to do it in this PR?
There was a problem hiding this comment.
Fine by me. I expect a lot of test output will change, but most of the changes will be somewhat trivial to inspect.
| exists(SsaVariable def | | ||
| def = any(SsaVariable var).getAnUltimateDefinition() and | ||
| def.getDefinition() = nodeFrom.asCfgNode() and | ||
| def.getVariable() = nodeTo.(ModuleVariableNode).getVariable() | ||
| ) |
There was a problem hiding this comment.
Hmm... I guess it's somewhat debatable whether this is flow that happens at import time or at runtime. Ultimately I guess it doesn't matter, since there's no runtime reading of ModuleVariableNodes.
There was a problem hiding this comment.
Indeed, this is at the interface. I take it you mean no import time reading of ModuleVariableNodes.
There was a problem hiding this comment.
Yes, you're quite right. I meant import time.
I think part of what bothers me is that this predicate claims to talk about local flow, but I would not classify the writing of a value to a module variable node as "local". Everywhere else, we would treat this as a jump step.
There was a problem hiding this comment.
I think that is because "everywhere else" has been runtime flow so far. Seeing as it is on the interface, though, I am not really against moving it. That would put the line between the ultimate definition of an SSA variable and the corresponding module variable node. Let us try and see how that looks :-)
There was a problem hiding this comment.
The immediate effect is that we do not see this flow in the tests, because of how they are written, but that could be fixed..
python/ql/test/experimental/dataflow/module-initialization/localFlow.ql
Outdated
Show resolved
Hide resolved
python/ql/test/experimental/dataflow/module-initialization/localFlow.ql
Outdated
Show resolved
Hide resolved
Co-authored-by: Taus <tausbn@github.com>
into runtime jump steps.
python/ql/test/experimental/dataflow/module-initialization/multiphase.py
Outdated
Show resolved
Hide resolved
|
|
||
| def set_foo(): | ||
| global foo | ||
| foo = SOURCE #$ runtimeFlow="ModuleVariableNode for multiphase.SOURCE, l:-31 -> SOURCE" MISSING:importTimeFlow="ModuleVariableNode for multiphase.foo" |
There was a problem hiding this comment.
Apart from this mention of importTimeFlow, I see no other references. That seems... Odd. Perhaps we need to extend the tests?
There was a problem hiding this comment.
Correct, ImportTimeLocalFlowTest currently only considers flow to ModuleVariableNodes, but such flow no longer exists at import time.
tausbn
left a comment
There was a problem hiding this comment.
Made a few suggestions, but apart from that I think this looks good. 👍
Co-authored-by: Taus <tausbn@github.com>
This PR addresses https://github.com/github/codeql-python-team/issues/417. The discussion in that issue outlines the strategy.
Basically, data flow occurs at two different times, import time and runtime, and there is therefor really two dataflow graphs. We create a single local flow relation by partitioning code into import time code and runtime code based on whether code occurs at the top level in the program text. This is mostly correct, except functions (or classes, methods, etc..) called from the top-level should also contribute to import time flow. This oversight means that we may get some incorrect ultimate definitions (those succeeded by function calls), leading to some extra flow.