Check enum members are compared by identity

[scop]

Current problem

Enum members should be compared by identity rather than equality: https://docs.python.org/3/library/enum.html#comparisons

Enumeration members are compared by identity:

Desired solution

Extending singleton-comparison to cover this would seem to be one possible implementation.

Additional context

No response

[Pierre-Sassoulas]

Thank you, I did not do that myself, but the doc is very clear about it and it definitely make sense to avoid issue with the "real value" being compared to something semantically different.

[Pierre-Sassoulas]

Some additional consideration. Considering the following code:

            if action in {
                VariableVisitConsumerAction.RETURN,
                VariableVisitConsumerAction.CONSUME,
            }:
                return

Should pylint suggest this ?

            if action is VariableVisitConsumerAction.RETURN or action is VariableVisitConsumerAction.CONSUME:
                return

[scop]

For completeness, yes I think it would be good. Or

            if any(action is x for x in (VariableVisitConsumerAction.RETURN, VariableVisitConsumerAction.CONSUME)):
                return

...which subjectively scales better stylewise if there are many things to compare against.

But if it makes things harder, basic comparisons would be a good first step, no need to delay this to get everything covered IMHO.

[cdce8p]

I would recommend this:

if action in {
    VariableVisitConsumerAction.RETURN,
    VariableVisitConsumerAction.CONSUME,
}:
    return

It's a single set contains operation that should be faster and easier to read, especially compared to this:

if any(action is x for x in (VariableVisitConsumerAction.RETURN, VariableVisitConsumerAction.CONSUME)):
    return

A function call (any), generator expression, and individual is comparisons.

--

from enum import Enum
from timeit import timeit

class VariableVisitConsumerAction(Enum):
    RETURN = 0
    CONSUME = 1
    NONE = 2

action = VariableVisitConsumerAction.RETURN

ftuple = (
    VariableVisitConsumerAction.RETURN,
    VariableVisitConsumerAction.CONSUME,
    VariableVisitConsumerAction.NONE,
)
fset = frozenset(ftuple)


def func1():
    if action in fset:
        return True
    return False

def func2():
    return any(action is x for x in ftuple)

print(timeit(func1, number=1_000_000))
print(timeit(func2, number=1_000_000))

0.3418969129998004
0.7927539209922543

The result isn't truly representative as it depends on action (e.g. for NONE the difference would be bigger) and if the set / tuple are written inline. It's enough for a general idea though.

[scop]

That proposal surely is much easier on the eye, and I'm not surprised at all that it's more performant. However it arguably is not correct, == vs is is the point of this issue. For example:

>>> from enum import IntEnum
>>> class Enum1(IntEnum):
...  FOO = 1
... 
>>> class Enum2(IntEnum):
...  BAR = 1
... 
>>> Enum1.FOO in (Enum2.BAR,)
True
>>> any(Enum1.FOO is x for x in (Enum2.BAR,))
False

Maybe someone can come up with a cleaner expression that would use is semantics for these collection cases?

[cdce8p]

That proposal surely is much easier on the eye, and I'm not surprised at all that it's more performant. However it arguably is not correct, == vs is is the point of this issue.

It will only not work for IntEnum, but that is by design.

Comparisons against non-enumeration values will always compare not equal
(again, IntEnum was explicitly designed to behave differently, [...])

https://docs.python.org/3/library/enum.html#comparisons

[Pierre-Sassoulas]

OK, I read the documentation too fast the first time (only the part Enumeration members are compared by identity and not the following text). And did not test properly. There are no issues when using == vs is.

>>> class Color(Enum):
...     RED = 1
...     BLUE = 2
... 
>>> class Colored(Enum):
...     RED = 1
...     BLUE = 2
... 
>>> Colored.RED in [Color.RED]
False
>>> Colored.RED == Color.RED
False

It seems that the only issue that can happen is with IntEnum which were designed to be able to do that and probably are used for this reason when they are used.

I think adding a check to advise using is for Enum when both use work perfectly would be highly opinionated and not desirable for a default check. Warning for IntEnum on the other hand would be very seldom useful, because who is using an obscure subclass of Enum instead of main Enum class by mistake ? It could be an extension though.