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

x in enum.Flag member is True when x is not a Flag #77398

Closed
Dutcho mannequin opened this issue Apr 3, 2018 · 14 comments
Closed

x in enum.Flag member is True when x is not a Flag #77398

Dutcho mannequin opened this issue Apr 3, 2018 · 14 comments
Assignees
Labels
3.7 3.8 easy stdlib type-bug

Comments

@Dutcho
Copy link
Mannequin

@Dutcho Dutcho mannequin commented Apr 3, 2018

BPO 33217
Nosy @warsaw, @ethanfurman, @Dutcho, @RJ722, @remilapeyre
PRs
  • #6392
  • #6651
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ethanfurman'
    closed_at = <Date 2018-09-10.18:25:18.889>
    created_at = <Date 2018-04-03.20:16:12.943>
    labels = ['3.7', '3.8', 'type-bug', 'library', 'easy']
    title = 'x in enum.Flag member is True when x is not a Flag'
    updated_at = <Date 2019-05-08.15:30:53.240>
    user = 'https://github.com/Dutcho'

    bugs.python.org fields:

    activity = <Date 2019-05-08.15:30:53.240>
    actor = 'remi.lapeyre'
    assignee = 'ethan.furman'
    closed = True
    closed_date = <Date 2018-09-10.18:25:18.889>
    closer = 'ethan.furman'
    components = ['Library (Lib)']
    creation = <Date 2018-04-03.20:16:12.943>
    creator = 'Dutcho'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33217
    keywords = ['patch', 'easy']
    message_count = 14.0
    messages = ['314890', '314895', '314900', '314901', '314911', '314958', '315211', '315234', '315863', '315871', '324945', '324947', '341883', '341889']
    nosy_count = 7.0
    nosy_names = ['barry', 'eli.bendersky', 'ethan.furman', 'Dutcho', 'RJ722', 'remi.lapeyre', 'Peter T\xc3\xb6nz']
    pr_nums = ['6392', '6651']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue33217'
    versions = ['Python 3.7', 'Python 3.8']

    @Dutcho
    Copy link
    Mannequin Author

    @Dutcho Dutcho mannequin commented Apr 3, 2018

    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'

    @Dutcho Dutcho mannequin added stdlib type-bug labels Apr 3, 2018
    @ethanfurman ethanfurman self-assigned this Apr 3, 2018
    @ethanfurman
    Copy link
    Member

    @ethanfurman ethanfurman commented Apr 3, 2018

    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.

    @Dutcho
    Copy link
    Mannequin Author

    @Dutcho Dutcho mannequin commented Apr 3, 2018

    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.

    @Dutcho
    Copy link
    Mannequin Author

    @Dutcho Dutcho mannequin commented Apr 3, 2018

    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'

    @ethanfurman
    Copy link
    Member

    @ethanfurman ethanfurman commented Apr 4, 2018

    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.

    @ethanfurman
    Copy link
    Member

    @ethanfurman ethanfurman commented Apr 4, 2018

    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.

    @ethanfurman ethanfurman changed the 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 Apr 4, 2018
    @ethanfurman
    Copy link
    Member

    @ethanfurman ethanfurman commented Apr 12, 2018

    New changeset 3715176 by Ethan Furman in branch '3.7':
    [3.7] bpo-33217: deprecate non-Enum lookups in Enums (GH-6392)
    3715176

    @ethanfurman
    Copy link
    Member

    @ethanfurman ethanfurman commented Apr 12, 2018

    DeprecationWarning is in 3.7, now need to raise TypeError in 3.8.

    @RJ722
    Copy link
    Mannequin

    @RJ722 RJ722 mannequin commented Apr 28, 2018

    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?

    @ethanfurman
    Copy link
    Member

    @ethanfurman ethanfurman commented Apr 28, 2018

    Rahul Jha, sure, go ahead!

    @ethanfurman
    Copy link
    Member

    @ethanfurman ethanfurman commented Sep 10, 2018

    New changeset 9430652 by Ethan Furman (Rahul Jha) in branch 'master':
    bpo-33217: Raise TypeError for non-Enum lookups in Enums (GH-6651)
    9430652

    @ethanfurman
    Copy link
    Member

    @ethanfurman ethanfurman commented Sep 10, 2018

    Thank you, Rahul!

    @PeterTnz
    Copy link
    Mannequin

    @PeterTnz PeterTnz mannequin commented May 8, 2019

    I use python 3.8.0a3 to make our testframework ready for the future.

    Is it volitional that the Expression "if int in IntEnum:" raise also a TypeError?

    @remilapeyre
    Copy link
    Mannequin

    @remilapeyre remilapeyre mannequin commented May 8, 2019

    A test for this has been added for IntFlag so I think it must have been on purpose : 9430652#diff-d57e55a3bb4873aec10786e531a88947R2386

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 3.8 easy stdlib type-bug
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant