Merge pull request #1585 from Zalathar/shrink-offset-pairs

Fix a rare IndexError in shrink_offset_pairs

hypothesis-python/RELEASE.rst

@@ -0,0 +1,5 @@
+RELEASE_TYPE: patch
+
+This patch fixes a rare bug that would cause a particular shrinker pass to
+raise an IndexError, if a shrink improvement changed the underlying data
+in an unexpected way.

hypothesis-python/src/hypothesis/internal/conjecture/engine.py

@@ -1904,7 +1904,8 @@ def reoffset(o):
             self.clear_change_tracking()
 
     def shrink_offset_pairs(self):
-        """Lower any two blocks offset from each other the same ammount.
+        """Lowers pairs of blocks that need to maintain a constant difference
+        between their respective values.
 
         Before this shrink pass, two blocks explicitly offset from each
         other would not get minimized properly:
@@ -1914,12 +1915,13 @@ def shrink_offset_pairs(self):
 
         This expensive (O(n^2)) pass goes through every pair of non-zero
         blocks in the current shrink target and sees if the shrink
-        target can be improved by applying an offset to both of them.
+        target can be improved by applying a negative offset to both of them.
         """
-        current = [self.shrink_target.buffer[u:v] for u, v in self.blocks]
 
         def int_from_block(i):
-            return int_from_bytes(current[i])
+            u, v = self.blocks[i]
+            block_bytes = self.shrink_target.buffer[u:v]
+            return int_from_bytes(block_bytes)
 
         def block_len(i):
             u, v = self.blocks[i]
@@ -1951,16 +1953,13 @@ def reoffset_pair(pair, o):
         while i < len(self.blocks):
             if self.is_payload_block(i) and int_from_block(i) > 0:
                 j = i + 1
-                while j < len(self.shrink_target.blocks):
+                while j < len(self.blocks):
                     block_val = int_from_block(j)
                     i_block_val = int_from_block(i)
                     if self.is_payload_block(j) \
                        and block_val > 0 and i_block_val > 0:
                         offset = min(int_from_block(i),
                                      int_from_block(j))
-                        # Save current before shrinking
-                        current = [self.shrink_target.buffer[u:v]
-                                   for u, v in self.blocks]
                         Integer.shrink(
                             offset, lambda o: reoffset_pair((i, j), o),
                             random=self.random

hypothesis-python/tests/cover/test_conjecture_engine.py

@@ -57,6 +57,34 @@ def run_to_buffer(f):
         return hbytes(last_data.buffer)
 
 
+def shrink_to_data(buffer):
+    """Decorator that takes a test function and an interesting input buffer,
+    and runs the shrinker on that buffer."""
+
+    def decorator(f):
+        runner = ConjectureRunner(
+            f,
+            random=Random(0),
+            settings=settings(
+                phases=[Phase.shrink],
+                database=None,
+            ),
+        )
+
+        # Explicitly test the given buffer, so that the shrinker has
+        # a starting point.
+        data = ConjectureData.for_buffer(buffer)
+        runner.test_function(data)
+        assert data.status == Status.INTERESTING, \
+            'Initial buffer was not interesting'
+
+        runner.run()
+
+        return runner.interesting_examples[data.interesting_origin]
+
+    return decorator
+
+
 def test_can_index_results():
     @run_to_buffer
     def f(data):

hypothesis-python/tests/cover/test_simple_collections.py

@@ -124,6 +124,17 @@ def test_lists_of_lower_bounded_length(n):
 
 
 def test_can_find_unique_lists_of_non_set_order():
+    # This test checks that our strategy for unique lists doesn't accidentally
+    # depend on the iteration order of sets.
+    #
+    # Unfortunately, that means that *this* test has to rely on set iteration
+    # order. That makes it tricky to debug on CPython, because set iteration
+    # order changes every time the process is launched.
+    #
+    # To get around this, define the PYTHONHASHSEED environment variable to
+    # a consistent value. This could be 0, or it could be the PYTHONHASHSEED
+    # value listed in a failure log from CI.
+
     ls = minimal(
         lists(text(), unique=True),
         lambda x: list(set(reversed(x))) != x

hypothesis-python/tests/nocover/test_conjecture_engine.py

@@ -26,7 +26,8 @@
 from tests.common.utils import no_shrink, non_covering_examples
 from hypothesis.database import InMemoryExampleDatabase
 from hypothesis.internal.compat import hbytes, hrange, int_from_bytes
-from tests.cover.test_conjecture_engine import run_to_buffer
+from tests.cover.test_conjecture_engine import run_to_buffer, \
+    shrink_to_data
 from hypothesis.internal.conjecture.data import Status, ConjectureData
 from hypothesis.internal.conjecture.engine import RunIsComplete, \
     ConjectureRunner
@@ -153,6 +154,35 @@ def x(data):
     assert int_from_bytes(x[-2:]) in (254, 512)
 
 
+def test_shrink_offset_pairs_handles_block_structure_change():
+    """Regression test for a rare error in ``shrink_offset_pairs``.
+
+    This test should run without raising an ``IndexError`` in the
+    shrinker.
+    """
+
+    @shrink_to_data(buffer=[235, 0, 0, 255])
+    def f(data):
+        x = data.draw_bytes(1)[0]
+
+        # Change the block structure in response to a shrink improvement,
+        # to trigger the bug.
+        if x == 10:
+            data.draw_bytes(1)
+            data.draw_bytes(1)
+        else:
+            data.draw_bytes(2)
+
+        y = data.draw_bytes(1)[0]
+
+        # Require the target blocks to be non-trivial and have a fixed
+        # difference, so that the intended shrinker pass is used.
+        if x >= 10 and y - x == 20:
+            data.mark_interesting()
+
+    assert f.buffer == hbytes([10, 0, 0, 30])
+
+
 @given(st.integers(0, 255), st.integers(0, 255))
 def test_prescreen_with_masked_byte_agrees_with_results(byte_a, byte_b):
     def f(data):