The Wayback Machine - https://web.archive.org/web/20240109143236/https://github.com/python/cpython/issues/113320
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 typing be calling getattr on arbitrary classes? #113320

Closed
Gobot1234 opened this issue Dec 20, 2023 · 9 comments
Closed

Should typing be calling getattr on arbitrary classes? #113320

Gobot1234 opened this issue Dec 20, 2023 · 9 comments
Assignees
Labels
3.12 bugs and security fixes 3.13 new features, bugs and security fixes stdlib Python modules in the Lib dir topic-typing type-bug An unexpected behavior, bug, or error

Comments

@Gobot1234
Copy link
Contributor

Gobot1234 commented Dec 20, 2023

Bug report

Bug description:

I was updating my CI to support python3.12 and ran into the following error that isn't present in the 3.12 builds https://github.com/Gobot1234/steam.py/actions/runs/7276342755/job/19825996032

MRE:

from enum import IntEnum
from typing import Protocol

class classproperty:
    def __init__(self, func):
        self.__func__ = func

    def __get__(self, instance, type):
        return self.__func__(type)

class _CommentThreadType(IntEnum):
    WorkshopAccountDeveloper = 2

class Commentable(Protocol):
    @classproperty
    def _COMMENTABLE_TYPE(cls):
        return _CommentThreadType[cls.__name__]

Traceback on main:

Running Debug|x64 interpreter...
Traceback (most recent call last):
  File "C:\Users\alexw\coding\test_protocol.py", line 14, in <module>
    class Commentable(Protocol):
    ...<2 lines>...
            return _CommentThreadType[cls.__name__]
  File "C:\Users\alexw\coding\cpython\Lib\typing.py", line 1838, in __init__
    cls.__callable_proto_members_only__ = all(
                                          ~~~^
        callable(getattr(cls, attr, None)) for attr in cls.__protocol_attrs__
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "C:\Users\alexw\coding\cpython\Lib\typing.py", line 1839, in <genexpr>
    callable(getattr(cls, attr, None)) for attr in cls.__protocol_attrs__
             ~~~~~~~^^^^^^^^^^^^^^^^^
  File "C:\Users\alexw\coding\test_protocol.py", line 9, in __get__
    return self.__func__(type)
           ~~~~~~~~~~~~~^^^^^^
  File "C:\Users\alexw\coding\test_protocol.py", line 17, in _COMMENTABLE_TYPE
    return _CommentThreadType[cls.__name__]
           ~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
  File "C:\Users\alexw\coding\cpython\Lib\enum.py", line 763, in __getitem__
    return cls._member_map_[name]
           ~~~~~~~~~~~~~~~~^^^^^^
KeyError: 'Commentable'

Should typing be more careful about accessing random attributes on classes to prevent these kinds of errors in the future?

CPython versions tested on:

3.12

Operating systems tested on:

macOS

Linked PRs

@Gobot1234 Gobot1234 added the type-bug An unexpected behavior, bug, or error label Dec 20, 2023
@AlexWaygood AlexWaygood added stdlib Python modules in the Lib dir topic-typing labels Dec 20, 2023
@AlexWaygood
Copy link
Member

I was updating my CI to support python3.12 and ran into the following error that isn't present in the 3.12 builds

Did you mean: "That isn't present in the 3.11 builds"? :)

@AlexWaygood AlexWaygood added 3.12 bugs and security fixes 3.13 new features, bugs and security fixes labels Dec 20, 2023
@AlexWaygood AlexWaygood self-assigned this Dec 20, 2023
@AlexWaygood
Copy link
Member

Some scattered thoughts, some of which I already stated on Discord:

  • In general, I continue to feel that instances of descriptors like _COMMENTABLE_TYPE, which may raise non-AttributeError exceptions when the __get__ method of the descriptor is called, are pretty badly behaved and should be discouraged. However, we've already made the decision that typing.Protocol should be extremely careful about getattr() calls on instances, so perhaps it stands to reason that typng.Protocol should also be extremely careful about getattr() calls on class objects themselves as well.
  • The obvious fix is to use getattr_static() rather than getattr(). Currently we do the getattr() calls against the class object in various places, but we could fairly easily compute the set of callable members in _ProtocolMeta.__init__() to get rid of any performance penalty. I'd want to check whether this made protocol class creation significantly slower, though.
  • Need to think carefully about whether using getattr_static() for these getattr() calls could have any other unexpected/undesirable behaviour changes.

@AlexWaygood
Copy link
Member

This diff fixes things without breaking any existing tests, but I haven't done any benchmarking yet. It'll certainly slow down the import time of typing.py, since inspect is a very heavy import, and this patch would mean we'd depend on inspect at import time:

diff --git a/Lib/typing.py b/Lib/typing.py
index 61b88a560e..0697933eb3 100644
--- a/Lib/typing.py
+++ b/Lib/typing.py
@@ -27,6 +27,7 @@
 import operator
 import sys
 import types
+from inspect import getattr_static
 from types import WrapperDescriptorType, MethodWrapperType, MethodDescriptorType, GenericAlias

 from _typing import (
@@ -1671,6 +1672,7 @@ class _TypingEllipsis:
     '__parameters__', '__orig_bases__',  '__orig_class__',
     '_is_protocol', '_is_runtime_protocol', '__protocol_attrs__',
     '__callable_proto_members_only__', '__type_params__',
+    '__non_callable_proto_members__',
 })

 _SPECIAL_NAMES = frozenset({
@@ -1759,16 +1761,6 @@ def _allow_reckless_class_checks(depth=2):
 }


-@functools.cache
-def _lazy_load_getattr_static():
-    # Import getattr_static lazily so as not to slow down the import of typing.py
-    # Cache the result so we don't slow down _ProtocolMeta.__instancecheck__ unnecessarily
-    from inspect import getattr_static
-    return getattr_static
-
-
-_cleanups.append(_lazy_load_getattr_static.cache_clear)
-
 def _pickle_psargs(psargs):
     return ParamSpecArgs, (psargs.__origin__,)

@@ -1835,9 +1827,14 @@ def __init__(cls, *args, **kwargs):
             cls.__protocol_attrs__ = _get_protocol_attrs(cls)
             # PEP 544 prohibits using issubclass()
             # with protocols that have non-method members.
-            cls.__callable_proto_members_only__ = all(
-                callable(getattr(cls, attr, None)) for attr in cls.__protocol_attrs__
-            )
+            cls.__non_callable_proto_members__ = {
+                attr for attr in cls.__protocol_attrs__
+                if not callable(getattr_static(cls, attr, None))
+            }
+            # This attribute is retained for backwards compatibility,
+            # since some people use _ProtocolMeta in third-party code
+            # (even though they shouldn't)...
+            cls.__callable_proto_members_only__ = not cls.__non_callable_proto_members__

     def __subclasscheck__(cls, other):
         if cls is Protocol:
@@ -1851,10 +1848,7 @@ def __subclasscheck__(cls, other):
                 and cls.__dict__.get("__subclasshook__") is _proto_hook
             ):
                 _type_check_issubclass_arg_1(other)
-                non_method_attrs = sorted(
-                    attr for attr in cls.__protocol_attrs__
-                    if not callable(getattr(cls, attr, None))
-                )
+                non_method_attrs = sorted(cls.__non_callable_proto_members__)
                 raise TypeError(
                     "Protocols with non-method members don't support issubclass()."
                     f" Non-method members: {str(non_method_attrs)[1:-1]}."
@@ -1886,13 +1880,12 @@ def __instancecheck__(cls, instance):
         if _abc_instancecheck(cls, instance):
             return True

-        getattr_static = _lazy_load_getattr_static()
         for attr in cls.__protocol_attrs__:
             try:
                 val = getattr_static(instance, attr)
             except AttributeError:
                 break
-            if val is None and callable(getattr(cls, attr, None)):
+            if val is None and attr not in cls.__non_callable_proto_members__:
                 break
         else:
             return True

@AlexWaygood
Copy link
Member

@carljm, any thoughts from you on whether typing.Protocol should take care to account for the fact that attempting to access attributes on classes could potentially cause any exception to be raised, due to the possibility of badly behaved classproperties? :)

@carljm
Copy link
Contributor

carljm commented Dec 21, 2023

My first question looking at the MRE is, why are we introspecting the Protocol class at all when it hasn't been decorated as runtime-checkable? It seems like at the very least we should confine such problems to people who are explicitly opting in to runtime-checkable Protocols. But it seems like this would require some significant reorganization to do the introspection more lazily? (I could be way off base here, haven't had my head in this code for a while.)

@AlexWaygood
Copy link
Member

My first question looking at the MRE is, why are we introspecting the Protocol class at all when it hasn't been decorated as runtime-checkable?

Indeed. If you change the MRE to this, it fails with a similar traceback even on Python 3.11 (before we started mucking about with the code to make it faster). The only difference is that on Python 3.12, you get the exception raised even if the class isn't runtime-checkable:

Repro that triggers even on Python 3.11
from enum import IntEnum
from typing import Protocol, runtime_checkable

class classproperty:
    def __init__(self, func):
        self.__func__ = func

    def __get__(self, instance, type):
        return self.__func__(type)

class _CommentThreadType(IntEnum):
    WorkshopAccountDeveloper = 2

@runtime_checkable
class Commentable(Protocol):
    @classproperty
    def _COMMENTABLE_TYPE(cls):
        return _CommentThreadType[cls.__name__]

isinstance(0, Commentable)

The reason why it now triggers even for non-runtime protocols is that for performance reasons, we now inspect the Protocol class at the point when it's being created (as part of _ProtocolMeta.__init__(). And at class creation time, we don't know yet whether it's runtime-checkable or not! The @runtime_checkable decorator adds the _runtime_checkable attribute to the protocol class after the class has been initialised by _ProtocolMeta.__init__()

@AlexWaygood
Copy link
Member

Here's a patch that restores things to the way they were on Python 3.11 -- people using classproperties that raise non-AttributeErrors would still be broken if they used @runtime_checkable protocols, but wouldn't be broken if their protocols weren't @runtime_checkable:

Possible patch
diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py
index 2d10c39840..0a931751b1 100644
--- a/Lib/test/test_typing.py
+++ b/Lib/test/test_typing.py
@@ -4105,6 +4105,7 @@ def method(self) -> None: ...
         self.assertNotIsInstance(42, ProtocolWithMixedMembers)

     def test_protocol_issubclass_error_message(self):
+        @runtime_checkable
         class Vec2D(Protocol):
             x: float
             y: float
diff --git a/Lib/typing.py b/Lib/typing.py
index d7d793539b..65eb329ab1 100644
--- a/Lib/typing.py
+++ b/Lib/typing.py
@@ -1833,11 +1833,6 @@ def __init__(cls, *args, **kwargs):
         super().__init__(*args, **kwargs)
         if getattr(cls, "_is_protocol", False):
             cls.__protocol_attrs__ = _get_protocol_attrs(cls)
-            # PEP 544 prohibits using issubclass()
-            # with protocols that have non-method members.
-            cls.__callable_proto_members_only__ = all(
-                callable(getattr(cls, attr, None)) for attr in cls.__protocol_attrs__
-            )

     def __subclasscheck__(cls, other):
         if cls is Protocol:
@@ -1846,6 +1841,12 @@ def __subclasscheck__(cls, other):
             getattr(cls, '_is_protocol', False)
             and not _allow_reckless_class_checks()
         ):
+            if not getattr(cls, '_is_runtime_protocol', False):
+                _type_check_issubclass_arg_1(other)
+                raise TypeError(
+                    "Instance and class checks can only be used with "
+                    "@runtime_checkable protocols"
+                )
             if (
                 not cls.__callable_proto_members_only__
                 and cls.__dict__.get("__subclasshook__") is _proto_hook
@@ -1859,12 +1860,6 @@ def __subclasscheck__(cls, other):
                     "Protocols with non-method members don't support issubclass()."
                     f" Non-method members: {str(non_method_attrs)[1:-1]}."
                 )
-            if not getattr(cls, '_is_runtime_protocol', False):
-                _type_check_issubclass_arg_1(other)
-                raise TypeError(
-                    "Instance and class checks can only be used with "
-                    "@runtime_checkable protocols"
-                )
         return _abc_subclasscheck(cls, other)

     def __instancecheck__(cls, instance):
@@ -2114,6 +2109,13 @@ def close(self): ...
         raise TypeError('@runtime_checkable can be only applied to protocol classes,'
                         ' got %r' % cls)
     cls._is_runtime_protocol = True
+    # PEP 544 prohibits using issubclass()
+    # with protocols that have non-method members.
+    # See gh-113320 for why we compute this attribute here,
+    # rather than in `_ProtocolMeta.__init__`
+    cls.__callable_proto_members_only__ = all(
+        callable(getattr(cls, attr, None)) for attr in cls.__protocol_attrs__
+    )
     return cls

@carljm
Copy link
Contributor

carljm commented Dec 21, 2023

I do think that's a regression that we should fix. Runtime-checkable protocols are finicky (and arguably just a bad idea in the first place :) ) -- we shouldn't foist their issues onto every user of Protocol.

@AlexWaygood
Copy link
Member

AlexWaygood commented Dec 22, 2023

Cool, I'm working on a PR. Here's a slightly more minimal repro, FWIW:

from typing import Protocol

class classproperty:
    def __get__(self, instance, type):
        raise RuntimeError("NO")

class Commentable(Protocol):
    evil = classproperty()

Run the file (with py312):

Traceback (most recent call last):
  File "C:\Users\alexw\coding\evil_classproperty_repro.py", line 7, in <module>
    class Commentable(Protocol):
  File "C:\Users\alexw\AppData\Local\Programs\Python\Python312\Lib\typing.py", line 1825, in __init__
    cls.__callable_proto_members_only__ = all(
                                          ^^^^
  File "C:\Users\alexw\AppData\Local\Programs\Python\Python312\Lib\typing.py", line 1826, in <genexpr>
    callable(getattr(cls, attr, None)) for attr in cls.__protocol_attrs__
             ^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\alexw\coding\evil_classproperty_repro.py", line 5, in __get__
    raise RuntimeError("NO")
RuntimeError: NO

AlexWaygood added a commit to AlexWaygood/cpython that referenced this issue Dec 22, 2023
AlexWaygood added a commit that referenced this issue Jan 5, 2024
…tructing protocol classes (#113401)

- Only attempt to figure out whether protocol members are "method members" or not if the class is marked as a runtime protocol. This information is irrelevant for non-runtime protocols; we can safely skip the risky introspection for them.
- Only do the risky getattr() calls in one place (the runtime_checkable class decorator), rather than in three places (_ProtocolMeta.__init__, _ProtocolMeta.__instancecheck__ and _ProtocolMeta.__subclasscheck__). This reduces the number of locations in typing.py where the risky introspection could go wrong.
- For runtime protocols, if determining whether a protocol member is callable or not fails, give a better error message. I think it's reasonable for us to reject runtime protocols that have members which raise strange exceptions when you try to access them. PEP-544 clearly states that all protocol member must be callable for issubclass() calls against the protocol to be valid -- and if a member raises when we try to access it, there's no way for us to figure out whether it's a callable member or not!
AlexWaygood added a commit to AlexWaygood/cpython that referenced this issue Jan 5, 2024
…lls when constructing protocol classes (python#113401)

- Only attempt to figure out whether protocol members are "method members" or not if the class is marked as a runtime protocol. This information is irrelevant for non-runtime protocols; we can safely skip the risky introspection for them.
- Only do the risky getattr() calls in one place (the runtime_checkable class decorator), rather than in three places (_ProtocolMeta.__init__, _ProtocolMeta.__instancecheck__ and _ProtocolMeta.__subclasscheck__). This reduces the number of locations in typing.py where the risky introspection could go wrong.
- For runtime protocols, if determining whether a protocol member is callable or not fails, give a better error message. I think it's reasonable for us to reject runtime protocols that have members which raise strange exceptions when you try to access them. PEP-544 clearly states that all protocol member must be callable for issubclass() calls against the protocol to be valid -- and if a member raises when we try to access it, there's no way for us to figure out whether it's a callable member or not!

(cherry-picked from commit ed6ea3e)
AlexWaygood added a commit that referenced this issue Jan 5, 2024
…en constructing protocol classes (#113401) (#113722)

- Only attempt to figure out whether protocol members are "method members" or not if the class is marked as a runtime protocol. This information is irrelevant for non-runtime protocols; we can safely skip the risky introspection for them.
- Only do the risky getattr() calls in one place (the runtime_checkable class decorator), rather than in three places (_ProtocolMeta.__init__, _ProtocolMeta.__instancecheck__ and _ProtocolMeta.__subclasscheck__). This reduces the number of locations in typing.py where the risky introspection could go wrong.
- For runtime protocols, if determining whether a protocol member is callable or not fails, give a better error message. I think it's reasonable for us to reject runtime protocols that have members which raise strange exceptions when you try to access them. PEP-544 clearly states that all protocol member must be callable for issubclass() calls against the protocol to be valid -- and if a member raises when we try to access it, there's no way for us to figure out whether it's a callable member or not!

(cherry-picked from commit ed6ea3e)
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 new features, 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

3 participants