Skip to content

Fix static_pointer_cast build failure with virtual inheritance in holder_caster_foreign_helpers.h#6014

Merged
oremanj merged 2 commits intopybind:masterfrom
rwgk:static_pointer_cast_where_dynamic_pointer_cast_is_needed
Mar 27, 2026
Merged

Fix static_pointer_cast build failure with virtual inheritance in holder_caster_foreign_helpers.h#6014
oremanj merged 2 commits intopybind:masterfrom
rwgk:static_pointer_cast_where_dynamic_pointer_cast_is_needed

Conversation

@rwgk
Copy link
Copy Markdown
Collaborator

@rwgk rwgk commented Mar 26, 2026

When a class uses virtual inheritance and its holder type is shared_ptr, any method taking a shared_ptr of the derived type as an argument fails to compile. This is because set_via_shared_from_this in holder_caster_foreign_helpers.h unconditionally uses static_pointer_cast to downcast from the enable_shared_from_this base to the target type. With virtual inheritance, static_cast from base to derived is ill-formed.

The fix introduces a SFINAE-dispatched esft_downcast helper with two overloads (using the "tag dispatch via overload priority" trick):

  • Preferred (int): uses static_pointer_cast — selected when static_cast<type*>(esft_base*) is well-formed (non-virtual inheritance). This is the common case, with zero behavioral change from before.
  • Fallback (...): uses dynamic_pointer_cast — selected via SFINAE when the static_cast is ill-formed (virtual inheritance).

Why not simpler alternatives?

  • dynamic_pointer_cast unconditionally: requires polymorphic types (at least one virtual function). Would break existing code that uses enable_shared_from_this with non-polymorphic type hierarchies.
  • std::is_polymorphic dispatch: orthogonal to virtual inheritance. A type can be polymorphic without virtual inheritance (common case), so this would unnecessarily use dynamic_pointer_cast for all polymorphic types. There is no standard is_virtual_base_of trait.

Note on the test

The test binds .def("name", &SftVirtDerived2::name) directly on the derived class rather than relying on inheritance from SftVirtBase. This works around a separate pre-existing issue where pybind11's method dispatch through virtual inheritance chains uses incorrect pointer offsets, causing a segfault. That issue is unrelated to #5989 and is flagged with a TODO comment in the test code.

CI verification

The first commit (test only, no fix) was pushed to confirm the compilation error reproduces on all CI platforms:

  • GCC: cannot convert from pointer to base class 'SftVirtBase' to pointer to derived class 'SftVirtDerived2' via virtual base 'SftVirtDerived'
  • MSVC: error C2635: cannot convert a 'SftVirtBase*' to a 'SftVirtDerived2*'; conversion from a virtual base class is implied
  • Clang: same class of error

Every platform that compiles tests failed with the same error.

See: https://github.com/pybind/pybind11/actions/runs/23617176855?pr=6014

rwgk added 2 commits March 26, 2026 13:43
…irtual inheritance

When a class uses virtual inheritance and its holder type is shared_ptr,
passing a shared_ptr of the derived type as a method argument triggers
a compilation error because static_pointer_cast cannot downcast through
a virtual base (dynamic_pointer_cast is needed instead).

Made-with: Cursor
…esft downcast

Replace the unconditional static_pointer_cast in set_via_shared_from_this
with a SFINAE-dispatched esft_downcast helper that falls back to
dynamic_pointer_cast when static_cast through a virtual base is ill-formed.

Also add a workaround in the test binding (.def("name") on SftVirtDerived2)
for a separate pre-existing issue with inherited method dispatch through
virtual bases.

Made-with: Cursor
@rwgk
Copy link
Copy Markdown
Collaborator Author

rwgk commented Mar 26, 2026

Summarizing the CI results for the new test, without a fix:

https://github.com/pybind/pybind11/actions/runs/23617176855?pr=6014

Every platform that compiles the tests fails with the exact same static_pointer_cast error from holder_caster_foreign_helpers.h — exactly as expected.

  • GCC: cannot convert from pointer to base class 'SftVirtBase' to pointer to derived class 'SftVirtDerived2' via virtual base 'SftVirtDerived'
  • MSVC: error C2635: cannot convert a 'SftVirtBase*' to a 'SftVirtDerived2*'; conversion from a virtual base class is implied

@rwgk rwgk requested a review from oremanj March 26, 2026 21:38
@rwgk rwgk changed the title WIP fixing issue #5989 Fix static_pointer_cast build failure with virtual inheritance in holder_caster_foreign_helpers.h Mar 26, 2026
@oremanj oremanj merged commit 3b62426 into pybind:master Mar 27, 2026
89 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Mar 27, 2026
rwgk added a commit to rwgk/pybind11 that referenced this pull request Mar 30, 2026
…holder_caster_foreign_helpers.h` (pybind#6014)

* Add regression test for pybind#5989: static_pointer_cast fails with virtual inheritance

When a class uses virtual inheritance and its holder type is shared_ptr,
passing a shared_ptr of the derived type as a method argument triggers
a compilation error because static_pointer_cast cannot downcast through
a virtual base (dynamic_pointer_cast is needed instead).

Made-with: Cursor

* Fix pybind#5989: use dynamic_pointer_cast for virtual inheritance in esft downcast

Replace the unconditional static_pointer_cast in set_via_shared_from_this
with a SFINAE-dispatched esft_downcast helper that falls back to
dynamic_pointer_cast when static_cast through a virtual base is ill-formed.

Also add a workaround in the test binding (.def("name") on SftVirtDerived2)
for a separate pre-existing issue with inherited method dispatch through
virtual bases.

Made-with: Cursor
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Mar 31, 2026
@rwgk rwgk deleted the static_pointer_cast_where_dynamic_pointer_cast_is_needed branch March 31, 2026 23:56
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