Skip to content

tests: add or delete copy/move ctors where needed to make type traits match reality - #5833

Merged
rwgk merged 1 commit into
pybind:masterfrom
oremanj:allow-trying-to-copy
Sep 8, 2025
Merged

tests: add or delete copy/move ctors where needed to make type traits match reality#5833
rwgk merged 1 commit into
pybind:masterfrom
oremanj:allow-trying-to-copy

Conversation

@oremanj

Copy link
Copy Markdown
Collaborator

#5800 will attempt to instantiate copy/move constructors for every bound type that appears copy/move constructible. (Currently, only types that are returned from a bound function will try to use these constructors, so if a type is used only in argument position, it's possible that it satisfies is_copy_constructible but actually odr-using the copy constructor would fail. Ditto for moves.) See that PR for more on the rationale.

There are several classes used in pybind11's test suite that satisfy is_copy_constructible, but produce warnings or errors if you try to actually odr-use their (implicitly-defined) copy constructor:

  • In recent C++ versions, use of the implicitly-defaulted copy constructor is deprecated for any class with a nontrivial (including virtual) destructor. This produces several warnings due to the test suite's frequent use of patterns that count how many destructions have occurred.
  • If a type has no copy constructor and none of its immediate members are non-copyable but some of their subobjects are non-copyable, it will look copyable to std type traits, but actually generating the copy constructor produces an error. This is relevant to DataFieldsHolder in test_class_sh_property_non_owning.cpp, which contains a std::vector of move-only type DataField.

This PR adds defaulted or deleted copy constructors for these cases, as semantically appropriate.

(No user-visible impact, so no changelog entry.)

@rwgk

rwgk commented Sep 8, 2025

Copy link
Copy Markdown
Collaborator

I don't remember seeing this flake before:

CI / 🐍 (macos-latest, 3.14t, -DCMAKE_CXX_STANDARD=20) / 🧪 (pull_request) Failing after 7m

FAILED ../../tests/test_callbacks.py::test_async_callbacks - assert 17 == 22
+ where 17 = sum([6, 4, 7])
+ and 22 = sum(<generator object test_async_callbacks.<locals>.<genexpr> at 0x23deaef8f10>)
== 1 failed, 959 passed, 34 skipped, 2 xfailed, 1 xpassed in 75.23s (0:01:15) ==

I'll try a rerun.

rwgk
rwgk approved these changes Sep 8, 2025
@rwgk

rwgk commented Sep 8, 2025

Copy link
Copy Markdown
Collaborator

Question for my understanding: After #5800 is merged, will general users have to make similar changes? (I think that's fine, but if yes, we shouldn't forget to mention that in the release notes and maybe upgrade guide.)

@oremanj

Copy link
Copy Markdown
CollaboratorAuthor

After #5800 is merged, will general users have to make similar changes?

Yes. There's some discussion of that in the PR description.

@rwgk

rwgk commented Sep 8, 2025

Copy link
Copy Markdown
Collaborator

Ugh

CI / 🐍 (macos-latest, 3.14t, -DCMAKE_CXX_STANDARD=20) / 🧪 (pull_request) Cancelled after 29m

Now it was stuck here, which I've seen at least twice before:

Run cmake --build build --target cpptest
cmake --build build --target cpptest
shell: /bin/bash -e {0}
env:
PYTHONDEVMODE: 1
PIP_BREAK_SYSTEM_PACKAGES: 1
PIP_ONLY_BINARY: numpy
FORCE_COLOR: 3
PYTEST_TIMEOUT: 300
VERBOSE: 1
CMAKE_COLOR_DIAGNOSTICS: 1
pythonLocation: /Users/runner/hostedtoolcache/Python/3.14.0-rc.2/arm64-freethreaded
PKG_CONFIG_PATH: /Users/runner/hostedtoolcache/Python/3.14.0-rc.2/arm64-freethreaded/lib/pkgconfig
Python_ROOT_DIR: /Users/runner/hostedtoolcache/Python/3.14.0-rc.2/arm64-freethreaded
Python2_ROOT_DIR: /Users/runner/hostedtoolcache/Python/3.14.0-rc.2/arm64-freethreaded
Python3_ROOT_DIR: /Users/runner/hostedtoolcache/Python/3.14.0-rc.2/arm64-freethreaded
UV_CACHE_DIR: /Users/runner/work/_temp/setup-uv-cache
CMAKE_GENERATOR: Ninja
Change Dir: '/Users/runner/work/pybind11/pybind11/build'
Run Build Command(s): /opt/homebrew/bin/ninja -v cpptest
[0/2] /opt/homebrew/bin/cmake -P /Users/runner/work/pybind11/pybind11/build/CMakeFiles/VerifyGlobs.cmake
Error: The operation was canceled.

Trying again...

@rwgk
rwgk merged commit 68cbae6 into pybind:masterSep 8, 2025
209 of 211 checks passed
@github-actionsgithub-actionsBot added the needs changelog Possibly needs a changelog entry label Sep 8, 2025
@rwgkrwgk removed the needs changelog Possibly needs a changelog entry label Sep 8, 2025
@oremanj
oremanj deleted the allow-trying-to-copy branch October 14, 2025 00:01
Sign up for freeto 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

@oremanj@rwgk