-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix enum truthiness for StrEnum #18379
Conversation
f20c12f
to
aab9bc3
Compare
aab9bc3
to
83cf4c2
Compare
elif isinstance(t, Instance): | ||
if t.type.is_final or t.type.is_enum: | ||
return UninhabitedType(line=t.line) | ||
elif isinstance(t, LiteralType) and t.is_enum_literal(): | ||
return UninhabitedType(line=t.line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metaclasses are a red herring, enums are just effectively final, so this is safe to do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure it's not about the order of operations? I.e. first __bool__
gets checked, then __len__
and then the same thing happens for the metaclass.
But here __bool__
gets checked and then immediately it goes to the metaclass.
I think falling back to the metaclass is correct, it just happens too early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright I had a read of the blog post, maybe metaclasses aren't involved in truthiness after all and it just so happens that the default is to return True
if you have neither a __bool__
or __len__
, although I'm a little confused about the purpose of __bool__
on EnumMeta
, when that is the default truthiness anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked, looks like the metaclass does matter, but only for the truthiness of type[Enum]
, not the instances.
>>> from enum import EnumMeta
>>> EnumMeta.__bool__ = lambda self: False
>>> from enum import Enum
>>> bool(Enum)
False
>>> class Foo(Enum):
... a = "a"
...
>>> bool(Foo.a)
True
>>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. It would also affect bool(Foo)
... continuing from your monkey patch ...
>>> bool(Foo)
False
>>> type(Foo.a).__mro__
(<enum 'Foo'>, <enum 'Enum'>, <class 'object'>)
>>> type(Foo).__mro__
(<class 'enum.EnumType'>, <class 'type'>, <class 'object'>)
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
Fixes #18376
See also https://snarky.ca/unravelling-not-in-python/