pybind / pybind11

Seamless operability between C++11 and Python
https://pybind11.readthedocs.io/
Other
15.11k stars 2.05k forks source link

[BUG]: Unexpected behavior for binding to a data member, which is returned-by-reference by a member function #4858

Open hyliu1989 opened 9 months ago

hyliu1989 commented 9 months ago

Required prerequisites

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

hyliu1989 commented 9 months ago

I push the codes to here. Hopefully you can use these codes to recreate the bug.