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]: std::vector containing non-trivially copyable types are unexpectedly moved #307

Closed
JasperRobeer opened this issue Sep 27, 2023 · 5 comments

Comments

@JasperRobeer
Copy link

Problem description

We found some unexpected behavior when creating a binding for some types, which are not trivially copyable, and passing them between Python and C++ in lists/vectors using the type casters.

We would expect that objects in the lists/vectors remain unchanged after passing them to functions bound in C++. The Python example below illustrates this.

Inspecting the issue in a debugging session, we see a move constructor being called, but we would expect to see the copy constructor being called.

We have reproduced this issue in version v.1.5.2.

Reproducible example code

//
// C++
//

#include <nanobind/nanobind.h>
#include <nanobind/stl/string.h>
#include <nanobind/stl/vector.h>
namespace nb = nanobind;

struct example
{
    std::string text;
};

NB_MODULE(example, m) {
    nb::class_<example>(m, "Example")
        .def(nb::init<const std::string&>())
        .def_rw("text", &example::text)
        .def(
            "__repr__",
            [](const example& e) {
                return std::string("Example(\"") + e.text + "\")";
            }
        );

    m.def(
        "process",
        [](const std::vector<example>& v) {
            return v.size();
        },
        nb::arg("v")
    );
}


#
# Python
#

# To reproduce the issue in Python:
from example import Example, process
a = [Example("A"), Example("B")]
b = process(a)
print(a)

# We expect `a` to be unchanged, but if we inspect it we get:
#   [Example(""), Example("")]
@wjakob
Copy link
Owner

wjakob commented Sep 28, 2023

This is very bad 😱. I am surprised that this has apparently always been an issue, and that you are the first to stumble into it. Will have a fix coming up soon (unfortunately a quite intrusive one that changes many things and will need to stay on master for 1-2 weeks to stabilize a bit and gather feedback)

wjakob added a commit that referenced this issue Sep 29, 2023
This commit addresses a serious issue involving combinations of bound
types (e.g., ``T``) and type casters (e.g., ``std::vector<T>``), where
nanobind was too aggressive in its use of move semantics.

Calling a bound function from Python taking such a list (e.g., ``f([t1,
t2, ..])``) would lead to the destruction of the Python objects ``t1,
t2, ..`` if the type ``T`` exposed a move constructor, which is highly
non-intuitive.

This commit fixes, simplifies, and documents the rules of this process.
In particular, ``type_caster::Cast`` continues to serve its role to
infer what types a type caster can and wants to produce. It aggressively
tries to move, but now only does so when this is legal (i.e., when the
type caster created a temporary copy that is safe to move without
affecting program state elsewhere).

This involves two new default casting rules: ``movable_cast_t`` (adapted
to be more aggressive with moves) and ``precise_cast_t`` for bound types
that does not involve a temporary and consequently should only move when
this was explicitly requested by the user.

The fix also revealed inefficiencies in the previous implementation
where moves were actually possible but not done (e.g. for functions
taking an STL list by value). Some binding projects may see speedups as
a consequence of this change.

Fixes issue #307.
@wjakob
Copy link
Owner

wjakob commented Sep 29, 2023

Fixed in the master branch -- could I ask you to double-check that it addresses your problem?

@wjakob wjakob closed this as completed Sep 29, 2023
@JasperRobeer
Copy link
Author

Yes, the issue appears to be fixed. Thank you very much for your quick resolution!

@wjakob
Copy link
Owner

wjakob commented Oct 2, 2023

Great to hear -- I think I will actually push this out fairly soon to get feedback. I think that the current bug is more serious than concerns about keeping compatibility.

@matthewdcong
Copy link
Contributor

I just ran into this myself. After updating to the latest, it appears to be fixed.

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

No branches or pull requests

3 participants