Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typing: undocumented behaviour change for protocols decorated with @final and @runtime_checkable in 3.11 #103171

Closed
AlexWaygood opened this issue Apr 1, 2023 · 14 comments
Labels
3.11 only security fixes stdlib Python modules in the Lib dir topic-typing type-bug An unexpected behavior, bug, or error

Comments

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 1, 2023

Python 3.11 introduced an undocumented behaviour change for protocols decorated with both @final and @runtime_checkable. On 3.10:

>>> from typing import *
>>> @final
... @runtime_checkable
... class Foo(Protocol):
...     def bar(self): ...
...
>>> class Spam:
...     def bar(self): ...
...
>>> issubclass(Spam, Foo)
True
>>> isinstance(Spam(), Foo)
True

On 3.11:

>>> from typing import *
>>> @final
... @runtime_checkable
... class Foo(Protocol):
...     def bar(self): ...
...
>>> class Spam:
...     def bar(self): ...
...
>>> issubclass(Spam, Foo)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<frozen abc>", line 123, in __subclasscheck__
  File "C:\Users\alexw\coding\cpython\Lib\typing.py", line 1547, in _proto_hook
    raise TypeError("Protocols with non-method members"
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: Protocols with non-method members don't support issubclass()
>>> isinstance(Spam(), Foo)
False

This is because, following 0bbf30e (by @JelleZijlstra), the @final decorator sets a __final__ attribute wherever it can, so that it is introspectable by runtime tools. But the runtime-checkable-protocol isinstance() machinery doesn't know anything about the __final__ attribute, so it assumes that the __final__ attribute is just a regular protocol member.

This should be pretty easy to fix: we just need to add __final__ to the set of "special attributes" ignored by runtime-checkable-protocol isinstance()/issubclass() checks here:

cpython/Lib/typing.py

Lines 1906 to 1909 in 848bdbe

_TYPING_INTERNALS = frozenset({
'__parameters__', '__orig_bases__', '__orig_class__',
'_is_protocol', '_is_runtime_protocol'
})

@JelleZijlstra do you agree with that course of action?

Linked PRs

@AlexWaygood AlexWaygood added type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir 3.11 only security fixes topic-typing 3.12 bugs and security fixes labels Apr 1, 2023
@AlexWaygood AlexWaygood changed the title Undocumented behaviour change for protocols decorated with @final and @runtime_checkable in 3.11 Typing: undocumented behaviour change for protocols decorated with @final and @runtime_checkable in 3.11 Apr 1, 2023
@JelleZijlstra
Copy link
Member

My first instinct is to leave this as is. Marking Protocols as final is a weird thing to do in the first place (the whole point of protocols is to subclass from them, though usually implicitly). Adding more names to that _TYPING_INTERNALS list is risky because if people use those names in their protocols, we'll silently ignore them. You could imagine a Protocol that is meant to match only classes decorated with @final.

@JelleZijlstra
Copy link
Member

This might be a bigger issue for PEP-702 though: what if you want to @deprecated a runtime-checkable Protocol? That makes more sense than making it final, so I'll amend the PEP to say that __deprecated__ should be ignored by @runtime_checkable.

@AlexWaygood
Copy link
Member Author

My first instinct is to leave this as is. Marking Protocols as final is a weird thing to do in the first place (the whole point of protocols is to subclass from them, though usually implicitly). Adding more names to that _TYPING_INTERNALS list is risky because if people use those names in their protocols, we'll silently ignore them. You could imagine a Protocol that is meant to match only classes decorated with @final.

Sure. In that case, I think we should probably document the behaviour change with a .. versionchanged notice in the docs somewhere — sound good?

@JelleZijlstra
Copy link
Member

Sure, that's fine.

@sobolevn
Copy link
Member

sobolevn commented Apr 1, 2023

I mark all things as @final by default. Because inside an internal code-base no things should be extended without a good reason.

So, I would consider this as a bug. __final__ is an internal thing that should not be exposed to users and should not affect the runtime of regular operations.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Apr 1, 2023

__final__ is an internal thing that should not be exposed to users and should not affect the runtime of regular operations.

I'm not sure that makes sense. The fact that @final sets the __final__ attribute is documented, and I don't think we ever intended it to be an internal thing. typing.py doesn't need the attribute to be set at all; the only reason why we introduced the behaviour in 3.11 where it sets the attribute wherever possible was so that third-party tools could more easily introspect whether a method or class had been decorated with @final. That's the opposite of it being an internal thing. https://docs.python.org/3/library/typing.html#typing.final

Personally, I wasn't initially sure this new behaviour makes sense for isinstance(), but I can see arguments either way. What really concerns me is the new behaviour with issubclass(), which seems pretty unfortunate to me.

@AlexWaygood
Copy link
Member Author

@ilevkivskyi, do you have any opinions on how @final and @runtime_checkable should interact at runtime?

@JelleZijlstra
Copy link
Member

I haven't tried, but won't the change from #103160 fix this issue, because the __final__ attribute is added after we compute the set of attributes?

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Apr 2, 2023

I haven't tried, but won't the change from #103160 fix this issue, because the __final__ attribute is added after we compute the set of attributes?

Good point, #103160 does indeed fix this issue!

I wasn't planning on backporting #103160 (if it's even accepted), though, as I was thinking of it as a performance optimisation (and it does change behaviour in a few other subtle ways, as discussed in the PR thread). So if #103160 is merged, that means that 3.12 will have the same behaviour as we had in 3.10 for @final runtime-checkable protocols, but 3.11 will be the "odd one out".

@ilevkivskyi
Copy link
Member

FWIW I think protocols should not be final. So the behaviour change is fine.

@AlexWaygood
Copy link
Member Author

Maybe we should just add a note to the docs for @final saying that combining the decorator with @runtime_checkable isn't supported and might have unpredictable consequences.

@sobolevn
Copy link
Member

sobolevn commented Apr 3, 2023

I think protocols should not be final

But, they can be.

What is the reason not to ignore __final__ attribute? 🤔
It surely does not affect @runtime_checkable in any other manner.

I think that having an expected default behaviour in this case is easier than writting a warning in the docs.

@AlexWaygood AlexWaygood removed the 3.12 bugs and security fixes label Apr 5, 2023
@AlexWaygood
Copy link
Member Author

Good point, #103160 does indeed fix this issue!

I wasn't planning on backporting #103160 (if it's even accepted), though, as I was thinking of it as a performance optimisation (and it does change behaviour in a few other subtle ways, as discussed in the PR thread). So if #103160 is merged, that means that 3.12 will have the same behaviour as we had in 3.10 for @final runtime-checkable protocols, but 3.11 will be the "odd one out".

#103160 has now been merged, so it is just 3.11 that has the behaviour change now.

@JelleZijlstra
Copy link
Member

I think we should fix 3.11 so that it excludes __final__ when looking at protocol compatibility, as that will make 3.11 behave consistently with both 3.10 and 3.12.

AlexWaygood added a commit to AlexWaygood/cpython that referenced this issue Jun 7, 2023
…ime-checkable protocols decorated with `@final`
AlexWaygood added a commit that referenced this issue Jun 7, 2023
AlexWaygood added a commit to AlexWaygood/cpython that referenced this issue Jun 7, 2023
…ls decorated with `@final`

Forward-port of the tests that were added to the 3.11 branch in python#105445
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 7, 2023
…ls dec orated with `@final` (pythonGH-105473)

Forward-port of the tests that were added to the 3.11 branch in pythonGH-105445
(cherry picked from commit f5df347)

Co-authored-by: Alex Waygood <[email protected]>
AlexWaygood added a commit that referenced this issue Jun 7, 2023
… orated with `@final` (#105473)

Forward-port of the tests that were added to the 3.11 branch in #105445
AlexWaygood added a commit that referenced this issue Jun 7, 2023
…ols decorated with `@final` (GH-105473) (#105474)

Forward-port of the tests that were added to the 3.11 branch in GH-105445
(cherry picked from commit f5df347)

Co-authored-by: Alex Waygood <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes stdlib Python modules in the Lib dir topic-typing type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants