python / cpython

The Python programming language
https://www.python.org/
Other
61.17k stars 29.53k forks source link

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

Open ygale opened 1 month ago

ygale commented 1 month ago

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

nineteendo commented 1 month ago

This sounds like it could break user code.

ygale commented 1 month ago

@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 commented 1 month ago

Ah sorry, I thought you meant to make EnumType a subclass of EnumMeta (whatever that means). Technically EnumType is now an instance (not a subclass) of ABCMeta by making EnumMeta a subclass of ABCMeta. A class is an instance of a meta class.

ygale commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

sobolevn commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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

ygale commented 1 month ago

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