gh-102433: Use inspect.getattr_static in typing._ProtocolMeta.__instancecheck__

[AlexWaygood]

Use getattr_static rather than hasattr in _ProtocolMeta.__instancecheck__, in order to avoid unexpectedly evaluating descriptors and __getattr__ methods in the process of calling isinstance() against runtime-checkable protocols.

This slows down isinstance() checks against runtime-checkable protocols by around 46% (on my machine), but the consensus in python/typing#1363 is that the performance penalty is worth it because:

• Any isinstance() check causing an expensive (or otherwise badly behaved) property or __getattr__ method to be evaluated is pretty surprising behaviour.
• Bringing the runtime's understanding of runtime-checkable protocols closer to the way static type checkers understand these classes is highly desirable.
• Runtime-checkable protocols should not be used for performance-critical code anyway (this is now clearly documented following Typing: Document performance pitfalls of protocols decorated with @runtime_checkable #102936.)

In various subtle edge cases, this change will mean that some classes that used to be considered structural "subclasses" of a certain runtime-checkable protocol will no longer be considered subclasses of that protocol (and vice versa). As such, this can't be backported. (Perhaps we should consider documenting the existing behaviour for 3.11 and 3.10.)

• Issue: isinstance on runtime_checkable Protocol has side-effects for @property methods #102433

Note that most discussion has actually taken place in a python/typing issue rather than a CPython issue:

• How should isinstance() work at runtime when comparing classes with properties against runtime-checkable protocols? typing#1363

[AlexWaygood]

I'd be very interested to know if any maintainers of libraries that use type annotations at runtime have any concerns about this proposed change. Off the top of my head, I can think of:

• pydantic (@samuelcolvin?)
• cattrs (@Tinche?)
• beartype (@leycec)
• typeguard (@agronholm?)
• fastAPI (@tiangolo?)

[gvanrossum]

I’m on vacation, please find another reviewer.

[samuelcolvin]

Thanks for tagging me, I'll discuss with the pydantic team and respond in a few days.

[leycec]

Thanks so much for the apropos ping, @AlexWaygood. You do the good typing work! Sadly, this...

This slows down isinstance() checks against runtime-checkable protocols by around 46% (on my machine)...

...is a non-starter. That's measurably unacceptable degradation. The existing Protocol API works. There's no demonstrable justification for both breaking that implementation in backwards-incompatible edge cases and imposing additional runtime costs while doing so.

Python ≥ 3.11's renewed focus on runtime performance is germane here. Efficiency is no longer orthogonal to our goals in the Python ecosystem. Efficiency is increasingly a goal in and of itself. Do not sacrifice quantifiable speed at the golden altar of well-intended but ultimately futile attempts to force parity between static and runtime type-checkers.

Static and runtime type-checkers will inevitably disagree. These are profoundly different understandings of what type-checking even means. Reducing the scope of that disagreement is useful – but preserving (or ideally improving) runtime performance is substantially more useful.

Static type-checking serves Python's runtime needs by improving the robustness and reliability of Python at runtime. Static analysis is subordinate to runtime. Runtime always comes first. Not last. We're all here for what Python accomplishes at runtime. To paraphrase President Truman:

The buck stops at runtime.

[Tinche]

Should be fine for cattrs, we have separate build and run stages for the de/serializers.

[AlexWaygood]

Thanks for the feedback @leycec! To be clear, I fully agree with your sentiment here, and have no wish to make the CPython runtime subservient to the needs of static typing:

Static type-checking serves Python's runtime needs by improving the robustness and reliability of Python at runtime. Static analysis is subordinate to runtime. Runtime always comes first. Not last. We're all here for what Python accomplishes at runtime.

I realise that in my summary of this PR at the top, I maybe should have been clearer about the bug that this PR is trying to fix. Ultimately this PR is first and foremost about fixing unexpected behaviour at runtime. Improving consistency between how static type checkers and the runtime see certain classes is a "nice to have" that comes along with the fix.

Here is a demonstration of the user-visible bug that this PR is trying to fix (which has been reported to the CPython repo multiple times, so is clearly surprising behaviour for users). Properties, other descriptors, and __getattr__ methods are all currently "called" if you compare an object against a runtime-checkable protocol using isinstance(). This is the issue that this PR is trying to fix.

REPL session demonstrating the bug as it exists on CPython `main`

>>> import time
>>> from typing import runtime_checkable, Protocol
>>> @runtime_checkable
... class HasX(Protocol):
...     x: int
...
>>> class A:
...     @property
...     def x(self):
...         raise RuntimeError('no')
...
>>> class B:
...     @property
...     def x(self):
...         time.sleep(3600)
...         return 42
...
>>> class C:
...     def __getattr__(self, name):
...         raise RuntimeError('no')
...
>>> isinstance(A(), 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: no
>>> isinstance(C(), 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 3, in __getattr__
RuntimeError: no
>>> isinstance(B(), HasX)  # this will take an hour to evaluate

I don't like (at all) that this results in a 46% performance degradation for simple isinstance() checks against runtime-checkable protocols, and I suggested a few workarounds in python/typing#1363 that would still fix the bug for most users of runtime-checkable protocols, while having a much smaller performance degradation (e.g. python/typing#1363 (comment)). These were criticized, however, on the grounds that:

• they weren't particularly "principled" solutions, which might have bugs in subtle edge cases
• they felt like premature optimizations, since isinstance() checks against runtime-checkable protocols are abysmally slow anyway, so users should probably not be using runtime-checkable protocols for performance-critical code.

[AlexWaygood]

There are three options here that I can see:

1. Go with this solution (take the performance hit for now, but see if we can optimise _ProtocolMeta.__instancecheck__ and/or inspect.getattr_static in followup PRs).
2. Go with a "less principled" solution that doesn't use inspect.getattr_static, and therefore has a smaller performance degradation, but might have bugs in subtle edge cases.
3. Declare the bug as "wontfix"; document clearly in the typing documentation that you should be careful about comparing instances of a class X against a runtime-checkable protocol if class X has properties with side effects, which would be triggered during the course of an isinstance() check against a runtime-checkable protocol.

[agronholm]

IMHO, correct behavior should always come first, then maybe good performance.

As far as typeguard is concerned, I don't see a problem here.

[samuelcolvin]

I'm pretty sure this shouldn't affect pydantic since we tend to avoid isinstance() checks unless they're unavoidable, we also don't have a validator for Protocol, and if we do, I guess we would need our own logic.

I'm therefore neutral on it.

FWIW (and it's not worth much since it's probably too late), my personal opinion is that torturing isinstance to provide partial runtime type checking is a mistake - it would be better if isinstance just checked that the first arg's type inherits from the second arg - no magic, predicable behaviour, predictably fast. Then python could provide a second method for more powerful type checking with worse performance and more magic. I would argue that the decision to contort isinstance is the route cause headaches like this, and many more.

[AlexWaygood]

FWIW (and it's not worth much since it's probably too late), my personal opinion is that torturing isinstance to provide partial runtime type checking is a mistake - it would be better if isinstance just checked that the first arg's type inherits from the second arg - no magic, predicable behaviour, predictably fast. Then python could provide a second method for more powerful type checking with worse performance and more magic. I would argue that the decision to contort isinstance is the route cause headaches like this, and many more.

FWIW I agree fully with this, and I'm quite glad that protocols are by default not runtime-checkable. But, as you say, it's almost certainly too late to change this now without going through a painful deprecation period that (in my opinion) would cause more trouble than it's worth.

[agronholm]

I agree with you both. The checking of run-time protocols would probably be best left to run-time type checkers, but getting rid of run-time checkable protocols would be a PITA at this point.

typeguard is lazy and uses instance() on run-time protocols, and doesn't currently provide support for static protocols. That could be changed with enough motivation and free time though.

[carljm]

Note the recent comment on the issue reinforcing that the current behavior of @runtime_checkable executing arbitrary properties causes (and hides) real bugs.

There are three options here that I can see:

1. Go with this solution (take the performance hit for now, but see if we can optimise _ProtocolMeta.__instancecheck__ and/or inspect.getattr_static in followup PRs).
2. Go with a "less principled" solution that doesn't use inspect.getattr_static, and therefore has a smaller performance degradation, but might have bugs in subtle edge cases.
3. Declare the bug as "wontfix"; document clearly in the typing documentation that you should be careful about comparing instances of a class X against a runtime-checkable protocol if class X has properties with side effects, which would be triggered during the course of an isinstance() check against a runtime-checkable protocol.

I agree that these are the options.

I think the advice we would have to document in the case of (3) is basically un-followable. If you are doing isinstance(x, SomeProtocol) in your code, it's because you don't already know the type of x, so there's no way you can "be careful" that x isn't of a type with dangerous or costly executable properties.

I agree with @agronholm that we should prioritize correct behavior first, and then seek to optimize it. IMO correct behavior (purely from a runtime perspective, ignoring static typing) is that we should not execute arbitrary descriptor code in an isinstance check, and inspect.getattr_static is how to reliably do that. So I favor (1).

[AlexWaygood]

I'm going to merge this once the CI passes.

We've already merged two optimisation PRs into main, which, taken together, basically make up for the performance hit that this PR gives:

• gh-74690: typing: Don't unnecessarily call _get_protocol_attrs twice in _ProtocolMeta.__instancecheck__ #103141
• gh-74690: Micro-optimise typing._get_protocol_attrs #103152

There are two more PRs (which are slightly riskier) that have yet to be decided on:

• gh-74690: typing: Simplify and optimise _ProtocolMeta.__instancecheck__ #103159
• gh-74690: typing: Call _get_protocol_attrs and _callable_members_only at protocol class creation time, not during isinstance() checks #103160

If anybody has any opinions on either of those, feedback is very welcome! Once decisions have been taken on the two open PRs, I'll create another PR adding a separate NEWS entry summarising all of the performance work that's been done (and the internal changes to typing.py that were needed for the performance optimisations).

I'd also like to look into optimising inspect.getattr_static itself, as well, since the core reason for the performance hit here is obviously that inspect.getattr_static is very slow compared to hasattr.

[AlexWaygood]

Many thanks to everybody who took their time to provide feedback on this PR, I really appreciate it!

[tiangolo]

Sorry for my delay replying here. 🙈 Thanks a lot for pinging me. 🙏

As FastAPI and my other tools heavily depend and interact with Pydantic (or copy its ideas), if this doesn't affect Pydantic (as Samuel confirmed above), it doesn't affect FastAPI and friends. 🚀

[GDYendell]

inspect.getattr_static uses inspect._check_instance which uses object.__getattribute__(obj, "__dict__"). This fails for bound instance methods (although obj.__dict__ does work), so they cannot fulfill runtime_checkable Protocols anymore.

from typing import Protocol, runtime_checkable


@runtime_checkable
class Labelled(Protocol):
    label: str


class A:
    def f(self):
        pass


A.f.label = "f"

a = A()
print(a.f.__dict__)  # {'label': 'f'}
print(object.__getattribute__(a.f, "__dict__"))  # AttributeError

assert isinstance(a.f, Labelled)  # AssertionError

Is this an intended/known change? Could it be noted in the changelog here as an example of case that will no longer work?

[AlexWaygood]

@GDYendell, I will admit that I did indeed fail to consider how this would change the behaviour of runtime-checkable protocols when used in conjunction with bound instance methods that have extra attributes monkeypatched onto them.

Can you possibly open a new issue about this? I haven't got the time to look at it right now, and comment threads on long-merged PRs are unfortunately likely to get forgotten about.