Update callback documentation for references

The callback documentation was out of date after the ban on non-const
references were allowed. Add documentation about binding values for
non-const reference parameters and the use of base::OwnedRef().

Also add a compile time error message if bound parameter can't be
forwarded to a non-const reference parameter then mention std::ref() and
base::OwnedRef() to give a hint.

Bug: 1182328
Change-Id: Ic4ca06d6781442651cd09735c66f7d28f105453b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2756709
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#863842}

base/bind_internal.h

@@ -1046,6 +1046,17 @@ struct BindArgument {
     struct ToParamWithType {
       static constexpr bool kCanBeForwardedToBoundFunctor =
           std::is_constructible<FunctorParamType, ForwardingType>::value;
+
+      // If the bound type can't be forwarded then test if `FunctorParamType` is
+      // a non-const lvalue reference and a reference to the unwrapped type
+      // *could* have been successfully forwarded.
+      static constexpr bool kNonConstRefParamMustBeWrapped =
+          kCanBeForwardedToBoundFunctor ||
+          !(std::is_lvalue_reference<FunctorParamType>::value &&
+            !std::is_const<std::remove_reference_t<FunctorParamType>>::value &&
+            std::is_convertible<std::decay_t<ForwardingType>&,
+                                FunctorParamType>::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.
@@ -1115,6 +1126,11 @@ struct AssertConstructible {
       "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(
+      BindArgument<i>::template ForwardedAs<Unwrapped>::
+          template ToParamWithType<Param>::kNonConstRefParamMustBeWrapped,
+      "Bound argument for non-const reference parameter must be wrapped in "
+      "std::ref() or base::OwnedRef().");
   static_assert(
       BindArgument<i>::template ForwardedAs<Unwrapped>::
           template ToParamWithType<Param>::kCanBeForwardedToBoundFunctor,

base/bind_unittest.cc

@@ -540,6 +540,94 @@ TEST_F(BindTest, IgnoreResultForOnceCallback) {
   EXPECT_EQ(s, "Run2");
 }
 
+void SetFromRef(int& ref) {
+  EXPECT_EQ(ref, 1);
+  ref = 2;
+  EXPECT_EQ(ref, 2);
+}
+
+TEST_F(BindTest, BindOnceWithNonConstRef) {
+  int v = 1;
+
+  // Mutates `v` because it's not bound to callback instead it's forwarded by
+  // Run().
+  auto cb1 = BindOnce(SetFromRef);
+  std::move(cb1).Run(v);
+  EXPECT_EQ(v, 2);
+  v = 1;
+
+  // Mutates `v` through std::reference_wrapper bound to callback.
+  auto cb2 = BindOnce(SetFromRef, std::ref(v));
+  std::move(cb2).Run();
+  EXPECT_EQ(v, 2);
+  v = 1;
+
+  // Everything past here following will make a copy of the argument. The copy
+  // will be mutated and leave `v` unmodified.
+  auto cb3 = BindOnce(SetFromRef, base::OwnedRef(v));
+  std::move(cb3).Run();
+  EXPECT_EQ(v, 1);
+
+  int& ref = v;
+  auto cb4 = BindOnce(SetFromRef, base::OwnedRef(ref));
+  std::move(cb4).Run();
+  EXPECT_EQ(v, 1);
+
+  const int cv = 1;
+  auto cb5 = BindOnce(SetFromRef, base::OwnedRef(cv));
+  std::move(cb5).Run();
+  EXPECT_EQ(cv, 1);
+
+  const int& cref = v;
+  auto cb6 = BindOnce(SetFromRef, base::OwnedRef(cref));
+  std::move(cb6).Run();
+  EXPECT_EQ(cref, 1);
+
+  auto cb7 = BindOnce(SetFromRef, base::OwnedRef(1));
+  std::move(cb7).Run();
+}
+
+TEST_F(BindTest, BindRepeatingWithNonConstRef) {
+  int v = 1;
+
+  // Mutates `v` because it's not bound to callback instead it's forwarded by
+  // Run().
+  auto cb1 = BindRepeating(SetFromRef);
+  std::move(cb1).Run(v);
+  EXPECT_EQ(v, 2);
+  v = 1;
+
+  // Mutates `v` through std::reference_wrapper bound to callback.
+  auto cb2 = BindRepeating(SetFromRef, std::ref(v));
+  std::move(cb2).Run();
+  EXPECT_EQ(v, 2);
+  v = 1;
+
+  // Everything past here following will make a copy of the argument. The copy
+  // will be mutated and leave `v` unmodified.
+  auto cb3 = BindRepeating(SetFromRef, base::OwnedRef(v));
+  std::move(cb3).Run();
+  EXPECT_EQ(v, 1);
+
+  int& ref = v;
+  auto cb4 = BindRepeating(SetFromRef, base::OwnedRef(ref));
+  std::move(cb4).Run();
+  EXPECT_EQ(v, 1);
+
+  const int cv = 1;
+  auto cb5 = BindRepeating(SetFromRef, base::OwnedRef(cv));
+  std::move(cb5).Run();
+  EXPECT_EQ(cv, 1);
+
+  const int& cref = v;
+  auto cb6 = BindRepeating(SetFromRef, base::OwnedRef(cref));
+  std::move(cb6).Run();
+  EXPECT_EQ(cref, 1);
+
+  auto cb7 = BindRepeating(SetFromRef, base::OwnedRef(1));
+  std::move(cb7).Run();
+}
+
 // Functions that take reference parameters.
 //  - Forced reference parameter type still stores a copy.
 //  - Forced const reference parameter type still stores a copy.

base/bind_unittest.nc

@@ -74,6 +74,8 @@ void VoidPolymorphic1(T t) {
 void TakesMoveOnly(std::unique_ptr<int>) {
 }
 
+void TakesIntRef(int& ref) {}
+
 struct NonEmptyFunctor {
   int x;
   void operator()() const {}
@@ -157,13 +159,41 @@ void WontCompile() {
   ref_arg_cb.Run(p);
 }
 
-#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\."]
+#elif defined(NCTEST_BIND_ONCE_WITH_NON_CONST_REF_PARAM)  // [r"static_assert failed due to requirement 'BindArgument<0>::ForwardedAs<.+?>::ToParamWithType<.+?>::kNonConstRefParamMustBeWrapped' \"Bound argument for non-const reference parameter must be wrapped in std::ref\(\) or base::OwnedRef\(\).\""]
 
-// Binding functions with reference parameters requires `std::ref()`.
+// Binding functions with reference parameters requires `std::ref()` or
+// 'base::OwnedRef()`.
 void WontCompile() {
-  Parent p;
-  RepeatingCallback<int()> ref_cb = BindRepeating(&UnwrapParentRef, p);
-  ref_cb.Run();
+  int v = 1;
+  auto cb = BindOnce(&TakesIntRef, v);
+}
+
+#elif defined(NCTEST_BIND_REPEATING_WITH_NON_CONST_REF_PARAM)  // [r"static_assert failed due to requirement 'BindArgument<0>::ForwardedAs<.+?>::ToParamWithType<.+?>::kNonConstRefParamMustBeWrapped' \"Bound argument for non-const reference parameter must be wrapped in std::ref\(\) or base::OwnedRef\(\).\""]
+
+// Binding functions with reference parameters requires `std::ref()` or
+// 'base::OwnedRef()`.
+void WontCompile() {
+  int v = 1;
+  auto cb = BindRepeating(&TakesIntRef, v);
+}
+
+#elif defined(NCTEST_NON_CONST_REF_PARAM_WRONG_TYPE)  // [r"static_assert failed due to requirement 'BindArgument<0>::ForwardedAs<.+?>::ToParamWithType<.+?>::kCanBeForwardedToBoundFunctor' \"Type mismatch between bound argument and bound functor's parameter.\""]
+
+// If the argument and parameter types mismatch then the compile error should be
+// the generic type mismatch error.
+void WontCompile() {
+  float f = 1.0f;
+  auto cb = BindOnce(&TakesIntRef, f);
+}
+
+#elif defined(NCTEST_NON_CONST_REF_PARAM_WRONG_TYPE_AND_WRAPPED)  // [r"static_assert failed due to requirement 'BindArgument<0>::ForwardedAs<.+?>::ToParamWithType<.+?>::kCanBeForwardedToBoundFunctor' \"Type mismatch between bound argument and bound functor's parameter.\""]
+
+// If the argument and parameter types mismatch then the compile error should be
+// the generic type mismatch error even if the argument is wrapped in
+// base::OwnedRef().
+void WontCompile() {
+  float f = 1.0f;
+  auto cb = BindOnce(&TakesIntRef, base::OwnedRef(f));
 }
 
 #elif defined(NCTEST_NO_IMPLICIT_ARRAY_PTR_CONVERSION)  // [r"fatal error: static_assert failed due to requirement '!std::is_array<base::HasRef \[10\]>::value' \"First bound argument to a method cannot be an array.\""]

docs/callback.md

@@ -676,25 +676,74 @@ base::Closure cb = base::Bind(&DontTakeRef, base::RetainedRef(f));
 `base::RetainedRef` holds a reference to the object and passes a raw pointer to
 the object when the Callback is run.
 
-### Passing Parameters By Reference
+### Binding Const Reference Parameters
 
-References are *copied* unless `std::ref` or `std::cref` is used. Example:
+If the callback function takes a const reference parameter then the value is
+*copied* when bound unless `std::ref` or `std::cref` is used. Example:
 
 ```cpp
 void foo(const int& arg) { printf("%d %p\n", arg, &arg); }
 int n = 1;
-base::Closure has_copy = base::Bind(&foo, n);
-base::Closure has_ref = base::Bind(&foo, std::cref(n));
+base::OnceClosure has_copy = base::BindOnce(&foo, n);
+base::OnceClosure has_ref = base::BindOnce(&foo, std::cref(n));
 n = 2;
-foo(n);                        // Prints "2 0xaaaaaaaaaaaa"
-has_copy.Run();                // Prints "1 0xbbbbbbbbbbbb"
-has_ref.Run();                 // Prints "2 0xaaaaaaaaaaaa"
+foo(n);                                   // Prints "2 0xaaaaaaaaaaaa"
+std::move(has_copy).Run();                // Prints "1 0xbbbbbbbbbbbb"
+std::move(has_ref).Run();                 // Prints "2 0xaaaaaaaaaaaa"
 ```
 
-Normally parameters are copied in the closure.
-**DANGER**: `std::ref` and `std::cref` store a (const) reference instead,
-referencing the original parameter. This means that you must ensure the object
-outlives the callback!
+Normally parameters are copied in the closure. **DANGER**: `std::ref` and
+`std::cref` store a (const) reference instead, referencing the original
+parameter. This means that you must ensure the object outlives the callback!
+
+### Binding Non-Const Reference Parameters
+
+If the callback function takes a non-const reference then the bind statement
+must specify what behavior is desired. If a reference that can mutate the
+original value is desired then `std::ref` is used. If the callback should take
+ownership of the value, either by making a copy or moving an existing value,
+then `base::OwnedRef` is used. If neither is used the bind statement will fail
+to compile. Example:
+
+```cpp
+void foo(int& arg) {
+  printf("%d\n", arg);
+  ++arg;
+}
+
+int n = 0;
+base::RepeatingClosure has_ref = base::BindRepeating(&foo, std::ref(n));
+base::RepeatingClosure has_copy = base::BindRepeating(&foo, base::OwnedRef(n));
+
+foo(n);                        // Prints "0"
+has_ref.Run();                 // Prints "1"
+has_ref.Run();                 // Prints "2"
+foo(n);                        // Prints "3"
+
+has_copy.Run();                // Prints "0"
+has_copy.Run();                // Prints "1"
+
+// This will fail to compile.
+base::RepeatingClosure cb = base::BindRepeating(&foo, n);
+```
+
+Normally parameters are copied in the closure. **DANGER**: `std::ref` stores a
+reference instead, referencing the original parameter. This means that you must
+ensure the object outlives the callback!
+
+If the callback function has an output reference parameter but the output value
+isn't needed then `base::OwnedRef()` is a convenient way to handle it. The
+callback owned value will be mutated by the callback function and then deleted
+along with the callback. Example:
+
+```cpp
+bool Compute(size_t index, int& output);
+
+// The `output` parameter isn't important for the callback, it only cares about
+// the return value.
+base::OnceClosure cb = base::BindOnce(&Compute, index, base::OwnedRef(0));
+bool success = std::move(cb).Run();
+```
 
 ## Implementation notes