pylint
pylint copied to clipboard
pylint-dev
Check enum members are compared by identity
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
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.
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
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.
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.
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?
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,
==vsisis 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
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.
To me the IntEnum thing is, as the quoted snippet reads, "Comparisons against non-enumeration values ...". My example and this issue is not really comparing against non-enumeration values, it's about comparing enum member values (possibly even between different enums which likely is an error) against each other.
It makes perfect sense to compare IntEnum members to ints with == when one wants that kind of a numeric comparison, but when one wants a typesafe comparison of enum values against each other (which is to me the common case), is would be the right thing to do. I don't think this is opinionated, but then again, who knows, I might be opinionated on that one.
Besides IntEnum, there will be StrEnum in 3.11 which IIRC exhibits the same behavior. And there can be user defined enums that have that.
But yeah, I do think the check should apply only when one compares enum values to other enum values. And it does get perhaps somewhat convoluted with collections, at least untyped ones.
As far as opinionatedness goes, we do have singleton-comparison enabled by default already, and IMO the checks to use is when comparing explicitly against None or the booleans is in the same boat as doing it for enum values. Gut feeling says it's actually even more important for enum members, as with them it's more likely to cause real bugs.
So if I understand you well it's comparing enum by value that is a problem, not using is or ==. Do you want something like that ?
from enum import Enum
class Color(Enum):
RED = 1
BLUE = 2
class Colored(Enum):
RED = 1
BLUE = 2
print(Colored.RED in [Color.RED]) # False
print(Colored.RED == Color.RED) # <= No unexpected True with == even if they both == to 1
print(Colored.RED.value == Color.RED.value) # [enum-comparison-by-value]
print(Colored.RED.value in [Color.RED.value]) # [enum-comparison-by-value]
Imho if you're comparing enum by value explicitly (like if you're using IntEnum) chance are it's what you want to do.
After the discussion so far, I don't see a need for such a checker.
- In case someone uses
IntEnumorStrEnumthey most likely what it to compare to ints and strings. - Same if someone is comparing
Color.RED.value. - As for
Colored.RED == Color.RED, that will already be false since enums are singletons.
In case someone uses
IntEnumorStrEnumthey most likely what it to compare to ints and strings.
They can do so by actually comparing to ints and strings, and in that case == is the right thing to do. But it's different from comparing to other enum members, in that case is is the right thing to do. As said earlier, I now think this check should be limited to cases where an enum member is compared against an enum member. So it should not be done when one is compared against something else (such as a string or an int).
Same if someone is comparing
Color.RED.value.
This is out of scope here, .value is not an enum member but its value, which is often a str or an int.
As for
Colored.RED == Color.RED, that will already be false since enums are singletons.
As noted in earlier comments here, this does not hold universally.
As for Colored.RED == Color.RED, that will already be false since enums are singletons.
As noted in earlier comments here, this does not hold universally.
There actually wasn't an explicit example of it, so here goes. Example using an IntEnum:
>>> from enum import IntEnum
>>> class Enum1(IntEnum):
... FOO = 1
...
>>> class Enum2(IntEnum):
... BAR = 1
...
>>> Enum1.FOO == Enum2.BAR
True
>>> Enum1.FOO is Enum2.BAR
False
And custom enums may be similarly affected:
>>> from enum import Enum
>>> class Enum3(str, Enum):
... QUUX = "x"
...
>>> class Enum4(str, Enum):
... BAZ = "x"
...
>>> Enum3.QUUX == Enum4.BAZ
True
>>> Enum3.QUUX is Enum4.BAZ
False
As for Colored.RED == Color.RED, that will already be false since enums are singletons.
As noted in earlier comments here, this does not hold universally.
There actually wasn't an explicit example of it, so here goes. Example using an
IntEnum:... And custom enums may be similarly affected: ...
As mentioned, that is by design. If you don't want that, just use a normal enum.
Ok, let's stop the "as said" circle, it doesn't seem to be going anywhere.
One final try/question: do you think the advice at https://docs.python.org/3/library/enum.html#comparisons is misguided, or disagree with it otherwise? They are very explicit about it (emphasis mine), it was also partially cited in the initial comment in this issue, just in case it was missed:
Enumeration members are compared by identity:
>>> Color.RED is Color.RED True >>> Color.RED is Color.BLUE False >>> Color.RED is not Color.BLUE True
If yes, can I ask to submit a PR to fix it or file a bug about it?
Enumeration members are compared by identity:
The doc also says just after that : Equality comparisons are defined though. There is a lot of working code that use equality comparison for enum and the fail case we identified look like the user would know exactly what they are doing is they inherit from IntEnum or str, Enum or compare by value.
Addressing the main point here:
As far as opinionatedness goes, we do have singleton-comparison enabled by default already
The issue with comparing to True, False or None with equality means that a custom __eq__ will make the result uncertain. But the problem happens with every classes that you can compare it to (depending on the instance you get, you'll have problem or not) in particular it depends on code you do not control. When you're comparing enum with equality you control the enum class and there won't be a special case for your enum class in the instance you compare it too unless it's a class you control and you did this particular equality operator yourself. To be very specific numpy will not be changing their equality operator specifically for your enum class. But they did for True, False and None. So there's known case where singleton-comparison will create problems, and a lot less uncertainty with enums.
I'm not opposed to an optional extension, we already have an extension to prevent the use of while loop and enforce ternary operators so we're not opposed to having opinionated extensions.
When you're comparing enum with equality you control the enum class
The enum classes whose members one compares (no matter how) can be defined in 3rd party code, outside one's control.