Fix crash in ExplicitNamespacePackageFinder
#1714
Conversation
Pull Request Test Coverage Report for Build 2769984269
💛 - Coveralls |
astroid/interpreter/_import/spec.py
Outdated
| @@ -201,7 +201,12 @@ def find_module( | |||
| if processed: | |||
| modname = ".".join(processed + [modname]) | |||
| if util.is_namespace(modname) and modname in sys.modules: | |||
| submodule_path = sys.modules[modname].__path__ | |||
| try: | |||
There was a problem hiding this comment.
Do we understand why __path__ isn't available?
https://docs.python.org/3/reference/import.html#path__
If the module is a package (either regular or namespace), the module object’s __path__ attribute must be set.
We probably shouldn't crash, but is this "normal" behaviour?
https://docs.python.org/3/library/importlib.html#importlib.machinery.ModuleSpec
Thus it is possible to update the module’s __path__ at runtime, and this will not be automatically reflected in __spec__.submodule_search_locations.
__path__ not being available seems to indicate that this isn't a package and thus perhaps we shouldn't return a ModuleSpec with a PY_NAMESPACE type?
There was a problem hiding this comment.
Just dashing off a quick reply that when I looked into it I thought it was because urllib3 was doing magic with six, so I wasn't concerned, but I can try to get a real answer :-)
There was a problem hiding this comment.
Coming back to this, I guess I don't want to take on the burden of understanding _SixMetaPathImporter inside and out -- there are four bugs filed against it anyway, so I wouldn't trust it to work in all scenarios, esp. if libraries like urllib3 are doing things on top of it. I'd rather just catch the error until someone can demonstrate that astroid is at fault.
There was a problem hiding this comment.
Sometime the code is too dynamic to be reasonably understood by astroid. Understanding this kind of dynamic code would be very good and reduce the use of brain and workarounds but I agree with @jacobtylerwalls. Ready to release the next patch version of astroid as soon as we merge this :)
There was a problem hiding this comment.
Note that this is still on my radar but I haven't been around much. Should be able to get to this in a 10 days or so. My main question is whether the check for __path__ should be included in the is_namespace function.
There was a problem hiding this comment.
Thanks Daniel, looking again, I think you have a point. I pushed a new commit to fix this in is_namespace(). ModuleSpec has submodule_search_locations as a better name than __path__ per PEP 451.
|
It looks pretty similar to an issue we have with pypy in pylint's main https://github.com/PyCQA/pylint/runs/7428964268?check_suite_focus=true |
Maybe astroid 2.12.3 will fix this because of pylint-dev/astroid#1714
Maybe astroid 2.12.3 will fix this because of pylint-dev/astroid#1714
Maybe astroid 2.12.3 will fix this because of pylint-dev/astroid#1714
| @pytest.mark.skipif(not HAS_URLLIB3, reason="This test requires urllib3.") | ||
| def test_file_info_from_modpath__SixMetaPathImporter() -> None: | ||
| pytest.raises( | ||
| ImportError, |
There was a problem hiding this comment.
So we no longer have a finder that will find the module created by _SixMetaPathImporter, but that's better than crashing while trying to find it with ExplicitNamespacePackageFinder (or incorrectly finding it with it!)
jacobtylerwalls
added
the
pylint-tested
|
|
||
| return found_spec.origin is None | ||
| return ( | ||
| found_spec is not None |
There was a problem hiding this comment.
Should we use last_submodule_search_locations here?
There was a problem hiding this comment.
I don't know, I suspect (without looking too closely) that could create false positives if a portion of the path was a valid namespace package but the rest of the path wasn't. That variable is really just a hack to get around the KeyError described in the comment, I wouldn't trust it to represent more than that.
|
I won't be able to release astroid until Friday, feel free to release if you want @jacobtylerwalls :) |
|
Cool. Looking forward to it :) |
* Add skip if no `six` * `urllib3` does appear to be required * Check `submodule_search_locations` Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
* Add skip if no `six` * `urllib3` does appear to be required * Check `submodule_search_locations` Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Steps
Description
Some weirdness with
_SixMetaPathImporterdemonstrated thatastroid.interpreter._import.util.is_namespaceneeds to check the presence ofsubmodule_search_locationsbefore reporting something to be a namespace package.Type of Changes
Related Issue
Closes #1708