Support overriding dunder attributes in Enum subclass #12138
Conversation
mypy/checker.py
Outdated
| if (isinstance(sym.node, Var) and sym.node.has_explicit_value and | ||
| sym.node.name == '__members__'): | ||
| self.fail( | ||
| 'Cannot declare read-only attribute "__members__"', sym.node |
There was a problem hiding this comment.
Can you please move this string to message_registry.py?
There was a problem hiding this comment.
Done, had a bit of a hard time coming up with a reasonable name for the variable. Let me know if you got any better than what I could come up with
| if defn.info.fullname not in ENUM_BASES: | ||
| for sym in defn.info.names.values(): | ||
| if (isinstance(sym.node, Var) and sym.node.has_explicit_value and | ||
| sym.node.name == '__members__'): |
There was a problem hiding this comment.
Let's add a comment: why don't we allow __members__
There was a problem hiding this comment.
So, to clarify:
>>> import enum
>>> class E(enum.Enum):
... __members__ = (1, 2)
...
>>> class D(E):
... __members__ = (3, 4)
...
>>> D.__members__
mappingproxy({})It is not a runtime error. But, it does not make sense, right?
There was a problem hiding this comment.
Yeah, exactly. __members__ will always be overwritten. So it doesn't make sense to try to declare any value to it.
This comment has been minimized.
This comment has been minimized.
test-data/unit/check-enum.test
Outdated
| from enum import Enum | ||
|
|
||
| class WritingMembers(Enum): | ||
| __members__: typing.Dict[Enum, Enum] = {} # E: Cannot declare read-only attribute "__members__" |
There was a problem hiding this comment.
I think we can improve this error message. __members__ is not read only, it more like "internal"?..
I would be quite lost with the current one.
There was a problem hiding this comment.
Yeah, agreed, I tried to generalise what's said in the documentation a bit: https://docs.python.org/3/library/enum.html#supported-dunder-names
__members__is a read-only ordered mapping ofmember_name:memberitems. It is only available on the class.
Inlining it here just for reference
What do you think about Cannot assign to internal attribute "__members__"? Not completely sure if "assign" is most appropriate though
There was a problem hiding this comment.
Maybe exposing the internal implementation would be a good idea in this case?
Like 'Assigned "__members__" will be overriden by "Enum" internally'?
There was a problem hiding this comment.
That reads good IMO, lets stick with that!
This comment has been minimized.
This comment has been minimized.
sobolevn
left a comment
There was a problem hiding this comment.
I like it! Thank you!
I am going to wait for someone else to merge this, since I am new here and might miss something 🙂
This comment has been minimized.
This comment has been minimized.
|
Sorry for the slow response, can you fix the merge conflicts so that I can do another review pass and get this merged? |
de303eb to
360fd99
Compare
|
No worries, rebased and squashed just now |
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
|
@JukkaL friendly ping in case you missed that I bumped the branch |
Allows any dunder (`__name__`) attributes except `__members__` (due to it being read-only) to be overridden in enum subclasses. Fixes #12132.
Description
Allows any dunder(
__name__) attributes except__members__(due to it being read-only) to be overridden in enum subclasses.Fixes #12132
Test Plan
Added tests to the
check-enumsuiteExtra notes
I'm quite new to mypy's code. So please help out with stuff I might've missed or so. I'm also not sure if I've placed the
__members__fail correctly or if it's better placed in the analyser.I also tried to fix sunder (
_name_) attributes raisingAttributeErrors, but my changes needs a bit more polishing, so I left it for a later PR. Invalids there will raise during runtime anyways.