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.

Sign up for GitHub

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

Jump to bottom

bpo-32218: make Flag and IntFlag members iterable #22221

Merged
merged 1 commit into from
Sep 16, 2020
Merged

bpo-32218: make Flag and IntFlag members iterable #22221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions Doc/library/enum.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,13 @@ be combined with them::
>>> Perm.X | 8
<Perm.8|X: 9>

:class:`IntFlag` members can also be iterated over::

>>> list(RW)
[<Perm.R: 4>, <Perm.W: 2>]

.. versionadded:: 3.10


Flag
^^^^
Expand Down Expand Up @@ -703,6 +710,14 @@ value::
>>> bool(Color.BLACK)
False

:class:`Flag` members can also be iterated over::

>>> purple = Color.RED | Color.BLUE
>>> list(purple)
[<Color.BLUE: 2>, <Color.RED: 1>]
Comment on lines +713 to +717
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The meaning of "members" should be clarified. Specifically, the iteration intentionally omits aliases and compound members.

The tests should verify this.

I would describe it as "iterating over the bits of a flag value", because that's what it does. Using "bits" should be OK since Flag is documented as supporting bitwise operators.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.


.. versionadded:: 3.10

.. note::

For the majority of new code, :class:`Enum` and :class:`Flag` are strongly
Expand Down
4 changes: 4 additions & 0 deletions Lib/enum.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,10 @@ def __contains__(self, other):
type(other).__qualname__, self.__class__.__qualname__))
return other._value_ & self._value_ == other._value_

def __iter__(self):
members, extra_flags = _decompose(self.__class__, self.value)
return (m for m in members if m._value_ != 0)

Comment on lines +731 to +734
Copy link
Contributor

@belm0 belm0 Oct 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _decompose function is heavyweight (fairly complex and includes sorting). I wouldn't expect this kind of overhead when iterating flag bits.

I can think of two efficient ways:

  1. compare to all members of the Flag class, excluding 0 and non-power-of-2:
return (v for v in self.__class__ if v.value != 0 and (v.value & (v.value-1) == 0) and v in self)
  1. walk through the bits with a helper
@staticmethod
def _bits(n):
    while n:
        b = n & (~n+1)
        yield b
        n ^= b

def __iter__(self):
    return (self.__class__(v) for v in self._bits(self._value_))

in fact _decompose is used in other places, like __invert__, making these operations really slow. Really the set of available bits should be available in a class-only attribute, like _all_.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proposal (2) is 2x faster than the implementation in this PR, for an enum with only about 6 values...

>>> from enum import Flag, auto
>>> class Foo(Flag):
...     NONE = 0
...     A = auto()
...     B = auto()
...     C = auto()
...     C2 = C
...     AB = A | B
...     D = auto()
...     E = auto()
...     F = auto()
...
>>> x = Foo.A | Foo.B | Foo.D | Foo.F
>>> import timeit
>>> timeit.timeit('list(x)', globals=locals(), number=10000)
0.6332780510000013

proposal (2):

>>> timeit.timeit('list(x)', globals=locals(), number=10000)
0.36078684099999236

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have become increasingly unhappy with the the multiple calls to _decompose. Your ideas (and others, too) are welcome.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have become increasingly unhappy with the the multiple calls to _decompose. Your ideas (and others, too) are welcome.

I do have ideas on it, and intended to open a separate issue for refactoring _decompose() and make a PR replacing it with something simpler, faster, and less buggy. (E.g. use the _bits() iterator; and I noticed that all callers of it either use the members or extra_flags output, never both; and the computation of extra_flags would be trivial if we had an __all__ attribute on the class.)

Regarding bugs, when it's used by repr() or str(), I don't think it's intended that a lone compound value is represented as-is, while a compound plus other value is represented as a redundant "compound plus underlying bits":

>>> class Foo(Flag):
...     A = auto()
...     B = auto()
...     C = auto()
...     BC = B | C
>>> repr(Foo.BC)
'<Foo.BC: 6>'
>>> repr(Foo.A | Foo.BC)
'<Foo.BC|C|B|A: 7>'

Comment on lines +731 to +734
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the use of __iter__() is problematic given that __contains__() has different semantics.

__contains__() returns true for members with value 0 or compound members, and __iter__() does not. It's bad form for these not to match.

>>> class Foo(Flag):
...     NONE = 0
...     A = 1
...     B = 2
...     C = 4
...     C2 = C
...     AB = A | B
>>> x = Foo.A | Foo.B
>>> list(x)
[<Foo.B: 2>, <Foo.A: 1>]
>>> Foo.NONE in x
True
>>> Foo.AB in x
True

I'd argue that __contains__() is completely broken and no one wants that behavior. But assuming that can't be changed, using __iter__() to iterate the bits is probably not a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the zero-value flag should not be contained in a non-zero flag, but I have no problem with a compound flag showing up if its bits are present. You are welcome to try and convince me otherwise, but it will take more than "no one wants that behavior".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I just learned that Flag.__contains__() is technically a subset operator:

>>> class Foo(Flag):
...     NONE = 0
...     A = 1
...     B = 2
...     C = 4
...     AB = A | B
>>> Foo.AB in (Foo.AB | Foo.C)
True
>>> (Foo.A|Foo.C) in (Foo.AB | Foo.C)
True

If that's the case, then actually it's OK for Foo.None in Foo.A to be True, since Foo.None is the empty set:

>>> set().issubset({1, 2})
True

I would not have made the API this way. __contains__ should be strictly for testing whether a bit is in the value (a Flag value is a set of enabled bits, in my mind), while issubset() and operator <= should be used for subset.

def __repr__(self):
cls = self.__class__
if self._name_ is not None:
Expand Down
12 changes: 12 additions & 0 deletions Lib/test/test_enum.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2246,6 +2246,12 @@ def test_member_contains(self):
self.assertFalse(W in RX)
self.assertFalse(X in RW)

def test_member_iter(self):
Color = self.Color
self.assertEqual(list(Color.PURPLE), [Color.BLUE, Color.RED])
self.assertEqual(list(Color.BLUE), [Color.BLUE])
self.assertEqual(list(Color.GREEN), [Color.GREEN])

def test_auto_number(self):
class Color(Flag):
red = auto()
Expand Down Expand Up @@ -2701,6 +2707,12 @@ def test_member_contains(self):
with self.assertRaises(TypeError):
self.assertFalse('test' in RW)

def test_member_iter(self):
Color = self.Color
self.assertEqual(list(Color.PURPLE), [Color.BLUE, Color.RED])
self.assertEqual(list(Color.BLUE), [Color.BLUE])
self.assertEqual(list(Color.GREEN), [Color.GREEN])

def test_bool(self):
Perm = self.Perm
for f in Perm:
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2020-09-12-16-18-42.bpo-32218.IpYkEe.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
`enum.Flag` and `enum.IntFlag` members are now iterable