New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Should typing be calling getattr on arbitrary classes? #113320
Comments
Did you mean: "That isn't present in the 3.11 builds"? :) |
Some scattered thoughts, some of which I already stated on Discord:
|
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 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.) |
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.11from 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 |
Here's a patch that restores things to the way they were on Python 3.11 -- people using classproperties that raise non- Possible patchdiff --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 |
…n constructing protocol classes
…tructing 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!
…lls when constructing protocol classes (python#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! (cherry-picked from commit ed6ea3e)
…en constructing protocol classes (#113401) (#113722) - 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! (cherry-picked from commit ed6ea3e)
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) #113722The text was updated successfully, but these errors were encountered: