Document two surprises in `DoAllAction::NonFinalType`.

PiperOrigin-RevId: 441589196
Change-Id: Ic3e483ca70d72261046bad464d817f9dfd4bec65

2 files changed, 128 insertions, 0 deletions

diff --git a/googlemock/include/gmock/gmock-actions.h b/googlemock/include/gmock/gmock-actions.h
index 4ae18dac..1038b027 100644
--- a/googlemock/include/gmock/gmock-actions.h
+++ b/googlemock/include/gmock/gmock-actions.h
@@ -1295,6 +1295,51 @@ struct WithArgsAction {
 template <typename... Actions>
 struct DoAllAction {
  private:
+  // The type of reference that should be provided to an initial action for a
+  // mocked function parameter of type T.
+  //
+  // There are two quirks here:
+  //
+  //  *  Unlike most forwarding functions, we pass scalars through by value.
+  //     This isn't strictly necessary because an lvalue reference would work
+  //     fine too and be consistent with other non-reference types, but it's
+  //     perhaps less surprising.
+  //
+  //     For example if the mocked function has signature void(int), then it
+  //     might seem surprising for the user's initial action to need to be
+  //     convertible to Action<void(const int&)>. This is perhaps less
+  //     surprising for a non-scalar type where there may be a performance
+  //     impact, or it might even be impossible, to pass by value.
+  //
+  //  *  More surprisingly, `const T&` is often not a const reference type.
+  //     By the reference collapsing rules in C++17 [dcl.ref]/6, if T refers to
+  //     U& or U&& for some non-scalar type U, then NonFinalType<T> is U&. In
+  //     other words, we may hand over a non-const reference.
+  //
+  //     So for example, given some non-scalar type Obj we have the following
+  //     mappings:
+  //
+  //            T               NonFinalType<T>
+  //         -------            ---------------
+  //         Obj                const Obj&
+  //         Obj&               Obj&
+  //         Obj&&              Obj&
+  //         const Obj          const Obj&
+  //         const Obj&         const Obj&
+  //         const Obj&&        const Obj&
+  //
+  //     In other words, the initial actions get a mutable view of an non-scalar
+  //     argument if and only if the mock function itself accepts a non-const
+  //     reference type. They are never given an rvalue reference to an
+  //     non-scalar type.
+  //
+  //     This situation makes sense if you imagine use with a matcher that is
+  //     designed to write through a reference. For example, if the caller wants
+  //     to fill in a reference argument and then return a canned value:
+  //
+  //         EXPECT_CALL(mock, Call)
+  //             .WillOnce(DoAll(SetArgReferee<0>(17), Return(19)));
+  //
   template <typename T>
   using NonFinalType =
       typename std::conditional<std::is_scalar<T>::value, T, const T&>::type;
diff --git a/googlemock/test/gmock-actions_test.cc b/googlemock/test/gmock-actions_test.cc
index 7ff5780c..198510b1 100644
--- a/googlemock/test/gmock-actions_test.cc
+++ b/googlemock/test/gmock-actions_test.cc
@@ -1192,6 +1192,89 @@ TEST(AssignTest, CompatibleTypes) {
   EXPECT_DOUBLE_EQ(5, x);
 }
 
+// DoAll should never provide rvalue references to the initial actions. If the
+// mock action itself accepts an rvalue reference or a non-scalar object by
+// value then the final action should receive an rvalue reference, but initial
+// actions should receive only lvalue references.
+TEST(DoAll, ProvidesLvalueReferencesToInitialActions) {
+  struct Obj {};
+
+  // Mock action accepts by value: the initial action should be fed a const
+  // lvalue reference, and the final action an rvalue reference.
+  {
+    struct InitialAction {
+      void operator()(Obj&) const { FAIL() << "Unexpected call"; }
+      void operator()(const Obj&) const {}
+      void operator()(Obj&&) const { FAIL() << "Unexpected call"; }
+      void operator()(const Obj&&) const { FAIL() << "Unexpected call"; }
+    };
+
+    MockFunction<void(Obj)> mock;
+    EXPECT_CALL(mock, Call)
+        .WillOnce(DoAll(InitialAction{}, InitialAction{}, [](Obj&&) {}))
+        .WillRepeatedly(DoAll(InitialAction{}, InitialAction{}, [](Obj&&) {}));
+
+    mock.AsStdFunction()(Obj{});
+    mock.AsStdFunction()(Obj{});
+  }
+
+  // Mock action accepts by const lvalue reference: both actions should receive
+  // a const lvalue reference.
+  {
+    struct InitialAction {
+      void operator()(Obj&) const { FAIL() << "Unexpected call"; }
+      void operator()(const Obj&) const {}
+      void operator()(Obj&&) const { FAIL() << "Unexpected call"; }
+      void operator()(const Obj&&) const { FAIL() << "Unexpected call"; }
+    };
+
+    MockFunction<void(const Obj&)> mock;
+    EXPECT_CALL(mock, Call)
+        .WillOnce(DoAll(InitialAction{}, InitialAction{}, [](const Obj&) {}))
+        .WillRepeatedly(
+            DoAll(InitialAction{}, InitialAction{}, [](const Obj&) {}));
+
+    mock.AsStdFunction()(Obj{});
+    mock.AsStdFunction()(Obj{});
+  }
+
+  // Mock action accepts by non-const lvalue reference: both actions should get
+  // a non-const lvalue reference if they want them.
+  {
+    struct InitialAction {
+      void operator()(Obj&) const {}
+      void operator()(Obj&&) const { FAIL() << "Unexpected call"; }
+    };
+
+    MockFunction<void(Obj&)> mock;
+    EXPECT_CALL(mock, Call)
+        .WillOnce(DoAll(InitialAction{}, InitialAction{}, [](Obj&) {}))
+        .WillRepeatedly(DoAll(InitialAction{}, InitialAction{}, [](Obj&) {}));
+
+    Obj obj;
+    mock.AsStdFunction()(obj);
+    mock.AsStdFunction()(obj);
+  }
+
+  // Mock action accepts by rvalue reference: the initial actions should receive
+  // a non-const lvalue reference if it wants it, and the final action an rvalue
+  // reference.
+  {
+    struct InitialAction {
+      void operator()(Obj&) const {}
+      void operator()(Obj&&) const { FAIL() << "Unexpected call"; }
+    };
+
+    MockFunction<void(Obj &&)> mock;
+    EXPECT_CALL(mock, Call)
+        .WillOnce(DoAll(InitialAction{}, InitialAction{}, [](Obj&&) {}))
+        .WillRepeatedly(DoAll(InitialAction{}, InitialAction{}, [](Obj&&) {}));
+
+    mock.AsStdFunction()(Obj{});
+    mock.AsStdFunction()(Obj{});
+  }
+}
+
 // Tests using WithArgs and with an action that takes 1 argument.
 TEST(WithArgsTest, OneArg) {
   Action<bool(double x, int n)> a = WithArgs<1>(Invoke(Unary));  // NOLINT