Skip to content

Commit

Permalink
Use return_value_policy::_clif_automatic for callback args unless t…
Browse files Browse the repository at this point in the history
…he arg type is `PyObject`.

Fixes PyCLIF-pybind11 TGP test failures for this test:

```
blaze test //chubby/python/internal:pywraprecursivewatcher_test
```

This CL required prior cleanup: b/142422036#comment3 and related CLs (the main CL was cl/577383870).

That cleanup resolved 12 TGP test failures originally observed with this CL: b/287289622#comment44

PiperOrigin-RevId: 584061498
  • Loading branch information
rwgk committed Dec 11, 2023
1 parent 06b533f commit d62f49c
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 1 deletion.
4 changes: 3 additions & 1 deletion clif/pybind11/function_lib.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,9 @@ def generate_return_value_policy_for_type(
if param_type.lang_type == 'bytes':
return 'py::return_value_policy::_return_as_bytes'
elif is_callable_arg:
return 'py::return_value_policy::automatic_reference'
if param_type.lang_type == 'object':
return 'py::return_value_policy::automatic_reference'
return 'py::return_value_policy::_clif_automatic'
elif reference_internal:
return 'py::return_value_policy::reference_internal'
else:
Expand Down
19 changes: 19 additions & 0 deletions clif/testing/callback.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,11 @@

#include <functional>
#include <string>
#include <utility>
#include <vector>

#include "clif/testing/copy_move_types_library.h"

namespace cliff {
namespace testing {
typedef std::function<int(const std::vector<int>&, std::vector<int>*)>
Expand Down Expand Up @@ -91,4 +94,20 @@ inline int CallCallbackPassIntReturnInt(const std::function<int(int)>& cb,
return cb(val);
}

#define LOCAL_HELPER(CMLType) \
inline std::string CallCallbackPassConstPtr##CMLType( \
const std::function<std::string( \
const clif_testing::copy_move_types_library::CMLType*)>& cb) { \
clif_testing::copy_move_types_library::CMLType obj; \
std::string cb_trace = cb(&obj); \
return obj.GetTrace() + "@" + cb_trace; \
}

LOCAL_HELPER(CopyMoveType)
LOCAL_HELPER(CopyOnlyType)
LOCAL_HELPER(MoveOnlyType)
LOCAL_HELPER(StayPutType)

#undef LOCAL_HELPER

#endif // CLIF_TESTING_CALLBACK_H_
9 changes: 9 additions & 0 deletions clif/testing/python/callback.clif
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from "clif/testing/python/copy_move_types_library_clif.h" import *

from "clif/testing/callback.h":
def `FunctionNewStyleCallbackConstRef` as NewStyleCallbackConstRef(
input:list<int>,
Expand All @@ -36,3 +38,10 @@ from "clif/testing/callback.h":
def PyObjectCallback(cb: () -> object) -> object

def CallCallbackPassIntReturnInt(cb: (val: int) -> int, val: int) -> int

def CallCallbackPassConstPtrCopyMoveType(cb: (obj: CopyMoveType) -> str) -> str
def CallCallbackPassConstPtrCopyOnlyType(cb: (obj: CopyOnlyType) -> str) -> str
# TODO: Uncomment additional tests.
# These fail to build with PyCLIF-C-API, but only fail at runtime with PyCLIF-pybind11.
# def CallCallbackPassConstPtrMoveOnlyType(cb: (obj: MoveOnlyType) -> str) -> str
# def CallCallbackPassConstPtrStayPutType(cb: (obj: StayPutType) -> str) -> str
26 changes: 26 additions & 0 deletions clif/testing/python/callback_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ def RaisingCallback():
raise ValueError('raised a value error')


def ObjCopyMoveTraceCallback(obj):
return obj.trace


class CallbackTest(parameterized.TestCase):

def CallbackMethod(self, data):
Expand Down Expand Up @@ -155,6 +159,28 @@ def Cb(val):
'ValueError: Positive value: 5',
)

@parameterized.parameters(
callback.CallCallbackPassConstPtrCopyMoveType,
callback.CallCallbackPassConstPtrCopyOnlyType,
)
def testCallCallbackPassConstPtrCopyableType(self, call_cb):
trace = call_cb(ObjCopyMoveTraceCallback)
self.assertEqual(trace, 'DefaultCtor@DefaultCtor_CpCtor')

@parameterized.parameters('MoveOnlyType', 'StayPutType')
def testCallCallbackPassConstPtrMoveOnlyType(self, cml_type):
call_cb_fn_name = f'CallCallbackPassConstPtr{cml_type}'
call_cb = getattr(callback, call_cb_fn_name, None)
if call_cb is None:
self.skipTest(f'Unavailable: {call_cb_fn_name}')
# TODO: Uncomment in callback.clif
expected_error = (
'return_value_policy = copy, but type '
f'clif_testing::copy_move_types_library::{cml_type} is non-copyable!'
)
with self.assertRaisesWithLiteralMatch(RuntimeError, expected_error):
call_cb(ObjCopyMoveTraceCallback)


if __name__ == '__main__':
absltest.main()

0 comments on commit d62f49c

Please sign in to comment.