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

WIP: Conversions between Python's native (stdlib) enum and C++ enums. #4329

Closed
wants to merge 35 commits into from

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Nov 12, 2022

Description

Please ignore for now.

Suggested changelog entry:

@Skylion007
Copy link
Collaborator

Skylion007 commented Nov 13, 2022

@rwgk There has been a lot of discussion about doing this and other limitation with the enum class for a while btw: see especially #2332 and #1177 (for a long standing enum issue). Also see this gist from #2332 https://gist.github.com/anntzer/96f27c0b88634dbc61862d08bff46d10 . 2332 has a lot of ideas for how to do it properly and how people do it in their downstream project.

@rwgk
Copy link
Collaborator Author

rwgk commented Nov 13, 2022

@rwgk There has been a lot of discussion about doing this and other limitation with the enum class for a while btw: see especially #2332 and #1177 (for a long standing enum issue). Also see this gist from #2332 https://gist.github.com/anntzer/96f27c0b88634dbc61862d08bff46d10 . 2332 has a lot of ideas for how to do it properly and how people do it in their downstream project.

Thanks for the pointers. I was vaguely aware of #2332 but didn't actually look very much...

... because I basically don't have a choice: the native_enum is for PyCLIF compatibility. We have hundreds (hand-waving estimate) of extensions that use a native_enum equivalent. Global-test failures due to pybind11:enum_ vs native enum mismatches are one of the biggest remaining blockers for us.

Note that I'm not aiming to replace pybind11::enum_, but to add an alternative option.

I'll focus on making the native_enum implementation complete for all corner cases, then with that in hand

  1. the PyCLIF-pybind11 integration has one blocker less,
  2. we can step back and review what we want to do long term.

@Skylion007
Copy link
Collaborator

@rwgk There has been a lot of discussion about doing this and other limitation with the enum class for a while btw: see especially #2332 and #1177 (for a long standing enum issue). Also see this gist from #2332 https://gist.github.com/anntzer/96f27c0b88634dbc61862d08bff46d10 . 2332 has a lot of ideas for how to do it properly and how people do it in their downstream project.

Thanks for the pointers. I was vaguely aware of #2332 but didn't actually look very much...

... because I basically don't have a choice: the native_enum is for PyCLIF compatibility. We have hundreds (hand-waving estimate) of extensions that use a native_enum equivalent. Global-test failures due to pybind11:enum_ vs native enum mismatches are one of the biggest remaining blockers for us.

Note that I'm not aiming to replace pybind11::enum_, but to add an alternative option.

I'll focus on making the native_enum implementation complete for all corner cases, then with that in hand

1. the PyCLIF-pybind11 integration has one blocker less,

2. we can step back and review what we want to do long term.

Now that we only support Python >3.5, I don't see why we can't implement some of those changes to replace the old enum if needed.

@Skylion007
Copy link
Collaborator

Also, some of the proposed methods don't require an ABI break. The Python Enum objects are singletons so we just need to generate and map between them in the caster somehow.

@rwgk
Copy link
Collaborator Author

rwgk commented Nov 13, 2022

Also, some of the proposed methods don't require an ABI break. The Python Enum objects are singletons so we just need to generate and map between them in the caster somehow.

I think it needs to go into "something like internals", but not necessarily internals.

@wjakob I think it would be great to have #4293 (with the tiny #4307 tweak) merged, for the get_internals_state_dict() function, which I could reuse for something like get_native_enums_from_state_dict(). (Maybe a better name for the helper function would be get_python_state_dict().)

Ralf W. Grosse-Kunstleve added 18 commits November 13, 2022 15:53
…um_type`

Also introducing `EnumType *value_ptr`, although that's not critical to fixing the `*obj.cast<color *>()` use case.
… cast_is_temporary_value_reference, replace static check in cast<> with runtime check
```
D:\a\pybind11\pybind11\include\pybind11/native_enum.h(37,1): warning C4458: declaration of 'name' hides class member
```
Ralf W. Grosse-Kunstleve added 7 commits November 16, 2022 10:34
Modifications are:

* Backward compatibility (no ABI break), as originally under PR pybind#4307.
* Naming: `get_python_state_dict()`, `has_pybind11_internals_capsule()`
* Report error retrieving `internals**` from capsule instead of clearing it.

Locally tested with ASAN, MSAN, TSAN, UBSAN (Google-internal toolchain).
…touched (compared to master).

Retested with all sanitizers.
@rwgk
Copy link
Collaborator Author

rwgk commented Nov 17, 2022

EDIT: It turned out, fixing the new leak was much easier than expected:

commit 3debb51

Organize internals-like functionality as struct native_enum_type_map_v1, including cleanup in embed.h (no more leaking).

Note that the old leaks still exist.


This PR currently includes this code (in pybind11.h):

        // Intentionally leak Python reference.
        detail::get_native_enum_type_map()[data.enum_type_index] = py_enum.release().ptr();

This will only matter for embedding.

This small experiment shows that it currently does not even matter for embedding:

https://github.com/rwgk/issues/tree/0e7ebbda7494a3d1165aaf45523fad4c506199f7/pybind11/initialize_finalize_loop

That experiment shows:

  • A simple initialize-finalize loop does not leak with Python 3.10.
  • But if class_<>(...); is called it leaks substantially.

top output files are included under the link above.

Conclusion: Any effort trying to avoid the "Intentional leak" above would have no tangible benefit. There is a bigger general problem that would need to be solved first, and probably that solution would make it straightforward to also avoid this documented leak.

For future easy reference, this was the state @ master: https://github.com/pybind/pybind11/tree/296615ad34f9d8f680efbab22553881ad9843063

Ralf W. Grosse-Kunstleve added 5 commits November 18, 2022 10:28
…_v1`, including cleanup in embed.h (no more leaking).
```
/__w/pybind11/pybind11/include/pybind11/detail/internals.h:707:17: error: Forming reference to null pointer [clang-analyzer-core.NonNullParamChecker,-warnings-as-errors]
                AdapterType::payload_clear(**payload_pp());
                ^
```
…sion_shared_state.h from internals.h

IWYU cleanup in the new files and internals.h
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Nov 20, 2022
The patch .rej below are resolved, but THIS STATE DOES NOT BUILD:

```
clang++ -o pybind11/tests/test_constants_and_functions.os -c -std=c++17 -fPIC -fvisibility=hidden -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Wunused-result -Werror -isystem /usr/include/python3.10 -isystem /usr/include/eigen3 -DPYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/local/google/home/rwgk/forked/pybind11/include -I/usr/local/google/home/rwgk/clone/pybind11/include /usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp
In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp:11:
In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/pybind11_tests.h:3:
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/eval.h:14:
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:1781:9: error: static_assert failed due to requirement '!holder_is_smart_holder == type_caster_type_is_type_caster_base_subtype' "py::class_ holder vs type_caster mismatch: missing PYBIND11_TYPE_CASTER_BASE_HOLDER(T, ...) or collision with custom py::detail::type_caster<T>?"
        static_assert(!holder_is_smart_holder == type_caster_type_is_type_caster_base_subtype,
        ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:2460:11: note: in instantiation of function template specialization 'pybind11::class_<MyEnum>::class_<>' requested here
        : class_<Type>(scope, name, extra...), m_base(*this, scope) {
          ^
/usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp:106:5: note: in instantiation of function template specialization 'pybind11::enum_<MyEnum>::enum_<>' requested here
    py::enum_<MyEnum>(m, "MyEnum")
    ^
1 error generated.
```

________

```
rwgk.c.googlers.com:~/forked/pybind11 $ patch -p 1 < ~/native_enum_git_diff_master_2022-11-19+131942.patch
patching file .github/workflows/python312.yml
patching file CMakeLists.txt
Hunk #1 FAILED at 111.
Hunk #2 succeeded at 138 (offset 5 lines).
1 out of 2 hunks FAILED -- saving rejects to file CMakeLists.txt.rej
patching file include/pybind11/cast.h
Hunk #1 FAILED at 12.
Hunk #2 succeeded at 78 (offset 30 lines).
Hunk #3 succeeded at 1173 (offset 41 lines).
1 out of 3 hunks FAILED -- saving rejects to file include/pybind11/cast.h.rej
patching file include/pybind11/detail/abi_platform_id.h
patching file include/pybind11/detail/cross_extension_shared_state.h
patching file include/pybind11/detail/internals.h
Hunk #1 FAILED at 16.
Hunk #2 succeeded at 109 (offset 1 line).
Hunk #3 FAILED at 203.
Hunk #4 succeeded at 398 (offset 11 lines).
Hunk #5 succeeded at 457 (offset 11 lines).
2 out of 5 hunks FAILED -- saving rejects to file include/pybind11/detail/internals.h.rej
patching file include/pybind11/detail/native_enum_data.h
patching file include/pybind11/detail/type_map.h
patching file include/pybind11/embed.h
patching file include/pybind11/native_enum.h
patching file include/pybind11/pybind11.h
Hunk #1 FAILED at 12.
Hunk #2 succeeded at 1269 (offset 1 line).
Hunk #3 succeeded at 2457 (offset 255 lines).
1 out of 3 hunks FAILED -- saving rejects to file include/pybind11/pybind11.h.rej
patching file include/pybind11/pytypes.h
patching file tests/CMakeLists.txt
Hunk #1 succeeded at 160 (offset 18 lines).
patching file tests/conftest.py
patching file tests/extra_python_package/test_files.py
Hunk #2 FAILED at 47.
1 out of 2 hunks FAILED -- saving rejects to file tests/extra_python_package/test_files.py.rej
patching file tests/test_embed/test_interpreter.cpp
patching file tests/test_enum.cpp
patching file tests/test_enum.py
patching file tests/test_native_enum.cpp
patching file tests/test_native_enum.py
```
@rwgk rwgk closed this Mar 9, 2023
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Apr 18, 2023
* Transfer PR pybind#4329 from master to smart_holder branch, STEP 1.

The patch .rej below are resolved, but THIS STATE DOES NOT BUILD:

```
clang++ -o pybind11/tests/test_constants_and_functions.os -c -std=c++17 -fPIC -fvisibility=hidden -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Wunused-result -Werror -isystem /usr/include/python3.10 -isystem /usr/include/eigen3 -DPYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/local/google/home/rwgk/forked/pybind11/include -I/usr/local/google/home/rwgk/clone/pybind11/include /usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp
In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp:11:
In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/pybind11_tests.h:3:
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/eval.h:14:
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:1781:9: error: static_assert failed due to requirement '!holder_is_smart_holder == type_caster_type_is_type_caster_base_subtype' "py::class_ holder vs type_caster mismatch: missing PYBIND11_TYPE_CASTER_BASE_HOLDER(T, ...) or collision with custom py::detail::type_caster<T>?"
        static_assert(!holder_is_smart_holder == type_caster_type_is_type_caster_base_subtype,
        ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:2460:11: note: in instantiation of function template specialization 'pybind11::class_<MyEnum>::class_<>' requested here
        : class_<Type>(scope, name, extra...), m_base(*this, scope) {
          ^
/usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp:106:5: note: in instantiation of function template specialization 'pybind11::enum_<MyEnum>::enum_<>' requested here
    py::enum_<MyEnum>(m, "MyEnum")
    ^
1 error generated.
```

________

```
rwgk.c.googlers.com:~/forked/pybind11 $ patch -p 1 < ~/native_enum_git_diff_master_2022-11-19+131942.patch
patching file .github/workflows/python312.yml
patching file CMakeLists.txt
Hunk #1 FAILED at 111.
Hunk #2 succeeded at 138 (offset 5 lines).
1 out of 2 hunks FAILED -- saving rejects to file CMakeLists.txt.rej
patching file include/pybind11/cast.h
Hunk #1 FAILED at 12.
Hunk #2 succeeded at 78 (offset 30 lines).
Hunk #3 succeeded at 1173 (offset 41 lines).
1 out of 3 hunks FAILED -- saving rejects to file include/pybind11/cast.h.rej
patching file include/pybind11/detail/abi_platform_id.h
patching file include/pybind11/detail/cross_extension_shared_state.h
patching file include/pybind11/detail/internals.h
Hunk #1 FAILED at 16.
Hunk #2 succeeded at 109 (offset 1 line).
Hunk #3 FAILED at 203.
Hunk #4 succeeded at 398 (offset 11 lines).
Hunk #5 succeeded at 457 (offset 11 lines).
2 out of 5 hunks FAILED -- saving rejects to file include/pybind11/detail/internals.h.rej
patching file include/pybind11/detail/native_enum_data.h
patching file include/pybind11/detail/type_map.h
patching file include/pybind11/embed.h
patching file include/pybind11/native_enum.h
patching file include/pybind11/pybind11.h
Hunk #1 FAILED at 12.
Hunk #2 succeeded at 1269 (offset 1 line).
Hunk #3 succeeded at 2457 (offset 255 lines).
1 out of 3 hunks FAILED -- saving rejects to file include/pybind11/pybind11.h.rej
patching file include/pybind11/pytypes.h
patching file tests/CMakeLists.txt
Hunk #1 succeeded at 160 (offset 18 lines).
patching file tests/conftest.py
patching file tests/extra_python_package/test_files.py
Hunk #2 FAILED at 47.
1 out of 2 hunks FAILED -- saving rejects to file tests/extra_python_package/test_files.py.rej
patching file tests/test_embed/test_interpreter.cpp
patching file tests/test_enum.cpp
patching file tests/test_enum.py
patching file tests/test_native_enum.cpp
patching file tests/test_native_enum.py
```

* Make `smart_holder` code compatible with `type_caster_enum_type`

* Fix `if` condition guarding `Unable to cast native enum type to reference`

* WIP native_enum_add_to_parent

* PYBIND11_SILENCE_MSVC_C4127

* Transfer upstream.yml/python312.yml changes from PR pybind#4397

* clang-tidy (clang 15) auto-fix

* Remove python312.yml: this is handled separately under PR #30006

* Fixes for ruff

* Replace `PyEval_GetBuiltins()` in `finalize_interpreter()` with `get_python_state_dict()`

* Bug fix: Ensure `state_dict` is destroyed before `Py_Finalize()`

* Restore tests/test_embed/test_interpreter.cpp from google_pywrapcc_main

* Restore include/pybind11/embed.h from google_pywrapcc_main

* Back out all native_enum changes, leaving only the cross_extension_shared_state changes.

* Undo unrelated one-line change (from `auto` to `internals`).
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Apr 18, 2023
…. (#30005)

* Transfer PR pybind#4329 from master to smart_holder branch, STEP 1.

The patch .rej below are resolved, but THIS STATE DOES NOT BUILD:

```
clang++ -o pybind11/tests/test_constants_and_functions.os -c -std=c++17 -fPIC -fvisibility=hidden -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Wunused-result -Werror -isystem /usr/include/python3.10 -isystem /usr/include/eigen3 -DPYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/local/google/home/rwgk/forked/pybind11/include -I/usr/local/google/home/rwgk/clone/pybind11/include /usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp
In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp:11:
In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/pybind11_tests.h:3:
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/eval.h:14:
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:1781:9: error: static_assert failed due to requirement '!holder_is_smart_holder == type_caster_type_is_type_caster_base_subtype' "py::class_ holder vs type_caster mismatch: missing PYBIND11_TYPE_CASTER_BASE_HOLDER(T, ...) or collision with custom py::detail::type_caster<T>?"
        static_assert(!holder_is_smart_holder == type_caster_type_is_type_caster_base_subtype,
        ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:2460:11: note: in instantiation of function template specialization 'pybind11::class_<MyEnum>::class_<>' requested here
        : class_<Type>(scope, name, extra...), m_base(*this, scope) {
          ^
/usr/local/google/home/rwgk/forked/pybind11/tests/test_constants_and_functions.cpp:106:5: note: in instantiation of function template specialization 'pybind11::enum_<MyEnum>::enum_<>' requested here
    py::enum_<MyEnum>(m, "MyEnum")
    ^
1 error generated.
```

________

```
rwgk.c.googlers.com:~/forked/pybind11 $ patch -p 1 < ~/native_enum_git_diff_master_2022-11-19+131942.patch
patching file .github/workflows/python312.yml
patching file CMakeLists.txt
Hunk #1 FAILED at 111.
Hunk #2 succeeded at 138 (offset 5 lines).
1 out of 2 hunks FAILED -- saving rejects to file CMakeLists.txt.rej
patching file include/pybind11/cast.h
Hunk #1 FAILED at 12.
Hunk #2 succeeded at 78 (offset 30 lines).
Hunk #3 succeeded at 1173 (offset 41 lines).
1 out of 3 hunks FAILED -- saving rejects to file include/pybind11/cast.h.rej
patching file include/pybind11/detail/abi_platform_id.h
patching file include/pybind11/detail/cross_extension_shared_state.h
patching file include/pybind11/detail/internals.h
Hunk #1 FAILED at 16.
Hunk #2 succeeded at 109 (offset 1 line).
Hunk #3 FAILED at 203.
Hunk #4 succeeded at 398 (offset 11 lines).
Hunk #5 succeeded at 457 (offset 11 lines).
2 out of 5 hunks FAILED -- saving rejects to file include/pybind11/detail/internals.h.rej
patching file include/pybind11/detail/native_enum_data.h
patching file include/pybind11/detail/type_map.h
patching file include/pybind11/embed.h
patching file include/pybind11/native_enum.h
patching file include/pybind11/pybind11.h
Hunk #1 FAILED at 12.
Hunk #2 succeeded at 1269 (offset 1 line).
Hunk #3 succeeded at 2457 (offset 255 lines).
1 out of 3 hunks FAILED -- saving rejects to file include/pybind11/pybind11.h.rej
patching file include/pybind11/pytypes.h
patching file tests/CMakeLists.txt
Hunk #1 succeeded at 160 (offset 18 lines).
patching file tests/conftest.py
patching file tests/extra_python_package/test_files.py
Hunk #2 FAILED at 47.
1 out of 2 hunks FAILED -- saving rejects to file tests/extra_python_package/test_files.py.rej
patching file tests/test_embed/test_interpreter.cpp
patching file tests/test_enum.cpp
patching file tests/test_enum.py
patching file tests/test_native_enum.cpp
patching file tests/test_native_enum.py
```

* Make `smart_holder` code compatible with `type_caster_enum_type`

* Fix `if` condition guarding `Unable to cast native enum type to reference`

* WIP native_enum_add_to_parent

* PYBIND11_SILENCE_MSVC_C4127

* Transfer upstream.yml/python312.yml changes from PR pybind#4397

* clang-tidy (clang 15) auto-fix

* Remove python312.yml: this is handled separately under PR #30006

* Fixes for ruff

* Replace `PyEval_GetBuiltins()` in `finalize_interpreter()` with `get_python_state_dict()`

* Bug fix: Ensure `state_dict` is destroyed before `Py_Finalize()`

* Restore tests/test_embed/test_interpreter.cpp from google_pywrapcc_main

* Restore include/pybind11/embed.h from google_pywrapcc_main

* Undo unrelated one-line change (from `auto` to `internals`).

* Add missing `test_class_with_enum`

* Add note to docs/classes.rst pointing to PR #30005
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