Skip to content

ModuleType.__repr__ can fail on py312+ #112414

Closed
Closed
ModuleType.__repr__ can fail on py312+#112414
Labels
3.12only security fixes3.13only security fixestopic-importlibtype-bugAn unexpected behavior, bug, or error

Description

@AlexWaygood
AlexWaygood
opened on Nov 25, 2023

Bug report

Bug description:

Calling repr() on a ModuleType instance can fail with AttributeError on py312+, due to changes made in #98870. This is because the implementation of importlib._bootstrap._module_repr_from_spec assumes that all loaders will have a _path attribute. But if the module had a custom loader, that won't necessarily hold true: _path is an undocumented, internal, private attribute that CPython's import loaders have, but custom third-party loaders won't necessarily have.

An easy way to see this is the following (in a py312 venv with setuptools installed):

Python 3.12.0 (tags/v3.12.0:0fb18b0, Oct  2 2023, 13:03:39) [MSC v.1935 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from setuptools._vendor import packaging
>>> repr(packaging)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<frozen importlib._bootstrap>", line 545, in _module_repr
  File "<frozen importlib._bootstrap>", line 830, in _module_repr_from_spec
AttributeError: 'VendorImporter' object has no attribute '_path'

setuptools implements the custom VendorImporter loader here: https://github.com/pypa/setuptools/blob/b1bf87be7869df40872e947e27296ef87e3125ae/setuptools/extern/__init__.py#L5-L70. Instances of VendorImporter do not have a _path attribute.

This kind of thing trips up external tools such as mypy's stubtest.py script, which calls repr() on arbitrary instances of types.ModuleType on certain code paths and (reasonably) expects that to always succeed: https://github.com/python/mypy/blob/1200d1d956e589a0a33c86ef8a7cb3f5a9b64f1f/mypy/stubtest.py#L224

Cc. @FFY00 and @jaraco, as participants in #98139 (the issue #98870 was linked to)

CPython versions tested on:

3.12

Operating systems tested on:

Windows

Linked PRs

Activity

  1. AlexWaygood
    added
    type-bugAn unexpected behavior, bug, or error
    topic-importlib
    3.12only security fixes
    3.13only security fixes
    on Nov 25, 2023

  2. AlexWaygood commented on Nov 25, 2023

    @AlexWaygood
    AlexWaygood
    on Nov 25, 2023
    MemberAuthor

    Maybe something like this would be a reasonable fix?

    --- a/Lib/importlib/_bootstrap.py
+++ b/Lib/importlib/_bootstrap.py
@@ -826,8 +826,10 @@ def _module_repr_from_spec(spec):
     if spec.origin is None:
         if spec.loader is None:
             return f'<module {name!r}>'
-        else:
+        elif hasattr(spec.loader, "_path"):
             return f'<module {name!r} (namespace) from {list(spec.loader._path)}>'
+        else:
+            return f'<module {name!r} ({spec.loader!r})>'

  3. TeamSpen210 commented on Nov 25, 2023

    @TeamSpen210
    TeamSpen210
    on Nov 25, 2023

    Maybe it'd be better to do an isinstance() check. Then someone defining _path in their own loader wouldn't have the repr mysteriously change. Seems like an entirely reasonable attribute to have, and if it was a string you'd get rather silly results.

  4. AlexWaygood commented on Nov 26, 2023

    @AlexWaygood
    AlexWaygood
    on Nov 26, 2023
    MemberAuthor

    Maybe it'd be better to do an isinstance() check. Then someone defining _path in their own loader wouldn't have the repr mysteriously change. Seems like an entirely reasonable attribute to have, and if it was a string you'd get rather silly results.

    Makes sense. That implies a diff like this:

    --- a/Lib/importlib/_bootstrap.py
+++ b/Lib/importlib/_bootstrap.py
@@ -824,10 +824,16 @@ def _module_repr_from_spec(spec):
     """Return the repr to use for the module."""
     name = '?' if spec.name is None else spec.name
     if spec.origin is None:
-        if spec.loader is None:
+        loader = spec.loader
+        if loader is None:
             return f'<module {name!r}>'
+        elif (
+            _bootstrap_external is not None
+            and isinstance(loader, _bootstrap_external.NamespaceLoader)
+        ):
+            return f'<module {name!r} (namespace) from {list(loader._path)}>'
         else:
-            return f'<module {name!r} (namespace) from {list(spec.loader._path)}>'
+            # the loader could be a custom third-party object without a `_path` attribute
+            return f'<module {name!r} ({loader!r})>'
  5. AlexWaygood
    added a commit that references this issue on Nov 26, 2023

    pythongh-112414: Fix `AttributeError` when calling `repr()` on a modu…

    54f03c8

  6. AlexWaygood commented on Nov 26, 2023

    @AlexWaygood
    AlexWaygood
    on Nov 26, 2023
    MemberAuthor

    Another easy way to observe this bug: apply this diff to main:

    diff --git a/Lib/test/test_importlib/import_/test___loader__.py b/Lib/test/test_importlib/import_/test___loader__.py
index 858b37effc..4ae60574ed 100644
--- a/Lib/test/test_importlib/import_/test___loader__.py
+++ b/Lib/test/test_importlib/import_/test___loader__.py
@@ -23,6 +23,7 @@ def test___loader__(self):
         with util.uncache('blah'), util.import_state(meta_path=[loader]):
             module = self.__import__('blah')
         self.assertEqual(loader, module.__loader__)
+        repr(module)

    And then run python -m test test_importlib.import_.test___loader__ -v:

    Test output 
    Using random seed: 2645428552
0:00:00 Run 1 test sequentially
0:00:00 [1/1] test_importlib.import_.test___loader__
test___loader__ (test.test_importlib.import_.test___loader__.Frozen_SpecLoaderAttributeTests.test___loader__) ... ERROR
test___loader__ (test.test_importlib.import_.test___loader__.Source_SpecLoaderAttributeTests.test___loader__) ... ERROR

======================================================================
ERROR: test___loader__ (test.test_importlib.import_.test___loader__.Frozen_SpecLoaderAttributeTests.test___loader__)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\alexw\coding\cpython\Lib\test\test_importlib\import_\test___loader__.py", line 26, in test___loader__
    repr(module)
  File "<frozen importlib._bootstrap>", line 545, in _module_repr
  File "<frozen importlib._bootstrap>", line 830, in _module_repr_from_spec
AttributeError: 'SpecLoaderMock' object has no attribute '_path'

======================================================================
ERROR: test___loader__ (test.test_importlib.import_.test___loader__.Source_SpecLoaderAttributeTests.test___loader__)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\alexw\coding\cpython\Lib\test\test_importlib\import_\test___loader__.py", line 26, in test___loader__
    repr(module)
  File "<frozen importlib._bootstrap>", line 545, in _module_repr
  File "<frozen importlib._bootstrap>", line 830, in _module_repr_from_spec
AttributeError: 'SpecLoaderMock' object has no attribute '_path'

----------------------------------------------------------------------
Ran 2 tests in 0.022s

FAILED (errors=2)
test test_importlib.import_.test___loader__ failed
test_importlib.import_.test___loader__ failed (2 errors)

== Tests result: FAILURE ==

1 test failed:
    test_importlib.import_.test___loader__

Total duration: 254 ms
Total tests: run=2
Total test files: run=1/1 failed=1
Result: FAILURE

    I've filed #112425 to fix it.

  7. hauntsaninja
    added a commit that references this issue on Nov 27, 2023

    pythongh-112414: Fix repr for custom loader

    e21726d
  8. AlexWaygood
    added a commit that references this issue on Nov 27, 2023

    gh-112414: Fix `AttributeError` when calling `repr()` on a namespace …

    0622839
  9. miss-islington
    added a commit that references this issue on Nov 27, 2023

    pythongh-112414: Fix `AttributeError` when calling `repr()` on a name…

    f28e358
  10. AlexWaygood
    closed this as completedon Nov 27, 2023
  11. AlexWaygood
    added 3 commits that reference this issue on Nov 27, 2023

    [3.12] gh-112414: Fix `AttributeError` when calling `repr()` on a nam…

    f9861e6

    pythongh-112414: Add additional unit tests for calling `repr()` on a …

    e8ae7f0

    gh-112414: Add additional unit tests for calling `repr()` on a namesp…

    cf20540
  12. miss-islington
    added a commit that references this issue on Nov 28, 2023

    pythongh-112414: Add additional unit tests for calling `repr()` on a …

    39d5c89
  13. AlexWaygood
    added a commit that references this issue on Nov 28, 2023

    [3.12] gh-112414: Add additional unit tests for calling `repr()` on a…

    daf9ff9
  14. aisk
    added 2 commits that reference this issue on Feb 11, 2024

    pythongh-112414: Fix `AttributeError` when calling `repr()` on a name…

    e030110

    pythongh-112414: Add additional unit tests for calling `repr()` on a …

    7de710c
  15. Glyphack
    added 2 commits that reference this issue on Sep 2, 2024

    pythongh-112414: Fix `AttributeError` when calling `repr()` on a name…

    f5d2397

    pythongh-112414: Add additional unit tests for calling `repr()` on a …

    ba9e5f2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    3.12only security fixes3.13only security fixestopic-importlibtype-bugAn unexpected behavior, bug, or error

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions