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

Enum values on enum class can't be reassigned or deleted #7385

Closed
erictraut opened this issue Feb 25, 2022 · 7 comments · Fixed by #7388
Closed

Enum values on enum class can't be reassigned or deleted #7385

erictraut opened this issue Feb 25, 2022 · 7 comments · Fixed by #7388

Comments

@erictraut
Copy link
Contributor

The EnumMeta class in enum.pyi was recently modified to include a __setattr__ and __delattr__ method. These were flagged as a bug in pyright's unit tests because an attempt to set or delete an attribute on an enum class results in a runtime error.

from enum import Enum

class Color(Enum):
    red = "red"

# This should generate an error because reassignment of enum
# values is not allowed.
# Note: A change to typeshed's enum.pyi breaks this test.
Color.red = "new"  # Runtime exception

The above code is no longer flagged as an error by pyright although it generates a runtime exception.

@AlexWaygood, I think you submitted the PR for this change. Was the addition of __setattr__ and __delattr__ intentional?

@hauntsaninja
Copy link
Collaborator

Thanks for catching. Looks like they were added in #7231. I'd be in favour of removing them; similar situation to #7347

@AlexWaygood
Copy link
Member

I'll take a look tomorrow, thanks for flagging!

@AlexWaygood
Copy link
Member

AlexWaygood commented Feb 26, 2022

I assumed that the changes made to enum.pyi would not have this kind of impact on type checkers, due to the fact that EnumMeta already has __setattr__ and __delattr__ defined on the class. These methods are defined on object in builtins.pyi, and all classes inherit from object.

>>> from enum import EnumMeta
>>> EnumMeta.__mro__
(<class 'enum.EnumMeta'>, <class 'type'>, <class 'object'>)

The signatures of the overrides that were added in #7231 are identical to the signatures of these methods on builtins.object, except for the fact that the arguments are positional-only on builtins.object, and are positional-or-keyword on EnumMeta (reflecting the reality at runtime).


Moreover, it seems to me that enum classes require more subtle special casing than can be expressed in the stubs:

Python 3.10.2 (tags/v3.10.2:a58ebcc, Jan 17 2022, 14:12:15) [MSC v.1929 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from enum import Enum
>>> class Baz(Enum):
...     ONE = 1
...
>>> Baz.TWO = 2
>>> Baz
<enum 'Baz'>
>>> list(Baz)
[<Baz.ONE: 1>]
>>> Baz.TWO
2

There's no runtime exception here; the enumeration simply gains a new class attribute, which is not converted to become a new member.


All that said, I'd be fine with this change being reverted if it creates an inconvenience for pyright. Positional-only differences to the runtime aren't that important, and we can always allowlist these two methods if typeshed ever starts using a version of stubtest that has python/mypy#12184 and python/mypy#12203 incorporated. (The two stubtest changesets together would mean that stubtest would start emitting errors for EnumMeta.__setattr__ and EnumMeta.__delattr__, due to the positional-only differences with the runtime signatures.)

@erictraut
Copy link
Contributor Author

I think all type checkers special-case (ignore) the __getattr__, __setattr__, etc. on object. If they didn't, any member access would be allowed for any object.

I'd prefer not to add special-case logic to ignore additional __getattr__, __setattr__, etc. in other classes as well because that's not a scalable approach. Correct me if I'm wrong, but I don't think there's a legitimate reason (from a type safety perspective) to allow Bar.TWO = 2 outside of the class definition, and it should properly be flagged as an error.

@AlexWaygood
Copy link
Member

I think all type checkers special-case (ignore) the getattrsetattr, etc. on object.

Fair enough, I wasn't aware! Perhaps we should add a comment to the stub for object mentioning that.

I don't think there's a legitimate reason (from a type safety perspective) to allow Bar.TWO = 2 outside of the class definition, and it should properly be flagged as an error.

Yes, I agree. I'll send a PR to revert the changes to EnumMeta later today.

@AlexWaygood
Copy link
Member

It's a different discussion (and probably not worth the disruption to remove them, since they don't seem to be doing any harm and you never know which changes will cause unexpected breakages), but I wonder if there's even any point having __setattr__ and __delattr__ be defined on object.

@erictraut
Copy link
Contributor Author

I think all type checkers (I tested mypy and pyright) allow explicitly calling __setattr__, but they disallow implicit use of __setattr__.

class Foo: ...
foo = Foo()
foo.__setattr__("hello", 2) # allowed
foo.hello = 2 # not allowed

If we want to retain that behavior, then there is utility in retaining those methods on object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants