isinstance on runtime_checkable Protocol has side-effects for @property methods

For 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

gh-102433: Add tests for how classes with properties interact with isinstance() checks on typing.runtime_checkable protocols #102449
[3.11] gh-102433: Add tests for how classes with properties interact with isinstance() checks on typing.runtime_checkable protocols (GH-102449) #102592
[3.10] gh-102433: Add tests for how classes with properties interact with isinstance() checks on typing.runtime_checkable protocols (GH-102449) #102593
gh-102433: Use inspect.getattr_static in typing._ProtocolMeta.__instancecheck__ #103034

Apologies, if this is a duplicate, or the wrong place to raise this, but I couldn't find anything much on this issue outside of https://stackoverflow.com/questions/66641191/rationale-of-instancecheck-on-protocols

@chrisjsewell

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:)

Brilliant thanks

@JosephSBoyle

@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.

@AlexWaygood

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.

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
...

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.

😢 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

😢 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! 🙂

@AlexWaygood

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.

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 ...

Fix unused classes in a typing test (GH-102437)

Type checkers will assume that the isinstance() guard makes the obj.attr attribute access safe inside the guard.

Does it though?
what about:

class A:
    @property
    def a(self): ...

class B(A):
    @property
    def a(self):
        raise AttributeError

isinstance(B(), A)

that passes, then raises
is there any difference?

For me, trying to account for AttributeError inside of @property seems very niche, compared to the larger issue of side effects, but thats just my viewpoint

Fix unused classes in a typing test (pythonGH-102437)

I've opened an issue here, attempting to summarise the discussion, and seeking thoughts from the typing community more broadly:

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

Fix unused classes in a typing test (GH-102437)

gh-102433: Add tests for how classes with properties interact with `i…

pythongh-102433: Add tests for how classes with properties interact w…

gh-102433: Add tests for how classes with properties interact with `i…

pythongh-102433: Add tests for how classes with properties interact w…

gh-102433: Use `inspect.getattr_static` in `typing._ProtocolMeta.__in…

#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!

pythongh-102433: Use `inspect.getattr_static` in `typing._ProtocolMet…

Navigation Menu

Repository navigation

`isinstance` on `runtime_checkable` `Protocol` has side-effects for `@property` methods #102433

Linked PRs

23 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

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

Description

Linked PRs

Activity

chrisjsewell commented on Mar 5, 2023

JosephSBoyle commented on Mar 5, 2023

chrisjsewell commented on Mar 5, 2023

AlexWaygood commented on Mar 5, 2023

JosephSBoyle commented on Mar 5, 2023

AlexWaygood commented on Mar 5, 2023

AlexWaygood commented on Mar 5, 2023

chrisjsewell commented on Mar 5, 2023

AlexWaygood commented on Mar 5, 2023

JosephSBoyle commented on Mar 5, 2023

chrisjsewell commented on Mar 5, 2023

23 remaining items

chrisjsewell commented on Mar 6, 2023

AlexWaygood commented on Mar 6, 2023

AlexWaygood commented on Apr 2, 2023

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

`isinstance` on `runtime_checkable` `Protocol` has side-effects for `@property` methods #102433