R-value qualify Future<T>::then and deprecate lvalue.

Summary:
Overall plan to modify Future<T>::then to be r-value qualified and use Future<T>::thenTry or Future<T>::thenValue.

The goal is to disambiguate folly::Future and to improve type and lifetime safety of Future and its methods.

 * Deprecate l-value qualified calls to Future<T>::then.
 * Make appropriate changes to folly to make this correct without local deprecation warnings.

Reviewed By: yfeldblum

Differential Revision: D9175807

fbshipit-source-id: 9dc9812270ca3fb800f3a56d3aef8ef3d1d209e6

folly/futures/Future-inl.h

@@ -1018,9 +1018,10 @@ template <class F>
 typename std::
     enable_if<isFuture<F>::value, Future<typename isFuture<T>::Inner>>::type
     Future<T>::unwrap() {
-  return then([](Future<typename isFuture<T>::Inner> internal_future) {
-    return internal_future;
-  });
+  return std::move(*this).then(
+      [](Future<typename isFuture<T>::Inner> internal_future) {
+        return internal_future;
+      });
 }
 
 template <class T>
@@ -1063,13 +1064,14 @@ Future<T> Future<T>::via(Executor* executor, int8_t priority) & {
 
 template <typename T>
 template <typename R, typename Caller, typename... Args>
-  Future<typename isFuture<R>::Inner>
-Future<T>::then(R(Caller::*func)(Args...), Caller *instance) {
+Future<typename isFuture<R>::Inner> Future<T>::then(
+    R (Caller::*func)(Args...),
+    Caller* instance) && {
   typedef typename std::remove_cv<typename std::remove_reference<
       typename futures::detail::ArgType<Args...>::FirstArg>::type>::type
       FirstArg;
 
-  return then([instance, func](Try<T>&& t){
+  return std::move(*this).thenTry([instance, func](Try<T>&& t) {
     return (instance->*func)(t.template get<isTry<FirstArg>::value, Args>()...);
   });
 }
@@ -1126,7 +1128,7 @@ Future<T> Future<T>::thenError(F&& func) && {
 
 template <class T>
 Future<Unit> Future<T>::then() {
-  return then([]() {});
+  return std::move(*this).then([]() {});
 }
 
 // onError where the callback returns T
@@ -1209,10 +1211,11 @@ Future<T>::onError(F&& func) {
 template <class T>
 template <class F>
 Future<T> Future<T>::ensure(F&& func) {
-  return this->then([funcw = std::forward<F>(func)](Try<T>&& t) mutable {
-    std::forward<F>(funcw)();
-    return makeFuture(std::move(t));
-  });
+  return std::move(*this).then(
+      [funcw = std::forward<F>(func)](Try<T>&& t) mutable {
+        std::forward<F>(funcw)();
+        return makeFuture(std::move(t));
+      });
 }
 
 template <class T>
@@ -1741,7 +1744,7 @@ Future<T> reduce(It first, It last, T&& initial, F&& func) {
 
   auto sfunc = std::make_shared<std::decay_t<F>>(std::forward<F>(func));
 
-  auto f = first->then(
+  auto f = std::move(*first).then(
       [initial = std::forward<T>(initial), sfunc](Try<ItT>&& head) mutable {
         return (*sfunc)(
             std::move(initial), head.template get<IsTry::value, Arg&&>());
@@ -1833,14 +1836,15 @@ window(Executor* executor, Collection input, F func, size_t n) {
 template <class T>
 template <class I, class F>
 Future<I> Future<T>::reduce(I&& initial, F&& func) {
-  return then([minitial = std::forward<I>(initial),
-               mfunc = std::forward<F>(func)](T&& vals) mutable {
-    auto ret = std::move(minitial);
-    for (auto& val : vals) {
-      ret = mfunc(std::move(ret), std::move(val));
-    }
-    return ret;
-  });
+  return std::move(*this).then(
+      [minitial = std::forward<I>(initial),
+       mfunc = std::forward<F>(func)](T&& vals) mutable {
+        auto ret = std::move(minitial);
+        for (auto& val : vals) {
+          ret = mfunc(std::move(ret), std::move(val));
+        }
+        return ret;
+      });
 }
 
 // unorderedReduce (iterator)
@@ -2263,7 +2267,7 @@ Future<bool> Future<T>::willEqual(Future<T>& f) {
 template <class T>
 template <class F>
 Future<T> Future<T>::filter(F&& predicate) {
-  return this->then([p = std::forward<F>(predicate)](T val) {
+  return std::move(*this).then([p = std::forward<F>(predicate)](T val) {
     T const& valConstRef = val;
     if (!p(valConstRef)) {
       throw_exception<FuturePredicateDoesNotObtain>();
@@ -2281,10 +2285,8 @@ template <class P, class F>
 Future<Unit> whileDo(P&& predicate, F&& thunk) {
   if (predicate()) {
     auto future = thunk();
-    return future.then([
-      predicate = std::forward<P>(predicate),
-      thunk = std::forward<F>(thunk)
-    ]() mutable {
+    return std::move(future).then([predicate = std::forward<P>(predicate),
+                                   thunk = std::forward<F>(thunk)]() mutable {
       return whileDo(std::forward<P>(predicate), std::forward<F>(thunk));
     });
   }
@@ -2305,7 +2307,7 @@ template <class It, class F, class ItT, class Result>
 std::vector<Future<Result>> map(It first, It last, F func) {
   std::vector<Future<Result>> results;
   for (auto it = first; it != last; it++) {
-    results.push_back(it->then(func));
+    results.push_back(std::move(*it).then(func));
   }
   return results;
 }

folly/futures/Future.h

@@ -1147,11 +1147,18 @@ class Future : private futures::detail::FutureBase<T> {
   ///   i.e., as if `*this` was moved into RESULT.
   /// - `RESULT.valid() == true`
   template <typename F, typename R = futures::detail::callableResult<T, F>>
-  typename R::Return then(F&& func) {
+  typename R::Return then(F&& func) && {
     return this->template thenImplementation<F, R>(
         std::forward<F>(func), typename R::Arg());
   }
 
+  template <typename F, typename R = futures::detail::callableResult<T, F>>
+  [[deprecated("must be rvalue-qualified, e.g., std::move(future).then(...)")]]
+      typename R::Return
+      then(F&& func) & {
+    return std::move(*this).then(std::forward<F>(func));
+  }
+
   /// Variant where func is an member function
   ///
   ///   struct Worker { R doWork(Try<T>); }
@@ -1175,7 +1182,15 @@ class Future : private futures::detail::FutureBase<T> {
   template <typename R, typename Caller, typename... Args>
   Future<typename isFuture<R>::Inner> then(
       R (Caller::*func)(Args...),
-      Caller* instance);
+      Caller* instance) &&;
+
+  template <typename R, typename Caller, typename... Args>
+  [[deprecated(
+      "must be rvalue-qualified, e.g., std::move(future).then(...)")]] Future<typename isFuture<R>::
+                                                                                  Inner>
+  then(R (Caller::*func)(Args...), Caller* instance) & {
+    return std::move(*this).then(func, instance);
+  }
 
   /// Execute the callback via the given Executor. The executor doesn't stick.
   ///
@@ -1200,13 +1215,21 @@ class Future : private futures::detail::FutureBase<T> {
   ///   i.e., as if `*this` was moved into RESULT.
   /// - `RESULT.valid() == true`
   template <class Executor, class Arg, class... Args>
-  auto then(Executor* x, Arg&& arg, Args&&... args) {
+  auto then(Executor* x, Arg&& arg, Args&&... args) && {
     auto oldX = this->getExecutor();
     this->setExecutor(x);
-    return this->then(std::forward<Arg>(arg), std::forward<Args>(args)...)
+    return std::move(*this)
+        .then(std::forward<Arg>(arg), std::forward<Args>(args)...)
         .via(oldX);
   }
 
+  template <class Executor, class Arg, class... Args>
+  [[deprecated(
+      "must be rvalue-qualified, e.g., std::move(future).then(...)")]] auto
+  then(Executor* x, Arg&& arg, Args&&... args) & {
+    return std::move(*this).then(x, arg, args...);
+  }
+
   /// When this Future has completed, execute func which is a function that
   /// can be called with `Try<T>&&` (often a lambda with parameter type
   /// `auto&&` or `auto`).
@@ -1368,7 +1391,7 @@ class Future : private futures::detail::FutureBase<T> {
   ///   i.e., as if `*this` was moved into RESULT.
   /// - `RESULT.valid() == true`
   Future<Unit> unit() {
-    return then();
+    return std::move(*this).then();
   }
 
   /// Set an error continuation for this Future. The continuation should take an
@@ -1765,7 +1788,8 @@ class Future : private futures::detail::FutureBase<T> {
   template <class Callback, class... Callbacks>
   auto thenMulti(Callback&& fn, Callbacks&&... fns) {
     // thenMulti with two callbacks is just then(a).thenMulti(b, ...)
-    return then(std::forward<Callback>(fn))
+    return std::move(*this)
+        .then(std::forward<Callback>(fn))
         .thenMulti(std::forward<Callbacks>(fns)...);
   }
 
@@ -1783,7 +1807,7 @@ class Future : private futures::detail::FutureBase<T> {
   template <class Callback>
   auto thenMulti(Callback&& fn) {
     // thenMulti with one callback is just a then
-    return then(std::forward<Callback>(fn));
+    return std::move(*this).then(std::forward<Callback>(fn));
   }
 
   /// Create a Future chain from a sequence of callbacks. i.e.
@@ -1814,7 +1838,8 @@ class Future : private futures::detail::FutureBase<T> {
     // via(x).then(a).thenMulti(b, ...).via(oldX)
     auto oldX = this->getExecutor();
     this->setExecutor(x);
-    return then(std::forward<Callback>(fn))
+    return std::move(*this)
+        .then(std::forward<Callback>(fn))
         .thenMulti(std::forward<Callbacks>(fns)...)
         .via(oldX);
   }
@@ -1822,7 +1847,7 @@ class Future : private futures::detail::FutureBase<T> {
   template <class Callback>
   auto thenMultiWithExecutor(Executor* x, Callback&& fn) {
     // thenMulti with one callback is just a then with an executor
-    return then(x, std::forward<Callback>(fn));
+    return std::move(*this).then(x, std::forward<Callback>(fn));
   }
 
   /// Moves-out `*this`, creating/returning a corresponding SemiFuture.

folly/futures/FutureSplitter.h

@@ -53,7 +53,7 @@ class FutureSplitter {
       : promise_(std::make_shared<SharedPromise<T>>()),
         e_(getExecutorFrom(future)),
         priority_(future.getPriority()) {
-    future.then([promise = promise_](Try<T>&& theTry) {
+    std::move(future).then([promise = promise_](Try<T>&& theTry) {
       promise->setTry(std::move(theTry));
     });
   }

folly/futures/Retrying.h

@@ -83,31 +83,31 @@ template <class Policy, class FF, class Prom>
 void retryingImpl(size_t k, Policy&& p, FF&& ff, Prom prom) {
   using F = invoke_result_t<FF, size_t>;
   using T = typename F::value_type;
-  auto f = makeFutureWith([&] { return ff(k++); });
-  f.then([k,
-          prom = std::move(prom),
-          pm = std::forward<Policy>(p),
-          ffm = std::forward<FF>(ff)](Try<T>&& t) mutable {
-    if (t.hasValue()) {
-      prom.setValue(std::move(t).value());
-      return;
-    }
-    auto& x = t.exception();
-    auto q = makeFutureWith([&] { return pm(k, x); });
-    q.then([k,
-            prom = std::move(prom),
-            xm = std::move(x),
-            pm = std::move(pm),
-            ffm = std::move(ffm)](Try<bool> shouldRetry) mutable {
-      if (shouldRetry.hasValue() && shouldRetry.value()) {
-        retryingImpl(k, std::move(pm), std::move(ffm), std::move(prom));
-      } else if (shouldRetry.hasValue()) {
-        prom.setException(std::move(xm));
-      } else {
-        prom.setException(std::move(shouldRetry.exception()));
-      }
-    });
-  });
+  makeFutureWith([&] { return ff(k++); })
+      .then([k,
+             prom = std::move(prom),
+             pm = std::forward<Policy>(p),
+             ffm = std::forward<FF>(ff)](Try<T>&& t) mutable {
+        if (t.hasValue()) {
+          prom.setValue(std::move(t).value());
+          return;
+        }
+        auto& x = t.exception();
+        makeFutureWith([&] { return pm(k, x); })
+            .then([k,
+                   prom = std::move(prom),
+                   xm = std::move(x),
+                   pm = std::move(pm),
+                   ffm = std::move(ffm)](Try<bool> shouldRetry) mutable {
+              if (shouldRetry.hasValue() && shouldRetry.value()) {
+                retryingImpl(k, std::move(pm), std::move(ffm), std::move(prom));
+              } else if (shouldRetry.hasValue()) {
+                prom.setException(std::move(xm));
+              } else {
+                prom.setException(std::move(shouldRetry.exception()));
+              }
+            });
+      });
 }
 
 template <class Policy, class FF>

folly/futures/test/FutureTest.cpp

@@ -240,8 +240,8 @@ TEST(Future, hasPreconditionValid) {
   DOIT(f.hasException());
   DOIT(f.value());
   DOIT(f.poll());
-  DOIT(f.then());
-  DOIT(f.then([](auto&&) {}));
+  DOIT(std::move(f).then());
+  DOIT(std::move(f).then([](auto&&) {}));
 
 #undef DOIT
 }
@@ -1263,10 +1263,10 @@ TEST(Future, unwrap) {
   bool flag2 = false;
 
   // do a, then do b, and get the result of a + b.
-  Future<int> f = fa.then([&](Try<int>&& ta) {
+  Future<int> f = std::move(fa).then([&](Try<int>&& ta) {
     auto va = ta.value();
     flag1 = true;
-    return fb.then([va, &flag2](Try<int>&& tb) {
+    return std::move(fb).then([va, &flag2](Try<int>&& tb) {
       flag2 = true;
       return va + tb.value();
     });
@@ -1357,7 +1357,7 @@ TEST(Future, CircularDependencySharedPtrSelfReset) {
   Promise<int64_t> promise;
   auto ptr = std::make_shared<Future<int64_t>>(promise.getFuture());
 
-  ptr->then([ptr](folly::Try<int64_t>&& /* uid */) mutable {
+  std::move(*ptr).thenTry([ptr](folly::Try<int64_t>&& /* uid */) mutable {
     EXPECT_EQ(1, ptr.use_count());
 
     // Leaving no references to ourselves.

folly/futures/test/ThenTest.cpp

@@ -189,7 +189,7 @@ TEST(Then, constValue) {
 TEST(Then, objectAliveDuringImmediateNoParamContinuation) {
   auto f = makeFuture<CountedWidget>(23);
   auto called = false;
-  f.then([&] {
+  std::move(f).then([&] {
     EXPECT_EQ(CountedWidget::instances_.size(), 1u);
     EXPECT_EQ(CountedWidget::instances_[0]->v_, 23);
     called = true;

folly/futures/test/ViaTest.cpp

@@ -617,7 +617,7 @@ TEST(ViaFunc, isSticky) {
   auto f = via(&x, [&]{ count++; });
   x.run();
 
-  f.then([&]{ count++; });
+  std::move(f).then([&] { count++; });
   EXPECT_EQ(1, count);
   x.run();
   EXPECT_EQ(2, count);

folly/futures/test/WaitTest.cpp

@@ -57,16 +57,16 @@ TEST(Wait, wait) {
   std::atomic<int> result{1};
   std::atomic<std::thread::id> id;
 
-  std::thread th([&](Future<int>&& tf){
-      auto n = tf.then([&](Try<int> && t) {
+  std::thread th(
+      [&](Future<int>&& tf) {
+        auto n = std::move(tf).thenTry([&](Try<int>&& t) {
           id = std::this_thread::get_id();
           return t.value();
         });
-      flag = true;
-      result.store(n.wait().value());
-    },
-    std::move(f)
-    );
+        flag = true;
+        result.store(n.wait().value());
+      },
+      std::move(f));
   while(!flag){}
   EXPECT_EQ(result.load(), 1);
   p.setValue(42);
@@ -332,7 +332,8 @@ TEST(Wait, WaitPlusThen) {
     EXPECT_EQ(f.value(), 42);
     f.wait();
     auto continuation = 0;
-    EXPECT_NO_THROW(f.then([&](auto&& v) { continuation = v; }));
+    EXPECT_NO_THROW(
+        std::move(f).thenValue([&](auto&& v) { continuation = v; }));
     EXPECT_EQ(continuation, 42);
   }
 
@@ -354,7 +355,8 @@ TEST(Wait, WaitPlusThen) {
     f.wait();
     EXPECT_TRUE(f.isReady());
     auto continuation = 0;
-    EXPECT_NO_THROW(f.then([&](auto&& v) { continuation = v; }));
+    EXPECT_NO_THROW(
+        std::move(f).thenValue([&](auto&& v) { continuation = v; }));
     EXPECT_EQ(continuation, 42);
     t.join();
   }
@@ -369,7 +371,8 @@ TEST(Wait, WaitPlusThen) {
     EXPECT_EQ(f.value(), 42);
     f.wait(std::chrono::seconds(10));
     auto continuation = 0;
-    EXPECT_NO_THROW(f.then([&](auto&& v) { continuation = v; }));
+    EXPECT_NO_THROW(
+        std::move(f).thenValue([&](auto&& v) { continuation = v; }));
     EXPECT_EQ(continuation, 42);
   }
 
@@ -391,7 +394,8 @@ TEST(Wait, WaitPlusThen) {
     f.wait(std::chrono::seconds(10));
     EXPECT_TRUE(f.isReady()); // deterministically passes in practice
     auto continuation = 0;
-    EXPECT_NO_THROW(f.then([&](auto&& v) { continuation = v; }));
+    EXPECT_NO_THROW(
+        std::move(f).thenValue([&](auto&& v) { continuation = v; }));
     EXPECT_EQ(continuation, 42);
     t.join();
   }
@@ -403,7 +407,8 @@ TEST(Wait, WaitPlusThen) {
     auto f = p.getFuture();
     f.wait(milliseconds(1));
     auto continuation = 0;
-    EXPECT_NO_THROW(f.then([&](auto&& v) { continuation = v; }));
+    EXPECT_NO_THROW(
+        std::move(f).thenValue([&](auto&& v) { continuation = v; }));
     EXPECT_EQ(continuation, 0);
   }
 
@@ -419,7 +424,8 @@ TEST(Wait, WaitPlusThen) {
     auto continuation = 0;
     InlineExecutor e;
     auto f2 = std::move(f).via(&e);
-    EXPECT_NO_THROW(f2.then([&](auto&& v) { continuation = v; }));
+    EXPECT_NO_THROW(
+        std::move(f2).thenValue([&](auto&& v) { continuation = v; }));
     EXPECT_EQ(continuation, 42);
   }
 
@@ -443,7 +449,8 @@ TEST(Wait, WaitPlusThen) {
     auto continuation = 0;
     InlineExecutor e;
     auto f2 = std::move(f).via(&e);
-    EXPECT_NO_THROW(f2.then([&](auto&& v) { continuation = v; }));
+    EXPECT_NO_THROW(
+        std::move(f2).thenValue([&](auto&& v) { continuation = v; }));
     EXPECT_EQ(continuation, 42);
     t.join();
   }
@@ -460,7 +467,8 @@ TEST(Wait, WaitPlusThen) {
     auto continuation = 0;
     InlineExecutor e;
     auto f2 = std::move(f).via(&e);
-    EXPECT_NO_THROW(f2.then([&](auto&& v) { continuation = v; }));
+    EXPECT_NO_THROW(
+        std::move(f2).thenValue([&](auto&& v) { continuation = v; }));
     EXPECT_EQ(continuation, 42);
   }
 
@@ -484,7 +492,8 @@ TEST(Wait, WaitPlusThen) {
     auto continuation = 0;
     InlineExecutor e;
     auto f2 = std::move(f).via(&e);
-    EXPECT_NO_THROW(f2.then([&](auto&& v) { continuation = v; }));
+    EXPECT_NO_THROW(
+        std::move(f2).thenValue([&](auto&& v) { continuation = v; }));
     EXPECT_EQ(continuation, 42);
     t.join();
   }
@@ -498,7 +507,8 @@ TEST(Wait, WaitPlusThen) {
     auto continuation = 0;
     InlineExecutor e;
     auto f2 = std::move(f).via(&e);
-    EXPECT_NO_THROW(f2.then([&](auto&& v) { continuation = v; }));
+    EXPECT_NO_THROW(
+        std::move(f2).thenValue([&](auto&& v) { continuation = v; }));
     EXPECT_EQ(continuation, 0);
     p.setValue(42);
     EXPECT_EQ(continuation, 42);