Clarify type requirements on wrapped types in future and promise

[Dan Bonachea]

The current spec requirements for the embedded types in future<T...> and promise<T...> appear to be quite weak. The only relevant text appears to be:

The types in T... must not be void.

The spec doesn't say anything about whether T... need to be complete types, or anything about the C++ concepts they must provide.

I think the following questions highlight some ambiguities:

1. Do the future and promise wrapper types require the "wrapped" types to be complete types at the location where the wrapper type is "uttered"?
   1. What if a wrapped type is a pointer to an incomplete type?
   2. What if a wrapped type is a reference to an incomplete type?
2. Do the wrapped types need to provide any C++ concepts?
   1. DefaultConstructible, MoveConstructible and TriviallyCopyable all come to mind as properties the implementation might rely upon, either accidentally or by necessity.
   2. Note that in particular, none of these wrapper types are actually useful if missing Concepts in T prevent invocation of promise<T>::fulfill_result(T&& result) and make_future(T) to "wrap" values and the various methods that return or pass T's to "unwrap" values.

As a motivating example of some of these issues, consider the code below. The code outside #ifdefs type-checks, but activating any of the #ifdef's causes the code to skirt one of the ambiguities above and leads to type errors (in some cases, rather spectacular multi-page type errors).

I think we should eventually clarify the type requirements in the spec, and possibly add static_asserts in the implementation to provide more useful error messages when they are violated.

The current implementation status seems to be the wrapped types must be complete types or pointers/references to possibly incomplete types. The types need not be DefaultConstructible, but I'm not sure what the construction/assignment requirements are - it seems like CopyConstructible may be required, but perhaps @john bachan can clarify?

#include <upcxx/upcxx.hpp>

using namespace upcxx;

struct U; // U is an incomplete type

struct T {

  future<U&> get_future_Uref() {
    future<U&> r; // wrap a reference to incomplete type
    r = make_future<U&>(*this->up);
    return r;
  }

  future<U*> get_future_Uptr() {
    future<U*> r; // wrap a pointer to incomplete type
    r = make_future<U*>(this->up);
    return r;
  }

  future<U> get_future_U1();

#if USE_INC
  future<U> get_future_U2() {
    return get_future_U1(); // odr-use a wrapped incomplete type
  }
#endif

#if WRAP_INC
  void foo() {
    make_future<U>(*this->up); // wrap an incomplete type
  }
#endif

  T(int _x) : x(_x) {}

#if NO_CTOR
  T(T&&) = delete;  // not MoveConstructible
  T(const T&) = delete;   // not CopyConstructible
#endif

#if NO_ASSIGN // this actually works now
  T(T&&) {} // MoveConstructible
  T& operator=(const T&) = delete; // not CopyAssignable
  T& operator=(T&&) = delete; // not MoveAssignable
#endif

  T() = delete;  // not DefaultConstructible

  int x;
  U *up;
};


int main() {
  upcxx::init();

  T t(4);
  promise<T> p;
  p.fulfill_result(std::move(t));

  upcxx::finalize();
  return 0;
}

[Dan Bonachea]

At the 2019-08-28 meeting, we resolved to defer this issue, pending further investigation.

[Dan Bonachea]

This was discussed in the 2020-02-12 meeting.

It was re-assigned to John, who promised to investigate the implementation and provide a list of the type requirements for us to spec.

[Dan Bonachea]

Rollover issue to next release milestone

[Dan Bonachea]

The following test which I've just added to the repo as spec-issue144b.cpp demonstrates that futures CAN contain non-copyable types, as of upcxx-2020.3.7-2 b49d102. The same has been true since at least v2020.3.0.

#include <upcxx/upcxx.hpp>
#include <iostream>
#include <memory>

struct A {
  int x;
  A(int v) : x(v) {}
  A(A &&other) : x(other.x){} // move cons
  A(const A &) = delete; // non-copyable
  UPCXX_SERIALIZED_VALUES(x);
};

using namespace upcxx;

int main() {
  upcxx::init();

  // demonstrates that future can contain a non-copyable type (std::unique_ptr)
  future<std::unique_ptr<int>> boof;
  {
    std::unique_ptr<int> up(new int(17));
    boof = make_future(std::move(up)); // trivially ready future
  }
  assert(boof.ready());
  assert(*boof.result_reference() == 17);
  {
    promise<std::unique_ptr<int>> p;
    boof = p.get_future(); // real non-ready future
    assert(!boof.ready());
    std::unique_ptr<int> up(new int(42));
    p.fulfill_result(std::move(up));
  }
  assert(boof.ready());
  assert(*boof.result_reference() == 42);

  // demonstrate with a custom non-copyable type
  future<A> fa;
  { 
    fa = make_future(A(17)); // trivially ready future
  }
  assert(fa.result_reference().x == 17);
  {
    promise<A> p;
    fa = p.get_future(); // real non-ready future
    assert(!fa.ready());
    p.fulfill_result(A(16));
  }
  assert(fa.result_reference().x == 16);

  upcxx::barrier();
  if (!upcxx::rank_me()) { std::cout << "SUCCESS" << std::endl; }

  upcxx::finalize();
  return 0;
}

[Dan Bonachea]

I think PR #50 resolves most of the questions raised here, where instead of mandating a type-wide set of requirements on future and promise we've applied requirements on a per-function basis.

I think the only remaining clarification we may want to make is to augment the future/promise boilerplate sentence with something about requiring complete types.

Here is some analogous text from std::vector:

The requirements that are imposed on the elements depend on the actual operations performed on the container. Generally, it is required that element type is a complete type and meets the requirements of Erasable, but many member functions impose stricter requirements.

We could possibly change the existing future/promise boilerplate text:

The types in T... must not be void.

To say something like:

The types in T... must be complete types (and not void). Several member functions impose additional stricter requirements on T...

[Amir Kamil]

Resolved by PR #50.