Skip to content

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

Merged
srittau merged 2 commits into
python:masterfrom
brandtbucher:patch-1
Dec 3, 2018
Merged

Replace Loader with PEP 451 _Loader protocol in _importlib_modulespec.pyi annotations.#2626
srittau merged 2 commits into
python:masterfrom
brandtbucher:patch-1

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Nov 23, 2018

Copy link
Copy Markdown
Member

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

@brandtbucher
Change ModuleSpec.loader type to Any.
8d239b1 
For built-in imports, ModuleSpec.loader is an uninstantiated BuiltinImporter class, not an instance.
@brandtbucher

brandtbucher commented Nov 23, 2018

Copy link
Copy Markdown
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

srittau commented Nov 29, 2018

Copy link
Copy Markdown
Collaborator

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

brandtbucher commented Nov 30, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

srittau commented Dec 2, 2018

Copy link
Copy Markdown
Collaborator

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.

@brandtbucher
Add _Loader protocol.
bf23345 
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

brandtbucher commented Dec 3, 2018

Copy link
Copy Markdown
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.

srittau
srittau approved these changes Dec 3, 2018
View reviewed changes

@srittau srittau left a comment

Copy link
Copy Markdown
Collaborator

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
@brandtbucher Jiri Suchan
Replace Loader with PEP 451 _Loader protocol in _importlib_modulespec…
51ebe72 
….pyi annotations. (python#2626)
@rchen152 rchen152 mentioned this pull request May 20, 2019
Closed
fengguang pushed a commit to 0day-ci/linux that referenced this pull request Oct 22, 2021
@dlatypov @intel-lab-lkp
kunit: tool: fix typecheck errors about loading qemu configs
ead3fd6 
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srittau srittau srittau approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@brandtbucher @srittau