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

typing.Protocol implementation means that isinstance([], collections.abc.Mapping) can sometimes evaluate to True #105280

Closed
AlexWaygood opened this issue Jun 4, 2023 · 1 comment
Assignees
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes stdlib Python modules in the Lib dir topic-typing type-bug An unexpected behavior, bug, or error

Comments

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 4, 2023

(In very specific circumstances)

Bug report

Due to some changes that have been made to the implementation of typing.Protocol in Python 3.12, whether or not isinstance([], collections.abc.Mapping) evaluates to True or False now depends on precisely when and if garbage collection happens.

Minimal repro:

>>> import gc, collections.abc, typing
>>> gc.disable()
>>> try:
...     class Foo(collections.abc.Mapping, typing.Protocol): pass
... except TypeError:
...     pass
...
>>> isinstance([], collections.abc.Mapping)
True

Why does this happen?! It's because Foo is an illegal class, so TypeError is raised during class initialisation here:

cpython/Lib/typing.py

Lines 1905 to 1912 in ce558e6

# ... otherwise check consistency of bases, and prohibit instantiation.
for base in cls.__bases__:
if not (base in (object, Generic) or
base.__module__ in _PROTO_ALLOWLIST and
base.__name__ in _PROTO_ALLOWLIST[base.__module__] or
issubclass(base, Generic) and getattr(base, '_is_protocol', False)):
raise TypeError('Protocols can only inherit from other'
' protocols, got %r' % base)

TypeError being raised here means that Foo never has __protocol_attrs__ set on it. But, the error is raised after class construction, meaning that if garbage collection happens at the wrong time, a reference to Foo is still retained in collections.abc.Mapping.__subclasses__(). This then creates problems in the isinstance() check, because of some behaviour in the abc module where it iterates through all of the subclasses of collections.abc.Mapping in order to determine whether an object is an instance of collections.abc.Mapping.

>>> import gc, collections.abc, typing
>>> gc.disable()
>>> try:
...     class Foo(collections.abc.Mapping, typing.Protocol): pass
... except TypeError:
...     pass
...
>>> x = collections.abc.Mapping.__subclasses__()
>>> x
[<class 'collections.abc.MutableMapping'>, <class '__main__.Foo'>]
>>> x[-1].__protocol_attrs__
set()

The fix is to raise TypeError before the class has actually been created, rather than during class initialisation.

Why does this matter?

This might seem like an absurdly specific bug report, but it would be good to see it fixed. It was causing bizarre test failures in the typing_extensions backport. The issue was that we did this in one test method:

    def test_protocols_bad_subscripts(self):
        T = TypeVar('T')
        S = TypeVar('S')
        with self.assertRaises(TypeError):
            class P(typing.Mapping[T, S], Protocol[T]): pass

...And that final P class wasn't being cleaned up by the garbage collector immediately after that test having been run. That then caused an unrelated test elsewhere to fail due to the fact that isinstance([], collections.abc.Mapping) was evaluating to True, and it was very hard to figure out why.

Linked PRs

@AlexWaygood AlexWaygood added type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir topic-typing 3.12 bugs and security fixes 3.13 bugs and security fixes labels Jun 4, 2023
@AlexWaygood AlexWaygood self-assigned this Jun 4, 2023
AlexWaygood added a commit that referenced this issue Jun 5, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 5, 2023
…ays evaluates to `False` (pythonGH-105281)

(cherry picked from commit 08756db)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
AlexWaygood added a commit that referenced this issue Jun 5, 2023
…ways evaluates to `False` (GH-105281) (#105318)

gh-105280: Ensure `isinstance([], collections.abc.Mapping)` always evaluates to `False` (GH-105281)
(cherry picked from commit 08756db)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@AlexWaygood
Copy link
Member Author

Let us never again talk about the brief period in time when a list could sometimes be a mapping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes stdlib Python modules in the Lib dir topic-typing type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

1 participant