GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||
GeezerVison is showing github.com — gh-157614: Fix `lazy import a.b as c` incorrectly binding `a.b.b` by grgalex · Pull Request #157626 · python/cpython Conversation
Contributor
Root cause
FixCompute the base module before the The fast path must also be limited to the last Two-component imports and TestAdds It creates a package import a.b as c lazy import a.b as lazy_c assert lazy_c is c The test fails on Commits
Future Work / Not AddressedIf package For names with three or more components, an already-loaded module still resolves on first use rather than at the import statement because later Closes gh-157614.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsMove _run_subprocess_with_modules() and _assert_subprocess_ok() from FilterFunctionSignatureTests to LazyImportTestCase, so that any lazy import test can build a package tree in a temporary directory and run code against it in a subprocess. Signed-off-by: Georgios Alexopoulos <[email protected]> …e module _PyEval_LazyImportFrom() ran its sys.modules fast path on lz_from, which for lazy import a.b as c is the whole dotted name a.b rather than the module a that b has to be taken from. Changes to _PyEval_LazyImportFrom(): 1. Compute the base module before the fast path instead of after it, and look the attribute up on the base. The base is lz_from up to the first dot when lz_attr is NULL, lz_from + "." + lz_attr when lz_attr is a str, and lz_from itself for a from-import. 2. Add last_step and skip the fast path when it is false. It is true for a from-import and for a placeholder without lz_attr whose name has a single dot, and false when lz_attr is a str, that is on the later steps of a name with three or more components. Resolving one of those eagerly would hand a real module to the next IMPORT_FROM, which then imports the rest of the name right away instead of deferring it. 3. Hold the base in one owned reference, from, released on every exit, so the three _PyLazyImport_New() call sites collapse into one. Add a test that imports a.b, where a/b.py defines an attribute b, and checks that lazy import a.b as x binds the module. Signed-off-by: Georgios Alexopoulos <[email protected]>
grgalex
requested review from
DinoV,
Yhg1s,
markshannon and
pablogsal
as code owners
|
There was an error while loading. Please reload this page.
|
@grgalex Please could you resolve the merge conflict? @brittanyrey @pablogsal Please could you review this PR? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@hugovk I was tracking the progress (and merged PR) on the adjacent issue. I tested the very elegant merged solution by @brittanyrey and @pablogsal, and it appears to coincidentally also have fixed #157614, which the present PR tries to address. As such, I understand that the main code changes by this PR are no longer necessary. If it also makes sense to you, we can keep the regression test, as it is an interesting case. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Great, confirmed fixed :) I'll remove the release-blocker label and let @pablogsal to decide if we need the extra test. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
lazy import a.b as c incorrectly binds to a.b.b
There was an error while loading. Please reload this page.