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

[chrisjsewell]

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

[chrisjsewell]

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

[JosephSBoyle]

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]

Brilliant thanks

[AlexWaygood]

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

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]

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]

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]

😢 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

[AlexWaygood]

😢 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]

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]

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

[chrisjsewell]

It just feels very unexpected that @property is essentially incompatible with @runtime_checkable, since presumably one is using @property over self.x = ... for a good reason

[AlexWaygood]

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

I assume that you want to compare instances of this class against a HasX protocol along these lines:

@runtime_checkable 
class HasX(Protocol):
    x: Any

It would be much more idiomatic in this case to raise AttributeError rather than RuntimeError from the x property, since you are, after all, trying to fetch an attribute if you're accessing the property. If you change your code to raise AttributeError, then the isinstance(X(), HasX) check will (correctly) return False, and no exception will be raised.

[chrisjsewell]

Plus the fact you really have no idea what instance could be in isintance(instance, X), so it essentially poses a "security risk" I guess that you are could be running code that you did not expect to run

[chrisjsewell]

It would be much more idiomatic in this case to raise AttributeError ...

Thinking out loud, perhaps you have to have a separate Protocol for an "open" instance vs a "closed" one,
although this is not what e.g. IO does:

cpython/Lib/typing.py

Line 3164 in 7894bbe

class IO(Generic[AnyStr]):

[JosephSBoyle]

I agree that perhaps it's unintuitive that the code in a property would be run but it's not any more of a security risk than if it didn't run the contents of the property method, since by even importing a class in the first place you're allowing it to run with whatever permissions you have set anyways.

To add to Alex's suggestion of raising an AttributeError I would suggest maybe using a second object, which is returned by your __enter__ method. That way x can only be accessed when the resource is available and you remove the erroneous path from your code (thus simplifying it!).

[chrisjsewell]

I certainly take the suggestions onboard thanks 🙏 , but still...

I feel the point of using isinstance is that you don't know what instance is, and now you kind of do need to know. Because, depending on what instance is, it's going to effect the behaviour of isinstance; how long it takes to run, whether it will except, etc .... It's no longer a "trivial" call
It just feels like @runtime_checkable, in its current form, is not worth the hassle, as opposed to not using Protocol or using a custom isinstance function

[hmc-cs-mdrissi]

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

f = Foo()
if isinstance(f, HasBar):  # False with the current implementation, but True if we change _ProtocolMeta.__instancecheck__ so that it doesn't evaluate the `bar` property
    y = f.bar

I find this example messy as the behavior for type checkers depends on type checker. pyright disagrees with mypy here and pyright thinks that Foo is compatible with HasBar and behaves like if check is True. pyright even gives an error in strict mode of isinstance(f, HasBar) is always true.

I also don't think even mypy interprets that code as you stated. If I adjust code a little to have,

f: HasBar = Foo()

mypy gives 0 errors so mypy also believes that Foo matches HasBar protocol. I did a check with other main type checkers and pyright/pyre/pytype all are happy allowing HasBar annotation on that line.

This issue feels like it fits better to discuss in typing repository. I think requiring runtime evaluation though of properties is awkward enough behavior that I'd just avoid any code I work with mixing both runtime checkable and properties. I do sometimes use runtime checkable and it's behaviors make more sense with methods only.

edit: I'm confused on meaning of example/how mypy or another type checker is supposed to protect you. A simpler,

f = Foo()
f.bar

raises at runtime, but passes type checking with 4 type checkers.

[AlexWaygood]

I also don't think even mypy interprets that code as you stated. If I adjust code a little to have,

f: HasBar = Foo()

mypy gives 0 errors so mypy also believes that Foo matches HasBar protocol. I did a check with other main type checkers and pyright/pyre/pytype all are happy allowing HasBar annotation on that line.

I didn't mean to imply that mypy had different behaviour. What I'm concerned about is situations like the following:

def print_attr(obj: object) -> None:
    if isinstance(obj, HasBar):
        print(obj.attr)

Type checkers will assume that the isinstance() guard makes the obj.attr attribute access safe inside the guard. Currently that does always hold true. But if we change the __instancecheck__ implementation in the way that's been proposed in this issue, it will no longer always hold true.

Anyway, @JelleZijlstra also expressed reservations in #102449 (comment), so this is evidently more controversial than I realised :) I'll start a discussion on python/typing to see what others think.

[chrisjsewell]

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

[AlexWaygood]

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

[AlexWaygood]

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