Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Loader with PEP 451 _Loader protocol in _importlib_modulespec.pyi annotations. #2626

Merged
merged 2 commits into from
Dec 3, 2018
Merged

Conversation

brandtbucher
Copy link
Member

For built-in imports, ModuleSpec.loader is an uninstantiated BuiltinImporter class.

For built-in imports, ModuleSpec.loader is an uninstantiated BuiltinImporter class, not an instance.
@brandtbucher
Copy link
Member Author

Same for frozen imports and FrozenImporter.

@brandtbucher brandtbucher changed the title Change ModuleSpec.loader type from Optional[Loader] to Any. Change importlib.ModuleSpec.loader type from Optional[Loader] to Any. Nov 23, 2018
@srittau
Copy link
Collaborator

srittau commented Nov 29, 2018

I am not sure I like this change. The importlib documentation states that loaders are defined as objects having a load_module() function. I am not an importlib expert, but it sounds to me that replacing the Loader defined in importlib (but not that in importlib.abc) with a protocol and using that in the annotation of ModuleSpec.loader is the better option. isinstance() checks for asserting the correct type seem reasonable in user code in this instance.

@brandtbucher
Copy link
Member Author

brandtbucher commented Nov 30, 2018

Hi, thanks for the feedback @srittau.

I'm a bit confused with your suggestion - it looks to me like the only Loader in importlib is the one defined in importlib.abc. Are you saying we should create a new one-off _Loader protocol for this specific case?

@srittau
Copy link
Collaborator

srittau commented Dec 2, 2018

Sorry, I was a bit confused from the circular import prevention going on in the stubs. But yes I was suggesting to add a _Loader protocol, which just has a load_module() function (per PEP 451) and use that instead of the concrete Loader implementation in ModuleSpec.

This protocol implements the "loader" interface defined in PEP 451.
@brandtbucher brandtbucher changed the title Change importlib.ModuleSpec.loader type from Optional[Loader] to Any. Replace Loader with PEP 451 _Loader protocol in _importlib_modulespec.pyi annotations. Dec 3, 2018
@brandtbucher
Copy link
Member Author

I think that's a great solution; I actually wasn't familiar with Protocol before this. I also saw that I forgot to update the corresponding annotation in ModuleType.__loader__.

Here's a patch.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thank you!

@srittau srittau merged commit ac8f5da into python:master Dec 3, 2018
@brandtbucher brandtbucher deleted the patch-1 branch December 3, 2018 20:29
yedpodtrzitko pushed a commit to yedpodtrzitko/typeshed that referenced this pull request Jan 23, 2019
fengguang pushed a commit to 0day-ci/linux that referenced this pull request Oct 22, 2021
Currently, we have these errors:
$ mypy ./tools/testing/kunit/*.py
tools/testing/kunit/kunit_kernel.py:213: error: Item "_Loader" of "Optional[_Loader]" has no attribute "exec_module"
tools/testing/kunit/kunit_kernel.py:213: error: Item "None" of "Optional[_Loader]" has no attribute "exec_module"
tools/testing/kunit/kunit_kernel.py:214: error: Module has no attribute "QEMU_ARCH"
tools/testing/kunit/kunit_kernel.py:215: error: Module has no attribute "QEMU_ARCH"

exec_module
===========

pytype currently reports no errors, but that's because there's a comment
disabling it on 213.

This is due to python/typeshed#2626.
The fix is to assert the loaded module implements the ABC
(abstract base class) we want which has exec_module support.

QEMU_ARCH
=========

pytype is fine with this, but mypy is not:
python/mypy#5059

Add a check that the loaded module does indeed have QEMU_ARCH.
Note: this is not enough to appease mypy, so we also add a comment to
squash the warning.

Signed-off-by: Daniel Latypov <dlatypov@google.com>
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.

2 participants