Repository: OpenAssetIO/OpenAssetIO (language: python)

Issue:
Investigate pybind11 default args
## Description

Closes #987.  Add a couple of tests to assert the finding that `unordered_set` is safe to use as a default argument value in pybind11 bindings.  Add a short explanation of pybind11 default argument pitfalls in CODING_STANDARDS.

- [ ] ~~I have updated the release notes.~~
- [x] I have updated all relevant user documentation.

## Background

TL;DR: the `unordered_set` we supply as a default value to `py::arg(...)` is copied into a Python `set` and stored by pybind11, then recalled when needed, where it is copied back to an `unordered_set` to pass to C++.

A common pitfall in pure Python is setting a default value for an argument in a function to a mutable value, e.g.

```python 
def someFunction(*args, resultTraitSet=set()): ... 
```

In the above, any mutation to the default-provided `resultTraitSet` will persist in the next invocation.

In our pybind11 bindings we have a similar pattern, i.e.

```c++ 
py::arg("resultTraitSet") = trait::TraitSet{}
```

where `TraitSet` is an alias for `std::unordered_set`.

It turns out that this is not a problem, since we're relying on pybind11's automatic casting between a Python `set` and `unordered_set`, which necessitates a copy of the default value into a new C++ value on every invocation.

Stepping through in the debugger reveals that in pybind11 default arguments are first converted to a Python object before being squirrelled away in a `function_record`.

When calling the C++ function from Python, and supplying an insufficient number of arguments, the squirrelled-away default Python value is recalled (in `cpp_function::dispatcher`) then `cast`ed from the Python object back to a C++ object.

This is the bit that surprised me the most: to reiterate, the default value is stored in the function record as a _Python object_ converted from the input C++ object.  So subsequent use of the default value must obey whatever Python->C++ conversion is appropriate to that type.

For `unordered_set`, this means a Python `set` is stored on the `function_record`. `casti`ng is then done via an explicit `set_caster`, which constructs a new `std::unordered_set` by copying entries across from the Python `set`. This new `unordered_set` is then given to the C++ function.

On the other hand, a default value to an argument of a _generic_ user-defined type is stored in the `function_record` as a Python object pointing to a heap-allocated C++ object that was in turn move-constructed from the default value given in `py::arg(...) = ...`.  This is `cast`ed via a `type_caster_base`, which simply extracts the C++ pointer stored in the Python object, so no copies are made.

## Previous incarnation

This PR previously contained additional tests/logging to explore the inner workings of pybind11 default argument values. 

The original commits are still available under [a branch on my fork](https://github.com/feltech/OpenAssetIO/tree/prototype/987-investigatePybindDefaultArgs).

The following sections relate to that work.

### Reviewer Notes

Probably not worth merging any of this.  Certainly the `drop!` commit just pollutes things and is only useful for gdb explorations.  The first commit may have some value, though?

The test in the `drop!` commit intentionally fails.

### Test Instructions

Run the tests as normal, but don't capture any `stdout` output, in order to see the logs (from the work in the `drop!` commit).

For gdb exploration, build and install, then debug
```
export PYTHONPATH=build/dist/lib/python3.9/site-packages 
python -m pytest -s src/openassetio-python/tests/cmodule
```
specifically for gdb command-line
```
gdb --args build/test-venv/bin/python -m pytest -s src/openassetio-python/tests/cmodule
```


Discussion / hints:

Fix patch:
diff --git a/doc/contributing/CODING_STANDARDS.md b/doc/contributing/CODING_STANDARDS.md
index 5f326a14e..c04d5f1b6 100644
--- a/doc/contributing/CODING_STANDARDS.md
+++ b/doc/contributing/CODING_STANDARDS.md
@@ -293,6 +293,85 @@ will be converted to `PyRetainingSharedPtr<MyArg1>` or
 `PyRetainingSharedPtr<MyArg2>`, respectively. This is equivalent to
 manually wrapping in a lambda, as shown above.
 
+### Default arguments
+
+pybind11 allows bound methods to have keyword arguments with default
+values, for example
+```c++
+.def("myMethod",
+     &MyClass::myMethod, py::arg("myArg") = myDefaultValue, ...
+```
+
+In general, it is not safe to set the default value to an arbitrary
+user-defined type, since mutations persist to the next invocation.
+
+However, it is safe to set the default value to a primitive type, or
+C++ types that are auto-converted to/from Python native types.
+
+In particular, many STL types are [auto-converted by
+pybind11](https://pybind11.readthedocs.io/en/stable/advanced/cast/stl.html#automatic-conversion)
+to/from Python native types, necessitating a copy, and are therefore
+safe to use as default values.
+
+These types include `std::string`, `std::vector<>`, `std::deque<>`,
+`std::list<>`, `std::array<>`, `std::valarray<>`, `std::set<>`,
+`std::unordered_set<>`, `std::map<>`, and `std::unordered_map<>`.
+
+Note that any values within the container are subject to the same
+conditions.
+
+A more detailed explanation follows.
+
+#### Explanation
+
+We have to be careful with what types we use when setting default
+values. This is analogous to the well-known common pitfall in pure
+Python, where a keyword argument's default value is set to a mutable
+type, for example
+
+```python
+def myMethod(self, myArg=set()):
+  myArg.add("myValue")
+```
+
+Here, subsequent calls to `myMethod` will find that `myArg` is no longer
+empty - the default value has been mutated.
+
+A similar problem exists with pybind11 default argument values, but it
+is subtly different.
+
+When setting a default value for an argument, pybind11 will
+
+1. Convert the default value to a Python object
+2. Store the Python object in the function record.
+
+When a C++ function is called from Python and a defaulted argument not
+provided, then
+
+1. The default value is recalled from the function record and inserted
+   into the input Python arguments.
+2. The standard conversions from Python arguments to C++ arguments then
+   takes place.
+
+When the default value is a generic user-defined type, the stored Python
+object will contain a reference. In this case, the same problem occurs
+with pybind11 as with pure Python - default values can be mutated during
+a C++ function call and those mutations persist to the next invocation.
+
+However, some C++ types are converted to/from native Python types,
+including primitives such as `int` and `float`, but also (by default)
+`std::string` and container types like `std::vector` (converted to
+Python `list`) and `std::unordered_set` (converted to Python `set`).
+
+In these cases, the initial conversion of the C++ default value to a
+Python object (to be stored in the function record) necessitates a copy.
+Similarly, the conversion back to a C++ object when the function is
+called also necessitates a copy.
+
+So for types that are converted to/from native Python types, it is safe
+to assign them as default argument values.  Note that this includes
+e.g. `std::vector` (Python `list`), which is _not_ safe in pure Python.
+
 ## Environment variables
 
 All environment variables should be prefixed with `OPENASSETIO_`.


Test patch:
diff --git a/src/openassetio-python/tests/package/hostApi/test_manager.py b/src/openassetio-python/tests/package/hostApi/test_manager.py
index 9b1143f83..de7712397 100644
--- a/src/openassetio-python/tests/package/hostApi/test_manager.py
+++ b/src/openassetio-python/tests/package/hostApi/test_manager.py
@@ -454,6 +454,36 @@ def call_callbacks(*_args):
             mock.ANY,
         )
 
+    def test_when_default_resultTraitSet_modified_then_modifications_dont_persist(
+        self,
+        manager,
+        mock_manager_interface,
+        a_ref,
+        an_empty_traitsdata,
+        a_context,
+    ):
+        def mutate_resultTraitSet(_, __, result_trait_set, *_args):
+            result_trait_set.add("this shouldn't persist")
+
+        def assert_resultTraitSet_empty(_, __, result_trait_set, *_args):
+            assert result_trait_set == set()
+
+        # First call mutates the defaulted resultTraitSet parameter.
+        mock_manager_interface.mock.getWithRelationship.side_effect = mutate_resultTraitSet
+        manager.getWithRelationship(
+            [a_ref], an_empty_traitsdata, a_context, mock.Mock(), mock.Mock()
+        )
+
+        # Second call asserts that the mutation of the first call didn't
+        # persist in the default value.
+        mock_manager_interface.mock.getWithRelationship.side_effect = assert_resultTraitSet_empty
+        manager.getWithRelationship(
+            [a_ref], an_empty_traitsdata, a_context, mock.Mock(), mock.Mock()
+        )
+
+        # Confidence check: ensure we actually called the manager plugin.
+        assert mock_manager_interface.mock.getWithRelationship.call_count == 2
+
 
 class Test_Manager_getWithRelationships:
     def test_method_defined_in_cpp(self, method_introspector):
@@ -536,6 +566,36 @@ def test_when_relationship_is_None_then_exception_raised(
 
         assert not mock_manager_interface.mock.getWithRelationships.called
 
+    def test_when_default_resultTraitSet_modified_then_modifications_dont_persist(
+        self,
+        manager,
+        mock_manager_interface,
+        a_ref,
+        an_empty_traitsdata,
+        a_context,
+    ):
+        def mutate_resultTraitSet(_, __, result_trait_set, *_args):
+            result_trait_set.add("this shouldn't persist")
+
+        def assert_resultTraitSet_empty(_, __, result_trait_set, *_args):
+            assert result_trait_set == set()
+
+        # First call mutates the defaulted resultTraitSet parameter.
+        mock_manager_interface.mock.getWithRelationships.side_effect = mutate_resultTraitSet
+        manager.getWithRelationships(
+            a_ref, [an_empty_traitsdata], a_context, mock.Mock(), mock.Mock()
+        )
+
+        # Second call asserts that the mutation of the first call didn't
+        # persist in the default value.
+        mock_manager_interface.mock.getWithRelationships.side_effect = assert_resultTraitSet_empty
+        manager.getWithRelationships(
+            a_ref, [an_empty_traitsdata], a_context, mock.Mock(), mock.Mock()
+        )
+
+        # Confidence check: ensure we actually called the manager plugin.
+        assert mock_manager_interface.mock.getWithRelationships.call_count == 2
+
 
 class FakeEntityReferencePagerInterface(EntityReferencePagerInterface):
     """