Skip to content

Typing: runtime-checkable protocols are broken on main #104555

Description

@AlexWaygood

PEP-695 protocols don't work as intended:

Here's the behaviour you get with protocols that use pre-PEP 695 syntax, which is correct:

>>> from typing import Protocol, runtime_checkable, TypeVar
>>> T_co = TypeVar("T_co", covariant=True)
>>> @runtime_checkable
... class SupportsAbsOld(Protocol[T_co]):
...     def __abs__(self) -> T_co:
...         ...
...
>>> isinstance(0, SupportsAbsOld)
True
>>> issubclass(float, SupportsAbsOld)
True

And here's the behaviour you get on main with protocols that use PEP 695 syntax, which is incorrect:

>>> @runtime_checkable
... class SupportsAbsNew[T_co](Protocol):
...     def __abs__(self) -> T_co:
...         ...
...
>>> isinstance(0, SupportsAbsNew)
False
>>> issubclass(float, SupportsAbsNew)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\alexw\coding\cpython\Lib\abc.py", line 123, in __subclasscheck__
    return _abc_subclasscheck(cls, subclass)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\alexw\coding\cpython\Lib\typing.py", line 1875, in _proto_hook
    raise TypeError("Protocols with non-method members"
TypeError: Protocols with non-method members don't support issubclass()

Linked PRs

Activity

  1. AlexWaygood commented on May 16, 2023

    @AlexWaygood
    MemberAuthor

    (I'm working on a fix)

  2. JelleZijlstra commented on May 16, 2023

    @JelleZijlstra
    Member

    Thanks for catching! I suppose this is about __type_params__?

  3. AlexWaygood commented on May 16, 2023

    @AlexWaygood
    MemberAuthor

    Thanks for catching! I suppose this is about __type_params__?

    Yes.

  4. AlexWaygood commented on May 16, 2023

    @AlexWaygood
    MemberAuthor

    I'm getting very confused here. The fix for the original issue is simple, and I've fixed it locally. But as part of the PR, I tried adding this test, and the last assertion fails (the test reports that TypeError wasn't raised):

        def test_pep695_generic_protocol_noncallable_members(self):
            @runtime_checkable
            class Spam[T](Protocol):
                x: T
    
            class Eggs[T]:
                def __init__(self, x: T) -> None:
                    self.x = x
    
            self.assertIsInstance(Eggs(42), Spam)
            with self.assertRaises(TypeError):
                issubclass(Eggs, Spam)

    But I can't reproduce the failure in the REPL:

    >>> from typing import *
    >>> @runtime_checkable
    ... class Spam[T](Protocol):
    ...     x: T
    ... 
    ...     
    >>> class Eggs[T]:
    ...     def __init__(self, x: T) -> None:
    ...         self.x = x
    ... 
    ...         
    >>> issubclass(Eggs, Spam)
    Traceback (most recent call last):
      File "<pyshell#5>", line 1, in <module>
        issubclass(Eggs, Spam)
      File "C:\Users\alexw\coding\cpython\Lib\abc.py", line 123, in __subclasscheck__
        return _abc_subclasscheck(cls, subclass)
      File "C:\Users\alexw\coding\cpython\Lib\typing.py", line 1875, in _proto_hook
        raise TypeError("Protocols with non-method members"
    TypeError: Protocols with non-method members don't support issubclass()
  5. AlexWaygood commented on May 16, 2023

    @AlexWaygood
    MemberAuthor

    Anyway, I'll submit a PR without that test for now...

  6. JelleZijlstra commented on May 16, 2023

    @JelleZijlstra
    Member

    I'll look into this more.

  7. AlexWaygood commented on May 16, 2023

    @AlexWaygood
    MemberAuthor

    Okay, cool. I'll enable automerge on my PR for now, as it definitely gets us closer to correct behaviour.

  8. JelleZijlstra commented on May 16, 2023

    @JelleZijlstra
    Member

    The issue you saw also reproduces for me without PEP 695 syntax. It seems to be due to the _allow_reckless_class_checks check, which somehow thinks the caller is abc if we're in a test, but not if we're in the REPL.

  9. AlexWaygood commented on May 16, 2023

    @AlexWaygood
    MemberAuthor

    Aha, so we've discovered a bug, it just isn't a new bug.

  10. JelleZijlstra commented on May 16, 2023

    @JelleZijlstra
    Member

    It's possible this bug was introduced by the fact that Generic is now a C class, haven't tried on 3.11 yet.

  11. AlexWaygood commented on May 16, 2023

    @AlexWaygood
    MemberAuthor

    Yes, looks like the bug is new in 3.12. Running this test file passes on 3.11, but not on main:

    import unittest
    from typing import runtime_checkable, Generic, Protocol, TypeVar
    
    T = TypeVar("T")
    
    class TestProtocols(unittest.TestCase):
        def test_pep695_generic_protocol_noncallable_members(self):
            @runtime_checkable
            class Spam(Protocol[T]):
                x: T
    
            class Eggs(Generic[T]):
                def __init__(self, x: T) -> None:
                    self.x = x
    
            self.assertIsInstance(Eggs(42), Spam)
            with self.assertRaises(TypeError):
                issubclass(Eggs, Spam)
    
    
    if __name__ == "__main__":
        unittest.main()
  12. 6 remaining items

  13. JelleZijlstra commented on May 16, 2023

    @JelleZijlstra
    Member
    47753ecde21b79b5c5f11d883946fda2a340e427 is the first bad commit
    commit 47753ecde21b79b5c5f11d883946fda2a340e427
    Author: Alex Waygood <Alex.Waygood@Gmail.com>
    Date:   Wed Apr 5 10:07:30 2023 +0100
    
        gh-74690: typing: Simplify and optimise `_ProtocolMeta.__instancecheck__` (#103159)
    
     Lib/typing.py | 14 +++-----------
     1 file changed, 3 insertions(+), 11 deletions(-)
    

    47753ec

  14. AlexWaygood commented on May 16, 2023

    @AlexWaygood
    MemberAuthor
    47753ecde21b79b5c5f11d883946fda2a340e427 is the first bad commit
    commit 47753ecde21b79b5c5f11d883946fda2a340e427
    Author: Alex Waygood <Alex.Waygood@Gmail.com>
    Date:   Wed Apr 5 10:07:30 2023 +0100
    
        gh-74690: typing: Simplify and optimise `_ProtocolMeta.__instancecheck__` (#103159)
    
     Lib/typing.py | 14 +++-----------
     1 file changed, 3 insertions(+), 11 deletions(-)
    

    (I am, again, working on a fix :)

  15. added a commit that references this issue on May 16, 2023
  16. AlexWaygood commented on May 16, 2023

    @AlexWaygood
    MemberAuthor

    Incredible stuff:

    Python 3.12.0a7+ (heads/main:1163782868, May 16 2023, 18:09:28) [MSC v.1932 64 bit (AMD64)] on win32
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from typing import *
    >>> @runtime_checkable
    ... class Foo(Protocol):
    ...     x: int
    ...
    >>> class Bar:
    ...     x = 42
    ...
    >>> issubclass(Bar, Foo)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "C:\Users\alexw\coding\cpython\Lib\abc.py", line 123, in __subclasscheck__
        return _abc_subclasscheck(cls, subclass)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "C:\Users\alexw\coding\cpython\Lib\typing.py", line 1875, in _proto_hook
        raise TypeError("Protocols with non-method members"
    TypeError: Protocols with non-method members don't support issubclass()
    >>> isinstance(Bar(), Foo)
    True
    >>> issubclass(Bar, Foo)
    False
  17. sunmy2019 commented on May 16, 2023

    @sunmy2019
  18. AlexWaygood commented on May 16, 2023

    @AlexWaygood
    MemberAuthor

    @sunmy2019 I was just about to file 2601138 as a PR, which fixes things, but do you think we could fix it by just adjusting the depth value we pass to _allow_reckless_class_checks()?

  19. sunmy2019 commented on May 16, 2023

    @sunmy2019
    Member

    Oh sorry, ignore my previous comment.

  20. sunmy2019 commented on May 16, 2023

    @sunmy2019
    Member

    Confirmed that the code path that does not raise TypeError goes through L606.

    cpython/Modules/_abc.c

    Lines 589 to 608 in 03e3c34

    /* 2. Check negative cache; may have to invalidate. */
    if (impl->_abc_negative_cache_version < abc_invalidation_counter) {
    /* Invalidate the negative cache. */
    if (impl->_abc_negative_cache != NULL &&
    PySet_Clear(impl->_abc_negative_cache) < 0)
    {
    goto end;
    }
    impl->_abc_negative_cache_version = abc_invalidation_counter;
    }
    else {
    incache = _in_weak_set(impl->_abc_negative_cache, subclass);
    if (incache < 0) {
    goto end;
    }
    if (incache > 0) {
    result = Py_False;
    goto end;
    }
    }

    @AlexWaygood You are right!

  21. AlexWaygood commented on May 16, 2023

    @AlexWaygood
    MemberAuthor

    @sunmy2019 thanks for checking!

  22. sunmy2019 commented on May 16, 2023

    @sunmy2019
    Member

    Oh, now I see the whole picture!

    1. abc.ABCMeta cache __subclasscheck__ results if no exception occurs.
    2. Protocol assigns a __subclasshook__, which raises or does not raise exceptions depending on the caller.
    3. _ProtocolMeta.__instancecheck__ calls the __subclasshook__ in (2) (not raising exceptions), and the result is cached by (1)
    4. User call (1), and accidentally get the cached result (not the exception).
  23. sunmy2019 commented on May 16, 2023

    @sunmy2019
    Member

    More details for (2) (3)

    The __subclasshook__ in Protocol utilizes _allow_reckless_class_checks(). In normal cases, it is

    _caller(0)=typing  _allow_reckless_class_checks
    _caller(1)=typing  Protocol. _proto_hook
    _caller(2)=abc     ABCMeta.__subclasscheck__
    _caller(3)=__main__         <------- _allow_reckless_class_checks()=False
    

    It then raises an exception.

    In the problematic scenarios (3):

    _caller(0)=typing   _allow_reckless_class_checks
    _caller(1)=typing   Protocol. _proto_hook
    _caller(2)=abc      ABCMeta.__subclasscheck__
    _caller(3)=abc      ABCMeta.__instancecheck__              <------- _allow_reckless_class_checks()=True
    _caller(4)=typing   _ProtocolMeta.__instancecheck__
    

    It does raise an exception.

  24. added a commit that references this issue on May 17, 2023
  25. added a commit that references this issue on May 17, 2023
  26. added a commit that references this issue on May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions