Python: Clean up import resolution - #10861
Conversation
A fairly complicated bit of modelling, mostly due to the quirks of how imports are handled in Python. A few notes: - The handling of `__all__` is not actually needed (and perhaps not desirable, as it only pertains to `import *`, though it does match the current behaviour), but it might become useful at a later date, so I left it in. - Ideally, we would represent `foo as bar` in an `import` as a `DefinitionNode` in the CFG. I opted _not_ to do this, as it would also affect points-to, and I did not want to deal with any fallout arising from that.
Left as its own commit, as otherwise the diff would have been very confusing.
A slightly complicated test setup. I wanted to both make sure I captured the semantics of Python and also the fact that the kinds of global flow we expect to see are indeed present. The code is executable, and prints out both when the execution reaches certain files, and also what values are assigned to the various attributes that are referenced throughout the program. These values are validated in the test as well. My original version used introspection to avoid referencing attributes directly (thus enabling better error diagnostics), but unfortunately that made it so that the model couldn't follow what was going on. The current setup is a bit clunky (and Python's scoping rules makes it especially so -- cf. the explicit calls to `globals` and `locals`), but I think it does the job okay.
9037c13 to
ad13fba
Compare
No longer missing! 🎉
understandable 👍 (do we have an internal issue for remembering we should do this?)
👍 |
RasmusWL
left a comment
There was a problem hiding this comment.
Really nice, love that we're able to handle the python/ql/test/experimental/dataflow/typetracking_imports/pkg/use.py testcases now 💪 ❤️
The handling of all is not actually needed (and perhaps not desirable, as it only pertains to import *, though it does match the current behaviour), but it might become useful at a later date, so I left it in.
I don't think I understood this bit correct. You're saying that you decided to keep the wrong handling of __all__ (appling it to all imports, even though it actually only affects from .. import *). Is there a comment in the source code that I overlooked explaining this? (if not, I would really like us to have one)
yoff
left a comment
There was a problem hiding this comment.
Great effort modelling all this, and I love the testing part!
I was a bit confused by the names of module_export, which is stated to be an over-approximation, and potential_module_export, which sounds like an over-approximation. Why are both needed, and what are their respective roles?
It also took me a bit to discover the following structure:
getModuleReference, the new top-level predicate. ClosinggetImmediateModuleReferenceunder data flow and sub-scopes.getImmediateModuleReference, extendsgetReferenceToModuleNamewith additions tosys.modules, attribute reads, and implicit definitions. Recursive withgetModuleReferencethrough the disjuncts dealing withsys.modulesand attribute reads. (And with itself via the logic for reexports.)getReferenceToModuleName, deals with the various ways of naming modules in import statements.
Could this perhaps be guided a bit in comments?
Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com> Co-authored-by: yoff <lerchedahl@gmail.com>
I could have sworn I added all of them to the batch, but somehow these slipped through. Co-authored-by: yoff <lerchedahl@gmail.com> Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
- Swaps `module_reference_in_scope` and `module_name_in_scope`. - uses `AttrRead::accesses` instead of `getObject`, etc. - Removes an errant `none()`. - Expands the QLDoc for some of the predicates.
|
I added a test for the case where an attribute name overlaps with the name of a submodule. However, you may notice the |
Casting to `ImportExpr` caused the `typetracking_imports` test to fail.
Extends the tests to 1. Account parts of the test code that may be specific to Python 2 or 3, 2. Also track which arguments passed to `check` are references to modules. The latter revealed a bunch of spurious results, which I have annotated accordingly.
This was due to bad manual magic: restricting the attribute name makes sense when we're talking about submodules of a package, but it doesn't when we're talking about reexported modules. Also (hopefully) fixes the tests so that the Python 3-specific bits are ignored under Python 2.
This should make it so that the `prints3` tag is skipped when running then Python 2 Language tests.
This was causing issues with imports with many "dots" in the name. Previously, the test added in this commit would not have the desired result for the `check` call.
Depending on `localFlowStep` meant that this predicate ended up being recursive with itself (by way of flow summaries which depend on API graphs, which in turn depend on import resolution). Changing this to use the simple local flow step predicate that we use for type tracking should fix this issue.
There was a problem hiding this comment.
Really easy to follow the ql changes when I could see the changes in the tests as well 💪 ❤️
I can see that performance looks good now, and I looked at the first two result changes which also looks good (couldn't hold myself back, since I was excited about this work) 🎉
So from my POV ready to merge once we're confident about the rest of the result changes 👍
-- actually, the fact that this gives more results makes me wonder if we should have a change note as well? (I'm not feeling strongly about it)
yoff
left a comment
There was a problem hiding this comment.
Only one issue, which I do not think a blocker, so I have marked as approved.
| /** | ||
| * A data-flow node that is guarded by a version check. Only supports checks of the form `if | ||
| *sys.version_info[0] == ...` where the right hand side is either `2` or `3`. | ||
| */ |
There was a problem hiding this comment.
This is very specific. But its use seems even more specific, namely only those where version is 3. Are you keeping the option open for a ResolutionTest2? Otherwise this node should perhaps be Python3Node...unless the point is to exclude code guarded by version 2 from all tests?
There was a problem hiding this comment.
Yeah, I figured there was no harm in leaving in the possibility of extending it to also handle Python 2-only tests. However, as I was not aware of any Python 2 behaviour that needed testing, I decided not to implement it now. (In part also because I had only just gotten this to work reliably, after trying a whole bunch of variations that failed for one reason or another.)
If we end up needing Python 2 stuff, I think the better approach would be to simply rename ResolutionTest3 to VersionSpecificResolutionTest and use major_version() to decide which tag is active and which tag to look for in the actual results. That way, we don't have to (basically) duplicate the body of hasActualResult.
We probably should.. |
|
With two approvals in the bag, I'll merge this now, and create a follow-up PR with change note (and a bit of version-handling cleanup in the tests). Thank you both for the excellent reviews! ❤️ |
A fairly complicated bit of modelling, mostly due to the quirks of how imports are handled in Python.
A few notes:
__all__is not actually needed (and perhaps not desirable, as it only pertains toimport *, though it does match the current behaviour), but it might become useful at a later date, so I left it in.foo as barin animportas aDefinitionNodein the CFG. I opted not to do this, as it would also affect points-to, and I did not want to deal with any fallout arising from that.I'm not sure if this merits a change note -- in principle nearly all of the old behaviour is preserved. On the other hand, if the evaluation shows a big improvement in results, then this is probably worth mentioning. For now I've left the no-change-note-requiredThis PR does not need a change note
label on.