Mutations to holder type aren't visible to the outer scope

lucmans commented 1 year ago

Required prerequisites

[X] Make sure you've read the documentation. Your issue may be addressed there.
[X] Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
[x] Consider asking first in the Gitter chat room or in a Discussion.

What version (or hash if on master) of pybind11 are you using?

2.9.1

Problem description

When an argument is passed-by-reference in C++, we would expect overwriting the reference to be visible from the outside scope. Due to how Python argument passing works, this is not the case for wrapped C++ functions called from Python. However, mutations to the underlying object are visible to the outside.

When an object wrapped in a holder type is passed to C++ (by reference), we would again not expect to see the overwrite in the outside scope. However, when we update the pointer inside the holder type (mutating it), we would expect the changes to be visible to the outside world. This is not the case though.

Reproducible example code

See https://github.com/lucmans/pybind11_mutating_holder_bug for a minimal reproduction of the bug.
Specifically https://github.com/lucmans/pybind11_mutating_holder_bug/blob/1da5c36e15a3f6745e7a5e1093052203dbcd34e9/problem.cpp#L184

Is this a regression? Put the last known working version here if it is.

Not a regression

virtuald commented 1 year ago

I don't think pybind11 claims to support that, it only converts arguments as they come in, it doesn't convert them back. Additionally, I believe multiple calls to functions all use copies of the original holder object, so changes to the copy isn't going to be reflected in the original.

lucmans commented 1 year ago

It is indeed not explicitly stated pybind11 supports this. However, as it is inconsistent with non-holdertype, passed-by-reference variables, I would consider this a bug.

Would you know of any workaround to allow mutations to the holder type?

virtuald commented 1 year ago

However, as it is inconsistent with non-holdertype, passed-by-reference variables, I would consider this a bug.

In general, python doesn't support pass-by-reference in the same way that C++ does, so I wouldn't consider this a bug. Consider these two identical situations:

def some_fn(a):
    a = 7

b = 1
some_fn(b)
print(b) # obviously, this must still be 1, not 7

void some_fn(int &a) {
    a = 7
}

int main() {
    int b = 1;
    some_fn(b);
    // this is now 7, unlike the python case
    printf("%d\n", b);
}

Python just doesn't support references in the same way the C++ does, so if you ever did switch out the argument it would be very very surprising to a python user. I don't see why pybind11 would want to support such a thing.

lucmans commented 1 year ago

The pass-by-reference case is working as expected (as in, not working, just like Python). However, the mutation case (of the holder type) is not working.

A big reason to support this in pybind11 is to be able to match the API of the wrapped library. Now, in our project, we have to also return the input argument on top the normal return value in case the holder type was mutated. Mutating the holder type is supported by SWIG, which we hoped like many other to replace by pybind11. Unfortunately, switching to pybind11 from SWIG is not a strict upgrade anymore, as the wrapped API will have to suffer a lot, due to mutation the underlying pointer being very common in C++.

Even if there is no interest in having this work in pybind11, would you know how we can achieve this? We are using a custom fork of pybind11 anyways (shoutout to pywrapcc), so we wouldn't mind introducing more custom code.

virtuald commented 1 year ago

You would need to get the value_and_holder that's in the pybind11::detail::instance and swap out the value. PR https://github.com/pybind/pybind11/pull/4247/files touches on a lot of those pieces.

pybind / pybind11