GeezerVison is showing github.com — `isinstance` on `runtime_checkable` `Protocol` has side-effects for `@property` methods · Issue #102433 · python/cpython
isinstance on runtime_checkable Protocol has side-effects for @property methods #102433
DescriptionFor example:
from typing import Protocol, runtime_checkable
@runtime_checkable
class X(Protocol):
@property
def myproperty(self): ...
class Y:
@property
def myproperty(self):
raise RuntimeError("hallo")
isinstance(Y(), X)will raise the RuntimeError
This is an issue, for example, if myproperty is an expensive call, has unwanted side effects, or excepts outside of a context manager
Linked PRs
Activitychanged the title [-]isinstance on runtime_checkable Protocol has side-effects for @property methods[/-] [+]`isinstance` on `runtime_checkable` `Protocol` has side-effects for `@property` methods[/+] on Mar 5, 2023chrisjsewell commented on Mar 5, 2023 JosephSBoyle commented on Mar 5, 2023 Hi @chrisjsewell. I've traced the problem to the hasattr call in the _ProtocolMeta class referencd in that S.O question you linked.
hasattr calls getattr, which in your test case succeeds, thus executing the code in myproperty, which then errors. It's quite trivial to solve (we just replace the getattr with checking the existence of attr in dir(instance)).
I can't think of any particular downside to making this change so will open a PR shortly:) chrisjsewell commented on Mar 5, 2023 AlexWaygood commented on Mar 5, 2023 @JosephSBoyle, a better solution would probably be to use inspect.getattr_static, which is specifically designed for solving this kind of problem.
Looking in an object's dir() is problematic, because objects can define custom __dir__ methods:
>>> class Foo:
... def __init__(self):
... self.x = 5
... def __dir__(self):
... return []
...
>>> "x" in dir(Foo())
False A better solution is to look in an object's __dict__, but there are complications here as well.
>>> class Foo:
... a = 1
... def __init__(self):
... self.b = 2
...
>>> obj = Foo()
>>> "a" in obj.__dict__
False
>>> "b" in obj.__dict__
True
>>> "a" in obj.__class__.__dict__
True
>>> "b" in obj.__class__.__dict__
False (It gets even more complicated if the attribute is defined on a superclass, or if the class defines __slots__.)
inspect.getattr_static already tackles all of these complications, so it would provide a better solution than reinventing the wheel here :)
Having said that, importing inspect in typing.py might slow down importing typing at runtime, so we should be careful to check that. JosephSBoyle commented on Mar 5, 2023 Thanks @AlexWaygood, I wasn't aware of getattr_static.
The docs suggest that using this might result in changing some other existing result in a change in behaviour, for instance "not being able to fetch dynamically created attributes". I'm not sure if this is wrong, or if I'm misunderstanding what is stated.
Change:
# typing.py
if cls._is_protocol:
# Check if attr is defined on the instance using `inspect.getattr_static`
# to avoid triggering possible side-effects in function style properties
# (i.e those with @property decorators) that would occur if `hasattr`
# were used.
import inspect
def hasattr_static(instance, attr):
try:
inspect.getattr_static(instance, attr)
return True
except AttributeError:
return False
if all(hasattr_static(instance, attr) 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 _get_protocol_attrs(cls)):
return True
return super().__instancecheck__(instance)Test for dynamic attribute checking with getattr_static:
def test_dynamically_added_attribute(self):
"""Test a class dynamically becoming adherent to a Protocol."""
@runtime_checkable
class P(Protocol):
@property
def foo(self):
pass
class X:
pass
x = X()
self.assertFalse(isinstance(x, P))
x.foo = "baz"
self.assertTrue(isinstance(x, P))
x2 = X()
x2.foo = property(lambda self: "baz")
self.assertTrue(isinstance(x2, P))The above test passes, suggesting getattr static can in fact find dynamically added attributes. AlexWaygood commented on Mar 5, 2023
I'm not sure if this is wrong, or if I'm misunderstanding what is stated.
I think you're misunderstanding, but the inspect docs could probably be clearer on this point :) I believe the inspect docs are talking about situations with fancy descriptors where attributes are dynamically added in __get__ methods — the whole point of getattr_static is that it doesn't call __get__ methods, so that is indeed something it would struggle with.
This may or may not be what the inspect docs are talking about here, but here's a situation where using inspect.getattr_static instead of hasattr would lead to a bad regression that we should try to avoid:
>>> from typing import ClassVar, Protocol, runtime_checkable
>>> @runtime_checkable
... class HasBar(Protocol):
... @property
... def bar(self) -> int: ...
...
>>> class Foo:
... x: ClassVar[bool] = False
... @property
... def bar(self) -> int:
... if self.x:
... return 42
... raise AttributeError
...
>>> import inspect
>>> f = Foo()
>>> inspect.getattr_static(f, "bar")
<property object at 0x11f7b0f48>
>>> hasattr(f, 'bar')
False
>>> if isinstance(f, HasBar): # False with current implementation, True with getattr_static implementation
... y = f.bar # type checkers will assume this is a safe attribute access, but if we used getattr_static, this would be unsafe
... AlexWaygood commented on Mar 5, 2023 The best solution here may actually be to just add a warning to the docs for runtime_checkable stating that calling isinstance() against runtime-checkable protocols will result in (possibly expensive) properties being accessed. I don't think there's a way of doing it so that we avoid accessing properties on the object but maintain type-safe behaviour. chrisjsewell commented on Mar 5, 2023 😢 personally I would say this kind of makes Protocol and @runtime_checkable unusable in many practical applications, and I will just switch back to using ABC classes Reacted by Richard Brearton and Florian Brucker AlexWaygood commented on Mar 5, 2023
😢 personally I would say this kind of makes Protocol and @runtime_checkable unusable in many practical applications, and I will just switch back to using ABC classes
I'm open to suggestions, if anybody can think of a way of improving the implementation here while maintaining type safety! 🙂 JosephSBoyle commented on Mar 5, 2023 Thanks for the example, @AlexWaygood.
I agree it seems like we're unfortunately unable to support both use cases and so should stick with the one we've supported thus far and provide a warning w.r.t the second use case as you suggest.
I'm sorry to hear this makes Protocol unusable for the cases you had in mind @chrisjsewell, if you have a concrete case in mind feel free to open a S.O issue and link it here, I'd be happy to take a look. chrisjsewell commented on Mar 5, 2023
if you have a concrete case in mind fee
class X:
def __init__(self):
self._is_open = False
def __enter__(self):
# open some resource
self._is_open = True
def __exit__(self, *args):
# close the resource
self._is_open = False
@property
def x(self):
if not self._is_open:
raise RuntimeError("cannot access x unless in a context")
# fetch x ...AlexWaygood commented on Mar 6, 2023 I've opened an issue here, attempting to summarise the discussion, and seeking thoughts from the typing community more broadly:
added a commit that references this issue on Mar 11, 2023gh-102433: Add tests for how classes with properties interact with `i… added 4 commits that reference this issue on Mar 11, 2023gh-102433: Add tests for how classes with properties interact with `i… gh-102433: Add tests for how classes with properties interact with `i… added a commit that references this issue on Apr 2, 2023gh-102433: Use `inspect.getattr_static` in `typing._ProtocolMeta.__in… AlexWaygood commented on Apr 2, 2023 #103034 has been merged, meaning that descriptors, properties and __getattr__ methods will no longer be called during isinstance() checks on runtime-checkable protocols in Python 3.12+. The change introduces some minor backwards incompatibilities, however, so can't be backported to 3.11 and 3.10.
It might be possible to backport the change to typing_extensions, but that should be discussed in the typing_extensions repo, not here :)
Thanks for changing my mind on this one!
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|