Skip to content

Conversation

chadrik
Copy link
Contributor

@chadrik chadrik commented Oct 5, 2020

Description

Running stubgen on modules that import multiple packages using aliases did not produce the correct results:

Input:

import p.a as a
import p.b as b

x: a.X
y: b.Y

Result:

import p.b as a  # <--- wrong module!
import p.b as b

x: a.X
y: b.Y

Test Plan

I added a bunch of new tests to stubgen.test.

I had to change one of the expected results: testConditionalImportAll_semanal. Previously, if more than one module import introduced the same name into the namespace, the data structures used to track imports were simply allowed to become corrupted, and the generator would produce import FIXME as whatever. After correcting the internal state to fix the issue that prompted this PR, I realized it would be difficult to maintain the current output for this test, so I chose to change the behavior. I went with what I think is a sane solution: if multiple import statements target the same name, rather than produce invalid output, use the last import encountered. The last import would be the one used at runtime. I also considered using the first import, since this is what mypy would use.

@chadrik chadrik force-pushed the stubgen-fix-import branch from 96009e7 to c228073 Compare October 5, 2020 04:54
importing multiple packages using aliases (import p.a as a) did not work correctly.
@chadrik chadrik force-pushed the stubgen-fix-import branch from c228073 to 5e7f945 Compare October 5, 2020 06:07
@@ -1577,7 +1577,7 @@ else:
import cookielib

[out]
import FIXME as cookielib
import cookielib as cookielib
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for reference, here is the complete test:


[case testConditionalImportAll_semanal]
__all__ = ['cookielib']

if object():
    from http import cookiejar as cookielib
else:
    import cookielib

[out]
import cookielib as cookielib

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding and fixing this! Despite the review request, this is actually my first time really looking at stubgen code...
Anyway, this looks good to me. Your (very thorough) test cases all check out and the changes make sense.

I was a little bit worried about the FIXME test case that needed changing... On first read, I thought it was intended as a feature for stubgen to highlight conditional imports with a FIXME. But it looks like that isn't what's happening here, since we're happy enough to clobber reverse_alias. I.e., the change in behaviour here seems consistent with things stubgen does in other places.

I'll leave open for a day or two, just in case someone else who's worked with stubgen more has something to say.

@hauntsaninja hauntsaninja merged commit 4a89008 into python:master Oct 7, 2020
@chadrik chadrik deleted the stubgen-fix-import branch August 22, 2022 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants