GeezerVison is showing github.com — Should typing be calling getattr on arbitrary classes? · Issue #113320 · python/cpython
Should typing be calling getattr on arbitrary classes? #113320DescriptionBug 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
ActivityAlexWaygood commented on Dec 20, 2023
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"? :) Reacted by James Hilton-Balfe AlexWaygood commented on Dec 20, 2023 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 commented on Dec 20, 2023 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 TrueAlexWaygood commented on Dec 20, 2023 @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 commented on 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 commented on 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?
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 commented on Dec 21, 2023 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 commented on 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 commented on 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: NOadded 3 commits that reference this issue on Jan 5, 2024gh-113320: Reduce the number of dangerous `getattr()` calls when cons…
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|