Merge pull request #1751 from HypothesisWorks/DRMacIver/each-pair-ordering

Iterate over block pairs in a different order.

Author: DRMacIver

hypothesis-python/RELEASE.rst

@@ -0,0 +1,6 @@
+RELEASE_TYPE: patch
+
+This changes the order that the shrinker tries certain operations in its "emergency" phase which runs late in the process.
+The new order should be better at avoiding long stalls where the shrinker is failing to make progress,
+which may be helpful if you have difficult to shrink test cases.
+However this will not be noticeable in the vast majority of use cases.

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

@@ -570,24 +570,30 @@ def each_pair_of_blocks(self, accept_first, accept_second):
         """Yield each pair of blocks ``(a, b)``, such that ``a.index <
         b.index``, but only if ``accept_first(a)`` and ``accept_second(b)`` are
         both true."""
-        i = 0
-        while i < len(self.blocks):
-            j = i + 1
-            while j < len(self.blocks):
+
+        # Iteration order here is significant: Rather than fixing i and looping
+        # over each j, then doing the same, etc. we iterate over the gap between
+        # i and j and then over i. The reason for this is that it ensures that
+        # we try a different value for i and j on each iteration of the inner
+        # loop. This stops us from stalling if we happen to hit on a value of i
+        # where nothing useful can be done.
+        #
+        # In the event that nothing works, this doesn't help and we still make
+        # the same number of calls, but by ensuring that we make progress we
+        # have more opportunities to make shrinks that speed up the tests or
+        # that reduce the number of viable shrinks at the next gap size because
+        # we've lowered some values.
+        offset = 1
+        while offset < len(self.blocks):
+            i = 0
+            while i + offset < len(self.blocks):
+                j = i + offset
                 block_i = self.blocks[i]
-                if not accept_first(block_i):
-                    break
                 block_j = self.blocks[j]
-                if not accept_second(block_j):
-                    j += 1
-                    continue
-
-                yield (block_i, block_j)
-                # After this point, the shrink target could have changed,
-                # so blocks need to be re-checked.
-
-                j += 1
-            i += 1
+                if accept_first(block_i) and accept_second(block_j):
+                    yield (block_i, block_j)
+                i += 1
+            offset += 1
 
     def pass_to_descendant(self):
         """Attempt to replace each example with a descendant example.

hypothesis-python/tests/nocover/test_conjecture_engine.py

@@ -214,12 +214,12 @@ def shrinker(data):
         data.draw_bits(1)
         data.mark_interesting()
 
-    bounds = [
+    bounds = {
         (a.bounds, b.bounds)
         for a, b in shrinker.each_pair_of_blocks(lambda block: True, lambda block: True)
-    ]
+    }
 
-    assert bounds == [((0, 1), (1, 2)), ((0, 1), (2, 3)), ((1, 2), (2, 3))]
+    assert bounds == {((0, 1), (1, 2)), ((0, 1), (2, 3)), ((1, 2), (2, 3))}
 
 
 def test_each_pair_of_blocks_with_filters():
@@ -231,17 +231,18 @@ def shrinker(data):
             data.draw_bits(1)
         data.mark_interesting()
 
-    blocks = [
+    blocks = {
         (a.index, b.index)
         for a, b in shrinker.each_pair_of_blocks(
             lambda block: block.index != 1, lambda block: block.index != 3
         )
-    ]
+    }
 
-    assert blocks == [(0, 1), (0, 2), (0, 4), (2, 4), (3, 4)]
+    assert blocks == {(0, 1), (0, 2), (0, 4), (2, 4), (3, 4)}
 
 
-def test_each_pair_of_blocks_handles_change():
+@pytest.mark.parametrize("intervene_at", [(0, 1), (0, 6), (1, 3)])
+def test_each_pair_of_blocks_handles_change(intervene_at):
     initial = hbytes([9] + [0] * 10)
 
     @shrinking_from(initial)
@@ -251,20 +252,28 @@ def shrinker(data):
             data.draw_bits(1)
         data.mark_interesting()
 
-    blocks = []
-    for a, b in shrinker.each_pair_of_blocks(lambda block: True, lambda block: True):
-        if a.index == 0 and b.index == 6:
-            shrinker.incorporate_new_buffer(hbytes([3] + [0] * 10))
-        blocks.append((a.index, b.index))
-
-    assert blocks == [
-        (0, 1),
-        (0, 2),
-        (0, 3),
-        (0, 4),
-        (0, 5),
-        (0, 6),
-        (1, 2),
-        (1, 3),
-        (2, 3),
-    ]
+    def blocks(intervene=False):
+        blocks = []
+        for a, b in shrinker.each_pair_of_blocks(
+            lambda block: True, lambda block: True
+        ):
+            assert a.index < b.index < len(shrinker.shrink_target.blocks)
+            if intervene and (a.index, b.index) == intervene_at:
+                shrinker.incorporate_new_buffer(hbytes([3] + [0] * 10))
+            blocks.append((a.index, b.index))
+        return blocks
+
+    original_blocks = blocks()
+    blocks_with_intervention = blocks(intervene=True)
+    blocks_after_intervention = blocks()
+
+    # We should not abort when the change happens but should carry on to include
+    # every pair of indices that we would have if we'd iterated after the change.
+    assert set(blocks_after_intervention).issubset(blocks_with_intervention)
+
+    # Changing the iteration order shouldn't introduce new possible block pairs.
+    assert set(blocks_with_intervention).issubset(original_blocks)
+
+    # We should however not do that by repeating any pairs of block indexes -
+    # repetition should happen the next time the relevant passes are run.
+    assert len(set(blocks_with_intervention)) == len(blocks_with_intervention)