How should `isinstance()` work at runtime when comparing classes with properties against runtime-checkable protocols?

[AlexWaygood]

We've been having a debate in python/cpython#102433 about how isinstance() should work at runtime when comparing instances of classes with properties against runtime-checkable protocols. There's a few interrelated issues here, so I'm interested in knowing what the wider community thinks about the best way forward.

Current behaviour

The current behaviour at runtime is that a property x on an object obj will be evaluated if you call isinstance() on obj against a runtime-checkable protocol that specifies x as part of the interface:

>>> from typing import Protocol, runtime_checkable
>>> @runtime_checkable
... class HasX(Protocol):
...     x: int
...
>>> class Foo:
...     @property
...     def x(self):
...         print('Called!')
...         return 42
...
>>> obj = Foo()
>>> isinstance(obj, HasX)
Called!
True

The reason for this is that calling isinstance() on obj against HasX performs a structural check comparing obj to HasX. If all attributes and methods specified in the HasX protocol are present on obj, the isinstance() check returns True. This is taken care of by _ProtocolMeta.__instancecheck__, which just calls hasattr on obj for every attribute specified in the HasX protocol; calling hasattr(obj, x) will result in the x property on obj being evaluated as part of the isinstance() check.

An alternative to the current behaviour at runtime

An alternative to the current implementation would be to change the hasattr call in _ProtocolMeta.__instancecheck__ to use inspect.getattr_static (or similar):

--- a/Lib/typing.py
+++ b/Lib/typing.py
@@ -30,6 +30,7 @@
 import sys
 import types
 import warnings
+from inspect import getattr_static
 from types import WrapperDescriptorType, MethodWrapperType, MethodDescriptorType, GenericAlias


@@ -1990,7 +1991,9 @@ def __instancecheck__(cls, instance):
                 issubclass(instance.__class__, cls)):
             return True
         if cls._is_protocol:
-            if all(hasattr(instance, attr) and
+            sentinel = object()
+            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)

This would mean that properties would not be evaluated against instances, since getattr_static avoids evaluating descriptors at runtime:

>>> class Foo:
...     @property
...     def x(self):
...         print('Called!')
...         return 42
...
>>> getattr(Foo(), 'x')
Called!
42
>>> import inspect
>>> inspect.getattr_static(Foo(), 'x')
<property object at 0x0000015A789F5120>

Criticisms of the current behaviour at runtime

The current behaviour at runtime has been criticised due to the fact that it can result in unexpected behaviour from isinstance() checks, which don't usually result in properties being evaluated on instances:

>>> class Bar:
...     @property
...     def x(self):
...         raise RuntimeError("what were you thinking?")
...
>>> isinstance(Bar(), HasX)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\alexw\AppData\Local\Programs\Python\Python311\Lib\typing.py", line 1968, in __instancecheck__
    if all(hasattr(instance, attr) and
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\alexw\AppData\Local\Programs\Python\Python311\Lib\typing.py", line 1968, in <genexpr>
    if all(hasattr(instance, attr) and
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "<stdin>", line 4, in x
RuntimeError: what were you thinking?
>>> import time
>>> class Baz:
...     @property
...     def x(self):
...         time.sleep(3600)
...         return 42
...
>>> isinstance(Baz(), HasX)  # oh no, now we have to wait for an hour

These criticisms have been levied in the CPython repo more than once (a previous instance of this being reported was python/cpython#89445), so this is clearly pretty surprising behaviour to some people.

Possible rebuttal to the criticisms of the current behaviour

You could say that the whole point of runtime-checkable protocols is that they do something "a little different" when you call isinstance() against them. Arguing "I didn't expect this because it's not what isinstance() usually does" may not be the strongest case here.

When it comes to raising exceptions inside property getter methods, it may also be unwise in general to raise exceptions that aren't subclasses of AttributeError -- doing so will generally lead to unexpected things happening with hasattr() or getattr() calls.

Defence of the current behaviour at runtime

The current behaviour has things to recommend it. Firstly, using hasattr() here is probably faster than an alternative implementation using getattr_static. _ProtocolMeta.__instancecheck__ has previously been criticised for on performance grounds. This could make things even worse.

Moreover, while the current implementation leads to surprising outcomes for some users, it's possible that changing the implementation here could also lead to surprising outcomes. getattr_static has different semantics to hasattr: sometimes it finds attributes where hasattr doesn't, and sometimes it doesn't find attributes when hasattr does. It's hard to predict exactly what the effect of changing this would be, and it could be a breaking change for some users. The semantics of _ProtocolMeta.__instancecheck__ have been the same for a long time now; it's possible that the better course of action might be to simply document that calling isinstance on an instance obj against a runtime-checkable protocol might result in properties on obj being evaluated.

Possible rebuttal to the defence of the current behaviour

We probably shouldn't prioritise speed over correctness here; users who care deeply about performance probably shouldn't be using runtime-checkable protocols anyway.

Just because the semantics have been the same for several years doesn't mean we should necessarily stick with them if they're bad; sometimes breaking changes are necessary.

A specific case where changing the implementation would lead to different semantics

One reason why I lean towards keeping the current implementation is because I'd like to keep the invariant where the isinstance guard in the following snippet is enough to ensure that accessing the x attribute is safe:

from typing import Protocol, runtime_checkable

@runtime_checkable
class HasX(Protocol):
    x: int

def do_something(obj: object) -> None:
    if isinstance(obj, HasX):
        print(obj.x)

If we changed the implementation of _ProtocolMeta.__instancecheck__ to use getattr_static or similar, this wouldn't necessarily be the case. Consider:

class Foo:
    SWITCH: bool = False
    @property
    def x(self) -> int:
        if self.SWITCH:
            return 42
        raise AttributeError

obj: object = Foo()

# At runtime this is currently a safe call:
# - `isinstance(obj, HasX)` returns True if `self.SWITCH` is True, (in which case accessing `x` is safe)
# - `isinstance(obj, HasX)` returns False if `self.SWITCH` is False (in which case accessing `x` is unsafe)
#
# If we changed the implementation to use getattr_static, `isinstance(obj, HasX)` would be True
# regardless of the value of `self.SWITCH`,
# so the `isinstance()` guard in the `do_something` function would be ineffective
# at ensuring that the `x` attribute can be safely accessed
do_something(obj)

To me, being able to use isinstance() guards in this way -- to provide structural narrowing at runtime as well as statically, ensuring attributes can always be accessed safely -- is kind of the whole point of the runtime_checkable decorator.

Others have argued, however, that they would find it more intuitive if the runtime isinstance() check was closer to how static type checkers understood the above snippet. (Static type checkers have no special understanding of the fact that properties can sometimes raise AttributeError, so will always consider the Foo class above as being a valid subtype of HasX, contradicting the current behaviour at runtime.)

Thoughts?

[hmc-cs-mdrissi]

I think other issue has sorta dominated conversion and most of the points in it I think partly carry over here. I think runtime should generally be lax and if type checkers disagree on expected semantic with some being more lenient, runtime should not treat that interpretation as an error.

I also think the property evaluation behavior is too easy footgun. It can be documented more, but it was definitely surprise to me to read cpython issue and have to review if any of my runtime_checkable usages were affected. I occasionally have expensive cached_properties that could even produce error depending on where isinstance code is executed, but be fine to call in intended execution. An example of this is property that loads file that a user may not have permission to read at time of isinstance call, but if property is executed later elsewhere can be safe.

[carljm]

Explicitly-raised exceptions are not part of the Python type system. We don't annotate functions for what exceptions they can raise, we don't expect callers to handle all exceptions that might be raised (i.e. we don't have checked exceptions), so "prevent all exceptions" is not a form of safety that we expect the type system to provide.

If we say that we want the type-checker to ensure "safety" of the isinstance guard in the HasX / Foo example above, what we are effectively saying is that we want runtime checking of protocols to special-case the specific situation of raising AttributeError (unlike any other exception type) in the specific context of a property implementation, in a way that type checkers don't do statically. I don't really see a strong rationale for that: I think it is less surprising if runtime checking of protocols provides equivalent guarantees to what is statically checked. (And less surprising if isinstance doesn't execute property bodies.)

[AlexWaygood]

If we say that we want the type-checker to ensure "safety" of the isinstance guard in the HasX / Foo example above, what we are effectively saying is that we want runtime checking of protocols to special-case the specific situation of raising AttributeError (unlike any other exception type) in the specific context of a property implementation, in a way that type checkers don't do statically.

Let's be clear here that this issue doesn't actually have much to do with the behaviour of static type checkers (other than whether we should work to align the runtime behaviour more with how static type checkers currently see the code). This issue is purely about what should happen at runtime when using isinstance to compare objects against runtime-checkable protocols.

[carljm]

I do think we should attempt (as much as possible) to have consistent semantics for what it means for an instance to be compatible with a Protocol, rather than intentionally having different semantics for static vs runtime checking.

So I think the question of whether runtime checking should mark Foo as incompatible does depend on whether we view static checkers not marking it incompatible as a bug that we would hope to fix, or as correct behavior. I think that it is correct behavior, because explicitly-raised exceptions aren't part of the Python type system, and I don't think raising AttributeError in a property is special enough to break the rules.

[AlexWaygood]

That makes sense to me.

What about the concerns about backwards compatibility here? Do we need some kind of deprecation warning? Or would it be better to just go ahead with the change, document it, and hope that not too many people are relying on the current behaviour? (They probably aren't, I think? But I don't really know how to judge that.)

[hmc-cs-mdrissi]

I'd lean to notify/include a couple other notable runtime type libraries and whether they are, a. aware of this behavior, and b. whether it'd likely affect usage. Sourcegraph can help find usages. I've manually spot checked 5 files at random and found 0 attributes in any of their protocols, only methods. I may after work see if sourcegraph api is easy enough to have small script check percent of runtime_checkable protocols using attributes.

Even though I sorta feel this is a bug, it's probably safer not to backport and make a change only for main/3.12+.

[carljm]

TBH, if we agree that we ideally do want runtime checking and static checking to agree on Protocol compatibility, that really implies even deeper changes to runtime_checkable than just switching from hasattr to getattr_static. It implies that really the specific instance should not even be involved in the runtime check, only its type should be checked. Because static checkers will never reasonably be able to handle per-instance variation (e.g. random attributes added to an instance dict that don't exist on the type, or vice versa.)

I still do think that a single consistent interpretation is better, because static type checkers will make narrowing decisions based on runtime isinstance checks, and so having differing definitions of compatibility will lead to problems in edge cases. E.g. one would not expect this assert to ever fail if the codebase is fully annotated and passes static type checking, but today it certainly could fail:

def f(x: MyProto):
    assert isinstance(x, MyProto)

It looks like @ilevkivskyi did the initial implementation of @runtime_checkable -- I wonder if Ivan has thoughts here. Was the option of only checking against the type for better compatibility with static protocol checking already considered and rejected? Is it an explicit goal for @runtime_checkable to provide finer-grained per-instance checking, even though that can result in odd inconsistencies with static checking?

[AlexWaygood]

@hmc-cs-mdrissi

Even though I sorta feel this is a bug, it's probably safer not to backport and make a change only for main/3.12+.

Agreed, I'd definitely be very wary about backporting this behaviour change

[AlexWaygood]

@carljm

It implies that really the specific instance should not even be involved in the runtime check, only its type should be checked.

That would be very problematic for situations like the following:

from typing import Protocol, runtime_checkable

@runtime_checkable
class HasX(Protocol):
    x: int

class Foo:
    def __init__(self) -> None:
        self.x: int = 42

f = Foo()

# evaluates to True currently
# evaluates to False if we only look at the type of f, and don't look at the instance
isinstance(f, HasX)

[carljm]

Yes, of course, good point; at runtime the type can't tell us what attributes instances will have. So the more general case of instances with atypical sets of attributes is just an unavoidable inconsistency.

I suspect the case of properties conditionally raising AttributeError is rare enough that it should be OK to make the getattr_static change without a deprecation process, though checking existing usages and popular libraries is a good idea. I also wouldn't backport.

[hauntsaninja]

I think runtime_checkable is quite flawed and I'm grateful that the authors of PEP 544 had the wisdom to not make it the default.

Frankly, I think there is no behaviour surrounding properties that will make everyone happy. The fact that runtime_checkable only checks existence and is not a type check is quite surprising to users and in practice is much more surprising for general attributes than it is for methods.

python/cpython#89138 (comment) for some more general thoughts on runtime_checkable (maybe we actually should alias it to isinstance_checks_methods_exist ;-) )

We probably shouldn't prioritise speed over correctness here; users who care deeply about performance probably shouldn't be using runtime-checkable protocols anyway.

While this is true, a) I don't think we clearly communicate anywhere that runtime_checkable can be a hard to spot performance footgun, b) runtime_checkable is so incorrect to start with that it's hard to take an absolutist stance on this. I would be curious to see benchmarks on reasonably sized Protocols.

In general, hasattr has historically had bad support from type checkers, and I'd love to make it and make it be seen as a more viable runtime_checkable replacement. It is very explicit about its behaviour, relatively consistent in its performance, and is the most idiomatic way to do a structural existence check in Python.

[AlexWaygood]

We probably shouldn't prioritise speed over correctness here; users who care deeply about performance probably shouldn't be using runtime-checkable protocols anyway.

While this is true, a) I don't think we clearly communicate anywhere that runtime_checkable can be a hard to spot performance footgun, b) runtime_checkable is so incorrect to start with that it's hard to take an absolutist stance on this. I would be curious to see benchmarks on reasonably sized Protocols.

Agreed that we should definitely point out the performance implications of using runtime-checkable protocols in the docs, whether we go through with this change or not.

---

I just did some benchmarking using this script, and it does seem like the performance costs of using getattr_static instead of hasattr in typing.py are pretty pronounced:

import time
from typing import Protocol, runtime_checkable

@runtime_checkable
class HasX(Protocol):
    x: int

class Foo:
    @property
    def x(self) -> int:
        return 42

start_time = time.perf_counter()
for _ in range(100_000):
    isinstance(Foo(), HasX)
print(time.perf_counter() - start_time)

On main on my machine, this script completes in 5.00 seconds. With the patch I proposed in the original post to use getattr_static, it completes in 8.39 seconds.

Even if we agree that the semantics using getattr_static would be better here, and even if we think the backwards-compatibility considerations are surmountable -- are the improved semantics from using getattr_static here worth the performance penalty? The performance penalty will be felt by everybody who uses runtime-checkable protocols, whereas issues about protocols being unexpectedly evaluated from isinstance() checks have only been opened in the CPython repo a couple of times.

[carljm]

I'm beginning to think that @hauntsaninja has it right, and the OP's comment over in the CPython issue

Just personally I won't be using this feature, and I wouldn't advise anyone else to, its just too "unpredictable"

is actually the best available answer here :)

As in, maybe runtime_checkable has enough inherent problems that we shouldn't really be spending time improving it and encouraging its use; we should just document its quirks and limitations and move on.

I think better runtime checking of protocols is possible, but likely only in something like Static Python.