Python: Allow absolute imports in directories with scripts #5506
Conversation
This one will require some explanation... First, the file structure. This commit adds a test consisting representing a few different kinds of imports. - Absolute imports, from `module.py` to `main.py` when the latter is executed directly. - A package (contained in the `package` folder) - A namespace package (contained in the `namespace_package` folder) All of these are inside a folder called `code` for reasons I will detail later. The file `main.py` is identified as a script, by the presence of the `!#` comment in its first line. The files themselves are executable, and `python3 main.py` will print out all modules in the order they are imported. The test itself is very simple. It simply lists all modules and their corresponding names. As is plainly visible, without modification we only pick up `package` and its component modules as having names. This is the bit that needs to be fixed. Convincing the test runner to extract this test in a way that mimics reality is, unfortunately, a bit complicated. By default, the test runner itself includes any Python files in the test directory as modules in the invocation of the extractor, and so we must hide everything in the `code` subdirectory. Secondly, a `--path` argument (set to the test directory) is automatically added, and this would also interfere with extraction, and hence we must prevent this. Luckily, if we supply our own `--path` argument -- even if it doesn't make any sense -- then the other argument is left out. Finally, we must actually tell the extractor to extract the files (or it would just happily pass the test with zero files extracted), so the `-R .` argument ensures that we recurse over the files in the test directory after all.
Fixes the import logic to account for absolute imports. We do this by classifying which files and folders may serve as the entry point for execution, based on a few simple heuristics. If the file `module.py` is in the same folder as a file `main.py` that may be executed directly, then we allow `module` to be a valid name for `module.py` so that `import module` will work as expected.
| // The file doesn't have the extension `.py` but still contains Python statements | ||
| not this.getExtension() = "py" and | ||
| exists(Stmt s | s.getLocation().getFile() = this) | ||
| or |
There was a problem hiding this comment.
Have we seen this? Can we have a test for this?
There was a problem hiding this comment.
I haven't personally seen it in the wild, but I based the above on this similar construction in ObjectAPI.qll, so I assume there's a reason for it. (I added the check that the file contains Python statements, because we don't want random non-Python files to affect the status of the Python files in the same directory.)
That being said, I'm not sure we can test this without modifying the extractor. I tried renaming main.py to have a non-.py extension, and I can include it directly using --filter:include=..., but even though the file appears in the database, it seems we're not interpreting it as Python code. My best guess is that it is being filtered out by a more restrictive "is this a script" check in the extractor, which seems unfortunate.
I'm also thinking that the above should read this.getExtension().matches("py%"), since there are many Python-related files that have extensions starting with py, and we certainly don't want to have those affecting the analysis, even if they do contain Python statements.
There was a problem hiding this comment.
Ok, I think it is fine to have this structure in place.
There was a problem hiding this comment.
Oh, I do this every now and then if I write a small helper script in Python, that just needs to look like an executable. With the #!/usr/bin/env python everything works out anyway 👍
OHH, but that is covered by the exists(Comment c | case... So I guess this is for python my-script-without-extension, which does seem strange.
I'm also thinking that the above should read
this.getExtension().matches("py%"), since there are many Python-related files that have extensions starting withpy, and we certainly don't want to have those affecting the analysis, even if they do contain Python statements.
Hmm, is this about .pyc, .pyo or .pyd files? I'm under the impression that since they contain byte-code, that wouldn't be valid Python statements.
(This last point is not super import to me, just curious)
There was a problem hiding this comment.
Indeed, not all .py? files contain Python code, so those would be filtered out anyway. I was thinking more of stub files like .pyi which do contain valid Python, but not code that is actually executed as a script.
(I also found this random list of Python extensions. There are more than I had expected.)
There was a problem hiding this comment.
Good point about .pyi 👍 I personally didn't know about .py3 usage either 😄
Note that the test in `no_py_extension` isn't complete, since we're not extracting the `main` file there.
Sadly, it seems we're not interpreting this as Python code, even if we explicitly ask to have it included.
|
Merging this now as I believe it fixes a bunch of grievances. We can always refine it later.. |
Python: Revert #5506 due to bad performance
Fixes the import logic to account for absolute imports.
We do this by classifying which files and folders may serve as the
entry point for execution, based on a few simple heuristics. If the
file
module.pyis in the same folder as a filemain.pythat may beexecuted directly, then we allow
moduleto be a valid name formodule.pyso thatimport modulewill work as expected.There are still some issues around namespace packages, but the fix for these is beyond the scope of the present PR.
The first commit message goes into quite some detail about the peculiarities of the test setup.
As this is fixing a minor but annoying bug, I don't think a change note is necessary (but I will gladly be convinced otherwise).