Should typing be calling getattr on arbitrary classes?

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

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"? :)

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.

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:

Details

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

@carljm

@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? :)

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.)

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__()

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

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.

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

pythongh-113320: Reduce the number of dangerous `getattr()` calls whe…

gh-113320: Reduce the number of dangerous `getattr()` calls when cons…

[3.12] pythongh-113320: Reduce the number of dangerous `getattr()` ca…

[3.12] gh-113320: Reduce the number of dangerous `getattr()` calls wh…

pythongh-113320: Reduce the number of dangerous `getattr()` calls whe…

Navigation Menu

Repository navigation

Should typing be calling getattr on arbitrary classes? #113320

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Should typing be calling getattr on arbitrary classes? #113320

Description

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

Activity

AlexWaygood commented on Dec 20, 2023

AlexWaygood commented on Dec 20, 2023

AlexWaygood commented on Dec 20, 2023

AlexWaygood commented on Dec 20, 2023

carljm commented on Dec 21, 2023

AlexWaygood commented on Dec 21, 2023

AlexWaygood commented on Dec 21, 2023

carljm commented on Dec 21, 2023

AlexWaygood commented on Dec 22, 2023

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions