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

Should importlib.util.spec_from_loader use a Protocol for the loader parameter ? #11882

Closed
Avasam opened this issue May 9, 2024 · 2 comments · Fixed by #11890
Closed

Should importlib.util.spec_from_loader use a Protocol for the loader parameter ? #11882

Avasam opened this issue May 9, 2024 · 2 comments · Fixed by #11890

Comments

@Avasam
Copy link
Sponsor Collaborator

Avasam commented May 9, 2024

Essentially a question asked by @abravalheri at https://github.com/pypa/setuptools/pull/4246/files#r1593000435 .

  1. Should importlib.util.spec_from_loader use a Protocol for the loader parameter ?
  2. Would it make sense for pkg_resources.extern.VendorImporter and setuptools.extern.VendorImporter to subclass importlib.abc.Loader ?
  3. Just type: ignore[arg-type] the call to importlib.util.spec_from_loader ?
@abravalheri
Copy link
Contributor

abravalheri commented May 10, 2024

Regarding 1:

After further consideration of the Python docs, we can see that the text references the glossary which describes loader as "an object that loads a module. It must define a method named load_module()" and points to PEP 302 which describes "The Importer Protocol"1.

We can see that in the standard library some perfectly valid loaders do not inherit from this abc:

>>> from importlib.abc import Loader
>>> import math
>>> math.__loader__
<class '_frozen_importlib.BuiltinImporter'>
>>> isinstance(math.__loader__, Loader)
False
>>> import abc
>>> abc.__loader__
<class '_frozen_importlib.FrozenImporter'>
>>> isinstance(abc.__loader__, Loader)
False
>>> import importlib.util
>>> spec = importlib.util.spec_from_loader('math', math.__loader__)
>>> module = importlib.util.module_from_spec(spec)
>>> spec.loader.exec_module(module)
>>> module.sin(module.pi / 2)
1.0

We also can see that the type hints that typeshed provides for some pkgutil functions are incorrect:

>>> import pkgutil
>>> print(loader := pkgutil.get_loader('os'), f"{isinstance(loader, Loader)=}")
<class '_frozen_importlib.FrozenImporter'> isinstance(loader, Loader)=False
>>> print(loader := pkgutil.get_loader('stat'), f"{isinstance(loader, Loader)=}")
<class '_frozen_importlib.FrozenImporter'> isinstance(loader, Loader)=False
>>> print(loader := pkgutil.get_loader('math'), f"{isinstance(loader, Loader)=}")
<class '_frozen_importlib.BuiltinImporter'> isinstance(loader, Loader)=False
>>> print(loader := pkgutil.get_loader('abc'), f"{isinstance(loader, Loader)=}")
<class '_frozen_importlib.FrozenImporter'> isinstance(loader, Loader)=False
>>> print(loader := pkgutil.get_loader('builtins'), f"{isinstance(loader, Loader)=}")
<class '_frozen_importlib.BuiltinImporter'> isinstance(loader, Loader)=False
>>> print(loader := pkgutil.get_loader('site'), f"{isinstance(loader, Loader)=}")
<class '_frozen_importlib.FrozenImporter'> isinstance(loader, Loader)=False
>>> from importlib.abc import PathEntryFinder, MetaPathFinder
>>> print(importer := next(pkgutil.iter_importers('stat')), f"{isinstance(importer, (PathEntryFinder, MetaPathFi
nder))=}")
<class '_frozen_importlib.BuiltinImporter'> isinstance(importer, (PathEntryFinder, MetaPathFinder))=False
>>> print(importer := next(pkgutil.iter_importers('math')), f"{isinstance(importer, (PathEntryFinder, MetaPathFi
nder))=}")
<class '_frozen_importlib.BuiltinImporter'> isinstance(importer, (PathEntryFinder, MetaPathFinder))=False

These are all evidences that the definition in typeshed is incorrect and leads to false positives/negatives during type checking.

I think that in general the best assumption is that importlib.abc abstract classes should not be used for other signatures in typeshed. Instead corresponding protocols should be defined.

The likely reason is that the PEPs related to the importlib where designed in a time when a formal definition of Protocol was not available for Python and abc was used a conceptual replacement... Some of them serve as informational purposes (specially the generic ones like Loader, PathEntryFinder, MetaPathFinder) and others are just handy ways of sharing optional implementation details.

Footnotes

  1. Note that even the word "protocol" is used in the standard... We can conclude that the abstract classes are provided as a documentation device and as means of sharing code among a few implementation of the standard library.

@srittau
Copy link
Collaborator

srittau commented May 10, 2024

Using a protocol sounds about right to me for the reasons @abravalheri gave, especially due to the glossary definition and the fact that this is such a simple protocol.

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 a pull request may close this issue.

3 participants