Properly constrain folly::Function's generic conversion constructor and fix...

Properly constrain folly::Function's generic conversion constructor and fix its noexcept specification

Summary:
The generic conversion constructor for `folly::Function` was not checking that the source object could successfully be copy/move constructed, leading to some `is_constructible` false positives.

Also, the `noexcept` specification on the `Function` constructor wasn't taking into account that the source object might be copied into the Function, instead of moved. The copy could throw.

Reviewed By: yfeldblum

Differential Revision: D4775037

fbshipit-source-id: f337b41bf9ac431baa9457a501e63c18ca099e57

folly/Function.h

 /*
- * Copyright 2017 Facebook, Inc.
- *
- * @author Eric Niebler (eniebler@fb.com), Sven Over (over@fb.com)
+ * Copyright 2017-present Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,7 +12,9 @@
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
- *
+ */
+/*
+ * @author Eric Niebler (eniebler@fb.com), Sven Over (over@fb.com)
  * Acknowledgements: Giuseppe Ottaviano (ott@fb.com)
  */
 
@@ -225,6 +225,7 @@
 
 #include <folly/CppAttributes.h>
 #include <folly/Portability.h>
+#include <folly/Traits.h>
 
 namespace folly {
 
@@ -246,14 +247,22 @@ union Data {
 };
 
 template <typename Fun, typename FunT = typename std::decay<Fun>::type>
-using IsSmall = std::integral_constant<
-    bool,
-    (sizeof(FunT) <= sizeof(Data::tiny) &&
-     // Same as is_nothrow_move_constructible, but w/ no template instantiation.
-     noexcept(FunT(std::declval<FunT&&>())))>;
+using IsSmall = Conjunction<
+    std::integral_constant<bool, (sizeof(FunT) <= sizeof(Data::tiny))>,
+    std::is_nothrow_move_constructible<FunT>>;
 using SmallTag = std::true_type;
 using HeapTag = std::false_type;
 
+template <class T>
+struct NotFunction : std::true_type {};
+template <class T>
+struct NotFunction<Function<T>> : std::false_type {};
+
+template <typename Fun, typename FunT = typename std::decay<Fun>::type>
+using DecayIfConstructible = typename std::enable_if<
+    Conjunction<NotFunction<FunT>, std::is_constructible<FunT, Fun>>::value,
+    FunT>::type;
+
 struct CoerceTag {};
 
 template <typename T>
@@ -351,7 +360,7 @@ struct FunctionTraits<ReturnType(Args...) const> {
     return fn.call_(fn.data_, static_cast<Args&&>(args)...);
   }
 
-  struct SharedProxy {
+  class SharedProxy {
     std::shared_ptr<Function<ReturnType(Args...) const>> sp_;
 
    public:
@@ -436,8 +445,6 @@ class Function final : private detail::function::FunctionTraits<FunctionType> {
   template <typename Fun>
   using IsSmall = detail::function::IsSmall<Fun>;
 
-  using OtherSignature = typename Traits::OtherSignature;
-
   // The `data_` member is mutable to allow `constCastFunction` to work without
   // invoking undefined behavior. Const-correctness is only violated when
   // `FunctionType` is a const function type (e.g., `int() const`) and `*this`
@@ -449,7 +456,7 @@ class Function final : private detail::function::FunctionTraits<FunctionType> {
   friend Traits;
   friend Function<typename Traits::ConstSignature> folly::constCastFunction<>(
       Function<typename Traits::NonConstSignature>&&) noexcept;
-  friend class Function<OtherSignature>;
+  friend class Function<typename Traits::OtherSignature>;
 
   template <typename Fun>
   Function(Fun&& fun, SmallTag) noexcept {
@@ -469,7 +476,13 @@ class Function final : private detail::function::FunctionTraits<FunctionType> {
     exec_ = &detail::function::execBig<FunT>;
   }
 
-  Function(Function<OtherSignature>&& that, CoerceTag) noexcept {
+  template <typename Signature>
+  Function(Function<Signature>&& that, CoerceTag)
+      : Function(static_cast<Function<Signature>&&>(that), HeapTag{}) {}
+
+  Function(
+      Function<typename Traits::OtherSignature>&& that,
+      CoerceTag) noexcept {
     that.exec_(Op::MOVE, &that.data_, &data_);
     std::swap(call_, that.call_);
     std::swap(exec_, that.exec_);
@@ -482,13 +495,7 @@ class Function final : private detail::function::FunctionTraits<FunctionType> {
   Function() = default;
 
   // not copyable
-  // NOTE: Deleting the non-const copy constructor is unusual but necessary to
-  // prevent copies from non-const `Function` object from selecting the
-  // perfect forwarding implicit converting constructor below
-  // (i.e., `template <typename Fun> Function(Fun&&)`).
-  Function(Function&) = delete;
   Function(const Function&) = delete;
-  Function(const Function&&) = delete;
 
   /**
    * Move constructor
@@ -505,32 +512,46 @@ class Function final : private detail::function::FunctionTraits<FunctionType> {
   /* implicit */ Function(std::nullptr_t) noexcept {}
 
   /**
-   * Constructs a new `Function` from any callable object. This
-   * handles function pointers, pointers to static member functions,
-   * `std::reference_wrapper` objects, `std::function` objects, and arbitrary
-   * objects that implement `operator()` if the parameter signature
-   * matches (i.e. it returns R when called with Args...).
-   * For a `Function` with a const function type, the object must be
-   * callable from a const-reference, i.e. implement `operator() const`.
-   * For a `Function` with a non-const function type, the object will
-   * be called from a non-const reference, which means that it will execute
-   * a non-const `operator()` if it is defined, and falls back to
-   * `operator() const` otherwise.
+   * Constructs a new `Function` from any callable object that is _not_ a
+   * `folly::Function`. This handles function pointers, pointers to static
+   * member functions, `std::reference_wrapper` objects, `std::function`
+   * objects, and arbitrary objects that implement `operator()` if the parameter
+   * signature matches (i.e. it returns an object convertible to `R` when called
+   * with `Args...`).
    *
    * \note `typename = ResultOf<Fun>` prevents this overload from being
    * selected by overload resolution when `fun` is not a compatible function.
+   *
+   * \note The noexcept requires some explanation. IsSmall is true when the
+   * decayed type fits within the internal buffer and is noexcept-movable. But
+   * this ctor might copy, not move. What we need here, if this ctor does a
+   * copy, is that this ctor be noexcept when the copy is noexcept. That is not
+   * checked in IsSmall, and shouldn't be, because once the Function is
+   * constructed, the contained object is never copied. This check is for this
+   * ctor only, in the case that this ctor does a copy.
    */
-  template <class Fun, typename = typename Traits::template ResultOf<Fun>>
-  /* implicit */ Function(Fun&& fun) noexcept(IsSmall<Fun>::value)
+  template <
+      typename Fun,
+      typename FunT = detail::function::DecayIfConstructible<Fun>,
+      typename = typename Traits::template ResultOf<Fun>>
+  /* implicit */ Function(Fun&& fun) noexcept(
+      IsSmall<Fun>::value && noexcept(FunT(std::declval<Fun>())))
       : Function(static_cast<Fun&&>(fun), IsSmall<Fun>{}) {}
 
   /**
-   * For moving a `Function<X(Ys..) const>` into a `Function<X(Ys...)>`.
+   * For move-constructing from a `folly::Function<X(Ys...) [const?]>`.
+   * For a `Function` with a `const` function type, the object must be
+   * callable from a `const`-reference, i.e. implement `operator() const`.
+   * For a `Function` with a non-`const` function type, the object will
+   * be called from a non-const reference, which means that it will execute
+   * a non-const `operator()` if it is defined, and falls back to
+   * `operator() const` otherwise.
    */
   template <
-      bool Const = Traits::IsConst::value,
-      typename std::enable_if<!Const, int>::type = 0>
-  Function(Function<OtherSignature>&& that) noexcept
+      typename Signature,
+      typename = typename Traits::template ResultOf<Function<Signature>>>
+  Function(Function<Signature>&& that) noexcept(
+      noexcept(Function(std::move(that), CoerceTag{})))
       : Function(std::move(that), CoerceTag{}) {}
 
   /**
@@ -553,7 +574,6 @@ class Function final : private detail::function::FunctionTraits<FunctionType> {
     exec_(Op::NUKE, &data_, nullptr);
   }
 
-  Function& operator=(Function&) = delete;
   Function& operator=(const Function&) = delete;
 
   /**
@@ -579,7 +599,7 @@ class Function final : private detail::function::FunctionTraits<FunctionType> {
    * \note `typename = ResultOf<Fun>` prevents this overload from being
    * selected by overload resolution when `fun` is not a compatible function.
    */
-  template <class Fun, typename = typename Traits::template ResultOf<Fun>>
+  template <typename Fun, typename = decltype(Function(std::declval<Fun>()))>
   Function& operator=(Fun&& fun) noexcept(
       noexcept(/* implicit */ Function(std::declval<Fun>()))) {
     // Doing this in place is more efficient when we can do so safely.
@@ -595,6 +615,17 @@ class Function final : private detail::function::FunctionTraits<FunctionType> {
     return *this;
   }
 
+  /**
+   * For assigning from a `Function<X(Ys..) [const?]>`.
+   */
+  template <
+      typename Signature,
+      typename = typename Traits::template ResultOf<Function<Signature>>>
+  Function& operator=(Function<Signature>&& that) noexcept(
+      noexcept(Function(std::move(that)))) {
+    return (*this = Function(std::move(that)));
+  }
+
   /**
    * Clears this `Function`.
    */

folly/test/FunctionTest.cpp

@@ -54,8 +54,127 @@ struct Functor {
 template <typename Ret, typename... Args>
 void deduceArgs(Function<Ret(Args...)>) {}
 
+struct CallableButNotCopyable {
+  CallableButNotCopyable() {}
+  CallableButNotCopyable(CallableButNotCopyable const&) = delete;
+  CallableButNotCopyable(CallableButNotCopyable&&) = delete;
+  CallableButNotCopyable& operator=(CallableButNotCopyable const&) = delete;
+  CallableButNotCopyable& operator=(CallableButNotCopyable&&) = delete;
+  template <class... Args>
+  void operator()(Args&&...) const {}
+};
+
 } // namespace
 
+// TEST =====================================================================
+// Test constructibility and non-constructibility for some tricky conversions
+static_assert(
+    !std::is_assignable<Function<void()>, CallableButNotCopyable>::value,
+    "");
+static_assert(
+    !std::is_constructible<Function<void()>, CallableButNotCopyable&>::value,
+    "");
+static_assert(
+    !std::is_constructible<Function<void() const>, CallableButNotCopyable>::
+        value,
+    "");
+static_assert(
+    !std::is_constructible<Function<void() const>, CallableButNotCopyable&>::
+        value,
+    "");
+
+static_assert(
+    !std::is_assignable<Function<void()>, CallableButNotCopyable>::value,
+    "");
+static_assert(
+    !std::is_assignable<Function<void()>, CallableButNotCopyable&>::value,
+    "");
+static_assert(
+    !std::is_assignable<Function<void() const>, CallableButNotCopyable>::value,
+    "");
+static_assert(
+    !std::is_assignable<Function<void() const>, CallableButNotCopyable&>::value,
+    "");
+
+static_assert(
+    std::is_constructible<Function<int(int)>, Function<int(int) const>>::value,
+    "");
+static_assert(
+    !std::is_constructible<Function<int(int) const>, Function<int(int)>>::value,
+    "");
+static_assert(
+    std::is_constructible<Function<int(short)>, Function<short(int) const>>::
+        value,
+    "");
+static_assert(
+    !std::is_constructible<Function<int(short) const>, Function<short(int)>>::
+        value,
+    "");
+static_assert(
+    !std::is_constructible<Function<int(int)>, Function<int(int) const>&>::
+        value,
+    "");
+static_assert(
+    !std::is_constructible<Function<int(int) const>, Function<int(int)>&>::
+        value,
+    "");
+static_assert(
+    !std::is_constructible<Function<int(short)>, Function<short(int) const>&>::
+        value,
+    "");
+static_assert(
+    !std::is_constructible<Function<int(short) const>, Function<short(int)>&>::
+        value,
+    "");
+
+static_assert(
+    std::is_assignable<Function<int(int)>, Function<int(int) const>>::value,
+    "");
+static_assert(
+    !std::is_assignable<Function<int(int) const>, Function<int(int)>>::value,
+    "");
+static_assert(
+    std::is_assignable<Function<int(short)>, Function<short(int) const>>::value,
+    "");
+static_assert(
+    !std::is_assignable<Function<int(short) const>, Function<short(int)>>::
+        value,
+    "");
+static_assert(
+    !std::is_assignable<Function<int(int)>, Function<int(int) const>&>::value,
+    "");
+static_assert(
+    !std::is_assignable<Function<int(int) const>, Function<int(int)>&>::value,
+    "");
+static_assert(
+    !std::is_assignable<Function<int(short)>, Function<short(int) const>&>::
+        value,
+    "");
+static_assert(
+    !std::is_assignable<Function<int(short) const>, Function<short(int)>&>::
+        value,
+    "");
+
+static_assert(
+    std::is_nothrow_constructible<
+        Function<int(int)>,
+        Function<int(int) const>>::value,
+    "");
+static_assert(
+    !std::is_nothrow_constructible<
+        Function<int(short)>,
+        Function<short(int) const>>::value,
+    "");
+static_assert(
+    std::is_nothrow_assignable<Function<int(int)>, Function<int(int) const>>::
+        value,
+    "");
+static_assert(
+    !std::is_nothrow_assignable<
+        Function<int(short)>,
+        Function<short(int) const>>::value,
+    "");
+
 // TEST =====================================================================
 // InvokeFunctor & InvokeReference
 
@@ -938,3 +1057,22 @@ TEST(Function, DeducableArguments) {
   deduceArgs(Function<void(int, float)>{[](int, float) {}});
   deduceArgs(Function<int(int, float)>{[](int i, float) { return i; }});
 }
+
+TEST(Function, CtorWithCopy) {
+  struct X {
+    X() {}
+    X(X const&) noexcept(true) {}
+    X& operator=(X const&) = default;
+  };
+  struct Y {
+    Y() {}
+    Y(Y const&) noexcept(false) {}
+    Y(Y&&) noexcept(true) {}
+    Y& operator=(Y&&) = default;
+    Y& operator=(Y const&) = default;
+  };
+  auto lx = [x = X()]{};
+  auto ly = [y = Y()]{};
+  EXPECT_TRUE(noexcept(Function<void()>(lx)));
+  EXPECT_FALSE(noexcept(Function<void()>(ly)));
+}