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

Bug fixes: Add missing handle_type_name specializations. #5073

Merged
merged 2 commits into from Mar 27, 2024

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Mar 27, 2024

Description

This PR adds a number of missing handle_type_name specializations that have accumulated over time unnoticed, until work under #4888 exposed the issue. This PR is based on #4888, but omits behavior changes other than the pure bug fixes.

To prevent future accumulation of missing handle_type_name specializations, this PR adds the -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION cmake option, which is OFF by default, but set to ON for some (but not all) ci.yml jobs. When ON, missing handle_type_name specializations trigger compilation errors.

The quote_cpp_type_name() function introduced in #4888 is included here, but with the implementation changed to a no-op. This pin-points where C++ type names "slip through" (and could confuse stubgen), and enables easy future experimentation similar to #4888.

Caveat: The test coverage for the handle_type_name specializations is incomplete: while the -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION option ensures that no specializations are missing, some of the names could be changed without triggering unit test failures.

Suggested changelog entry:

@rwgk rwgk requested a review from henryiii as a code owner March 27, 2024 07:30
@rwgk
Copy link
Collaborator Author

rwgk commented Mar 27, 2024

@henryiii It's past bedtime in my time zone. I'll look at the GHA results tomorrow.

Current thinking based on the hope that the GHA results look good:

  • With -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION=ON (default is OFF) we can be sure that we're not missing any specializations.

  • But full test coverage for all specializations is still missing. I believe it's at least a couple hours of leg work to achieve full coverage. Do we really need it? The specializations are pretty trivial: are code reviews sufficient?

@rwgk
Copy link
Collaborator Author

rwgk commented Mar 27, 2024

@henryiii The PR description is complete now, with "Caveat" regarding unit test coverage.

I believe it's a minor caveat and we should include these bug fixes in the release as is. Or I could work on making the unit test coverage complete over the weekend, and we release next week.

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

Otherwise, I think this is fine. We don't have 100% coverage currently, and we've often focused on keeping the test suite compile time down in the past over complete coverage, so I think it's fine to not worry about covering every new specialization. I'd like to have seen a small smattering of them added (like 3), but at least the anyset one is there.

CMakeLists.txt Show resolved Hide resolved
@rwgk rwgk merged commit 0efff79 into pybind:master Mar 27, 2024
84 checks passed
@rwgk rwgk deleted the handle_type_name_fixes branch March 27, 2024 19:39
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.

None yet

2 participants