gh-149828: make Protocol compatible with Enum instantiation#149830
gh-149828: make Protocol compatible with Enum instantiation#149830randolf-scholz wants to merge 2 commits into
Conversation
randolf-scholz
requested review from
AlexWaygood,
JelleZijlstra and
ethanfurman
as code owners
sobolevn
left a comment
sobolevn
left a comment
There was a problem hiding this comment.
Looks fine to me! Let's wait for @ethanfurman :)
AlexWaygood
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @AlexWaygood for commit 5ab82ba 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F149830%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
randolf-scholz
changed the title
|
Had to update the PR title since I copy-pasted the wrong issue number. The |
| # Note: not using cls._is_protocol here, | ||
| # since this may be called before __init_subclass__ is resolved. | ||
| # for example when subclassing enum.Enum. | ||
| if any(b is Protocol for b in cls.__bases__): |
There was a problem hiding this comment.
Could this break when inheriting from typing_extensions.Protocol?
| globals()['T'] = T | ||
| test_pickle_dump_load(self.assertIs, SomeEnum.first) | ||
|
|
||
| def test_protocol_mixin_as_enum_member_value(self): |
There was a problem hiding this comment.
The change you made was to typing.py. Can you add a test case to test_typing that demonstrates the issue without relying on enum itself?
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
| globals()['Example'] = Example | ||
| globals()['Impl'] = Impl | ||
| globals()['ProtoEnum'] = ProtoEnum |
There was a problem hiding this comment.
I also feel like we shouldn't be mutating the global environment in a test like this. If these classes need to be global, can't we just define them in the global scope? I was honestly surprised the refleak buildbots didn't complain about this
There was a problem hiding this comment.
They don't because we run a warmup round first; refleak buildbots only complain if you keep leaking on every test.
6 participants
Fixes #149828
This was surprisingly easy, the issue is that with enums,
__init_subclass__is only called afterEnumType.__new__is finished.However, when subclassing from
typing.Protocol,_is_protocolis only set during__init_subclass__. The fix is to replace the lookup of_is_protocolinside_no_init_or_replace_initwith the actual check.A more robust solution would probably be for
_is_protocolto be a lazy class property, but that would be a larger change and class-properties are not natively supported...cc @sobolevn