classification
Title: x in enum.Flag member is True when x is not a Flag
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ethan.furman Nosy List: Dutcho, RJ722, barry, eli.bendersky, ethan.furman
Priority: normal Keywords: easy, patch

Created on 2018-04-03 20:16 by Dutcho, last changed 2018-09-10 18:25 by ethan.furman. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 6392 merged ethan.furman, 2018-04-05 20:46
PR 6651 merged RJ722, 2018-04-30 08:23
Messages (12)
msg314890 - (view) Author: (Dutcho) Date: 2018-04-03 20:16
While `Flag() in Flag()` and `Flag() | Flag()` result in the expected outcomes, e.g. `str() in Flag()` unexpectedly returns `True`, whereas `Flag() | str()` as expected raises a TypeError.
    >>> import enum
    >>> ABC = enum.Flag('ABC', 'a, b, c')
    >>> ac = ABC.a | ABC.c
    >>> def test():
    ...     for x in (*ABC, 'test'):
    ...         print(x, end=' ')
    ...         try:
    ...             print(x in ac, end=' ')
    ...         except TypeError as e:
    ...             print(e, end=' ')
    ...         try:
    ...             print(x | ac)
    ...         except TypeError as e:
    ...             print(e)
    >>> test()
    ABC.a True ABC.c|a
    ABC.b False ABC.c|b|a
    ABC.c True ABC.c|a
    test True unsupported operand type(s) for |: 'str' and 'ABC'

Likely cause is modelling of Flag.__contains__ after Flag.__and__ etc., which is incorrect as __contains__ doesn't have a reflected version like __and__ etc. have. The returned `NotImplemented` is therefore not handled by the interpreter, although it is converted to bool to satisfy __contains__ return type.

This can be fixed by redefinition of Flag.__contains__ to raise TypeError:
    >>> def __contains__(self, other):
    ...     if not isinstance(other, self.__class__):
    ...         raise TypeError(f"unsupported operand type(s) for 'in': "
    ...                         f"{type(other).__qualname__!r} and {type(self).__qualname__!r}")
    ...     return other & self == other
    >>> enum.Flag.__contains__ = __contains__
    >>> test()
    ABC.a True ABC.c|a
    ABC.b False ABC.c|b|a
    ABC.c True ABC.c|a
    test unsupported operand type(s) for 'in': 'str' and 'ABC' unsupported operand type(s) for |: 'str' and 'ABC'
msg314895 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2018-04-03 21:05
Thanks for finding this!

However, TypeError, an exception, is not the correct type of answer -- the correct type of answer for a yes/no question is either True or False.

So the code should be changed from returning NotImplemented to returning False.
msg314900 - (view) Author: (Dutcho) Date: 2018-04-03 21:36
Fully agree that Flag.__contains__ must RETURN False or True; that's why I suggested it RAISES TypeError
The exception was to be consistent with e.g. Flag.__and__, which by returning NotImplemented transfers to type(other).__rand__, and assuming __rand__ cannot handle Flag and also returns NotImplemented, falls back to the interpreter raising TypeError. Also e.g. 1 in 'hello' raises TypeError.
For Flag, I wouldn't strongly oppose to False for other being a different type. However, classes derived from Flag could face issues, e.g. `class MyIntFlag (Flag, int): pass`.
msg314901 - (view) Author: (Dutcho) Date: 2018-04-03 21:39
Ah, the mixin logic mentioned in 33219 solves most of my objections to returning False; agree that would make sense
Only remaining inconsistency would be with 1 in 'hello'
msg314911 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2018-04-04 00:29
Strings are actually the odd-man out -- dicts, sets, lists, tuples, etc., all return False instead of raising TypeError.

The reason str raises an error is because `in`, for str, is a substring check, not a membership check.
msg314958 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2018-04-04 23:08
Stepping back slightly, it is more general to say that str, and in certain other cases dict and set (and possibly others) will raise instead of return False when it is impossible for the target type to ever hold the checked-for type.  A couple examples of what will raise:

    1 in 'hello'       # integers will never be in a string
    list() in dict()   # dict keys must be hashable (and lists are not)

So, yes, at least for pure Enums and Flags, raising TypeError when a non-Enum/Flag is checked for would be appropriate.

Since there may be code currently relying on always getting True/False, though, a deprecation period is called for.  I'll see if I can get that into 3.7.
msg315211 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2018-04-12 01:56
New changeset 3715176557cf87925c8f89b98939c7daf9bf48e2 by Ethan Furman in branch '3.7':
[3.7] bpo-33217: deprecate non-Enum lookups in Enums (GH-6392)
https://github.com/python/cpython/commit/3715176557cf87925c8f89b98939c7daf9bf48e2
msg315234 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2018-04-12 21:36
DeprecationWarning is in 3.7, now need to raise TypeError in 3.8.
msg315863 - (view) Author: Rahul Jha (RJ722) * Date: 2018-04-28 10:05
Hi Ethan,

The only thing which is left is to change the Deprecation Warning to raise a `TypeError` and alter the tests accordingly.

Seeing that most of the work for the issue has already been done, can I take it forward from here on wards, please?
msg315871 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2018-04-28 23:46
Rahul Jha, sure, go ahead!
msg324945 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2018-09-10 18:21
New changeset 9430652535f88125d8003f342a8884d34885d876 by Ethan Furman (Rahul Jha) in branch 'master':
bpo-33217: Raise TypeError for non-Enum lookups in Enums (GH-6651)
https://github.com/python/cpython/commit/9430652535f88125d8003f342a8884d34885d876
msg324947 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2018-09-10 18:25
Thank you, Rahul!
History
Date User Action Args
2018-09-10 18:25:18ethan.furmansetstatus: open -> closed
resolution: fixed
messages: + msg324947

stage: patch review -> resolved
2018-09-10 18:21:09ethan.furmansetmessages: + msg324945
2018-04-30 08:23:56RJ722setstage: needs patch -> patch review
pull_requests: + pull_request6347
2018-04-28 23:46:36ethan.furmansetmessages: + msg315871
2018-04-28 10:05:14RJ722setnosy: + RJ722
messages: + msg315863
2018-04-12 21:36:00ethan.furmansetstage: patch review -> needs patch
messages: + msg315234
versions: - Python 3.6
2018-04-12 21:34:06ethan.furmanlinkissue33219 superseder
2018-04-12 01:56:29ethan.furmansetmessages: + msg315211
2018-04-05 20:46:31ethan.furmansetkeywords: + patch
stage: patch review
pull_requests: + pull_request6100
2018-04-04 23:08:03ethan.furmansetmessages: + msg314958
title: x in enum.Flag() is True when x is no Flag -> x in enum.Flag member is True when x is not a Flag
2018-04-04 00:29:55ethan.furmansetmessages: + msg314911
2018-04-03 21:39:30Dutchosetmessages: + msg314901
2018-04-03 21:36:22Dutchosetmessages: + msg314900
2018-04-03 21:05:17ethan.furmansetkeywords: + easy

messages: + msg314895
2018-04-03 20:47:09ethan.furmansetassignee: ethan.furman

nosy: + barry, eli.bendersky, ethan.furman
versions: + Python 3.7, Python 3.8
2018-04-03 20:16:12Dutchocreate