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

Cannot define Enum class with a mixin class derived from ABC #119946

Open
ygale opened this issue Jun 2, 2024 · 14 comments
Open

Cannot define Enum class with a mixin class derived from ABC #119946

ygale opened this issue Jun 2, 2024 · 14 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@ygale
Copy link

ygale commented Jun 2, 2024

Bug report

Bug description:

from abc import ABC, abstractmethod
from enum import Enum

class HasColor(ABC):
    @abstractmethod
    def color(self):
        ...

class Flowers(HasColor, Enum):
    ROSES = 1
    VIOLETS = 2
    def color(self):
        match self:
            case self.ROSES:
                return 'red'
            case self.VIOLETS:
                return 'blue'

This results in:

TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

There is a work-around by metaclass hacking. But these two basic Python features are expected to Just Work together in a simple, intuitive, and pythonic way.

The fix should be very simple - make EnumType a subclass of ABCMeta. But it's slightly more complicated than that, so that the _simple_enum decorator and _test_simple_enum continue to work as expected. See the provided patch.

CPython versions tested on:

3.12, 3.13

Operating systems tested on:

macOS

Linked PRs

@ygale ygale added the type-bug An unexpected behavior, bug, or error label Jun 2, 2024
@nineteendo

This comment was marked as resolved.

@AlexWaygood AlexWaygood added the stdlib Python modules in the Lib dir label Jun 2, 2024
@ygale
Copy link
Author

ygale commented Jun 2, 2024

@nineteendo Can you come up with a case where it would break user code? It seems pretty safe to me.

If you do not use an ABC-derived mixin with Enum, the only change is a few harmless additional ABC-related members of the metaclass for an Enum. None of the ABC machinery will fire, and the resulting Enum class itself will be unchanged.

If you do use an ABC-derived mixin, how did you get it to work until now? If you used the @_simple_enum decorator, nothing changed and it will still work. If you used the simple work-around of defining an empty metaclass that inherits from both ABCMeta and EnumType, everything will still work. Same if you used one of the older workarounds where you define a custom metaclass with some methods and use that.

If you did some really deep metaclass hacking that modifies the way ABCs work AND you didn't override all of the metaclass methods AND you use your custom ABC machinery together with an Enum class, then this might require some tweaking. It seems to me that this very specialized corner case for very advanced users is a small cost compared to the surprising common failure we are fixing.

If you can construct a plausible concrete case where this might actually break user code, it's likely we can tweak this solution to provide backwards compatibility.

@nineteendo

This comment was marked as resolved.

@ygale
Copy link
Author

ygale commented Jun 2, 2024

We have this in enum.py:

EnumMeta = EnumType         # keep EnumMeta name for backwards compatibility
class Enum(metaclass=EnumType):
    ...

So in my example, we have that Flowers.ROSES is an instance of the class Flowers, which is a subclass of Enum. But we have type(Flowers) is EnumType - which is a bit weird, because usually the type of class objects is type. That allows the Flowers class object to have methods that provide enum features. When defining Flowers with functional syntax in typed Python, you would write Flowers: EnumType = Enum('Flowers', ...).

After this change, Flowers is also a descendant of ABCMeta. The effect of that is that there are a few ABC-related members (such as _abc_impl) that are only used when you use it as a metaclass to create an ABC or a subclass of an ABC. To use an ABC as a mixin for the enum - that is exactly what you need. In any other case, they are not used.

@nineteendo
Copy link
Contributor

nineteendo commented Jun 2, 2024

Thanks, I think EnumType is a confusing name for the meta class of Enum. EnumType sounds like EnumType = Enum to me, like all types in types.

Flowers.ROSES is an instance of Flowers, which is a subclass of Enum. Enum is an instance of EnumType, which is a subclass of ABCMeta.

@ygale
Copy link
Author

ygale commented Jun 2, 2024

Agreed. I am guessing that the motivation behind the name change was that it is now also used as a type in typed Python.

@ethanfurman ethanfurman self-assigned this Jun 2, 2024
@ethanfurman
Copy link
Member

One of the big concerns for enums are their creation times. We'll need performance numbers with and without the ABC addition.

@sobolevn
Copy link
Member

sobolevn commented Jun 3, 2024

I think that we should not complicate EnumType which is very complex as it is.

But, instead you can solve this by adding several lines to your code:

>>> import abc
>>> import enum
>>> class A(abc.ABC): ...
... 
>>> class MyEnum(A, enum.Enum):
...     a = A()
...     
Traceback (most recent call last):
  File "<python-input-3>", line 1, in <module>
    class MyEnum(A, enum.Enum):
        a = A()
TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

Solution:

>>> class ABCEnum(abc.ABCMeta, enum.EnumType):
...     ...
...     
>>> class MyEnum(A, enum.Enum, metaclass=ABCEnum):
...     a = A()
...     
>>> MyEnum.a
<MyEnum.a: <__main__.A object at 0x105210f80>>

@ygale
Copy link
Author

ygale commented Jun 3, 2024

@sobolevn We are not complicating EnumType at all. it's essentially a simple one-line change. And on the contrary, I think forcing users to hack on metaclasses for a simple use case is not reasonable.

@ygale
Copy link
Author

ygale commented Jun 3, 2024

One of the big concerns for enums are their creation times. We'll need performance numbers with and without the ABC addition.

Yep, will do. I'm also considering disabling this for construction via the @_simple_enum decorator. But let's see what the benchmarks say.

I found some discussion (in which you participated) in #93910. Any other pointers?

@ygale
Copy link
Author

ygale commented Jun 3, 2024

Another point that arises from #93910. In my example in the description, and also in my tests, I implemented the color() method in what I thought was the simplest way: match self ... case self.ROSES .... But that uses the Color.RED.RED behavior which now I see is controversial. I'll change them to match on self.name.

@ethanfurman
Copy link
Member

@ygale Using self.ROSES is the preferred way; I was stuck in a purist mode when I made those arguments, but practicality won out in the end (thank goodness!).

@ethanfurman
Copy link
Member

In addition to enum creation time, Python start-up time is a concern.

@ygale
Copy link
Author

ygale commented Jun 4, 2024

@ethanfurman OK. While I'm at it, I might as well profile member access time as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

5 participants