[CORRECTION] Boost.Python handling return types: FEEDBACK REQUEST

From: David Abrahams (abrahams_at_[hidden])
Date: 2001-03-24 19:55:30

The previous posting came out with annoying line-wraps (sorry!)
I hope this one looks better
----------

A number of things have come at once which argue for a rework of part of the
Boost.Python library. Since these changes would affect the existing library
interface, I'd like to discuss possible approaches here before doing
anything.

A Missing Feature [Ullrich and Ralf are already familiar with this part]
=================

One issue is the need to be able to return references and pointers to the
internals of classes exposed to Python.  For example, if the following
classes are exported to Python:

    struct inner {
      void set_x(int x) { this->x = x; }
      int get_x() const { return x; }
      int x;
    };

    struct outer {
       inner& get_inner() { return b; }
       inner b;
    };

We should be able to do the following:

   >>> o = outer()
   >>> o.get_inner().set_x(1)
   >>> assert o.get_inner().get_x() == 1

[This ought to work identically if outer::get_inner() returned an inner*
instead of an inner&]

It seems rather obvious at first that this should work, but getting it to
work safely is problematic. The problem occurs in wrapping
outer::get_inner(): we need to return an object which acts just like a inner
instance created from Python via

   >>> i = inner()

but which contains only a reference to the inner embedded in outer instead
of a new inner instance. Although we have the technology to do this, it is
problematic because the lifetime of the inner instance is tied to the
lifetime of the outer instance:

   >>> o = outer()
   >>> i = o.get_inner()
   >>> del o            # its inner is gone, now, too
   >>> i.get_x()        # crash!!

The obvious fix for this case is to have i manage a reference-count on o. In
other words, the reference-count on o is incremented when get_inner()
returns, and it is decremented when the return value is finally destroyed.

It is tempting to think we should automatically manage a reference-count on
the "self" object whenever a function returning a reference is wrapped, but
this approach doesn't generalize well:

1. A member function may return a reference into one of its arguments:

   // unlikely but plausible example
   struct outer {
      inner& get_other_inner(outer& other)
        { return other.b; }
   };

2. Functions at namespace scope can also return references:

   inner& get_inner2(outer& x) { return x.get_inner(); }

So we need a way to tell Boost.Python how a function wrapper should manage
its return value. I suggest the following interface which I have already
discussed a bit with Ullrich and Ralf:

   outer_class_builder.def(boost::python::owned_by_arg(0),
                           &outer::get_inner, "get_inner");

Since the "self" parameter is implicitly passed as argument zero, this will
cause a reference count on the self parameter to be managed by the resulting
object.

This usage suggests a generalization wherein an option initial parameter to
def() could be used to indicate that a pointer return value should be
completely managed by Python, allowing us to conveniently wrap factory
functions as recently requested by Karl Bellve:

    inner* make_inner() { return new inner; }
    ...
    my_module_builder.def(boost::python::factory_function,
                          make_inner, "make_inner");

An Existing Bug
===============

How should we convert the return values of wrapped functions to python?

* For types that have an immutable analogue in Python (int, std::string) the
  answer is simple: values and const references should be converted to the
  corresponding Python type, but an attempt to wrap a function returning a
  nonconst reference should not compile (presumably, the intention of such a
  function is to eventually allow the caller to modify the referent).

* For class types with an accessible copy constructor that have been exposed
  to Python with a class_builder, we can convert returned values by copying
  them into a new extension class instance. The existing Boost.Python
  strategy is to convert const references the same way as values. This
  strategy has two problems:

  1. The referent is copied unneccessarily.

  2. It is not exactly equivalent to returning a value; even though the
     returned reference is const, another function is free to modify the
     value of the referent:

     struct inner2 {
        inner2() : x(0) {}
        void get_x() { return this->x; }
        int x;
     };

     struct outer2 {
        const inner2& get_inner() { return this->y; }
        void set_x(int x) { this->y.x = x; }
        inner y;
     };

     >>> o = outer2()
     >>> i = o.get_inner() # copies o.y into a new extension instance
     >>> o.set_x(1)        # affects o.y, but not i
     >>> i.get_x()         # expecting to see 1 here!
     0

  With the approach outlined above under "A Missing Feature", both of the
  above problems could be avoided. The downside is that existing code
  wrapping functions that return references would have to change to use the
  owned_by_arg(n) syntax. My opinion is that it's worth it, but I would like
  to hear if there are any objections.

* Boost.Python was intended to prevent the wrapping of functions returning
  non-const references to class types, for reasons already discussed. This
  is where the bug occurs.

  The mechanism that converts return values of wrapped functions to python
  is simple: the function's return value is passed to the overloaded
  to_python function, and the resulting PyObject* is passed on to
  Python. The problem with this strategy is that if you allow the conversion
  of values or const references to python, you also allow the conversion of
  non-const references. This occurs due to the ordinary C++ type conversion
  rules. In other words:

  PyObject* to_python(inner);
  PyObject* to_python(const outer&);
  void f(inner& i, outer& o)
  {
     PyObject* p1 = to_python(i); // if inner is copyable, this compiles!
     PyObject* p2 = to_python(o); // compiles no matter what!
  }

  As Charlie Barrows discovered in
  http://groups.yahoo.com/group/boost/message/10024, the result is that a
  returned non-const reference is silently treated just like a returned
  value or const reference (the referent is copied into a new object).

Possible Fixes
==============

The most conservative approach I can think of (short of not fixing the bug!)
goes like this:

  1. Change return-value handling so that to_python is passed an additional
     parameter of type boost::type<R>, where R is the return type of the
     wrapped function. By carefully generating only to_python overloads for
     the types we want to implicitly convert to python (i.e. without using
     owned_by_arg(n)), we can control which types get converted. This works
     because boost::type<R&> is not implicitly converted to boost::type<R>
     or boost::type<R const&>. We would then have a nice symmetry between
     the functions used for conversion: from_python(PyObject*, type<T>)
     to_python(const T&, type<T>)

        from_python(PyObject*, type<const T&>)
        to_python(const T&, type<const T&>)

  2. Retain the single-argument version of to_python() as a template
     function. Why? Because the availability of the single-argument
     to_python has already been exposed to users, who may be using it for a
     variety of things. It would look something like this:

     template <class T>
     PyObject* to_python(const T& x)
     {
       return to_python(x, boost::type<T>());
     }

     [Lots of gory detail omitted here, since we need additional
     metaprogramming to deal with noncopyable types as described in the
     declaration of python_extension_class_converters in
     boost/python/detail/extension_class.hpp]

  This fix breaks any user code that supplies customized single-argument
  to_python() functions, since the function return mechanism will now be
  searching for a two-argument version.

A more radical possibility would be to completely change the conversion
mechanism to use converter objects instead of functions:

  Ullrich proposed this approach months ago, and even coded up a sample
  implementation (though as I recall it requires partial specialization). It
  would allow us to stop using C++ exception-handling to deal with argument
  type errors and overload resolution. There are two disadvantages to using
  EH for this purpose:

    1. Using EH for overload resolution causes exceptions to be handled when
       there's really no error, potentially slowing down function
       dispatching.

    2. Some compilers (e.g. GCC 2.95.2) still have EH bugs which can cause
       leaks or, presumably, worse problems.

  On the other hand, I see several potential downsides:

    1. Complexity: Each converter object would have to contain
       suitably-aligned uninitialized storage for an instance of the type
       being converted from_python in addition to a pointer.

    2. Code size: It looks to me like it would generate a lot more inline
       code than the current approach, since we would have do deal with
       explicit conversion error checking in addition to any exceptions
       thrown by the wrapped function which we'd have to handle anyway.

    3. Incompatibility: as a more radical change, it would probably break
       more user code - customized from_python functions would no longer
       work either.

I'd like to get some feedback from the user and developer communities about
these approaches. If there are alternatives, I'd love to hear them.

Regards,
Dave