Improve the failure messages of ElementsAre(), ElementsAreArray(), etc.

NOTE: if you suspect that this change breaks your build, it's likely that your tests depend on the exact messages of `ElementsAre()` and friends. The messages are implementation details of these matcher and are subject to change without notice. Depending on the messages is not supported. In that case, please rewrite your tests to avoid the dependency.

When the array being matched is long, it can be hard for the user to find the mismatched element in the message generated by `ElementsAre()` or `ElementsAreArray()` - even though these matchers print the index of the mismatched element, the user still has to count to find the actual element and its corresponding matcher.

With this change, these matchers will include the actual value and corresponding matcher in the failure message, making it easier for the user.

Also make a small style improvement: now it's advised to write
```
EXPECT_EQ(actual, expected);
```
as opposed to
```
EXPECT_EQ(expected, actual);
```

PiperOrigin-RevId: 738039133
Change-Id: I3b94f7d01a6a4c92e2daf268df8cfb04a21d4294

googlemock/include/gmock/gmock-matchers.h

@@ -257,6 +257,7 @@
 
 #include <algorithm>
 #include <cmath>
+#include <cstddef>
 #include <exception>
 #include <functional>
 #include <initializer_list>
@@ -2097,11 +2098,11 @@ class WhenDynamicCastToMatcher<To&> : public WhenDynamicCastToMatcherBase<To&> {
 template <typename Class, typename FieldType>
 class FieldMatcher {
  public:
-  FieldMatcher(FieldType Class::*field,
+  FieldMatcher(FieldType Class::* field,
                const Matcher<const FieldType&>& matcher)
       : field_(field), matcher_(matcher), whose_field_("whose given field ") {}
 
-  FieldMatcher(const std::string& field_name, FieldType Class::*field,
+  FieldMatcher(const std::string& field_name, FieldType Class::* field,
                const Matcher<const FieldType&>& matcher)
       : field_(field),
         matcher_(matcher),
@@ -2145,7 +2146,7 @@ class FieldMatcher {
     return MatchAndExplainImpl(std::false_type(), *p, listener);
   }
 
-  const FieldType Class::*field_;
+  const FieldType Class::* field_;
   const Matcher<const FieldType&> matcher_;
 
   // Contains either "whose given field " if the name of the field is unknown
@@ -3291,8 +3292,8 @@ class PairMatcher {
 };
 
 template <typename T, size_t... I>
-auto UnpackStructImpl(const T& t, std::index_sequence<I...>,
-                      int) -> decltype(std::tie(get<I>(t)...)) {
+auto UnpackStructImpl(const T& t, std::index_sequence<I...>, int)
+    -> decltype(std::tie(get<I>(t)...)) {
   static_assert(std::tuple_size<T>::value == sizeof...(I),
                 "Number of arguments doesn't match the number of fields.");
   return std::tie(get<I>(t)...);
@@ -3581,7 +3582,7 @@ class ElementsAreMatcherImpl : public MatcherInterface<Container> {
     StlContainerReference stl_container = View::ConstReference(container);
     auto it = stl_container.begin();
     size_t exam_pos = 0;
-    bool mismatch_found = false;  // Have we found a mismatched element yet?
+    bool unmatched_found = false;
 
     // Go through the elements and matchers in pairs, until we reach
     // the end of either the elements or the matchers, or until we find a
@@ -3597,11 +3598,23 @@ class ElementsAreMatcherImpl : public MatcherInterface<Container> {
       }
 
       if (!match) {
-        mismatch_found = true;
+        unmatched_found = true;
+        // We cannot store the iterator for the unmatched element to be used
+        // later, as some users use ElementsAre() with a "container" whose
+        // iterator is not copy-constructible or copy-assignable.
+        //
+        // We cannot store a pointer to the element either, as some container's
+        // iterators return a temporary.
+        //
+        // We cannot store the element itself either, as the element may not be
+        // copyable.
+        //
+        // Therefore, we just remember the index of the unmatched element,
+        // and use it later to print the unmatched element.
         break;
       }
     }
-    // If mismatch_found is true, 'exam_pos' is the index of the mismatch.
+    // If unmatched_found is true, exam_pos is the index of the mismatch.
 
     // Find how many elements the actual container has.  We avoid
     // calling size() s.t. this code works for stream-like "containers"
@@ -3622,10 +3635,27 @@ class ElementsAreMatcherImpl : public MatcherInterface<Container> {
       return false;
     }
 
-    if (mismatch_found) {
+    if (unmatched_found) {
       // The element count matches, but the exam_pos-th element doesn't match.
       if (listener_interested) {
-        *listener << "whose element #" << exam_pos << " doesn't match";
+        // Find the unmatched element.
+        auto unmatched_it = stl_container.begin();
+        // We cannot call std::advance() on the iterator, as some users use
+        // ElementsAre() with a "container" whose iterator is incompatible with
+        // std::advance() (e.g. it may not have the difference_type member
+        // type).
+        for (size_t i = 0; i != exam_pos; ++i) {
+          ++unmatched_it;
+        }
+
+        // If the array is long or the elements' print-out is large, it may be
+        // hard for the user to find the mismatched element and its
+        // corresponding matcher description. Therefore we print the index, the
+        // value of the mismatched element, and the corresponding matcher
+        // description to ease debugging.
+        *listener << "whose element #" << exam_pos << " ("
+                  << PrintToString(*unmatched_it) << ") ";
+        matchers_[exam_pos].DescribeNegationTo(listener->stream());
         PrintIfNotEmpty(explanations[exam_pos], listener->stream());
       }
       return false;
@@ -4031,15 +4061,15 @@ GTEST_API_ std::string FormatMatcherDescription(
 // Overloads to support `OptionalMatcher` being used with a type that either
 // supports implicit conversion to bool or a `has_value()` method.
 template <typename Optional>
-auto IsOptionalEngaged(const Optional& optional,
-                       Rank1) -> decltype(!!optional) {
+auto IsOptionalEngaged(const Optional& optional, Rank1)
+    -> decltype(!!optional) {
   // The use of double-negation here is to preserve historical behavior where
   // the matcher used `operator!` rather than directly using `operator bool`.
   return !static_cast<bool>(!optional);
 }
 template <typename Optional>
-auto IsOptionalEngaged(const Optional& optional,
-                       Rank0) -> decltype(!optional.has_value()) {
+auto IsOptionalEngaged(const Optional& optional, Rank0)
+    -> decltype(!optional.has_value()) {
   return optional.has_value();
 }
 
@@ -4567,7 +4597,7 @@ WhenDynamicCastTo(const Matcher<To>& inner_matcher) {
 // matches a Foo object x if and only if x.number >= 5.
 template <typename Class, typename FieldType, typename FieldMatcher>
 inline PolymorphicMatcher<internal::FieldMatcher<Class, FieldType>> Field(
-    FieldType Class::*field, const FieldMatcher& matcher) {
+    FieldType Class::* field, const FieldMatcher& matcher) {
   return MakePolymorphicMatcher(internal::FieldMatcher<Class, FieldType>(
       field, MatcherCast<const FieldType&>(matcher)));
   // The call to MatcherCast() is required for supporting inner
@@ -4580,7 +4610,7 @@ inline PolymorphicMatcher<internal::FieldMatcher<Class, FieldType>> Field(
 // messages.
 template <typename Class, typename FieldType, typename FieldMatcher>
 inline PolymorphicMatcher<internal::FieldMatcher<Class, FieldType>> Field(
-    const std::string& field_name, FieldType Class::*field,
+    const std::string& field_name, FieldType Class::* field,
     const FieldMatcher& matcher) {
   return MakePolymorphicMatcher(internal::FieldMatcher<Class, FieldType>(
       field_name, field, MatcherCast<const FieldType&>(matcher)));

googlemock/test/gmock-matchers-containers_test.cc

@@ -33,6 +33,7 @@
 
 #include <algorithm>
 #include <array>
+#include <cstddef>
 #include <deque>
 #include <forward_list>
 #include <iterator>
@@ -1271,10 +1272,11 @@ TEST(WhenSortedByTest, CanDescribeSelf) {
 
 TEST(WhenSortedByTest, ExplainsMatchResult) {
   const int a[] = {2, 1};
-  EXPECT_EQ("which is { 1, 2 } when sorted, whose element #0 doesn't match",
-            Explain(WhenSortedBy(less<int>(), ElementsAre(2, 3)), a));
-  EXPECT_EQ("which is { 1, 2 } when sorted",
-            Explain(WhenSortedBy(less<int>(), ElementsAre(1, 2)), a));
+  EXPECT_EQ(
+      Explain(WhenSortedBy(less<int>(), ElementsAre(2, 3)), a),
+      "which is { 1, 2 } when sorted, whose element #0 (1) isn't equal to 2");
+  EXPECT_EQ(Explain(WhenSortedBy(less<int>(), ElementsAre(1, 2)), a),
+            "which is { 1, 2 } when sorted");
 }
 
 // WhenSorted() is a simple wrapper on WhenSortedBy().  Hence we don't
@@ -1775,6 +1777,295 @@ TEST(IsSubsetOfTest, WorksWithMoveOnly) {
   helper.Call(MakeUniquePtrs({2}));
 }
 
+// A container whose iterator returns a temporary. This can iterate over the
+// characters in a string.
+class CharString {
+ public:
+  using value_type = char;
+
+  class const_iterator {
+   public:
+    using iterator_category = std::input_iterator_tag;
+    using value_type = char;
+    using difference_type = std::ptrdiff_t;
+    using pointer = const char*;
+    using reference = const char&;
+
+    // Create an iterator that points to the given character.
+    explicit const_iterator(const char* ptr) : ptr_(ptr) {}
+
+    // Returns the current character. IMPORTANT: this must return a temporary,
+    // not a reference, to test that ElementsAre() works with containers whose
+    // iterators return temporaries.
+    char operator*() const { return *ptr_; }
+
+    // Advances to the next character.
+    const_iterator& operator++() {
+      ++ptr_;
+      return *this;
+    }
+
+    // Compares two iterators.
+    bool operator==(const const_iterator& other) const {
+      return ptr_ == other.ptr_;
+    }
+    bool operator!=(const const_iterator& other) const {
+      return ptr_ != other.ptr_;
+    }
+
+   private:
+    const char* ptr_ = nullptr;
+  };
+
+  // Creates a CharString that contains the given string.
+  explicit CharString(const std::string& s) : s_(s) {}
+
+  // Returns an iterator pointing to the first character in the string.
+  const_iterator begin() const { return const_iterator(s_.c_str()); }
+
+  // Returns an iterator pointing past the last character in the string.
+  const_iterator end() const { return const_iterator(s_.c_str() + s_.size()); }
+
+ private:
+  std::string s_;
+};
+
+// Tests using ElementsAre() with a container whose iterator returns a
+// temporary.
+TEST(ElementsAreTest, WorksWithContainerThatReturnsTempInIterator) {
+  CharString s("abc");
+  EXPECT_THAT(s, ElementsAre('a', 'b', 'c'));
+  EXPECT_THAT(s, Not(ElementsAre('a', 'b', 'd')));
+}
+
+// Tests using ElementsAreArray() with a container whose iterator returns a
+// temporary.
+TEST(ElementsAreArrayTest, WorksWithContainerThatReturnsTempInIterator) {
+  CharString s("abc");
+  EXPECT_THAT(s, ElementsAreArray({'a', 'b', 'c'}));
+  EXPECT_THAT(s, Not(ElementsAreArray({'a', 'b', 'd'})));
+}
+
+// A container whose iterator returns a temporary and is not copy-assignable.
+// This simulates the behavior of the proxy object returned by absl::StrSplit().
+class CharString2 {
+ public:
+  using value_type = char;
+
+  class const_iterator {
+   public:
+    using iterator_category = std::input_iterator_tag;
+    using value_type = char;
+    using difference_type = std::ptrdiff_t;
+    using pointer = const char*;
+    using reference = const char&;
+
+    // Make const_iterator copy-constructible but not copy-assignable,
+    // simulating the behavior of the proxy object returned by absl::StrSplit().
+    const_iterator(const const_iterator&) = default;
+    const_iterator& operator=(const const_iterator&) = delete;
+
+    // Create an iterator that points to the given character.
+    explicit const_iterator(const char* ptr) : ptr_(ptr) {}
+
+    // Returns the current character. IMPORTANT: this must return a temporary,
+    // not a reference, to test that ElementsAre() works with containers whose
+    // iterators return temporaries.
+    char operator*() const { return *ptr_; }
+
+    // Advances to the next character.
+    const_iterator& operator++() {
+      ++ptr_;
+      return *this;
+    }
+
+    // Compares two iterators.
+    bool operator==(const const_iterator& other) const {
+      return ptr_ == other.ptr_;
+    }
+    bool operator!=(const const_iterator& other) const {
+      return ptr_ != other.ptr_;
+    }
+
+   private:
+    const char* ptr_ = nullptr;
+  };
+
+  // Creates a CharString that contains the given string.
+  explicit CharString2(const std::string& s) : s_(s) {}
+
+  // Returns an iterator pointing to the first character in the string.
+  const_iterator begin() const { return const_iterator(s_.c_str()); }
+
+  // Returns an iterator pointing past the last character in the string.
+  const_iterator end() const { return const_iterator(s_.c_str() + s_.size()); }
+
+ private:
+  std::string s_;
+};
+
+// Tests using ElementsAre() with a container whose iterator returns a
+// temporary and is not copy-assignable.
+TEST(ElementsAreTest, WorksWithContainerThatReturnsTempInUnassignableIterator) {
+  CharString2 s("abc");
+  EXPECT_THAT(s, ElementsAre('a', 'b', 'c'));
+  EXPECT_THAT(s, Not(ElementsAre('a', 'b', 'd')));
+}
+
+// Tests using ElementsAreArray() with a container whose iterator returns a
+// temporary and is not copy-assignable.
+TEST(ElementsAreArrayTest,
+     WorksWithContainerThatReturnsTempInUnassignableIterator) {
+  CharString2 s("abc");
+  EXPECT_THAT(s, ElementsAreArray({'a', 'b', 'c'}));
+  EXPECT_THAT(s, Not(ElementsAreArray({'a', 'b', 'd'})));
+}
+
+// A container whose iterator returns a temporary and is neither
+// copy-constructible nor copy-assignable.
+class CharString3 {
+ public:
+  using value_type = char;
+
+  class const_iterator {
+   public:
+    using iterator_category = std::input_iterator_tag;
+    using value_type = char;
+    using difference_type = std::ptrdiff_t;
+    using pointer = const char*;
+    using reference = const char&;
+
+    // Make const_iterator neither copy-constructible nor copy-assignable.
+    const_iterator(const const_iterator&) = delete;
+    const_iterator& operator=(const const_iterator&) = delete;
+
+    // Create an iterator that points to the given character.
+    explicit const_iterator(const char* ptr) : ptr_(ptr) {}
+
+    // Returns the current character. IMPORTANT: this must return a temporary,
+    // not a reference, to test that ElementsAre() works with containers whose
+    // iterators return temporaries.
+    char operator*() const { return *ptr_; }
+
+    // Advances to the next character.
+    const_iterator& operator++() {
+      ++ptr_;
+      return *this;
+    }
+
+    // Compares two iterators.
+    bool operator==(const const_iterator& other) const {
+      return ptr_ == other.ptr_;
+    }
+    bool operator!=(const const_iterator& other) const {
+      return ptr_ != other.ptr_;
+    }
+
+   private:
+    const char* ptr_ = nullptr;
+  };
+
+  // Creates a CharString that contains the given string.
+  explicit CharString3(const std::string& s) : s_(s) {}
+
+  // Returns an iterator pointing to the first character in the string.
+  const_iterator begin() const { return const_iterator(s_.c_str()); }
+
+  // Returns an iterator pointing past the last character in the string.
+  const_iterator end() const { return const_iterator(s_.c_str() + s_.size()); }
+
+ private:
+  std::string s_;
+};
+
+// Tests using ElementsAre() with a container whose iterator returns a
+// temporary and is neither copy-constructible nor copy-assignable.
+TEST(ElementsAreTest, WorksWithContainerThatReturnsTempInUncopyableIterator) {
+  CharString3 s("abc");
+  EXPECT_THAT(s, ElementsAre('a', 'b', 'c'));
+  EXPECT_THAT(s, Not(ElementsAre('a', 'b', 'd')));
+}
+
+// Tests using ElementsAreArray() with a container whose iterator returns a
+// temporary and is neither copy-constructible nor copy-assignable.
+TEST(ElementsAreArrayTest,
+     WorksWithContainerThatReturnsTempInUncopyableIterator) {
+  CharString3 s("abc");
+  EXPECT_THAT(s, ElementsAreArray({'a', 'b', 'c'}));
+  EXPECT_THAT(s, Not(ElementsAreArray({'a', 'b', 'd'})));
+}
+
+// A container whose iterator returns a temporary, is neither
+// copy-constructible nor copy-assignable, and has no member types.
+class CharString4 {
+ public:
+  using value_type = char;
+
+  class const_iterator {
+   public:
+    // Do not define difference_type, etc.
+
+    // Make const_iterator neither copy-constructible nor copy-assignable.
+    const_iterator(const const_iterator&) = delete;
+    const_iterator& operator=(const const_iterator&) = delete;
+
+    // Create an iterator that points to the given character.
+    explicit const_iterator(const char* ptr) : ptr_(ptr) {}
+
+    // Returns the current character. IMPORTANT: this must return a temporary,
+    // not a reference, to test that ElementsAre() works with containers whose
+    // iterators return temporaries.
+    char operator*() const { return *ptr_; }
+
+    // Advances to the next character.
+    const_iterator& operator++() {
+      ++ptr_;
+      return *this;
+    }
+
+    // Compares two iterators.
+    bool operator==(const const_iterator& other) const {
+      return ptr_ == other.ptr_;
+    }
+    bool operator!=(const const_iterator& other) const {
+      return ptr_ != other.ptr_;
+    }
+
+   private:
+    const char* ptr_ = nullptr;
+  };
+
+  // Creates a CharString that contains the given string.
+  explicit CharString4(const std::string& s) : s_(s) {}
+
+  // Returns an iterator pointing to the first character in the string.
+  const_iterator begin() const { return const_iterator(s_.c_str()); }
+
+  // Returns an iterator pointing past the last character in the string.
+  const_iterator end() const { return const_iterator(s_.c_str() + s_.size()); }
+
+ private:
+  std::string s_;
+};
+
+// Tests using ElementsAre() with a container whose iterator returns a
+// temporary, is neither copy-constructible nor copy-assignable, and has no
+// member types.
+TEST(ElementsAreTest, WorksWithContainerWithIteratorWithNoMemberTypes) {
+  CharString4 s("abc");
+  EXPECT_THAT(s, ElementsAre('a', 'b', 'c'));
+  EXPECT_THAT(s, Not(ElementsAre('a', 'b', 'd')));
+}
+
+// Tests using ElementsAreArray() with a container whose iterator returns a
+// temporary, is neither copy-constructible nor copy-assignable, and has no
+// member types.
+TEST(ElementsAreArrayTest, WorksWithContainerWithIteratorWithNoMemberTypes) {
+  CharString4 s("abc");
+  EXPECT_THAT(s, ElementsAreArray({'a', 'b', 'c'}));
+  EXPECT_THAT(s, Not(ElementsAreArray({'a', 'b', 'd'})));
+}
+
 // Tests using ElementsAre() and ElementsAreArray() with stream-like
 // "containers".
 
@@ -2155,7 +2446,7 @@ TEST_P(EachTestP, ExplainsMatchResultCorrectly) {
   Matcher<set<int>> m = Each(2);
   EXPECT_EQ("", Explain(m, a));
 
-  Matcher<const int(&)[1]> n = Each(1);  // NOLINT
+  Matcher<const int (&)[1]> n = Each(1);  // NOLINT
 
   const int b[1] = {1};
   EXPECT_EQ("", Explain(n, b));
@@ -2290,7 +2581,7 @@ TEST(PointwiseTest, MakesCopyOfRhs) {
   rhs.push_back(4);
 
   int lhs[] = {1, 2};
-  const Matcher<const int(&)[2]> m = Pointwise(IsHalfOf(), rhs);
+  const Matcher<const int (&)[2]> m = Pointwise(IsHalfOf(), rhs);
   EXPECT_THAT(lhs, m);
 
   // Changing rhs now shouldn't affect m, which made a copy of rhs.
@@ -2418,7 +2709,7 @@ TEST(UnorderedPointwiseTest, MakesCopyOfRhs) {
   rhs.push_back(4);
 
   int lhs[] = {2, 1};
-  const Matcher<const int(&)[2]> m = UnorderedPointwise(IsHalfOf(), rhs);
+  const Matcher<const int (&)[2]> m = UnorderedPointwise(IsHalfOf(), rhs);
   EXPECT_THAT(lhs, m);
 
   // Changing rhs now shouldn't affect m, which made a copy of rhs.
@@ -2669,11 +2960,11 @@ TEST_P(ElementsAreTestP, CanExplainMismatchRightSize) {
   vector<int> v;
   v.push_back(2);
   v.push_back(1);
-  EXPECT_EQ("whose element #0 doesn't match", Explain(m, v));
+  EXPECT_EQ(Explain(m, v), "whose element #0 (2) isn't equal to 1");
 
   v[0] = 1;
-  EXPECT_EQ("whose element #1 doesn't match, which is 4 less than 5",
-            Explain(m, v));
+  EXPECT_EQ(Explain(m, v),
+            "whose element #1 (1) is <= 5, which is 4 less than 5");
 }
 
 TEST(ElementsAreTest, MatchesOneElementVector) {
@@ -3073,7 +3364,7 @@ TEST(ContainsTest, SetDoesNotMatchWhenElementIsNotInContainer) {
 
 TEST_P(ContainsTestP, ExplainsMatchResultCorrectly) {
   const int a[2] = {1, 2};
-  Matcher<const int(&)[2]> m = Contains(2);
+  Matcher<const int (&)[2]> m = Contains(2);
   EXPECT_EQ("whose element #1 matches", Explain(m, a));
 
   m = Contains(3);