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

Enable Conversions Between Python's Native (stdlib) Enum Types and C++ Enums #5555

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Mar 8, 2025

Description

This PR requires a PYBIND11_INTERNALS_VERSION bump (from 7 to 8).


Alternative to py::enum_, binding C++ enum types to Python's native (stdlib) enum types. For example:

#include <pybind11/pybind11.h>
#include <pybind11/native_enum.h> // Not already included with pybind11.h

enum class altitude : char {
    high = 'h',
    low = 'l'
};

PYBIND11_MODULE(native_enum_demo, m) {
    py::native_enum<altitude>(m, "altitude")
             .value("high", altitude::high)
             .value("low", altitude::low)
             .finalize();
}

The .finalize() call is needed because Python's native enums cannot be built incrementally, but all name/values need to be passed at once. To achieve this, py::native_enum acts as a buffer to collect the name/value pairs. The .finalize() call uses the accumulated name/value pairs to build the arguments for constructing a native Python enum type.

The established py::enum_ is not PEP 435 compatible (see also #2332). py::native_enum provides an alternative without breaking backwards compatibility. Ideally, existing uses of py::enum_ should migrate to py::native_enum, but py::enum_ will remain supported indefinitely.

In rare cases it may be necessary to specialize pybind11::detail::type_caster_enum_type_enabled:

#if defined(PYBIND11_HAS_NATIVE_ENUM) // This guard is only needed for backwards compatibility.
namespace pybind11::detail {
struct type_caster_enum_type_enabled<UnusualEnumType>: std::false_type {};                                                       
}
#endif                                                                          

This is needed only for UnusualEnumTypes that have a custom pybind11::detail::type_caster.


This PR sidesteps issues discovered under #2744.

This PR fixes #3643 (comment), more-or-less as a side-effect.


cloc --diff results for include/ between current master and this PR:

-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C/C++ Header
 same                           39           1814           3332          16327
 modified                        5              0              0              6
 added                           2             50             16            278
 removed                         0              1              0              7

I.e., this PR increases the production code size by 1.7%.


This PR is based on google/pybind11clif#30005 (including accumulated follow-on fixes) and google/pybind11clif#30140 (remove_cross_extension_shared_state branch).

Until August 2024, this code was included in PyCLIF-pybind11 global testing in the Google environment, which provided thousands of diverse use cases that needed to satisfy many million unit tests (the PyCLIF-pybind11 success was about 99.999%).

Suggested changelog entry:

``py::native_enum`` was added, for conversions between Python's native (stdlib) enum types and C++ enums.

@rwgk rwgk changed the title Conversions between Python's native (stdlib) enum types and C++ enums. Enable Conversions Between Python's Native (stdlib) Enum Types and C++ Enums Mar 8, 2025
@rwgk rwgk marked this pull request as ready for review March 8, 2025 23:05
@rwgk rwgk requested a review from henryiii as a code owner March 8, 2025 23:05
TEST_SUBMODULE(native_enum, m) {
using namespace test_native_enum;

m += py::native_enum<smallenum>("smallenum", py::native_enum_kind::IntEnum)
Copy link
Contributor

Choose a reason for hiding this comment

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

This API is pretty different than anything else in pybind11, since most everything else is in the form py::TYPE<CTYPE> varname(scope, "NAME")?

... I understand why, but the += is really weird and is just not doing it for me. I asked ChatGPT what it thought such an API would look like, and it's basically the same thing I was thinking (except I started with commit instead of finalize, but I think finalize is better:

 auto my_enum = py::native_enum<MyEnum>(m, "MyEnum")
        .value("VALUE_A", MyEnum::VALUE_A)
        .value("VALUE_B", MyEnum::VALUE_B)
        .value("VALUE_C", MyEnum::VALUE_C);

    my_enum.finalize(); 

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you please share your ChatGPT prompt and the response? Usually it's giving constructive feedback (e.g. enumerates alternatives), it'd be great to see that.

Copy link
Contributor

Choose a reason for hiding this comment

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

It didn't because of the way I asked it, but sure: https://chatgpt.com/share/67cd0f1f-6df8-800f-b092-b459cea53485

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very useful, thanks!

Here is my continuation:

https://chatgpt.com/share/67cd1767-d63c-8008-acb8-4c50cf739e85

It's explaining three options to preserve the safety net mechanism that I have in place. I'll work through that asap.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ugh, I spent half an hour on a really good conversation with ChatGPT, coming to a great conclusion, but I was accidentally not logged in (wrong profile), I tried to login, that failed, and now the convo is lost 😭.

I'll try to see if I can redo it with the same level of authenticity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It turns out there is no way to get back the original conversation. ChatGPT took a different turn, maybe because my prompt wasn't exactly as before.

But, I think the conclusion is now even better!

Here is the new convo:

And here the conclusion for easy reference:

py::native_enum<MyEnum>(m, "MyEnum")
    .value("VALUE_A", MyEnum::VALUE_A)
    .value("VALUE_B", MyEnum::VALUE_B)
    .value("VALUE_C", MyEnum::VALUE_C)
    .finalize();

I'll take a stab at that. I like it a lot better myself.

@rwgk
Copy link
Collaborator Author

rwgk commented Mar 9, 2025

@virtuald FYI, I made the API change, but still need to change the error messages (they still direct users to use += instead of .finalize()).

Copy link
Contributor

@virtuald virtuald left a comment

Choose a reason for hiding this comment

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

I haven't tested it, but the tests you added seem comprehensive enough so I'm sure it's fine.

If it were possible to make this like the other types instead of having its own separate type map in internals, I'd definitely prefer that... it seems like this could introduce special cases that might accidentally introduce bugs? Looking at the type_info struct it of course has a lot of extra things that this wouldn't need, but I suspect maybe we could jam it in?

If you had looked at that previously and decided it wasn't possible, then I accept that and I'm not asking for additional research.

Edit: .. and even if you haven't done that research, it's probably not necessary. I like the new API change though.

@rwgk
Copy link
Collaborator Author

rwgk commented Mar 9, 2025

Assuming GitHub Actions are passing now, this is now feature complete; the error message is updated.

Things I still want to do:

  • Add tests for .finalize().value(...) and .finalize().finalize() error handling.

  • See if there is a reasonably easy way to make some native_enum_data member functions inaccessible through the public API (I need them to be accessible for testing).

@rwgk
Copy link
Collaborator Author

rwgk commented Mar 9, 2025

Looking at the type_info struct it of course has a lot of extra things that this wouldn't need, but I suspect maybe we could jam it in?

I very much rather not do that. I have a lot of bad experiences trying to jam things together.

We have a golden opportunity right now to change the internals struct, to keep the implementation clean, which is why I'm sinking my time into this PR now.

@rwgk
Copy link
Collaborator Author

rwgk commented Mar 10, 2025

All done with

  • error handling update,
  • public API cleanup,
  • PR description update,
  • docs/classes.rst update.

@virtuald Could you please take another look, especially at the docs/classes.rst update?

@timohl It would be awesome if you could look, too.

For easy access: https://pybind11--5555.org.readthedocs.build/en/5555/classes.html#enumerations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants