Skip to content

bpo-44242: [Enum] remove missing bits test from Flag creation #26586

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

Merged
merged 4 commits into from
Jun 9, 2021
Merged

bpo-44242: [Enum] remove missing bits test from Flag creation #26586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
68c2701
[Enum] remove missing bits test from Flag creation
ethanfurman Jun 7, 2021
d29d623
fix import in docs
ethanfurman Jun 7, 2021
ebaf57d
updates
ethanfurman Jun 9, 2021
b96dcb0
fix doc tests
ethanfurman Jun 9, 2021
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
86 changes: 82 additions & 4 deletions Doc/library/enum.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ An enumeration:

* is a set of symbolic names (members) bound to unique values
* can be iterated over to return its members in definition order
* uses :meth:`call` syntax to return members by value
* uses :meth:`index` syntax to return members by name
* uses *call* syntax to return members by value
* uses *index* syntax to return members by name

Enumerations are created either by using the :keyword:`class` syntax, or by
using function-call syntax::
Expand Down Expand Up @@ -91,6 +91,12 @@ Module Contents
the bitwise operators without losing their :class:`IntFlag` membership.
:class:`IntFlag` members are also subclasses of :class:`int`.

:class:`EnumCheck`

An enumeration with the values ``CONTINUOUS``, ``NAMED_FLAGS``, and
``UNIQUE``, for use with :func:`verify` to ensure various constraints
are met by a given enumeration.

:class:`FlagBoundary`

An enumeration with the values ``STRICT``, ``CONFORM``, ``EJECT``, and
Expand All @@ -117,9 +123,14 @@ Module Contents

Enum class decorator that ensures only one name is bound to any one value.

:func:`verify`

Enum class decorator that checks user-selectable constraints on an
enumeration.


.. versionadded:: 3.6 ``Flag``, ``IntFlag``, ``auto``
.. versionadded:: 3.10 ``StrEnum``
.. versionadded:: 3.10 ``StrEnum``, ``EnumCheck``, ``FlagBoundary``


Data Types
Expand Down Expand Up @@ -514,6 +525,65 @@ Data Types
Using :class:`auto` with :class:`IntFlag` results in integers that are powers
of two, starting with ``1``.

.. class:: EnumCheck

*EnumCheck* contains the options used by the :func:`verify` decorator to ensure
various constraints; failed constraints result in a :exc:`TypeError`.

.. attribute:: UNIQUE
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any guidance on when to use @unique vs @verify(UNIQUE)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only difference between the two is unique raises ValueError and verify raises TypeError. I don't see unique going away, so it's pretty much personal preference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed that so both raise ValueError. I added UNIQUE to verify() for two reasons:

  • so multiple checks would not require multiple decorators
  • so unique() could eventually go away (not that it will any time soon)


Ensure that each value has only one name::

>>> from enum import Enum, verify, UNIQUE
>>> @verify(UNIQUE)
... class Color(Enum):
... RED = 1
... GREEN = 2
... BLUE = 3
... CRIMSON = 1
Traceback (most recent call last):
...
ValueError: aliases found in <enum 'Color'>: CRIMSON -> RED


.. attribute:: CONTINUOUS

Ensure that there are no missing values between the lowest-valued member
and the highest-valued member::

>>> from enum import Enum, verify, CONTINUOUS
>>> @verify(CONTINUOUS)
... class Color(Enum):
... RED = 1
... GREEN = 2
... BLUE = 5
Traceback (most recent call last):
...
ValueError: invalid enum 'Color': missing values 3, 4

.. attribute:: NAMED_FLAGS

Ensure that any flag groups/masks contain only named flags -- useful when
values are specified instead of being generated by :func:`auto`

>>> from enum import Flag, verify, NAMED_FLAGS
>>> @verify(NAMED_FLAGS)
... class Color(Flag):
... RED = 1
... GREEN = 2
... BLUE = 4
... WHITE = 15
... NEON = 31
Traceback (most recent call last):
...
ValueError: invalid Flag 'Color': 'WHITE' is missing a named flag for value 8; 'NEON' is missing named flags for values 8, 16

.. note::

CONTINUOUS and NAMED_FLAGS are designed to work with integer-valued members.

.. versionadded:: 3.10

.. class:: FlagBoundary

*FlagBoundary* controls how out-of-range values are handled in *Flag* and its
Expand Down Expand Up @@ -575,7 +645,7 @@ Data Types
>>> KeepFlag(2**2 + 2**4)
KeepFlag.BLUE|0x10

.. versionadded:: 3.10 ``FlagBoundary``
.. versionadded:: 3.10


Utilites and Decorators
Expand Down Expand Up @@ -632,3 +702,11 @@ Utilites and Decorators
Traceback (most recent call last):
...
ValueError: duplicate values found in <enum 'Mistake'>: FOUR -> THREE

.. decorator:: verify

A :keyword:`class` decorator specifically for enumerations. Members from
:class:`EnumCheck` are used to specify which constraints should be checked
on the decorated enumeration.

.. versionadded:: 3.10
107 changes: 96 additions & 11 deletions Lib/enum.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
__all__ = [
'EnumType', 'EnumMeta',
'Enum', 'IntEnum', 'StrEnum', 'Flag', 'IntFlag',
'auto', 'unique',
'property',
'auto', 'unique', 'property', 'verify',
'FlagBoundary', 'STRICT', 'CONFORM', 'EJECT', 'KEEP',
'global_flag_repr', 'global_enum_repr', 'global_enum',
'EnumCheck', 'CONTINUOUS', 'NAMED_FLAGS', 'UNIQUE',
]


Expand Down Expand Up @@ -89,6 +89,9 @@ def _break_on_call_reduce(self, proto):
setattr(obj, '__module__', '<unknown>')

def _iter_bits_lsb(num):
# num must be an integer
if isinstance(num, Enum):
num = num.value
while num:
b = num & (~num + 1)
yield b
Expand Down Expand Up @@ -538,13 +541,6 @@ def __new__(metacls, cls, bases, classdict, *, boundary=None, _simple=False, **k
else:
# multi-bit flags are considered aliases
multi_bit_total |= flag_value
if enum_class._boundary_ is not KEEP:
missed = list(_iter_bits_lsb(multi_bit_total & ~single_bit_total))
if missed:
raise TypeError(
'invalid Flag %r -- missing values: %s'
% (cls, ', '.join((str(i) for i in missed)))
)
enum_class._flag_mask_ = single_bit_total
#
# set correct __iter__
Expand Down Expand Up @@ -688,7 +684,10 @@ def __members__(cls):
return MappingProxyType(cls._member_map_)

def __repr__(cls):
return "<enum %r>" % cls.__name__
if Flag is not None and issubclass(cls, Flag):
return "<flag %r>" % cls.__name__
else:
return "<enum %r>" % cls.__name__

def __reversed__(cls):
"""
Expand Down Expand Up @@ -1303,7 +1302,8 @@ def __invert__(self):
else:
# calculate flags not in this member
self._inverted_ = self.__class__(self._flag_mask_ ^ self._value_)
self._inverted_._inverted_ = self
if isinstance(self._inverted_, self.__class__):
self._inverted_._inverted_ = self
return self._inverted_


Expand Down Expand Up @@ -1561,6 +1561,91 @@ def convert_class(cls):
return enum_class
return convert_class

@_simple_enum(StrEnum)
class EnumCheck:
"""
various conditions to check an enumeration for
"""
CONTINUOUS = "no skipped integer values"
NAMED_FLAGS = "multi-flag aliases may not contain unnamed flags"
UNIQUE = "one name per value"
CONTINUOUS, NAMED_FLAGS, UNIQUE = EnumCheck


class verify:
"""
Check an enumeration for various constraints. (see EnumCheck)
"""
def __init__(self, *checks):
self.checks = checks
def __call__(self, enumeration):
checks = self.checks
cls_name = enumeration.__name__
if Flag is not None and issubclass(enumeration, Flag):
enum_type = 'flag'
elif issubclass(enumeration, Enum):
enum_type = 'enum'
else:
raise TypeError("the 'verify' decorator only works with Enum and Flag")
for check in checks:
if check is UNIQUE:
# check for duplicate names
duplicates = []
for name, member in enumeration.__members__.items():
if name != member.name:
duplicates.append((name, member.name))
if duplicates:
alias_details = ', '.join(
["%s -> %s" % (alias, name) for (alias, name) in duplicates])
raise ValueError('aliases found in %r: %s' %
(enumeration, alias_details))
elif check is CONTINUOUS:
values = set(e.value for e in enumeration)
if len(values) < 2:
continue
low, high = min(values), max(values)
missing = []
if enum_type == 'flag':
# check for powers of two
for i in range(_high_bit(low)+1, _high_bit(high)):
if 2**i not in values:
missing.append(2**i)
elif enum_type == 'enum':
# check for powers of one
for i in range(low+1, high):
if i not in values:
missing.append(i)
else:
raise Exception('verify: unknown type %r' % enum_type)
if missing:
raise ValueError('invalid %s %r: missing values %s' % (
enum_type, cls_name, ', '.join((str(m) for m in missing)))
Copy link
Contributor

@akulakov akulakov Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Would it make sense to limit missing to missing[:100] or so? It can be arbitrarily large and it's not obvious to users that a large flag value can create a very large exception (and log msg) here. [edit: should have reloaded, didn't realize it's already merged]

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it would.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can make a PR on the same issue, if that's okay?

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 merged it after your comment, as it fixes a regression; we can still make your change if needed.]

Having thought about it a little more, I'm not sure we need that: For any static enum, and probably most dynamic ones, the exception will happen the first time the .py file is loaded and not at some random time on the end-users machine. Which specific scenarios were you thinking about, and is your experience different?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. For some subset of dynamic Enums where let's say a developer tests it with small values and the app logic logs an error and proceeds. In actual use the dynamic value might be calculated or loaded from somewhere and might be much larger.
  2. If a value is calculated or loaded from a web request, it can be used as a DoS attack or to overfill the log files.

Both seem pretty unlikely so I'm not sure it's a needed fix, just wanted to point it out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good points. I changed the error message to list the problem aliases and the combined value of missing flags, then limited that to 256 characters.

Thank you for your help!

)
elif check is NAMED_FLAGS:
# examine each alias and check for unnamed flags
member_names = enumeration._member_names_
member_values = [m.value for m in enumeration]
missing = []
for name, alias in enumeration._member_map_.items():
if name in member_names:
# not an alias
continue
values = list(_iter_bits_lsb(alias.value))
missed = [v for v in values if v not in member_values]
if missed:
plural = ('', 's')[len(missed) > 1]
a = ('a ', '')[len(missed) > 1]
missing.append('%r is missing %snamed flag%s for value%s %s' % (
name, a, plural, plural,
', '.join(str(v) for v in missed)
))
if missing:
raise ValueError(
'invalid Flag %r: %s'
% (cls_name, '; '.join(missing))
)
return enumeration

def _test_simple_enum(checked_enum, simple_enum):
"""
A function that can be used to test an enum created with :func:`_simple_enum`
Expand Down
Loading