Skip to content

refactor(enums)!: performance and functional improvements #556

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

Draft
wants to merge 54 commits into
base: master
Choose a base branch
from

Conversation

Sharp-Eyes
Copy link
Member

@Sharp-Eyes Sharp-Eyes commented Jun 9, 2022

Summary

This PR reworks disnake's custom enums. The existing enums come with a couple issues. First of all, the API does not match that of Python's built-in Enum class. Secondly, existing enums cannot be typed (e.g. class MyEnum(int, Enum)). This makes them both unwieldy and slower than necessary.

This PR makes the following changes:

  1. Enums now adhere to the vanilla Enum API (need to double-check some field names),
  2. Enums now must be typed,
  3. TODO: all explicit typecasts on enums and all explicit member.value access have been removed,
    This will be deferred to a later PR.
  4. implement unit tests for the new enums.

Explicitly typed enums come with the advantage that their members can be directly used in place for anything that accepts its type. For example, an int-enum member could be directly added to an integer:

>>> class Integer(int, Enum):
...     ONE = 1
...     TWO = 2

>>> # old enums:
>>> Integer.ONE.value + Integer.TWO.value + 3
6

>>> # new enums:
>>> Integer.ONE + Integer.TWO + 3
6

This comes with the advantage of not having to explicitly access value, which is both easier on the developer and faster. This also means that isinstance checks with an enum member and its type will pass, etc. Over the whole library, this means a lot of now unnecessary member access and function calls can be removed. Since enums are used extensively under the hood, this should lead to a small speed increase. A screenshot of tentative benchmarks of the projected performance increase can be seen here:
image

Lastly, because the API now more closely matches that of the default Enum implementation, these enums should be usable as a drop-in replacement for enums in a wide range of use cases, especially where the speed increase is necessary.

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running task lint
    • I have type-checked the code by running task pyright
  • This PR fixes an issue
  • This PR adds something new (e.g. new method or parameters)
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@onerandomusername
Copy link
Member

One thing that needs to be made sure, is that commands.option_enum() isn't broken, and that stdlib enums are supported for options but also allow internal enums. This may already be done, but I'm writing it down anyways.

@shiftinv shiftinv added t: enhancement New feature breaking change Includes breaking changes to code/packaging s: in progress Issue/PR is being worked on t: refactor/typing/lint Refactors, typing changes and/or linting changes labels Jun 10, 2022
@onerandomusername onerandomusername marked this pull request as ready for review July 28, 2022 23:33
@onerandomusername onerandomusername requested review from shiftinv and onerandomusername and removed request for shiftinv August 3, 2022 15:49
@onerandomusername
Copy link
Member

I think this can be reviewed, and as you said, we can change accessors of enums to not have the value attribute in a later pull request.

@onerandomusername
Copy link
Member

@Chromosomologist would you please resolve conflicts on this pr and get it ready for a review?

@Sharp-Eyes
Copy link
Member Author

Gotcha, we should be all good now.

Copy link
Contributor

@EmmmaTech EmmmaTech left a comment

Choose a reason for hiding this comment

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

Two things I noticed looking at the code

@onerandomusername
Copy link
Member

@Chromosomologist would you please resolve conflicts on this pr?

@onerandomusername
Copy link
Member

@Chromosomologist what is the status on this PR?

@Sharp-Eyes
Copy link
Member Author

Sharp-Eyes commented Sep 29, 2022

All changes to the underlying enum logic are done, assuming we don't run into any bugs over the testing period. As far as this PR goes, everything should be final.

As for the rest, enum subclasses' docs will need to be reintroduced as part of a separate PR, and we can then start looking into performance optimizations across the lib, too.

Copy link
Member

@onerandomusername onerandomusername left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@EQUENOS EQUENOS left a comment

Choose a reason for hiding this comment

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

Checked this PR before, forgot to actually approve it

Copy link
Member

@shiftinv shiftinv left a comment

Choose a reason for hiding this comment

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

Sorry for the huge review, I just spent some time trying to break things and comparing to the old and stdlib enums 😅
(note that i mostly looked at stdlib enums, so if some of these suggestions break old enum behavior we shouldn't use them)

Additionally:

  • Copying and pickling enum instances breaks identity checks, which work in stdlib enums.
    Pretty sure implementing __reduce_ex__ fixes both at the same time.
  • I don't quite remember, what was the verdict on having Enum.__bool__ always return True? That would certainly deviate from stdlib enums, but that's how it currently works in the old enums.
  • EnumMeta.__reversed__ is missing, python falls back to calling __getitem__ with int indices, which naturally doesn't work here.
  • disnake.MessageType.default in disnake.NotificationLevel is true (which technically makes sense but should be false)
  • Could we run some of the tests (particularly those checking functionality that aims to emulate stdlib enums, like __contains__) with both our and the stdlib enums?
    • a parametrized fixture would work nicely, I suppose
      @pytest.fixture(params=[enums.Enum, StdlibEnum])
      def enum_type(request):
          return request.param
      
      def test_thing(enum_type: Type[enums.Enum]):
          ...
  • stdlib enums validate __setattr__ and __delattr__ calls on the metaclass, maybe EnumMeta could do the same? Feels unnecessary but could be useful as a sanity check just in case.

# Set internal flag to True such that all following enums get proper member handling.
if not EnumMeta.__is_enum_instantiated__:
EnumMeta.__is_enum_instantiated__ = True
ns["__enum_type__"] = enum = super().__new__(metacls, name, bases, ns)
Copy link
Member

@shiftinv shiftinv Oct 3, 2022

Choose a reason for hiding this comment

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

Is __enum_type__ used for anything?
Either way the ns["__enum_type__"] assignment here won't do anything, I believe (which also means Enum.__enum_type__ is missing).

value_cls._actual_enum_cls_ = actual_cls # type: ignore
return actual_cls
# Create new enum class...
cls = super().__new__(metacls, name, bases, {**Enum.__dict__, **ns})
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to have a comment on why Enum.__dict__ is provided here again :p

def __prepare__(
metacls, # pyright: ignore[reportSelfClsParameterName]
name: str,
bases: Tuple[Type[Any], ...] = (),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bases: Tuple[Type[Any], ...] = (),
bases: Tuple[Type[Any], ...],

def __new__(
metacls: Type[_T], # pyright: ignore[reportSelfClsParameterName]
name: str,
bases: Tuple[Type[Any], Type[Enum]],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bases: Tuple[Type[Any], Type[Enum]],
bases: Tuple[Type[Any], ...],

This won't always contain exactly two elements

# fmt: off
guild_update = 1
Copy link
Member

Choose a reason for hiding this comment

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

accidental revert?

@@ -241,40 +334,28 @@ class PartyType(Enum):
ocho = 832025144389533716


class SpeakingState(Enum):
class SpeakingState(int, Enum): # TODO: Docs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class SpeakingState(int, Enum): # TODO: Docs
class SpeakingState(int, Enum):

I guess that's not needed anymore


def enum_if_int(cls: Type[T], val: Any) -> T:
except TypeError:
return val
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it could break other things... can this perhaps return a separate UnknownThing type instead, that at least has name and value attributes and perhaps even proxies the value's (and enum class's?) attributes?

Could make it pass isinstance(x, disnake.enums.SomeEnum) checks to some degree by implementing __instancecheck__ on EnumMeta, though isinstance(x, int) still wouldn't work.

tl;dr: would be great if this supported x.name, x.value, and things like x == y even if unknown


def enum_if_int(cls: Type[T], val: Any) -> T:
except TypeError:
Copy link
Member

Choose a reason for hiding this comment

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

The __new__ call can raise more than TypeError, e.g. int.__new__ raises ValueError if it receives an invalid string.

On that note, should this check isinstance(val, cls.__base_type__) before calling cls.__new__(cls, val), such that try_enum(MessageType, "10") fails? (which currently works correctly as it converts "10" to 10)
Not sure about either one, tbqh

if not isinstance(val, int):
return val
return try_enum(cls, val)
return try_enum(cls, val) if isinstance(val, int) else val
Copy link
Member

Choose a reason for hiding this comment

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

for int-valued enums, this will always call try_enum even if val is already an enum instance

new = enums.try_enum(Enum, unknown)

assert new not in Enum # type: ignore
assert new == unknown
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also check isinstance(new, Enum) here? Will only work with the first two test cases though.
(also see my other comment about try_enum)

@shiftinv shiftinv removed this from the disnake v2.8 milestone Feb 5, 2023
@onerandomusername
Copy link
Member

@Chromosomologist will you be working on this PR?

@shiftinv shiftinv marked this pull request as draft May 25, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.0 breaking change Includes breaking changes to code/packaging s: in progress Issue/PR is being worked on t: enhancement New feature t: refactor/typing/lint Refactors, typing changes and/or linting changes
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

5 participants