[mypyc] Avoid crash when importing unknown module with from import #10550
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add a test case that replicates the original crash by using `from x import y` when mypy doesn't realize that y is a module. That causes mypyc to generate code that crashes with an AttributeError when trying to import this.
Refactor the generation of code for checking if a specific module is loaded to a new function so that it can be used when we're generating the code to handle `from x import y` correctly.
Add 2 primitives that will be useful in improving the from import handling, which are: - PyImport_ImportModuleLevelObject - similar to the already existing import primitive but provides more arguments, with the most important argument being fromlist (the fourth argument), the list of items from the module to load - PyEval_GetLocals - gets the locals() dict, which we need for the third argument to the above primitive
Fix a crash that occurs when using `from x import y` if: - y is a module - mypy doesn't realize that y is a module (which can be caused by an ignore_missing_imports setting in a config file) This commit fixes that by using a different primitive for importing that handles from imports correctly, instead of relying on special casing `from x import y` if y is a module.
Add a test that makes sure mypyc handles `from x import y` imports correctly if it knows that y is a module and x is a package.
Don't special case `from x import y` if y is a module by checking if we've know about that module. This special casing doesn't work if we don't know if y is unknown because then we end up treating it like a regular attribute, and d523eed makes this special casing unnecessary anyway.
The driver for testFromImportWithUntypedModule is unnecessary because we're not doing anything involving interaction between compiled and not compiled code, so we can just use the default driver.
Fix irbuild tests that need to be changed due to the changes in d523eed to from import handling.
Change the irbuild tests to use WORD_SIZE for the initialization of some lists instead of hardcoded sizes so that the tests don't fail on 32-bit computers.
Add a return type to the test_import functions in the from import tests for consistency with the other tests.
Add a test that reproduces the CI failure that occurs when trying to run mypy after being compiled with mypyc. This failure happens when collecting tests because mypy fails with an AttributeError. For example, if the line in question is `from mypy import build`, it would fail with an AttributeError saying "module 'mypy' has no attribute 'build'". This is caused by the compiled code skipping the import call when the package being imported from (in this case, mypy) has already been imported, leading to the code trying to access an attribute (in this case, build) that hasn't been imported.
Remove the check whether the package being imported (the part before `import`) is already loaded, instead of avoiding the import entirely if it's already loaded as we do for regular imports. This avoids a crash when dealing with multiple modules imported from the same package because only the modules listed in the first import will actually be loaded. For example, if we have something like: ``` from pkg import mod1 from pkg import mod2 ``` The second import statement will skip the import because it thinks pkg is already imported, so attempts to access pkg.mod2 will fail with an AttributeError. It might be worth combining multiple from imports like the ones above into one from import statement, but that would probably involve analyzing a lot of separate code, which might slow down compile times, and would probably be more trouble than it's worth.
Fix the irbuild tests that broke due to the changes in 9bfd44e.
Change some irbuild tests to use WORD_SIZE instead of hardcoding the size of a pointer to prevent tests from failing on 32 bit.
msullivan
reviewed
Jun 8, 2021
mypyc/irbuild/builder.py
Outdated
| self.imports[id] = None | ||
|
|
||
| globals_dict = self.load_globals_dict() | ||
| locals_dict = self.call_c(get_locals, [], line) |
There was a problem hiding this comment.
Drop the whole 'PyEval_GetLocals thing and just pass in None for that. PyEval_GetLocals won't do anything that's even a little bit like being correct in our setting (I think it will return the locals dict for whatever the enclosing interpreted function is?), and the docs for __import__ reports " The standard implementation does not use its locals argument at all"
There was a problem hiding this comment.
This should be fixed in 8294834. I also removed the primitive for getting the locals dict because we aren't using it anymore.
The docs for __import__ (which is implemented using PyImport_ImportModuleLevelObject) say that CPython doesn't use the locals argument for anything, so we can just pass in NULL.
Fix irbuild tests that started failing due to previous change (not passing locals to PyImport_ImportModuleLevelObject).
We aren't using the locals() primitive anywhere after 8294834, so we can just remove it. The output of locals() is anyway not very useful for us, as it returns the locals of an interpreted CPython function, not our compiled functions, so it probably would return the locals of the interpreted function that called the compiled function.
pranavrajpal
added a commit
to pranavrajpal/mypy
that referenced
this pull request
Nov 4, 2021
The double import should be unnecessary because the bug it is talking about was fixed by python#10550.
JelleZijlstra
pushed a commit
that referenced
this pull request
Nov 4, 2021
The double import should be unnecessary because the bug it is talking about was fixed by #10550.
tushar-deepsource
pushed a commit
to DeepSourceCorp/mypy
that referenced
this pull request
Jan 20, 2022
The double import should be unnecessary because the bug it is talking about was fixed by python#10550.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes mypyc/mypyc#851
This fixes a bug where code compiled with mypyc would crash on from imports (
from x import y) if:The bug was caused by using getattr to import modules (i.e.
y = getattr(x, 'y')) and changing this toimport x.y as ywhen it can determine that y is a module. This doesn't work when we don't know that y is a module.I changed the from import handling to use something similar to the method shown in the
__import__docs. I also removed the special casing of from imports for modules (from x import ywhere y is a module) mentioned earlier, because these changes make that special casing unnecessary.Test Plan
I added 2 tests to mypyc/test-data/run-imports.test.