From 212fce296424db5ea809d0fcadd31e47dc3b9888 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Fri, 24 Mar 2023 19:49:00 +0000 Subject: [PATCH 1/7] Implementation --- Lib/typing.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/Lib/typing.py b/Lib/typing.py index 157a563bbecea8..c5c2e0bb5b6ed2 100644 --- a/Lib/typing.py +++ b/Lib/typing.py @@ -1994,6 +1994,17 @@ def _allow_reckless_class_checks(depth=3): } +@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) + + class _ProtocolMeta(ABCMeta): # This metaclass is really unfortunate and exists only because of # the lack of __instancehook__. @@ -2013,7 +2024,10 @@ def __instancecheck__(cls, instance): issubclass(instance.__class__, cls)): return True if cls._is_protocol: - if all(hasattr(instance, attr) and + sentinel = object() + getattr_static = _lazy_load_getattr_static() + if all( + (getattr_static(instance, attr, sentinel) is not sentinel) and # All *methods* can be blocked by setting them to None. (not callable(getattr(cls, attr, None)) or getattr(instance, attr) is not None) From ef713954946720ba28d194f73985667798712688 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Fri, 24 Mar 2023 20:16:31 +0000 Subject: [PATCH 2/7] Tests --- Lib/test/test_typing.py | 68 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py index f448b0ee60a92a..c09cf6d0b32f7a 100644 --- a/Lib/test/test_typing.py +++ b/Lib/test/test_typing.py @@ -2555,6 +2555,74 @@ def meth(x): ... with self.assertRaises(TypeError): isinstance(C(), BadPG) + def test_protocols_isinstance_attribute_access_with_side_effects(self): + class C: + @property + def attr(self): + raise AttributeError('no') + + class CustomDescriptor: + def __get__(self, obj, objtype=None): + raise RuntimeError("NO") + + class D: + attr = CustomDescriptor() + + # Check that properties set on superclasses + # are still found by the isinstance() logic + class E(C): ... + class F(D): ... + + class WhyWouldYouDoThis: + def __getattr__(self, name): + raise RuntimeError("wut") + + T = TypeVar('T') + + @runtime_checkable + class P(Protocol): + @property + def attr(self): ... + + @runtime_checkable + class P1(Protocol): + attr: int + + @runtime_checkable + class PG(Protocol[T]): + @property + def attr(self): ... + + @runtime_checkable + class PG1(Protocol[T]): + attr: T + + for protocol_class in P, P1, PG, PG1: + for klass in C, D, E, F: + with self.subTest( + klass=klass.__name__, + protocol_class=protocol_class.__name__ + ): + self.assertIsInstance(klass(), protocol_class) + + with self.subTest( + klass="WhyWouldYouDoThis", + protocol_class=protocol_class.__name__ + ): + self.assertNotIsInstance(WhyWouldYouDoThis(), protocol_class) + + def test_protocols_isinstance___slots__(self): + # As per the consensus in https://github.com/python/typing/issues/1367, + # this is desirable behaviour + @runtime_checkable + class HasX(Protocol): + x: int + + class HasNothingButSlots: + __slots__ = ("x",) + + self.assertIsInstance(HasNothingButSlots(), HasX) + def test_protocols_isinstance_properties_and_descriptors(self): class C: @property From 4407d604ab223cedb5e2065147270be421f0c464 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Sat, 25 Mar 2023 16:57:31 +0000 Subject: [PATCH 3/7] docs --- Doc/library/typing.rst | 9 +++++++++ Doc/whatsnew/3.12.rst | 11 +++++++++++ .../2023-03-25-16-57-18.gh-issue-102344.L-7x2Q.rst | 10 ++++++++++ 3 files changed, 30 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2023-03-25-16-57-18.gh-issue-102344.L-7x2Q.rst diff --git a/Doc/library/typing.rst b/Doc/library/typing.rst index 08ffa0310f0f23..ebd63e016a2e3a 100644 --- a/Doc/library/typing.rst +++ b/Doc/library/typing.rst @@ -1591,6 +1591,15 @@ These are not used in annotations. They are building blocks for creating generic import threading assert isinstance(threading.Thread(name='Bob'), Named) + .. versionchanged:: 3.12 + The internal implementation of :func:`isinstance` checks against + runtime-checkable protocols now uses :func:`inspect.getattr_static` + to look up attributes (previously, :func:`hasattr` was used). + As a result, some objects which used to be considered instances + of a runtime-checkable protocol may no longer be considered instances + of that protocol on Python 3.12+, and vice versa. + Most users are unlikely to be affected by this change. + .. note:: :func:`!runtime_checkable` will check only the presence of the required diff --git a/Doc/whatsnew/3.12.rst b/Doc/whatsnew/3.12.rst index e5bcfdecd9a487..144a54c9f56819 100644 --- a/Doc/whatsnew/3.12.rst +++ b/Doc/whatsnew/3.12.rst @@ -391,6 +391,17 @@ typing same name on a base class, as per :pep:`698`. (Contributed by Steven Troxler in :gh:`101564`.) +* :func:`isinstance` checks against + :func:`runtime-checkable protocols ` now use + :func:`inspect.getattr_static` rather than :func:`hasattr` to lookup whether + attributes exist. This means that descriptors and :meth:`~object.__getattr__` + methods are no longer unexpectedly evaluated during ``isinstance()`` checks + against runtime-checkable protocols. However, it may also mean that some + objects which used to be considered instances of a runtime-checkable protocol + may no longer be considered instances of that protocol on Python 3.12+, and + vice versa. Most users are unlikely to be affected by this change. + (Contributed by Alex Waygood in :gh:`102344`.) + sys --- diff --git a/Misc/NEWS.d/next/Library/2023-03-25-16-57-18.gh-issue-102344.L-7x2Q.rst b/Misc/NEWS.d/next/Library/2023-03-25-16-57-18.gh-issue-102344.L-7x2Q.rst new file mode 100644 index 00000000000000..a4de4b4448f391 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-03-25-16-57-18.gh-issue-102344.L-7x2Q.rst @@ -0,0 +1,10 @@ +:func:`isinstance` checks against :func:`runtime-checkable protocols +` now use :func:`inspect.getattr_static` rather +than :func:`hasattr` to lookup whether attributes exist. This means that +descriptors and :meth:`~object.__getattr__` methods are no longer +unexpectedly evaluated during ``isinstance()`` checks against +runtime-checkable protocols. However, it may also mean that some objects +which used to be considered instances of a runtime-checkable protocol may no +longer be considered instances of that protocol on Python 3.12+, and vice +versa. Most users are unlikely to be affected by this change. Patch by Alex +Waygood. From 44eba6b3142d15fcb65f1af6976b3276e5885270 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Sat, 25 Mar 2023 17:14:15 +0000 Subject: [PATCH 4/7] Wrong issue number --- Doc/whatsnew/3.12.rst | 2 +- ...-7x2Q.rst => 2023-03-25-16-57-18.gh-issue-102433.L-7x2Q.rst} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename Misc/NEWS.d/next/Library/{2023-03-25-16-57-18.gh-issue-102344.L-7x2Q.rst => 2023-03-25-16-57-18.gh-issue-102433.L-7x2Q.rst} (100%) diff --git a/Doc/whatsnew/3.12.rst b/Doc/whatsnew/3.12.rst index 144a54c9f56819..db6b57fcb6e9ef 100644 --- a/Doc/whatsnew/3.12.rst +++ b/Doc/whatsnew/3.12.rst @@ -400,7 +400,7 @@ typing objects which used to be considered instances of a runtime-checkable protocol may no longer be considered instances of that protocol on Python 3.12+, and vice versa. Most users are unlikely to be affected by this change. - (Contributed by Alex Waygood in :gh:`102344`.) + (Contributed by Alex Waygood in :gh:`102433`.) sys --- diff --git a/Misc/NEWS.d/next/Library/2023-03-25-16-57-18.gh-issue-102344.L-7x2Q.rst b/Misc/NEWS.d/next/Library/2023-03-25-16-57-18.gh-issue-102433.L-7x2Q.rst similarity index 100% rename from Misc/NEWS.d/next/Library/2023-03-25-16-57-18.gh-issue-102344.L-7x2Q.rst rename to Misc/NEWS.d/next/Library/2023-03-25-16-57-18.gh-issue-102433.L-7x2Q.rst From 5c15fbcd6de6272c9ed07c13f7d8e6b009664052 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Fri, 31 Mar 2023 18:58:24 +0100 Subject: [PATCH 5/7] Fix an edge case --- Lib/test/test_typing.py | 17 +++++++++++++++-- Lib/typing.py | 14 ++++++++------ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py index 3058c2776bee11..946942f8521d49 100644 --- a/Lib/test/test_typing.py +++ b/Lib/test/test_typing.py @@ -2705,7 +2705,15 @@ def attr(self): ... class PG1(Protocol[T]): attr: T - for protocol_class in P, P1, PG, PG1: + @runtime_checkable + class MethodP(Protocol): + def attr(self): ... + + @runtime_checkable + class MethodPG(Protocol[T]): + def attr(self) -> T: ... + + for protocol_class in P, P1, PG, PG1, MethodP, MethodPG: for klass in C, D, E, F: with self.subTest( klass=klass.__name__, @@ -2730,7 +2738,12 @@ def attr(self): ... class BadPG1(Protocol[T]): attr: T - for obj in PG[T], PG[C], PG1[T], PG1[C], BadP, BadP1, BadPG, BadPG1: + cases = ( + PG[T], PG[C], PG1[T], PG1[C], MethodPG[T], + MethodPG[C], BadP, BadP1, BadPG, BadPG1 + ) + + for obj in cases: for klass in C, D, E, F, Empty: with self.subTest(klass=klass.__name__, obj=obj): with self.assertRaises(TypeError): diff --git a/Lib/typing.py b/Lib/typing.py index 76ca14cb2864a4..9f44bee334a4b2 100644 --- a/Lib/typing.py +++ b/Lib/typing.py @@ -2034,13 +2034,15 @@ def __instancecheck__(cls, instance): if is_protocol_cls: sentinel = object() getattr_static = _lazy_load_getattr_static() - if all( - (getattr_static(instance, attr, sentinel) is not sentinel) and - # All *methods* can be blocked by setting them to None. - (not callable(getattr(cls, attr, None)) or - getattr(instance, attr) is not None) - for attr in protocol_attrs): + for attr in protocol_attrs: + val = getattr_static(instance, attr, sentinel) + if val is sentinel: + break + if callable(getattr(cls, attr, None)) and val is None: + break + else: return True + return super().__instancecheck__(instance) From 081218320d3173ccfebd1becebb2a8f7f0ecf28b Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Fri, 31 Mar 2023 19:05:38 +0100 Subject: [PATCH 6/7] Move tests around, add some more --- Lib/test/test_typing.py | 128 +++++++++++++++++++++------------------- 1 file changed, 68 insertions(+), 60 deletions(-) diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py index 946942f8521d49..7d2e6a6a9f6287 100644 --- a/Lib/test/test_typing.py +++ b/Lib/test/test_typing.py @@ -2597,15 +2597,15 @@ def meth(x): ... with self.assertRaises(TypeError): isinstance(C(), BadPG) - def test_protocols_isinstance_attribute_access_with_side_effects(self): + def test_protocols_isinstance_properties_and_descriptors(self): class C: @property def attr(self): - raise AttributeError('no') + return 42 class CustomDescriptor: def __get__(self, obj, objtype=None): - raise RuntimeError("NO") + return 42 class D: attr = CustomDescriptor() @@ -2615,9 +2615,7 @@ class D: class E(C): ... class F(D): ... - class WhyWouldYouDoThis: - def __getattr__(self, name): - raise RuntimeError("wut") + class Empty: ... T = TypeVar('T') @@ -2639,7 +2637,15 @@ def attr(self): ... class PG1(Protocol[T]): attr: T - for protocol_class in P, P1, PG, PG1: + @runtime_checkable + class MethodP(Protocol): + def attr(self): ... + + @runtime_checkable + class MethodPG(Protocol[T]): + def attr(self) -> T: ... + + for protocol_class in P, P1, PG, PG1, MethodP, MethodPG: for klass in C, D, E, F: with self.subTest( klass=klass.__name__, @@ -2647,33 +2653,60 @@ class PG1(Protocol[T]): ): self.assertIsInstance(klass(), protocol_class) - with self.subTest( - klass="WhyWouldYouDoThis", - protocol_class=protocol_class.__name__ - ): - self.assertNotIsInstance(WhyWouldYouDoThis(), protocol_class) + with self.subTest(klass="Empty", protocol_class=protocol_class.__name__): + self.assertNotIsInstance(Empty(), protocol_class) - def test_protocols_isinstance___slots__(self): - # As per the consensus in https://github.com/python/typing/issues/1367, - # this is desirable behaviour + class BadP(Protocol): + @property + def attr(self): ... + + class BadP1(Protocol): + attr: int + + class BadPG(Protocol[T]): + @property + def attr(self): ... + + class BadPG1(Protocol[T]): + attr: T + + cases = ( + PG[T], PG[C], PG1[T], PG1[C], MethodPG[T], + MethodPG[C], BadP, BadP1, BadPG, BadPG1 + ) + + for obj in cases: + for klass in C, D, E, F, Empty: + with self.subTest(klass=klass.__name__, obj=obj): + with self.assertRaises(TypeError): + isinstance(klass(), obj) + + def test_protocols_isinstance_not_fooled_by_custom_dir(self): @runtime_checkable class HasX(Protocol): x: int - class HasNothingButSlots: - __slots__ = ("x",) + class CustomDirWithX: + x = 10 + def __dir__(self): + return [] - self.assertIsInstance(HasNothingButSlots(), HasX) + class CustomDirWithoutX: + def __dir__(self): + return ["x"] - def test_protocols_isinstance_properties_and_descriptors(self): + self.assertIsInstance(CustomDirWithX(), HasX) + self.assertNotIsInstance(CustomDirWithoutX(), HasX) + + def test_protocols_isinstance_attribute_access_with_side_effects(self): class C: @property def attr(self): - return 42 + raise AttributeError('no') class CustomDescriptor: def __get__(self, obj, objtype=None): - return 42 + raise RuntimeError("NO") class D: attr = CustomDescriptor() @@ -2683,7 +2716,9 @@ class D: class E(C): ... class F(D): ... - class Empty: ... + class WhyWouldYouDoThis: + def __getattr__(self, name): + raise RuntimeError("wut") T = TypeVar('T') @@ -2721,50 +2756,23 @@ def attr(self) -> T: ... ): self.assertIsInstance(klass(), protocol_class) - with self.subTest(klass="Empty", protocol_class=protocol_class.__name__): - self.assertNotIsInstance(Empty(), protocol_class) - - class BadP(Protocol): - @property - def attr(self): ... - - class BadP1(Protocol): - attr: int - - class BadPG(Protocol[T]): - @property - def attr(self): ... - - class BadPG1(Protocol[T]): - attr: T - - cases = ( - PG[T], PG[C], PG1[T], PG1[C], MethodPG[T], - MethodPG[C], BadP, BadP1, BadPG, BadPG1 - ) - - for obj in cases: - for klass in C, D, E, F, Empty: - with self.subTest(klass=klass.__name__, obj=obj): - with self.assertRaises(TypeError): - isinstance(klass(), obj) + with self.subTest( + klass="WhyWouldYouDoThis", + protocol_class=protocol_class.__name__ + ): + self.assertNotIsInstance(WhyWouldYouDoThis(), protocol_class) - def test_protocols_isinstance_not_fooled_by_custom_dir(self): + def test_protocols_isinstance___slots__(self): + # As per the consensus in https://github.com/python/typing/issues/1367, + # this is desirable behaviour @runtime_checkable class HasX(Protocol): x: int - class CustomDirWithX: - x = 10 - def __dir__(self): - return [] - - class CustomDirWithoutX: - def __dir__(self): - return ["x"] + class HasNothingButSlots: + __slots__ = ("x",) - self.assertIsInstance(CustomDirWithX(), HasX) - self.assertNotIsInstance(CustomDirWithoutX(), HasX) + self.assertIsInstance(HasNothingButSlots(), HasX) def test_protocols_isinstance_py36(self): class APoint: From 6ba97e71b14c86ce703fc38feacb3cff48b4785a Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Sun, 2 Apr 2023 13:52:22 +0100 Subject: [PATCH 7/7] Simplify and optimise --- Lib/typing.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/typing.py b/Lib/typing.py index 1a3591fc38da94..442d684d14c928 100644 --- a/Lib/typing.py +++ b/Lib/typing.py @@ -2036,11 +2036,11 @@ def __instancecheck__(cls, instance): return True if is_protocol_cls: - sentinel = object() getattr_static = _lazy_load_getattr_static() for attr in protocol_attrs: - val = getattr_static(instance, attr, sentinel) - if val is sentinel: + try: + val = getattr_static(instance, attr) + except AttributeError: break if callable(getattr(cls, attr, None)) and val is None: break