Fix significant bug with storing composed matchers

Given that in the 2 or so years that matchers are thing nobody complained, it seems that people do not actually write this sort of code, and the possibility will be removed in v3. However, to avoid correctness bugs, we will have to support this weird code in v2.
catchorg · Feb 1, 2020 · f20a9db · f20a9db
1 parent 273c3f8
commit f20a9db
Show file tree

Hide file tree

Showing 9 changed files with 94 additions and 13 deletions.
diff --git a/docs/deprecations.md b/docs/deprecations.md
@@ -45,6 +45,34 @@ There should be no reason to ever have an empty `SourceLineInfo`, so the
 method will be removed.
 
 
+### Composing lvalues of already composed matchers
+
+Because a significant bug in this use case has persisted for 2+ years
+without a bug report, and to simplify the implementation, code that
+composes lvalues of composed matchers will not compile. That is,
+this code will no longer work:
+
+```cpp
+            auto m1 = Contains("string");
+            auto m2 = Contains("random");
+            auto composed1 = m1 || m2;
+            auto m3 = Contains("different");
+            auto composed2 = composed1 || m3;
+            REQUIRE_THAT(foo(), !composed1);
+            REQUIRE_THAT(foo(), composed2);
+```
+
+Instead you will have to write this:
+
+```cpp
+            auto m1 = Contains("string");
+            auto m2 = Contains("random");
+            auto m3 = Contains("different");
+            REQUIRE_THAT(foo(), !(m1 || m2));
+            REQUIRE_THAT(foo(), m1 || m2 || m3);
+```
+
+
 ## Planned changes
 
 

diff --git a/include/internal/catch_matchers.h b/include/internal/catch_matchers.h
@@ -91,9 +91,10 @@ namespace Matchers {
                 return description;
             }
 
-            MatchAllOf<ArgT>& operator && ( MatcherBase<ArgT> const& other ) {
-                m_matchers.push_back( &other );
-                return *this;
+            MatchAllOf<ArgT> operator && ( MatcherBase<ArgT> const& other ) {
+                auto copy(*this);
+                copy.m_matchers.push_back( &other );
+                return copy;
             }
 
             std::vector<MatcherBase<ArgT> const*> m_matchers;
@@ -124,9 +125,10 @@ namespace Matchers {
                 return description;
             }
 
-            MatchAnyOf<ArgT>& operator || ( MatcherBase<ArgT> const& other ) {
-                m_matchers.push_back( &other );
-                return *this;
+            MatchAnyOf<ArgT> operator || ( MatcherBase<ArgT> const& other ) {
+                auto copy(*this);
+                copy.m_matchers.push_back( &other );
+                return copy;
             }
 
             std::vector<MatcherBase<ArgT> const*> m_matchers;

diff --git a/projects/SelfTest/Baselines/compact.sw.approved.txt b/projects/SelfTest/Baselines/compact.sw.approved.txt
@@ -312,6 +312,8 @@ Condition.tests.cpp:<line number>: passed: 4 == ul for: 4 == 4
 Condition.tests.cpp:<line number>: passed: 5 == c for: 5 == 5
 Condition.tests.cpp:<line number>: passed: 6 == uc for: 6 == 6
 Condition.tests.cpp:<line number>: passed: (std::numeric_limits<uint32_t>::max)() > ul for: 4294967295 (0x<hex digits>) > 4
+Matchers.tests.cpp:<line number>: passed: testStringForMatching2(), !composed1 for: "some completely different text that contains one common word" not ( contains: "string" or contains: "random" )
+Matchers.tests.cpp:<line number>: passed: testStringForMatching2(), composed2 for: "some completely different text that contains one common word" ( contains: "string" or contains: "random" or contains: "different" )
 Matchers.tests.cpp:<line number>: failed: testStringForMatching(), Contains("not there", Catch::CaseSensitive::No) for: "this string contains 'abc' as a substring" contains: "not there" (case insensitive)
 Matchers.tests.cpp:<line number>: failed: testStringForMatching(), Contains("STRING") for: "this string contains 'abc' as a substring" contains: "STRING"
 Generators.tests.cpp:<line number>: passed: elem % 2 == 1 for: 1 == 1

diff --git a/projects/SelfTest/Baselines/console.std.approved.txt b/projects/SelfTest/Baselines/console.std.approved.txt
@@ -1380,6 +1380,6 @@ due to unexpected exception with message:
   Why would you throw a std::string?
 
 ===============================================================================
-test cases:  305 |  231 passed |  70 failed |  4 failed as expected
-assertions: 1664 | 1512 passed | 131 failed | 21 failed as expected
+test cases:  306 |  232 passed |  70 failed |  4 failed as expected
+assertions: 1666 | 1514 passed | 131 failed | 21 failed as expected
 
diff --git a/projects/SelfTest/Baselines/console.sw.approved.txt b/projects/SelfTest/Baselines/console.sw.approved.txt
@@ -2390,6 +2390,24 @@ Condition.tests.cpp:<line number>: PASSED:
 with expansion:
   4294967295 (0x<hex digits>) > 4
 
+-------------------------------------------------------------------------------
+Composed matchers are distinct
+-------------------------------------------------------------------------------
+Matchers.tests.cpp:<line number>
+...............................................................................
+
+Matchers.tests.cpp:<line number>: PASSED:
+  REQUIRE_THAT( testStringForMatching2(), !composed1 )
+with expansion:
+  "some completely different text that contains one common word" not (
+  contains: "string" or contains: "random" )
+
+Matchers.tests.cpp:<line number>: PASSED:
+  REQUIRE_THAT( testStringForMatching2(), composed2 )
+with expansion:
+  "some completely different text that contains one common word" ( contains:
+  "string" or contains: "random" or contains: "different" )
+
 -------------------------------------------------------------------------------
 Contains string matcher
 -------------------------------------------------------------------------------
@@ -13302,6 +13320,6 @@ Misc.tests.cpp:<line number>
 Misc.tests.cpp:<line number>: PASSED:
 
 ===============================================================================
-test cases:  305 |  215 passed |  86 failed |  4 failed as expected
-assertions: 1681 | 1512 passed | 148 failed | 21 failed as expected
+test cases:  306 |  216 passed |  86 failed |  4 failed as expected
+assertions: 1683 | 1514 passed | 148 failed | 21 failed as expected
 
diff --git a/projects/SelfTest/Baselines/junit.sw.approved.txt b/projects/SelfTest/Baselines/junit.sw.approved.txt
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <testsuitesloose text artifact
 >
-  <testsuite name="<exe-name>" errors="17" failures="132" tests="1682" hostname="tbd" time="{duration}" timestamp="{iso8601-timestamp}">
+  <testsuite name="<exe-name>" errors="17" failures="132" tests="1684" hostname="tbd" time="{duration}" timestamp="{iso8601-timestamp}">
     <properties>
       <property name="filters" value="~[!nonportable]~[!benchmark]~[approvals]"/>
       <property name="random-seed" value="1"/>
@@ -356,6 +356,7 @@ Exception.tests.cpp:<line number>
     <testcase classname="<exe-name>.global" name="Comparisons between ints where one side is computed" time="{duration}"/>
     <testcase classname="<exe-name>.global" name="Comparisons between unsigned ints and negative signed ints match c++ standard behaviour" time="{duration}"/>
     <testcase classname="<exe-name>.global" name="Comparisons with int literals don't warn when mixing signed/ unsigned" time="{duration}"/>
+    <testcase classname="<exe-name>.global" name="Composed matchers are distinct" time="{duration}"/>
     <testcase classname="<exe-name>.global" name="Contains string matcher" time="{duration}">
       <failure message="testStringForMatching(), Contains(&quot;not there&quot;, Catch::CaseSensitive::No)" type="CHECK_THAT">
 FAILED:

diff --git a/projects/SelfTest/Baselines/sonarqube.sw.approved.txt b/projects/SelfTest/Baselines/sonarqube.sw.approved.txt
@@ -892,6 +892,7 @@ Exception.tests.cpp:<line number>
   <file path="projects/<exe-name>/UsageTests/Matchers.tests.cpp">
     <testCase name="Arbitrary predicate matcher/Function pointer" duration="{duration}"/>
     <testCase name="Arbitrary predicate matcher/Lambdas + different type" duration="{duration}"/>
+    <testCase name="Composed matchers are distinct" duration="{duration}"/>
     <testCase name="Contains string matcher" duration="{duration}">
       <failure message="CHECK_THAT(testStringForMatching(), Contains(&quot;not there&quot;, Catch::CaseSensitive::No))">
 FAILED:

diff --git a/projects/SelfTest/Baselines/xml.sw.approved.txt b/projects/SelfTest/Baselines/xml.sw.approved.txt
@@ -2832,6 +2832,25 @@ Nor would this
       </Expression>
       <OverallResult success="true"/>
     </TestCase>
+    <TestCase name="Composed matchers are distinct" tags="[composed][matchers]" filename="projects/<exe-name>/UsageTests/Matchers.tests.cpp" >
+      <Expression success="true" type="REQUIRE_THAT" filename="projects/<exe-name>/UsageTests/Matchers.tests.cpp" >
+        <Original>
+          testStringForMatching2(), !composed1
+        </Original>
+        <Expanded>
+          "some completely different text that contains one common word" not ( contains: "string" or contains: "random" )
+        </Expanded>
+      </Expression>
+      <Expression success="true" type="REQUIRE_THAT" filename="projects/<exe-name>/UsageTests/Matchers.tests.cpp" >
+        <Original>
+          testStringForMatching2(), composed2
+        </Original>
+        <Expanded>
+          "some completely different text that contains one common word" ( contains: "string" or contains: "random" or contains: "different" )
+        </Expanded>
+      </Expression>
+      <OverallResult success="true"/>
+    </TestCase>
     <TestCase name="Contains string matcher" tags="[!hide][.][failing][matchers]" filename="projects/<exe-name>/UsageTests/Matchers.tests.cpp" >
       <Expression success="false" type="CHECK_THAT" filename="projects/<exe-name>/UsageTests/Matchers.tests.cpp" >
         <Original>
@@ -15893,7 +15912,7 @@ loose text artifact
       </Section>
       <OverallResult success="true"/>
     </TestCase>
-    <OverallResults successes="1512" failures="149" expectedFailures="21"/>
+    <OverallResults successes="1514" failures="149" expectedFailures="21"/>
   </Group>
-  <OverallResults successes="1512" failures="148" expectedFailures="21"/>
+  <OverallResults successes="1514" failures="148" expectedFailures="21"/>
 </Catch>
diff --git a/projects/SelfTest/UsageTests/Matchers.tests.cpp b/projects/SelfTest/UsageTests/Matchers.tests.cpp
@@ -552,6 +552,16 @@ namespace { namespace MatchersTests {
             REQUIRE_THROWS_MATCHES(throwsSpecialException(2), SpecialException, !Message("DerivedException::what"));
             REQUIRE_THROWS_MATCHES(throwsSpecialException(2), SpecialException,  Message("SpecialException::what"));
         }
+
+        TEST_CASE("Composed matchers are distinct", "[matchers][composed]") {
+            auto m1 = Contains("string");
+            auto m2 = Contains("random");
+            auto composed1 = m1 || m2;
+            auto m3 = Contains("different");
+            auto composed2 = composed1 || m3;
+            REQUIRE_THAT(testStringForMatching2(), !composed1);
+            REQUIRE_THAT(testStringForMatching2(), composed2);
+        }
 
 } } // namespace MatchersTests