[BUG]: Unexpected behavior for binding to a data member, which is returned-by-reference by a member function
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 18k
- Forks
- 2.3k
- Avg merge
- 5d 17h
- Merged PRs (30d)
- 10
Description
Required prerequisites
- Make sure you've read the documentation. Your issue may be addressed there.
- Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
- Consider asking first in the Gitter chat room or in a Discussion.
What version (or hash if on master) of pybind11 are you using?
pybind11-2.11.1-py3-none-any.whl.metadata (copied from pip install output when executing pip install -e .)
This was done in MacOS Ventura 13.5.2.
Python 3.10.13 (main, Aug 24 2023, 22:36:46) [Clang 14.0.3 (clang-1403.0.22.14.1)] on darwin
Problem description
The problem happens when I wrap a class to Python where that class enclosed another class.
class P {
public:
EnclosedClass& get();
EnclosedClass enclosed;
};
When calling get(), sometimes the copy constructor of EnclosedClass is called and sometimes it isn't. It all depends on if there is a Python object holding the data member in Python (e.g., when a = P() is defined in Python, setting another variable b = a.enclosed or not affects the calls of copy constructor of EnclosedClass).
The above dependence on a Python object is unexpected. I would hope that the copy constructor is never called at all cases because the codes was returning a reference. Could someone please point me out which thing I have missed?
Reproducible example code
I pushed to this repo.
cpp_src/example.cpp
#include <pybind11/pybind11.h>
#include <iostream>
namespace py = pybind11;
class Enclosed {
public:
Enclosed() { c = 4; }
Enclosed(const Enclosed& other): c(other.c) {
std::cout << "Enclosed copy constructor invoked!" << std::endl;
}
int c;
};
class TestBind {
public:
TestBind() : _enclosed() {
_enclosed.c = 5;
}
Enclosed& get(int level) {
std::cout << "TestBind::get() returns a reference." << std::endl;
_enclosed.c += level;
return _enclosed;
};
Enclosed _enclosed;
};
PYBIND11_MODULE(_example, m) {
m.doc() = R"pbdoc(Buggy!)pbdoc";
py::class_<Enclosed>(m, "Enclosed")
.def(py::init<>())
.def_readwrite("c", &Enclosed::c)
;
py::class_<TestBind>(m, "TestBind")
.def(py::init<>())
.def_readonly("enclosed", &TestBind::_enclosed)
.def("get", &TestBind::get)
;
m.attr("__version__") = "dev";
}
tests/test_enc.py
# Run by `pytest .`
import myproj._example
def test_enclosed_class():
a = myproj._example.TestBind()
# b = a.enclosed
enc = a.get(6)
enc2 = a.get(6)
assert a.enclosed.c == 17
assert enc2.c == 17
assert enc.c == 17
def test_enclosed_class_2():
a = myproj._example.TestBind()
b = a.enclosed
enc = a.get(6)
enc2 = a.get(6)
assert a.enclosed.c == 17
assert enc2.c == 17
assert enc.c == 17
# assert False. # uncomment this line to see the print out that copy constructor of Enclosed is not called.
pyproject.toml
# Run in your virtual environment with `pip install -e .`
[build-system]
requires = ["scikit-build-core>=0.5.0", "pybind11==2.11.1"]
build-backend = "scikit_build_core.build"
[tool.scikit-build]
wheel.expand-macos-universal-tags = true
[project]
name = "myproj"
version = "0.1.0"
description="MYPROJ"
readme = "README.md"
authors = [{ name = "My Name", email = "me@email.com" }]
requires-python = ">=3.10"
classifiers = [
"Development Status :: 3 - Alpha",
# "License :: OSI Approved :: MIT License",
"Programming Language :: Python :: 3",
"Programming Language :: Python :: 3.8",
"Programming Language :: Python :: 3.9",
"Programming Language :: Python :: 3.10",
"Programming Language :: Python :: 3.11",
# "Operating System :: OS Independent",
]
urls = { Homepage = "https://nodarsensor.com" }
dependencies = [ # related to "requires" in setup.py. See https://peps.python.org/pep-0621/#other-names-for-dependencies-optional-dependencies
]
# Originally called "extra-requires" but the python community decided to rename it.
# https://peps.python.org/pep-0621/#other-names-for-dependencies-optional-dependencies
[project.optional-dependencies]
dev = [
"black",
"isort",
"mypy",
"pre-commit",
"pytest",
"pybind11",
"ipython >= 8.15.0",
"jupyter >= 1.0.0",
"notebook >= 7.0.3",
]
all = [
"myproj[dev]",
]
CMakeLists.txt
cmake_minimum_required(VERSION 3.15...3.26)
# Setup the cmake environment
# dependencies required for python binding.
find_package(Python REQUIRED COMPONENTS Interpreter Development.Module)
find_package(pybind11 CONFIG REQUIRED)
# Python project definition.
project(
${SKBUILD_PROJECT_NAME}
VERSION ${SKBUILD_PROJECT_VERSION}
LANGUAGES CXX
)
# Build a Python-readable cpp module.
## compile the python-readable cpp module.
set(PYTHON_MODULE_NAME _example)
python_add_library(${PYTHON_MODULE_NAME} MODULE cpp_src/example.cpp WITH_SOABI)
target_link_libraries(
${PYTHON_MODULE_NAME}
PRIVATE
pybind11::headers
)
target_compile_definitions(
${PYTHON_MODULE_NAME}
PRIVATE
VERSION_INFO=${PROJECT_VERSION}
)
## install the python-readable wrapper module
install(
TARGETS ${PYTHON_MODULE_NAME}
DESTINATION ${SKBUILD_PROJECT_NAME}
)
Is this a regression? Put the last known working version here if it is.
Not a regression
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the reproducer in cpp_src/example.cpp and the two cases in tests/test_enc.py; build the extension using CMake and run pytest . Compare the behavior of get() with and without retaining a.enclosed. Done means establishing whether the copy-constructor behavior is a pybind11 bug or expected binding behavior, with the result captured in a regression test or clear explanation.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, python
- Domain
- api
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100