Deprecate collectXSemiFuture

Summary:
Migration from Future-returning executor-erasing collectX forms to
SemiFuture-returning forms, that are less risky in particular with coroutines.

Earlier changes added collectXSemiFuture and collectXUnsafe as a migration
path. We then migrated collectX callsites to collectXSemiFuture or
collectXUnsafe and switched the implementation of collectX to the SemiFuture
form.

This diff adds deprecation flag to collectXSemiFuture during the migration to
collectX and and removes from folly tests that are not specific to testing those
operations.

Reviewed By: yfeldblum

Differential Revision: D20841855

fbshipit-source-id: 30818711362c88c3296cfc976cd75fbd94e1c15c

folly/experimental/coro/test/CollectAllBenchmark.cpp

@@ -43,14 +43,6 @@ void collectAllFutureInline(size_t batchSize) {
   folly::collectAll(std::move(futures)).get();
 }
 
-void collectAllSemiFuture(size_t batchSize) {
-  std::vector<folly::SemiFuture<folly::Unit>> futures;
-  for (size_t i = 0; i < batchSize; ++i) {
-    futures.emplace_back(folly::via(&executor, [] { doWork(); }));
-  }
-  folly::collectAllSemiFuture(std::move(futures)).get();
-}
-
 folly::coro::Task<void> co_doWork() {
   co_await folly::coro::co_reschedule_on_current_executor;
   doWork();
@@ -91,12 +83,6 @@ BENCHMARK(collectAllFutureInline10000, iters) {
   }
 }
 
-BENCHMARK(collectAllSemiFuture10000, iters) {
-  for (size_t i = 0; i < iters; ++i) {
-    collectAllSemiFuture(10000);
-  }
-}
-
 BENCHMARK(collectAllCoro10000, iters) {
   for (size_t i = 0; i < iters; ++i) {
     collectAllCoro(10000);
@@ -121,12 +107,6 @@ BENCHMARK(collectAllFutureInline100, iters) {
   }
 }
 
-BENCHMARK(collectAllSemiFuture100, iters) {
-  for (size_t i = 0; i < iters; ++i) {
-    collectAllSemiFuture(100);
-  }
-}
-
 BENCHMARK(collectAllCoro100, iters) {
   for (size_t i = 0; i < iters; ++i) {
     collectAllCoro(100);

folly/fibers/test/FibersTest.cpp

@@ -1849,12 +1849,11 @@ void doubleBatchOuterDispatch(
           }
         }
 
-        folly::collectAllSemiFuture(
+        folly::collectAll(
             innerDispatchResultFutures.begin(),
             innerDispatchResultFutures.end())
-            .toUnsafeFuture()
-            .thenValue([&](std::vector<Try<std::vector<std::string>>>
-                               innerDispatchResults) {
+            .deferValue([&](std::vector<Try<std::vector<std::string>>>
+                                innerDispatchResults) {
               for (auto& unit : innerDispatchResults) {
                 for (auto& element : unit.value()) {
                   results.push_back(element);

folly/futures/Future-inl.h

@@ -871,7 +871,7 @@ SemiFuture<Unit> SemiFuture<T>::unit() && {
 
 template <typename T>
 SemiFuture<T> SemiFuture<T>::delayed(HighResDuration dur, Timekeeper* tk) && {
-  return collectAllSemiFuture(*this, futures::sleep(dur, tk))
+  return collectAll(*this, futures::sleep(dur, tk))
       .deferValue([](std::tuple<Try<T>, Try<Unit>> tup) {
         Try<T>& t = std::get<0>(tup);
         return makeFuture<T>(std::move(t));
@@ -1418,7 +1418,7 @@ collectAllSemiFuture(Fs&&... fs) {
 template <typename... Fs>
 Future<std::tuple<Try<typename remove_cvref_t<Fs>::value_type>...>>
 collectAllUnsafe(Fs&&... fs) {
-  return collectAllSemiFuture(std::forward<Fs>(fs)...).toUnsafeFuture();
+  return collectAll(std::forward<Fs>(fs)...).toUnsafeFuture();
 }
 
 // collectAll (iterator)
@@ -1604,7 +1604,8 @@ SemiFuture<std::tuple<typename remove_cvref_t<Fs>::value_type...>> collect(
 }
 
 template <typename... Fs>
-SemiFuture<std::tuple<typename remove_cvref_t<Fs>::value_type...>>
+[[deprecated("collectSemiFuture is deprecated and identical to plain collect. Please use collectAny instead.")]] SemiFuture<
+    std::tuple<typename remove_cvref_t<Fs>::value_type...>>
 collectSemiFuture(Fs&&... fs) {
   return collect(std::forward<Fs>(fs)...);
 }
@@ -1616,7 +1617,9 @@ Future<std::tuple<typename remove_cvref_t<Fs>::value_type...>> collectUnsafe(
 }
 
 template <class Collection>
-auto collectSemiFuture(Collection&& c)
+[[deprecated(
+    "collectSemiFuture is deprecated and identical to plain collect. Please use collectAny instead.")]] auto
+collectSemiFuture(Collection&& c)
     -> decltype(collectSemiFuture(c.begin(), c.end())) {
   return collectSemiFuture(c.begin(), c.end());
 }
@@ -1822,7 +1825,7 @@ Future<T> reduce(It first, It last, T&& initial, F&& func) {
       });
 
   for (++first; first != last; ++first) {
-    f = collectAllSemiFuture(f, *first).toUnsafeFuture().thenValue(
+    f = collectAllUnsafe(f, *first).thenValue(
         [sfunc](std::tuple<Try<T>, Try<ItT>>&& t) {
           return (*sfunc)(
               std::move(std::get<0>(t).value()),
@@ -2138,7 +2141,7 @@ SemiFuture<T>::within(HighResDuration dur, E e, Timekeeper* tk) && {
 template <class T>
 Future<T> Future<T>::delayed(HighResDuration dur, Timekeeper* tk) && {
   auto e = this->getExecutor();
-  return collectAllSemiFuture(*this, futures::sleep(dur, tk))
+  return collectAll(*this, futures::sleep(dur, tk))
       .via(e ? e : &InlineExecutor::instance())
       .thenValue([](std::tuple<Try<T>, Try<Unit>>&& tup) {
         return makeFuture<T>(std::get<0>(std::move(tup)));
@@ -2461,7 +2464,7 @@ struct TryEquals {
 
 template <class T>
 Future<bool> Future<T>::willEqual(Future<T>& f) {
-  return collectAllSemiFuture(*this, f).toUnsafeFuture().thenValue(
+  return collectAllUnsafe(*this, f).thenValue(
       [](const std::tuple<Try<T>, Try<T>>& t) {
         if (std::get<0>(t).hasValue() && std::get<1>(t).hasValue()) {
           return futures::detail::TryEquals<T>::equals(

folly/futures/Future.h

@@ -2372,13 +2372,16 @@ auto via(Executor::KeepAlive<>, Func&& func) -> Future<
   replaced with collectX(...).via(e) where e is a valid non-inline executor.
   */
 template <class InputIterator>
-SemiFuture<std::vector<
-    Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>
+[[deprecated("collectAllSemiFuture is deprecated and identical to plain collectAll. Please use collectAny instead.")]] SemiFuture<
+    std::vector<Try<
+        typename std::iterator_traits<InputIterator>::value_type::value_type>>>
 collectAllSemiFuture(InputIterator first, InputIterator last);
 
 /// Sugar for the most common case
 template <class Collection>
-auto collectAllSemiFuture(Collection&& c)
+[[deprecated(
+    "collectAllSemiFuture is deprecated and identical to plain collectAll. Please use collectAny instead.")]] auto
+collectAllSemiFuture(Collection&& c)
     -> decltype(collectAllSemiFuture(c.begin(), c.end())) {
   return collectAllSemiFuture(c.begin(), c.end());
 }
@@ -2411,7 +2414,8 @@ auto collectAll(Collection&& c) -> decltype(collectAll(c.begin(), c.end())) {
 /// is a SemiFuture<std::tuple<Try<T1>, Try<T2>, ...>>.
 /// The Futures are moved in, so your copies are invalid.
 template <typename... Fs>
-SemiFuture<std::tuple<Try<typename remove_cvref_t<Fs>::value_type>...>>
+[[deprecated("collectAllSemiFuture is deprecated and identical to plain collectAll. Please use collectAny instead.")]] SemiFuture<
+    std::tuple<Try<typename remove_cvref_t<Fs>::value_type>...>>
 collectAllSemiFuture(Fs&&... fs);
 
 // Unsafe variant of collectAll, see coment above for details. Returns
@@ -2477,10 +2481,13 @@ Future<std::pair<
     size_t,
     Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>
 collectAnyUnsafe(InputIterator first, InputIterator last);
+
 template <class InputIterator>
-SemiFuture<std::pair<
-    size_t,
-    Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>
+[[deprecated("collectAnySemiFuture is deprecated and identical to plain collectAny. Please use collectAny instead.")]] SemiFuture<
+    std::pair<
+        size_t,
+        Try<typename std::iterator_traits<
+            InputIterator>::value_type::value_type>>>
 collectAnySemiFuture(InputIterator first, InputIterator last);
 
 /// Sugar for the most common case
@@ -2496,7 +2503,9 @@ auto collectAnyUnsafe(Collection&& c)
   return collectAnyUnsafe(c.begin(), c.end());
 }
 template <class Collection>
-auto collectAnySemiFuture(Collection&& c)
+[[deprecated(
+    "collectAnySemiFuture is deprecated and identical to plain collectAny. Please use collectAny instead.")]] auto
+collectAnySemiFuture(Collection&& c)
     -> decltype(collectAnySemiFuture(c.begin(), c.end())) {
   return collectAnySemiFuture(c.begin(), c.end());
 }

folly/futures/test/CollectTest.cpp

@@ -97,8 +97,8 @@ TEST(Collect, collectAll) {
       futures.push_back(p.getFuture());
     }
 
-    auto allf = collectAllSemiFuture(futures).toUnsafeFuture().thenTry(
-        [](Try<std::vector<Try<Unit>>>&& ts) {
+    auto allf =
+        collectAllUnsafe(futures).thenTry([](Try<std::vector<Try<Unit>>>&& ts) {
           for (auto& f : ts.value()) {
             f.value();
           }
@@ -178,8 +178,8 @@ TEST(Collect, collectAllUnsafe) {
       futures.push_back(p.getFuture());
     }
 
-    auto allf = collectAllSemiFuture(futures).toUnsafeFuture().thenTry(
-        [](Try<std::vector<Try<Unit>>>&& ts) {
+    auto allf =
+        collectAllUnsafe(futures).thenTry([](Try<std::vector<Try<Unit>>>&& ts) {
           for (auto& f : ts.value()) {
             f.value();
           }
@@ -193,6 +193,8 @@ TEST(Collect, collectAllUnsafe) {
   }
 }
 
+FOLLY_PUSH_WARNING
+FOLLY_GNU_DISABLE_WARNING("-Wdeprecated-declarations")
 TEST(Collect, collectAllSemiFutureInline) {
   // inline future collection on same executor
   {
@@ -243,7 +245,7 @@ TEST(Collect, collectAllSemiFutureInline) {
     futures.emplace_back(makeFuture(42).via(&x1));
     futures.emplace_back(makeFuture(42).via(&x2));
 
-    auto allf = collectAllSemiFuture(futures).defer([](auto&&) {}).via(&x1);
+    auto allf = collectAll(futures).defer([](auto&&) {}).via(&x1);
     EXPECT_FALSE(allf.isReady());
     EXPECT_EQ(1, x1.run());
     EXPECT_FALSE(allf.isReady());
@@ -253,6 +255,7 @@ TEST(Collect, collectAllSemiFutureInline) {
     EXPECT_TRUE(allf.isReady());
   }
 }
+FOLLY_POP_WARNING
 
 TEST(Collect, collect) {
   // success case
@@ -417,6 +420,8 @@ TEST(Collect, collectNotDefaultConstructible) {
   }
 }
 
+FOLLY_PUSH_WARNING
+FOLLY_GNU_DISABLE_WARNING("-Wdeprecated-declarations")
 TEST(Collect, collectAny) {
   {
     std::vector<Promise<int>> promises(10);
@@ -538,6 +543,7 @@ TEST(Collect, collectAny) {
     EXPECT_TRUE(anyf.isReady());
   }
 }
+FOLLY_POP_WARNING
 
 TEST(Collect, collectAnyWithoutException) {
   auto& executor = folly::InlineExecutor::instance();
@@ -647,7 +653,7 @@ TEST(Collect, alreadyCompleted) {
       fs.push_back(makeFuture());
     }
 
-    collectAllSemiFuture(fs).toUnsafeFuture().thenValue(
+    collectAllUnsafe(fs).thenValue(
         [&](std::vector<Try<Unit>> ts) { EXPECT_EQ(fs.size(), ts.size()); });
   }
   {
@@ -892,8 +898,7 @@ TEST(Collect, collectAllVariadic) {
   Future<bool> fb = pb.getFuture();
   Future<int> fi = pi.getFuture();
   bool flag = false;
-  collectAllSemiFuture(std::move(fb), std::move(fi))
-      .toUnsafeFuture()
+  collectAllUnsafe(std::move(fb), std::move(fi))
       .thenValue([&](std::tuple<Try<bool>, Try<int>> tup) {
         flag = true;
         EXPECT_TRUE(std::get<0>(tup).hasValue());
@@ -933,14 +938,13 @@ TEST(Collect, collectAllVariadicReferences) {
   Future<bool> fb = pb.getFuture();
   Future<int> fi = pi.getFuture();
   bool flag = false;
-  collectAllSemiFuture(fb, fi).toUnsafeFuture().thenValue(
-      [&](std::tuple<Try<bool>, Try<int>> tup) {
-        flag = true;
-        EXPECT_TRUE(std::get<0>(tup).hasValue());
-        EXPECT_EQ(std::get<0>(tup).value(), true);
-        EXPECT_TRUE(std::get<1>(tup).hasValue());
-        EXPECT_EQ(std::get<1>(tup).value(), 42);
-      });
+  collectAllUnsafe(fb, fi).thenValue([&](std::tuple<Try<bool>, Try<int>> tup) {
+    flag = true;
+    EXPECT_TRUE(std::get<0>(tup).hasValue());
+    EXPECT_EQ(std::get<0>(tup).value(), true);
+    EXPECT_TRUE(std::get<1>(tup).hasValue());
+    EXPECT_EQ(std::get<1>(tup).value(), 42);
+  });
   pb.setValue(true);
   EXPECT_FALSE(flag);
   pi.setValue(42);
@@ -953,8 +957,7 @@ TEST(Collect, collectAllVariadicWithException) {
   Future<bool> fb = pb.getFuture();
   Future<int> fi = pi.getFuture();
   bool flag = false;
-  collectAllSemiFuture(std::move(fb), std::move(fi))
-      .toUnsafeFuture()
+  collectAllUnsafe(std::move(fb), std::move(fi))
       .thenValue([&](std::tuple<Try<bool>, Try<int>> tup) {
         flag = true;
         EXPECT_TRUE(std::get<0>(tup).hasValue());

folly/futures/test/SemiFutureTest.cpp

@@ -976,6 +976,8 @@ TEST(SemiFuture, semiFutureWithinNoValueReferenceWhenTimeOut) {
   std::move(f).get();
 }
 
+FOLLY_PUSH_WARNING
+FOLLY_GNU_DISABLE_WARNING("-Wdeprecated-declarations")
 TEST(SemiFuture, collectAllSemiFutureDeferredWork) {
   {
     Promise<int> promise1;
@@ -1050,7 +1052,10 @@ TEST(SemiFuture, collectAllSemiFutureDeferredWork) {
     EXPECT_TRUE(deferredDestroyed);
   }
 }
+FOLLY_POP_WARNING
 
+FOLLY_PUSH_WARNING
+FOLLY_GNU_DISABLE_WARNING("-Wdeprecated-declarations")
 TEST(SemiFuture, collectSemiFutureDeferredWork) {
   {
     Promise<int> promise1;
@@ -1125,6 +1130,7 @@ TEST(SemiFuture, collectSemiFutureDeferredWork) {
     EXPECT_TRUE(deferredDestroyed);
   }
 }
+FOLLY_POP_WARNING
 
 TEST(SemiFuture, collectNDeferredWork) {
   Promise<int> promise1;

folly/futures/test/WaitTest.cpp

@@ -40,7 +40,7 @@ TEST(Wait, waitImmediate) {
   vector<Future<Unit>> v_f;
   v_f.push_back(makeFuture());
   v_f.push_back(makeFuture());
-  auto done_v_f = collectAllSemiFuture(v_f).toUnsafeFuture().wait().value();
+  auto done_v_f = collectAll(v_f).wait().value();
   EXPECT_EQ(2, done_v_f.size());
 
   vector<Future<bool>> v_fb;