gh-145688: Fix _get_protocol_attrs matching user classes named "Protocol" or "Generic"#145724
gh-145688: Fix _get_protocol_attrs matching user classes named "Protocol" or "Generic"#145724mvanhorn wants to merge 3 commits intopython:mainfrom
Conversation
…"Protocol" or "Generic"
The `_get_protocol_attrs` function used `base.__name__ in {'Protocol', 'Generic'}`
to skip the base typing classes, which also skipped user-defined Protocol
subclasses that happened to be named "Protocol" or "Generic". This caused
`get_protocol_members()` and `__protocol_attrs__` to return empty results
for such classes.
Changed to check both `__name__` and `__module__` to ensure only the actual
`typing.Protocol` and `typing.Generic` classes are skipped.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Lib/typing.py
Outdated
| attrs = set() | ||
| for base in cls.__mro__[:-1]: # without object | ||
| if base.__name__ in {'Protocol', 'Generic'}: | ||
| if base.__name__ in {'Protocol', 'Generic'} and base.__module__ == 'typing': |
There was a problem hiding this comment.
I'm not sure if this is necessary, but maybe raise a ValueError if this is true?
If you end up raising an exception, just remember to update the test and NEWS entry.
There was a problem hiding this comment.
Thanks for the suggestion! I think skipping is the right behavior here though - a user class legitimately named Protocol should just have its attrs collected normally, not raise an error.
| @@ -0,0 +1,5 @@ | |||
| Fixed :func:`typing.get_protocol_members` and :attr:`~typing.Protocol.__protocol_attrs__` | |||
| returning empty results for Protocol subclasses named ``"Protocol"`` or ``"Generic"``. | |||
| The check in :func:`_get_protocol_attrs` now uses identity comparison instead of | |||
There was a problem hiding this comment.
I don't think that you need to explain how you fixed it. Maybe something like this:
Now a :class:typing.Protocol class can be named Protocol or Generic
This needs more refining, but I think it could lead to a better description.
There was a problem hiding this comment.
Good point - simplified to focus on the user-visible behavior. Thanks!
|
If I remember correctly, changing this might break the typing_extensions backport of |
…ttrs Address review feedback: also skip bases from typing_extensions module so the backported Protocol class is treated the same as typing.Protocol. Simplify NEWS entry to focus on user-visible behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Good catch, thanks @AlexWaygood! Updated the check to |
hmm, but there may be other third-party libraries as well that have come to rely on the current behaviour. For example, there is also |
Per @AlexWaygood's review, checking base.__module__ in {'typing', 'typing_extensions'} is fragile and would break third-party libraries like beartype that define their own Protocol using _ProtocolMeta. Use `base is Protocol or base is Generic` instead, which is the same pattern already used at line 1176 in _generic_class_getitem. This precisely targets the two base classes we want to skip without affecting any third-party Protocol implementations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All commit authors signed the Contributor License Agreement. |
|
Great point, @AlexWaygood - the module allowlist approach is fragile. Switched to identity checks ( |
3 participants
Fixes #145688
Summary
_get_protocol_attrs()usedbase.__name__ in {'Protocol', 'Generic'}to skip the basetypingclasses when collecting protocol members. This also matched user-defined Protocol subclasses that happened to share those names, causingget_protocol_members()and__protocol_attrs__to return empty results.Fix: Changed the check to also verify
base.__module__ == 'typing', so only the actualtyping.Protocolandtyping.Genericare skipped.Note: A pure identity check (
base is Protocol) was considered but doesn't work because_get_protocol_attrsis called during module initialization beforeProtocolis defined.Test plan
test_get_protocol_members_named_protocol_or_genericcovering both "Protocol" and "Generic" named subclassestest_typingtests passThis contribution was developed with AI assistance (Claude Code).