From ed6ea3ea79fac68b127c7eb457c7ecb996461010 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Fri, 5 Jan 2024 01:01:48 +0000 Subject: [PATCH] gh-113320: Reduce the number of dangerous `getattr()` calls when constructing 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! --- Lib/test/test_typing.py | 38 +++++++++++++- Lib/typing.py | 50 +++++++++++-------- ...-12-22-11-30-57.gh-issue-113320.Vp5suS.rst | 4 ++ 3 files changed, 70 insertions(+), 22 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2023-12-22-11-30-57.gh-issue-113320.Vp5suS.rst diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py index 2d10c39840d..8edab0cd6e3 100644 --- a/Lib/test/test_typing.py +++ b/Lib/test/test_typing.py @@ -3448,8 +3448,8 @@ class ProtocolTests(BaseTestCase): self.assertNotIn("__protocol_attrs__", vars(NonP)) self.assertNotIn("__protocol_attrs__", vars(NonPR)) - self.assertNotIn("__callable_proto_members_only__", vars(NonP)) - self.assertNotIn("__callable_proto_members_only__", vars(NonPR)) + self.assertNotIn("__non_callable_proto_members__", vars(NonP)) + self.assertNotIn("__non_callable_proto_members__", vars(NonPR)) self.assertEqual(get_protocol_members(P), {"x"}) self.assertEqual(get_protocol_members(PR), {"meth"}) @@ -4105,6 +4105,7 @@ class ProtocolTests(BaseTestCase): self.assertNotIsInstance(42, ProtocolWithMixedMembers) def test_protocol_issubclass_error_message(self): + @runtime_checkable class Vec2D(Protocol): x: float y: float @@ -4120,6 +4121,39 @@ class ProtocolTests(BaseTestCase): with self.assertRaisesRegex(TypeError, re.escape(expected_error_message)): issubclass(int, Vec2D) + def test_nonruntime_protocol_interaction_with_evil_classproperty(self): + class classproperty: + def __get__(self, instance, type): + raise RuntimeError("NO") + + class Commentable(Protocol): + evil = classproperty() + + # recognised as a protocol attr, + # but not actually accessed by the protocol metaclass + # (which would raise RuntimeError) for non-runtime protocols. + # See gh-113320 + self.assertEqual(get_protocol_members(Commentable), {"evil"}) + + def test_runtime_protocol_interaction_with_evil_classproperty(self): + class CustomError(Exception): pass + + class classproperty: + def __get__(self, instance, type): + raise CustomError + + with self.assertRaises(TypeError) as cm: + @runtime_checkable + class Commentable(Protocol): + evil = classproperty() + + exc = cm.exception + self.assertEqual( + exc.args[0], + "Failed to determine whether protocol member 'evil' is a method member" + ) + self.assertIs(type(exc.__cause__), CustomError) + class GenericTests(BaseTestCase): diff --git a/Lib/typing.py b/Lib/typing.py index d7d793539b3..d278b4effc7 100644 --- a/Lib/typing.py +++ b/Lib/typing.py @@ -1670,7 +1670,7 @@ class _TypingEllipsis: _TYPING_INTERNALS = frozenset({ '__parameters__', '__orig_bases__', '__orig_class__', '_is_protocol', '_is_runtime_protocol', '__protocol_attrs__', - '__callable_proto_members_only__', '__type_params__', + '__non_callable_proto_members__', '__type_params__', }) _SPECIAL_NAMES = frozenset({ @@ -1833,11 +1833,6 @@ class _ProtocolMeta(ABCMeta): 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,25 +1841,23 @@ class _ProtocolMeta(ABCMeta): getattr(cls, '_is_protocol', False) and not _allow_reckless_class_checks() ): - if ( - not cls.__callable_proto_members_only__ - 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)) - ) - raise TypeError( - "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" ) + if ( + # this attribute is set by @runtime_checkable: + cls.__non_callable_proto_members__ + and cls.__dict__.get("__subclasshook__") is _proto_hook + ): + _type_check_issubclass_arg_1(other) + 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]}." + ) return _abc_subclasscheck(cls, other) def __instancecheck__(cls, instance): @@ -1892,7 +1885,8 @@ class _ProtocolMeta(ABCMeta): val = getattr_static(instance, attr) except AttributeError: break - if val is None and callable(getattr(cls, attr, None)): + # this attribute is set by @runtime_checkable: + if val is None and attr not in cls.__non_callable_proto_members__: break else: return True @@ -2114,6 +2108,22 @@ def runtime_checkable(cls): 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.__non_callable_proto_members__ = set() + for attr in cls.__protocol_attrs__: + try: + is_callable = callable(getattr(cls, attr, None)) + except Exception as e: + raise TypeError( + f"Failed to determine whether protocol member {attr!r} " + "is a method member" + ) from e + else: + if not is_callable: + cls.__non_callable_proto_members__.add(attr) return cls diff --git a/Misc/NEWS.d/next/Library/2023-12-22-11-30-57.gh-issue-113320.Vp5suS.rst b/Misc/NEWS.d/next/Library/2023-12-22-11-30-57.gh-issue-113320.Vp5suS.rst new file mode 100644 index 00000000000..6cf74f335d4 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-12-22-11-30-57.gh-issue-113320.Vp5suS.rst @@ -0,0 +1,4 @@ +Fix regression in Python 3.12 where :class:`~typing.Protocol` classes that +were not marked as :func:`runtime-checkable ` +would be unnecessarily introspected, potentially causing exceptions to be +raised if the protocol had problematic members. Patch by Alex Waygood.