Repository navigation
Should typing be calling getattr on arbitrary classes? #113320
Description
Activity
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-BalfeSome 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-AttributeErrorexceptions when the__get__method of the descriptor is called, are pretty badly behaved and should be discouraged. However, we've already made the decision thattyping.Protocolshould be extremely careful aboutgetattr()calls on instances, so perhaps it stands to reason thattypng.Protocolshould also be extremely careful aboutgetattr()calls on class objects themselves as well. - The obvious fix is to use
getattr_static()rather thangetattr(). Currently we do thegetattr()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 thesegetattr()calls could have any other unexpected/undesirable behaviour changes.
- In general, I continue to feel that instances of descriptors like
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, sinceinspectis a very heavy import, and this patch would mean we'd depend oninspectat 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, 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.)
Reacted by Alex WaygoodMy 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_checkabledecorator adds the_runtime_checkableattribute 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_checkableprotocols, 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.
Reacted by Alex WaygoodCool, 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
Reacted by Carl Meyer
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:
Traceback on
main: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
getattr()calls when constructing protocol classes #113401getattr()calls when constructing protocol classes (#113401) #113722