Repository navigation
gh-141276: Avoid compiling zipimport source in get_filename - #153799
harjothkhara wants to merge 2 commits into
Conversation
|
This fixes the duplicated SyntaxWarning from #141276, could someone take a look? |
itamaro
left a comment
There was a problem hiding this comment.
the fix generally looks good to me, thanks for the PR!
see inline comments regarding skipping wasted source load.
a few additional requests:
- this is an observable behavior change, which needs to be called out in the NEWS.
the semantic change - previously, py files with syntax errors in zip would raise from find_spec() and get_filename(). after this change, find_spec() and get_filename() would succeed, and only exec_module() would raise.
while this sounds like a significant behavior change, I think it's acceptable and even desirable, because it brings zipimport in line with the standard filesystem based finder/loader - but it does warrant more explicit coverage in NEWS.
this also makes it somewhat risky to backport, so I'd like @serhiy-storchaka's opinion here as well.
- I'd like to see a few more test cases covering the expected behavior, and preventing unintended future regressions, especially around the subtler corners.
a test that with a py with SyntaxError. assert that find_spec() and get_filename() succeed and return expected spec / filename, and exec_module() raises SyntaxError.
perhaps also have a test that calls get_filename() directly and asserts no warnings.
I see testBadMagic/testBadMTime already cover stale/bad-magic .pyc falling back to .py. could you add:
- valid
.pyc+.pywith a syntax error → spec picks the.pycand import succeeds - package
__init__.pywith a syntax error + a valid sibling module of the same name → origin is__init__.pyand exec raisesSyntaxError(no fallthrough to the module)
| except KeyError: | ||
| pass | ||
| else: | ||
| modpath = toc_entry[0] |
There was a problem hiding this comment.
in the non-bytecode path, this skips the compile, but still reads the py content from the archive only to throw it out in the compile_source=False path. instead, we can return before reading the py.
| modpath = toc_entry[0] | |
| if not isbytecode and not compile_source: | |
| return None, ispackage, modpath |
| if not compile_source: | ||
| return None, ispackage, modpath |
There was a problem hiding this comment.
not needed with the suggestion above
| if not compile_source: | |
| return None, ispackage, modpath |
| Fix duplicate :exc:`SyntaxWarning` messages when importing source modules | ||
| from ZIP archives. |
There was a problem hiding this comment.
some prose you could use here:
zipimporter.get_filename() and find_spec() no longer compile source modules; a SyntaxError in a zipped source module is now raised when the module is executed, matching the behavior of filesystem-based loaders.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Summary
Root cause
zipimporter.get_filename()called_get_module_code(), which compiled source while import machinery was creating the module spec. Module execution then compiled the same source again.Tests
./python.exe -m test test_zipimport -m test_syntax_warning -m testBadMagic -m testBadMTime./python.exe -m test test_zipimportRefs #141276
Duplicate-work check
No open or merged pull request referencing #141276 was found before submission.
AI assistance
OpenAI Codex assisted with investigation, implementation, testing, and review.