Improve readability of base::Bind{Once,Repeating} errors.

Change-Id: If44f3d970b3de9aa2d8bc9d237665cbac9bf5e1a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490842
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822829}

base/bind.h

@@ -69,41 +69,99 @@ struct IsOnceCallback : std::false_type {};
 template <typename Signature>
 struct IsOnceCallback<OnceCallback<Signature>> : std::true_type {};
 
+// Helpers to make error messages slightly more readable.
+template <int i>
+struct BindArgument {
+  template <typename ForwardingType>
+  struct ForwardedAs {
+    template <typename FunctorParamType>
+    struct ToParamWithType {
+      static constexpr bool kCanBeForwardedToBoundFunctor =
+          std::is_constructible<FunctorParamType, ForwardingType>::value;
+      // Note that this intentionally drops the const qualifier from
+      // `ForwardingType`, to test if it *could* have been successfully
+      // forwarded if `Passed()` had been used.
+      static constexpr bool kMoveOnlyTypeMustUseBasePassed =
+          kCanBeForwardedToBoundFunctor ||
+          !std::is_constructible<FunctorParamType,
+                                 std::decay_t<ForwardingType>&&>::value;
+    };
+  };
+
+  template <typename BoundAsType>
+  struct BoundAs {
+    template <typename StorageType>
+    struct StoredAs {
+      static constexpr bool kBindArgumentCanBeCaptured =
+          std::is_constructible<StorageType, BoundAsType>::value;
+      // Note that this intentionally drops the const qualifier from
+      // `BoundAsType`, to test if it *could* have been successfully bound if
+      // `std::move()` had been used.
+      static constexpr bool kMoveOnlyTypeMustUseStdMove =
+          kBindArgumentCanBeCaptured ||
+          !std::is_constructible<StorageType,
+                                 std::decay_t<BoundAsType>&&>::value;
+    };
+  };
+};
+
 // Helper to assert that parameter |i| of type |Arg| can be bound, which means:
 // - |Arg| can be retained internally as |Storage|.
 // - |Arg| can be forwarded as |Unwrapped| to |Param|.
-template <size_t i,
+template <int i,
           typename Arg,
           typename Storage,
           typename Unwrapped,
           typename Param>
 struct AssertConstructible {
  private:
-  static constexpr bool param_is_forwardable =
-      std::is_constructible<Param, Unwrapped>::value;
-  // Unlike the check for binding into storage below, the check for
-  // forwardability drops the const qualifier for repeating callbacks. This is
-  // to try to catch instances where std::move()--which forwards as a const
-  // reference with repeating callbacks--is used instead of base::Passed().
+  // With `BindRepeating`, there are two decision points for how to handle a
+  // move-only type:
+  //
+  // 1. Whether the move-only argument should be moved into the internal
+  //    `BindState`. Either `std::move()` or `Passed` is sufficient to trigger
+  //    move-only semantics.
+  // 2. Whether or not the bound, move-only argument should be moved to the
+  //    bound functor when invoked. When the argument is bound with `Passed`,
+  //    invoking the callback will destructively move the bound, move-only
+  //    argument to the bound functor. In contrast, if the argument is bound
+  //    with `std::move()`, `RepeatingCallback` will attempt to call the bound
+  //    functor with a constant reference to the bound, move-only argument. This
+  //    will fail if the bound functor accepts that argument by value, since the
+  //    argument cannot be copied. It is this latter case that this
+  //    static_assert aims to catch.
+  //
+  // In contrast, `BindOnce()` only has one decision point. Once a move-only
+  // type is captured by value into the internal `BindState`, the bound,
+  // move-only argument will always be moved to the functor when invoked.
+  // Failure to use std::move will simply fail the `kMoveOnlyTypeMustUseStdMove`
+  // assert below instead.
+  //
+  // Note: `Passed()` is a legacy of supporting move-only types when repeating
+  // callbacks were the only callback type. A `RepeatingCallback` with a
+  // `Passed()` argument is really a `OnceCallback` and should eventually be
+  // migrated.
+  static_assert(
+      BindArgument<i>::template ForwardedAs<Unwrapped>::
+          template ToParamWithType<Param>::kMoveOnlyTypeMustUseBasePassed,
+      "base::BindRepeating() argument is a move-only type. Use base::Passed() "
+      "instead of std::move() to transfer ownership from the callback to the "
+      "bound functor.");
   static_assert(
-      param_is_forwardable ||
-          !std::is_constructible<Param, std::decay_t<Unwrapped>&&>::value,
-      "Bound argument |i| is move-only but will be forwarded by copy. "
-      "Ensure |Arg| is bound using base::Passed(), not std::move().");
+      BindArgument<i>::template ForwardedAs<Unwrapped>::
+          template ToParamWithType<Param>::kCanBeForwardedToBoundFunctor,
+      "Type mismatch between bound argument and bound functor's parameter.");
+
+  static_assert(BindArgument<i>::template BoundAs<Arg>::template StoredAs<
+                    Storage>::kMoveOnlyTypeMustUseStdMove,
+                "Attempting to bind a move-only type. Use std::move() to "
+                "transfer ownership to the created callback.");
+  // In practice, this static_assert should be quite rare as the storage type
+  // is deduced from the arguments passed to `BindOnce()`/`BindRepeating()`.
   static_assert(
-      param_is_forwardable,
-      "Bound argument |i| of type |Arg| cannot be forwarded as "
-      "|Unwrapped| to the bound functor, which declares it as |Param|.");
-
-  static constexpr bool arg_is_storable =
-      std::is_constructible<Storage, Arg>::value;
-  static_assert(arg_is_storable ||
-                    !std::is_constructible<Storage, std::decay_t<Arg>&&>::value,
-                "Bound argument |i| is move-only but will be bound by copy. "
-                "Ensure |Arg| is mutable and bound using std::move().");
-  static_assert(arg_is_storable,
-                "Bound argument |i| of type |Arg| cannot be converted and "
-                "bound as |Storage|.");
+      BindArgument<i>::template BoundAs<Arg>::template StoredAs<
+          Storage>::kBindArgumentCanBeCaptured,
+      "Cannot capture argument: is the argument copyable or movable?");
 };
 
 // Takes three same-length TypeLists, and applies AssertConstructible for each

base/bind_unittest.nc

@@ -79,7 +79,8 @@ struct NonEmptyFunctor {
   void operator()() const {}
 };
 
-#if defined(NCTEST_METHOD_ON_CONST_OBJECT)  // [r"fatal error: static_assert failed due to requirement 'param_is_forwardable' \"Bound argument |i| of type |Arg| cannot be forwarded as |Unwrapped| to the bound functor, which declares it as |Param|.\""]
+#if defined(NCTEST_METHOD_ON_CONST_OBJECT)  // [r"static_assert failed.+?BindArgument<0>::ForwardedAs<.+?>::ToParamWithType<.+?>::kCanBeForwardedToBoundFunctor.+?Type mismatch between bound argument and bound functor's parameter\."]
+
 // Method bound to const-object.
 //
 // Only const methods should be allowed to work with const objects.
@@ -116,8 +117,7 @@ void WontCompile() {
   no_ref_const_cb.Run();
 }
 
-#elif defined(NCTEST_CONST_POINTER)  // [r"fatal error: static_assert failed due to requirement 'param_is_forwardable' \"Bound argument |i| of type |Arg| cannot be forwarded as |Unwrapped| to the bound functor, which declares it as |Param|.\""]
-
+#elif defined(NCTEST_CONST_POINTER)  // [r"static_assert failed.+?BindArgument<0>::ForwardedAs<.+?>::ToParamWithType<.+?>::kCanBeForwardedToBoundFunctor.+?Type mismatch between bound argument and bound functor's parameter\."]
 // Const argument used with non-const pointer parameter of same type.
 //
 // This is just a const-correctness check.
@@ -128,7 +128,7 @@ void WontCompile() {
   pointer_same_cb.Run();
 }
 
-#elif defined(NCTEST_CONST_POINTER_SUBTYPE)  // [r"fatal error: static_assert failed due to requirement 'param_is_forwardable' \"Bound argument |i| of type |Arg| cannot be forwarded as |Unwrapped| to the bound functor, which declares it as |Param|.\""]
+#elif defined(NCTEST_CONST_POINTER_SUBTYPE)  // [r"static_assert failed.+?BindArgument<0>::ForwardedAs<.+?>::ToParamWithType<.+?>::kCanBeForwardedToBoundFunctor.+?Type mismatch between bound argument and bound functor's parameter\."]
 
 // Const argument used with non-const pointer parameter of super type.
 //
@@ -157,11 +157,9 @@ void WontCompile() {
   ref_arg_cb.Run(p);
 }
 
-#elif defined(NCTEST_DISALLOW_BIND_TO_NON_CONST_REF_PARAM)  // [r"fatal error: static_assert failed due to requirement 'param_is_forwardable' \"Bound argument |i| of type |Arg| cannot be forwarded as |Unwrapped| to the bound functor, which declares it as |Param|.\""]
+#elif defined(NCTEST_DISALLOW_BIND_TO_NON_CONST_REF_PARAM)  // [r"static_assert failed.+?BindArgument<0>::ForwardedAs<.+?>::ToParamWithType<.+?>::kCanBeForwardedToBoundFunctor.+?Type mismatch between bound argument and bound functor's parameter\."]
 
-// Binding functions with reference parameters, unsupported.
-//
-// See comment in NCTEST_DISALLOW_NON_CONST_REF_PARAM
+// Binding functions with reference parameters requires `std::ref()`.
 void WontCompile() {
   Parent p;
   RepeatingCallback<int()> ref_cb = BindRepeating(&UnwrapParentRef, p);
@@ -287,21 +285,21 @@ void WontCompile() {
   BindOnce(std::move(cb), 42);
 }
 
-#elif defined(NCTEST_BINDONCE_MOVEONLY_TYPE_BY_VALUE)  // [r"fatal error: static_assert failed due to requirement 'arg_is_storable || !std::is_constructible<std::__1::unique_ptr<int, std::__1::default_delete<int> >, std::__1::unique_ptr<int, std::__1::default_delete<int> > &&>::value' \"Bound argument |i| is move-only but will be bound by copy. Ensure |Arg| is mutable and bound using std::move\(\).\""]
+#elif defined(NCTEST_BINDONCE_MOVEONLY_TYPE_BY_VALUE)  // [r"static_assert failed.+?BindArgument<0>::BoundAs<.+?>::StoredAs<.+?>::kMoveOnlyTypeMustUseStdMove.+?Attempting to bind a move-only type\. Use std::move\(\) to transfer ownership to the created callback\."]
 
 void WontCompile() {
   std::unique_ptr<int> x;
   BindOnce(&TakesMoveOnly, x);
 }
 
-#elif defined(NCTEST_BIND_MOVEONLY_TYPE_BY_VALUE)  // [r"Bound argument \|i\| is move-only but will be forwarded by copy\. Ensure \|Arg\| is bound using base::Passed\(\), not std::move\(\)."]
+#elif defined(NCTEST_BIND_MOVEONLY_TYPE_BY_VALUE)  // [r"static_assert failed.+?BindArgument<0>::ForwardedAs<.+?>::ToParamWithType<.+?>::kMoveOnlyTypeMustUseBasePassed.+?base::BindRepeating\(\) argument is a move-only type\. Use base::Passed\(\) instead of std::move\(\) to transfer ownership from the callback to the bound functor\."]
 
 void WontCompile() {
   std::unique_ptr<int> x;
   BindRepeating(&TakesMoveOnly, x);
 }
 
-#elif defined(NCTEST_BIND_MOVEONLY_TYPE_WITH_STDMOVE)  // [r"Bound argument \|i\| is move-only but will be forwarded by copy\. Ensure \|Arg\| is bound using base::Passed\(\), not std::move\(\)."]
+#elif defined(NCTEST_BIND_MOVEONLY_TYPE_WITH_STDMOVE)  // [r"static_assert failed.+?BindArgument<0>::ForwardedAs<.+?>::ToParamWithType<.+?>::kMoveOnlyTypeMustUseBasePassed.+?base::BindRepeating\(\) argument is a move-only type\. Use base::Passed\(\) instead of std::move\(\) to transfer ownership from the callback to the bound functor\."]
 
 void WontCompile() {
   std::unique_ptr<int> x;
@@ -328,6 +326,20 @@ void WontCompile() {
   const auto mutable_lambda = [&]() mutable {};
   BindLambdaForTesting(std::move(mutable_lambda));
 }
+
+#elif defined(NCTEST_BIND_UNCOPYABLE_AND_UNMOVABLE_TYPE)  // [r"static_assert failed.+?BindArgument<0>::BoundAs<.+?>::StoredAs<.+?>::kBindArgumentCanBeCaptured.+?Cannot capture argument: is the argument copyable or movable\?"]
+
+void WontCompile() {
+  struct UncopyableUnmovable {
+    UncopyableUnmovable() = default;
+    UncopyableUnmovable(const UncopyableUnmovable&) = delete;
+    UncopyableUnmovable& operator=(const UncopyableUnmovable&) = delete;
+  };
+
+  UncopyableUnmovable u;
+  BindOnce([] (const UncopyableUnmovable&) {}, u);
+}
+
 #endif
 
 }  // namespace base