Is it ok to return by const reference in pybind11 wrapping?

[Francesco Ballarin]

While working on pull request #424 I had some problems with methods that return a const reference.

See e.g. the following code

import numpy
from dolfin import *

## Copied unit/mesh/test_mesh_editor.py ##

# Create mesh object and open editor
mesh = Mesh()
editor = MeshEditor()
editor.open(mesh, "triangle", 2, 2)
editor.init_vertices(3)  # test both versions of interface
editor.init_vertices_global(3, 3)
editor.init_cells(1)    # test both versions of interface
editor.init_cells_global(1, 1)

# Add vertices
editor.add_vertex(0, numpy.array([0.0, 0.0], dtype='float'))
editor.add_vertex(1, numpy.array([1.0, 0.0], dtype='float'))
editor.add_vertex(2, numpy.array([0.0, 1.0], dtype='float'))

# Add cell
editor.add_cell(0, numpy.array([0, 1, 2], dtype='uint'))

# Close editor
editor.close()

## ------------------------------------- ###

# Try to init facets global indices
mesh_topology = mesh.topology()
mesh_topology.init_global_indices(1, 3)
assert mesh_topology.have_global_indices(1)
assert mesh.topology().have_global_indices(1)

The assert at the last line fails, while in swig changes were retained (as if the output of mesh.topology() was not marked as const). The aim is obviously to change the underlying topology object.

To make things even weirder

>>> from dolfin import *
>>> mesh = UnitSquareMesh(3,3)
>>> id(mesh.topology())
139735462497280
>>> id(mesh.topology())
139735462497336
>>> id(mesh.topology())
139735462497280
>>> id(mesh.topology())
139735462497336
>>> id(mesh.topology())
139735462497280
>>> id(mesh.topology())
139735462497336

[Jan Blechta]

>>> id(mesh.topology())
139735462497336
>>> id(mesh.topology())
139735462497280

What's wrong about it? It's a shared pointer (referencing the topology object) which is created and destroyed.

[Francesco Ballarin]

You are probably right, I just thought it was weird that the first and third id were the same, as well as the second and fourth one, as if there were two copies lying around. Scratch that; the issue in the first part of the post remains.

[Jan Blechta]

You're right, the topology is returned by copy. That's horrible.

[Jan Blechta]

pybind11 is very simple. It very simply allows to write correct code with unintended behaviour.

There should be tests for things like this. I can think of simple checks like

assert mesh.topology().id() == mesh.topology().id()

[Prof Garth Wells]

But what should be done? From the pybind11 side it defaults to 'safe' options, which in the case of a reference is to return a copy, but we can specify the return value policy (see http://pybind11.readthedocs.io/en/stable/advanced/functions.html?highlight=return%20value#return-value-policies). return_value_policy::reference_internal may be appropriate.

Other option is to make the topology object a shared pointer.

[Francesco Ballarin]

So, is it best to return a std::shared_ptr<> rather than a reference for methods that would need their output to go through pybind11? Are there other options?

[apologies Garth, posted at the same time as you]

[Prof Garth Wells]

@francesco_ballarin See http://pybind11.readthedocs.io/en/stable/advanced/functions.html?highlight=return%20value#return-value-policies. A nice thing about pybind11 is that it gives a lot of control over the return policy.

[Jan Blechta]

But what should be done?

Indeed, the reference has to be returned:

diff --git a/python/src/mesh.cpp b/python/src/mesh.cpp
index daae2dc6e..9bfc3f35e 100644
--- a/python/src/mesh.cpp
+++ b/python/src/mesh.cpp
@@ -106,7 +106,8 @@ namespace dolfin_wrappers
       .def("have_shared_entities", &dolfin::MeshTopology::have_shared_entities)
       .def("shared_entities",
            (std::map<std::int32_t, std::set<unsigned int> >&(dolfin::MeshTopology::*)(unsigned int))
-           &dolfin::MeshTopology::shared_entities);
+           &dolfin::MeshTopology::shared_entities)
+      .def("str", &dolfin::MeshTopology::str);

     // dolfin::Mesh
     py::class_<dolfin::Mesh, std::shared_ptr<dolfin::Mesh>>(m, "Mesh",
@@ -179,8 +180,9 @@ namespace dolfin_wrappers
       .def("smooth_boundary", &dolfin::Mesh::smooth_boundary)
       .def("snap_boundary", &dolfin::Mesh::snap_boundary, py::arg("subdomain"),
            py::arg("harmonic_smoothing")=true)
-      .def("topology", (const dolfin::MeshTopology& (dolfin::Mesh::*)() const)
-           &dolfin::Mesh::topology, "Mesh topology")
+      .def("topology", (dolfin::MeshTopology& (dolfin::Mesh::*)())
+           &dolfin::Mesh::topology, "Mesh topology",
+           py::return_value_policy::reference)
       .def("translate", &dolfin::Mesh::translate)
       .def("type", (const dolfin::CellType& (dolfin::Mesh::*)() const) &dolfin::Mesh::type,
            py::return_value_policy::reference)

And this behaviour should be reviewed for other accessors and preferably tested. Variable::id() provides a simple way how to test this.

[Jan Blechta]

Oh word, reference_internal is appropriate here, otherwise mesh.topology() becomes dangling when mesh is destroyed.

It seems that we could be more Pythonic and provide topology as a read/write property mesh.topology instead of using getter/setter methods which are more appropriate for C++ side. It could be a right time to change this now if we want but it would be a lot of work.

[Prof Garth Wells]

@blechta Did you push a fix for this issue?

[Jan Blechta]

Fix in pull request #429.

[Francesco Ballarin]

Thanks. Is there an easy way to double check if other methods currently wrapped by pybind11 return by reference and thus would need the appropriate py::return_value_policy?

[Jan Blechta]

Wait, I have a fix or this to be merged before the release. Now testing in nexts.

[Jan Blechta]

a59cf70