From 17d335d7c7f15d989516255471c3d7f5d204308d Mon Sep 17 00:00:00 2001
From: Justin Bassett <jbassett@google.com>
Date: Thu, 30 Oct 2025 21:51:50 -0700
Subject: [PATCH] Remove short-circuiting from AllOf, for better failure
 messages

For `EXPECT_THAT` matcher usage, showing only the first failure meant
that users would sometimes have to make a fix and run the test again
only to notice that there's another failure. It's better to show more
failures so that the user can fix several issues in one go.

In practice, very little code actually wants the short circuiting here,
only a handful of cases with custom matchers used like
`AllOf(BoundsCheck(), UncheckedAccess())`. These cases are fixable by
refactoring `UncheckedAccess()` to instead also apply a bounds check to
fail the matcher rather than crash. Notably, this change doesn't affect
`AnyOf`, so another workaround is to change `AllOf(m1, m2, ...)` into
`Not(AnyOf(Not(m1), Not(m2), ...))`.

PiperOrigin-RevId: 826316273
Change-Id: Ie8186f75c10443d8da35b5d07b6a8cd9ae85b451
---
 googlemock/include/gmock/gmock-matchers.h          | 14 +++++++++++---
 googlemock/test/gmock-matchers-arithmetic_test.cc  |  3 ++-
 googlemock/test/gmock-matchers-comparisons_test.cc | 11 ++++-------
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/googlemock/include/gmock/gmock-matchers.h b/googlemock/include/gmock/gmock-matchers.h
index 236d3e5f0..666baaf79 100644
--- a/googlemock/include/gmock/gmock-matchers.h
+++ b/googlemock/include/gmock/gmock-matchers.h
@@ -1329,17 +1329,23 @@ class AllOfMatcherImpl : public MatcherInterface<const T&> {
     // However, if matcher doesn't provide one, this method uses matcher's
     // description.
     std::string all_match_result;
+    bool success = true;
     for (const Matcher<T>& matcher : matchers_) {
       StringMatchResultListener slistener;
       // Return explanation for first failed matcher.
       if (!matcher.MatchAndExplain(x, &slistener)) {
         const std::string explanation = slistener.str();
+        if (!success) {
+          // Already had a failure.
+          *listener << ", and ";
+        }
         if (!explanation.empty()) {
           *listener << explanation;
         } else {
           *listener << "which doesn't match (" << Describe(matcher) << ")";
         }
-        return false;
+        success = false;
+        continue;
       }
       // Keep track of explanations in case all matchers succeed.
       std::string explanation = slistener.str();
@@ -1356,8 +1362,10 @@ class AllOfMatcherImpl : public MatcherInterface<const T&> {
       }
     }
 
-    *listener << all_match_result;
-    return true;
+    if (success) {
+      *listener << all_match_result;
+    }
+    return success;
   }
 
  private:
diff --git a/googlemock/test/gmock-matchers-arithmetic_test.cc b/googlemock/test/gmock-matchers-arithmetic_test.cc
index c4c91b8b3..40a170467 100644
--- a/googlemock/test/gmock-matchers-arithmetic_test.cc
+++ b/googlemock/test/gmock-matchers-arithmetic_test.cc
@@ -770,7 +770,8 @@ TEST_P(AllOfTestP, ExplainsResult) {
   // Failed match.  The first matcher, which failed, needs to
   // explain.
   m = AllOf(GreaterThan(10), GreaterThan(20));
-  EXPECT_EQ("which is 5 less than 10", Explain(m, 5));
+  EXPECT_EQ("which is 5 less than 10, and which is 15 less than 20",
+            Explain(m, 5));
 
   // Failed match.  The second matcher, which failed, needs to
   // explain.  Since it doesn't given an explanation, the matcher text is
diff --git a/googlemock/test/gmock-matchers-comparisons_test.cc b/googlemock/test/gmock-matchers-comparisons_test.cc
index 413c2bb0e..cd2b80a0e 100644
--- a/googlemock/test/gmock-matchers-comparisons_test.cc
+++ b/googlemock/test/gmock-matchers-comparisons_test.cc
@@ -2389,22 +2389,19 @@ PolymorphicMatcher<DivisibleByImpl> DivisibleBy(int n) {
   return MakePolymorphicMatcher(DivisibleByImpl(n));
 }
 
-// Tests that when AllOf() fails, only the first failing matcher is
-// asked to explain why.
+// Tests that when AllOf() fails, all failing matchers are asked to explain why.
 TEST(ExplainMatchResultTest, AllOf_False_False) {
   const Matcher<int> m = AllOf(DivisibleBy(4), DivisibleBy(3));
-  EXPECT_EQ("which is 1 modulo 4", Explain(m, 5));
+  EXPECT_EQ("which is 1 modulo 4, and which is 2 modulo 3", Explain(m, 5));
 }
 
-// Tests that when AllOf() fails, only the first failing matcher is
-// asked to explain why.
+// Tests that when AllOf() fails, all failing matchers are asked to explain why.
 TEST(ExplainMatchResultTest, AllOf_False_True) {
   const Matcher<int> m = AllOf(DivisibleBy(4), DivisibleBy(3));
   EXPECT_EQ("which is 2 modulo 4", Explain(m, 6));
 }
 
-// Tests that when AllOf() fails, only the first failing matcher is
-// asked to explain why.
+// Tests that when AllOf() fails, all failing matchers are asked to explain why.
 TEST(ExplainMatchResultTest, AllOf_True_False) {
   const Matcher<int> m = AllOf(Ge(1), DivisibleBy(3));
   EXPECT_EQ("which is 2 modulo 3", Explain(m, 5));