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

Introduce py::native_enum_kind (mandatory argument without a default). #30118

Merged
merged 9 commits into from
Apr 19, 2024

Conversation

rwgk
Copy link
Contributor

@rwgk rwgk commented Apr 19, 2024

Description

The core change here is to introduce

    enum class native_enum_kind { Enum, IntEnum };

and to change the native_enum constructor to require passing one of those two values, e.g.:

    py::native_enum<smallenum>("smallenum", py::native_enum_kind::IntEnum)

This completely replaces the automatic mechanism for the determination of use_int_enum (a member of pybind11::detail::native_enum_data).

All other changes are of the boilerplate kind (updating one error message and the native_enum unit tests).

The rationale for this change is:

  • For manually written code: Simply put it in the hands of the author what native enum type is used. This also makes the bindings more self-documenting.

  • For automatically generated code (PyCLIF): Leave the choice of native enum type up to the code generator (PyCLIF-C-API has the corresponding logic already).

Google-internal global test ID: OCL:626240839:BASE:626375750:1713541129757:e746a257




For completeness: This is what happens if a user chooses IntEnum for a C++ enum with a char underlying type:

( cd /usr/local/google/home/rwgk/forked/pybind11/tests && PYTHONPATH=/usr/local/google/home/rwgk/forked/build_gcc/lib /usr/bin/python3 -m pytest test_native_enum.py )

Traceback (most recent call last):
  File "/usr/lib/python3.11/enum.py", line 714, in __call__
    return cls._create_(
           ^^^^^^^^^^^^^
  File "/usr/lib/python3.11/enum.py", line 891, in _create_
    return metacls.__new__(metacls, class_name, bases, classdict, boundary=boundary)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/enum.py", line 566, in __new__
    raise exc
  File "/usr/lib/python3.11/enum.py", line 259, in __set_name__
    enum_member = enum_class._new_member_(enum_class, *args)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: invalid literal for int() with base 10: 'h'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/local/google/home/rwgk/forked/pybind11/tests/conftest.py", line 20, in <module>
    import pybind11_tests
ImportError: initialization failed
ImportError while loading conftest '/usr/local/google/home/rwgk/forked/pybind11/tests/conftest.py'.
conftest.py:20: in <module>
    import pybind11_tests
E   ImportError: initialization failed

ERROR: completed_process.returncode=4



Note regarding commit history: this PR was originally based on #30117.

Suggested changelog entry:

Ralf W. Grosse-Kunstleve added 8 commits April 18, 2024 10:49
🐍 3 • Clang dev • C++11 • x64

```
-- The CXX compiler identification is Clang 19.0.0
```

```
/__w/pybind11k/pybind11k/include/pybind11/detail/function_record_pyobject.h:38:26: error: cast from 'PyObject *(*)(PyObject *, PyObject *, PyObject *)' (aka '_object *(*)(_object *, _object *, _object *)') to 'PyCFunction' (aka '_object *(*)(_object *, _object *)') converts to incompatible function type [-Werror,-Wcast-function-type-mismatch]
   38 |     = {{"__reduce_ex__", (PyCFunction) reduce_ex_impl, METH_VARARGS | METH_KEYWORDS, nullptr},
```
@@ -36,7 +36,7 @@ class native_enum_data {
std::string was_not_added_error_message() const {
return "`native_enum` was not added to any module."
" Use e.g. `m += native_enum<...>(\""
+ enum_name_encoded + "\")` to fix.";
+ enum_name_encoded + "\", py::native_enum_kind::IntEnum)` to fix.";
Copy link

Choose a reason for hiding this comment

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

This might give an impression that the IntEnum kind is preferred, which isn't really true, right? Should this just use a placeholder ellipsis , or full suggestion of both versions? (e.g., "Use m += native_enum<...>("Name", Enum) or m += native_enum<...>("Name", IntEnum) to fix)

Copy link
Contributor Author

@rwgk rwgk Apr 19, 2024

Choose a reason for hiding this comment

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

Changed (f3b8a03) to .... — I wasn't sure about this myself before. Considering that the += part is the critical part of the error message, I believe using ... is best (will distract people the least from the critical part).

@rwgk rwgk merged commit 9f58994 into google:main Apr 19, 2024
146 of 147 checks passed
@rwgk rwgk deleted the native_enum_kind branch April 19, 2024 18:36
rwgk pushed a commit to google/clif that referenced this pull request Aug 27, 2024
This update needs corresponding small changes in third_party/clif/pybind11/enums.py third_party/clif/testing/python/t3_test.py

TGP-tested via temporary cl/626240839: http://tap/OCL:626240839:BASE:626375750:1713541129757:e746a257

Note: Currently codesearch finds matches for `pybind11 native_enum` only in third_party/pybind11 and third_party/clif/pybind11.

This CL fixes 15 PyCLIF-pybind11 TGP failures: b/287289622#comment73

#MIGRATION_3P_PYBIND11__DEFAULT

  - 9f5899480ce875ad99daad0874110aba2658ee7b Introduce `py::native_enum_kind` (mandatory argument with... by Ralf W. Grosse-Kunstleve <[email protected]>

PiperOrigin-RevId: 626432303
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