Shrink one_of/1 towards earlier elements first

lib/stream_data.ex

@@ -423,9 +423,9 @@ defmodule StreamData do
 
   ## Shrinking
 
-  The generator returned by `bind/2` shrinks by first shrinking the value
-  generated by the inner generator and then by shrinking the outer generator
-  given as `data`. When `data` shrinks, `fun` is once more applied on the
+  The generator returned by `bind/2` shrinks by first shrinking the outer
+  generator given as `data` and then by shrinking the value generated by the
+  inner generator. When `data` shrinks, `fun` is once more applied on the
   shrunk value and returns a whole new generator, which will most likely
   emit new items.
   """
@@ -776,29 +776,9 @@ defmodule StreamData do
 
   ## Shrinking
 
-  Each generated value is shrunk, and then this generator shrinks towards
-  values generated by generators earlier in the list of `frequencies`.
+  This generator first shrinks towards values generated by generators earlier
+  in the list of `frequencies`, and then shrinks the generated value itself.
   """
-  # Right now, it shrinks by first shrinking the generated value, and then
-  # shrinking towards earlier generators in "frequencies". Clojure shrinks
-  # towards earlier generators *first*, and then shrinks the generated value.
-  # An implementation that does this can be:
-  #
-  #     new(fn seed, size ->
-  #       {frequency, next_seed} = uniform_in_range(0..sum - 1, seed)
-  #       index = pick_index(Enum.map(frequencies, &elem(&1, 0)), frequency)
-  #       {_frequency, data} = Enum.fetch!(frequencies, index)
-  #
-  #       tree = call(data, next_seed, size)
-  #
-  #       earlier_children =
-  #         frequencies
-  #         |> Stream.take(index)
-  #         |> Stream.map(&call(elem(&1, 1), seed2, size))
-  #
-  #       %Lazytree{root: tree.root, children: Stream.concat(earlier_children, tree.children)}
-  #     end)
-  #
   @spec frequency([{pos_integer(), t(a)}]) :: t(a) when a: term()
   def frequency(frequencies) when is_list(frequencies) do
     sum = List.foldl(frequencies, 0, fn {frequency, _data}, acc -> acc + frequency end)
@@ -827,9 +807,8 @@ defmodule StreamData do
 
   ## Shrinking
 
-  The generated value will be shrunk first according to the generator that
-  generated it, and then this generator will shrink towards earlier generators
-  in `datas`.
+  This generator first shrinks towards earlier generators in `datas`, and then
+  shrinks the generated value according to the generator that generated it.
   """
   @spec one_of([t(a)]) :: t(a) when a: term()
   def one_of([_ | _] = datas) do
@@ -2577,7 +2556,7 @@ defmodule StreamData do
 
       {:error, reason} ->
         shrinking_result =
-          shrink_failure(shrink_initial_cont(children), nil, reason, fun, 0, config)
+          shrink_failure(shrink_initial_cont(children), reason, fun, 0, config)
           |> Map.put(:original_failure, reason)
           |> Map.put(:successful_runs, runs)
 
@@ -2589,30 +2568,24 @@ defmodule StreamData do
     &Enumerable.reduce(nodes, &1, fn elem, acc -> {:suspend, [elem | acc]} end)
   end
 
-  defp shrink_failure(_cont, _parent_cont, smallest, _fun, nodes_visited, %{
+  defp shrink_failure(_cont, smallest, _fun, nodes_visited, %{
          max_shrinking_steps: nodes_visited
        }) do
     %{shrunk_failure: smallest, nodes_visited: nodes_visited}
   end
 
-  # We try to get the next element out of the current nodes. If the current
-  # nodes are finished, we check if this was the first check: if it was, it
-  # means we were visiting children of a node and this node has no children, so
-  # we recurse on the siblings of that node. Otherwise, we return the smallest
-  # failure. If we get the next nodes out of the current nodes, we check if it
-  # also fails: if it does, we "go down" and recurse on the children of that
-  # node but only if it has children, otherwise we move to the siblings. If it
-  # doesn't fail, we move to the siblings.
+  # Greedy depth-first search of the shrink tree. We walk through the children
+  # of the current smallest failure: if a child passes the property, we move on
+  # to its siblings; if a child fails, it becomes the new smallest failure and
+  # we commit to it by going down into its children (never coming back to its
+  # siblings). This way, the smallest failure is only ever replaced by one of
+  # its own descendants in the shrink tree, which is smaller by construction.
+  # When we run out of children (or out of shrinking steps), we return the
+  # smallest failure found.
 
-  defp shrink_failure(cont, parent_cont, smallest, fun, nodes_visited, config) do
+  defp shrink_failure(cont, smallest, fun, nodes_visited, config) do
     case cont.({:cont, []}) do
-      # If this list of nodes is over, we backtrack to the parent nodes and
-      # keep shrinking.
-      {state, _acc} when state in [:halted, :done] and not is_nil(parent_cont) ->
-        shrink_failure(parent_cont, nil, smallest, fun, nodes_visited, config)
-
-      # If this list of nodes is over and we don't have parent nodes, we
-      # return what we have now.
+      # If this list of nodes is over, we return what we have now.
       {state, _acc} when state in [:halted, :done] ->
         %{shrunk_failure: smallest, nodes_visited: nodes_visited}
 
@@ -2622,16 +2595,16 @@ defmodule StreamData do
           # anymore and move to the siblings (`cont` is the enumerable representing
           # the rest of the children).
           {:ok, _term} ->
-            shrink_failure(cont, parent_cont, smallest, fun, nodes_visited + 1, config)
+            shrink_failure(cont, smallest, fun, nodes_visited + 1, config)
 
-          # If this child still fails, we update the smallest failure to this failure.
-          # Then, we go down to the children of this node and update the parent continuations
-          # so that if we encounter a success in the children then we can resume from the
-          # siblings of this child.
+          # If this child still fails, it becomes the new smallest failure, and we
+          # commit to it: we go down to its children looking for even smaller
+          # failures. We never come back to this child's siblings, so that the
+          # smallest failure is only ever replaced by one of its descendants
+          # (which is smaller by construction).
           {:error, reason} ->
             shrink_failure(
               shrink_initial_cont(child.children),
-              cont,
               reason,
               fun,
               nodes_visited + 1,

lib/stream_data/lazy_tree.ex

@@ -84,8 +84,13 @@ defmodule StreamData.LazyTree do
   @doc """
   Takes a tree of trees and flattens it to a tree of elements in those trees.
 
-  The tree is flattened so that the root and its children always come "before"
-  (as in higher or more towards the left in the tree) the children of `tree`.
+  The tree is flattened so that the children of `tree` (the "outer" trees)
+  always come "before" (as in higher or more towards the left in the tree) the
+  children of the root of `tree` (the "inner" tree). Since children of `tree`
+  usually represent more structural shrinking (for example, choosing an
+  earlier generator in `StreamData.one_of/1`), this makes shrinking try
+  structural simplifications before simplifications of the generated values
+  themselves.
 
   ## Examples
 
@@ -103,7 +108,7 @@ defmodule StreamData.LazyTree do
 
     %__MODULE__{
       root: child_root,
-      children: Stream.concat(child_children, Stream.map(children, &flatten/1))
+      children: Stream.concat(Stream.map(children, &flatten/1), child_children)
     }
   end
 

test/stream_data/lazy_tree_test.exs

@@ -41,9 +41,9 @@ defmodule StreamData.LazyTreeTest do
 
     expected =
       new(:root1, [
+        new(:root2, [constant(:child2_a), constant(:child2_b)]),
         constant(:child1_a),
-        constant(:child1_b),
-        new(:root2, [constant(:child2_a), constant(:child2_b)])
+        constant(:child1_b)
       ])
 
     assert realize_tree(joined_tree) == realize_tree(expected)

test/stream_data_test.exs

@@ -221,9 +221,27 @@ defmodule StreamDataTest do
     assert Enum.count(values, &(&1 == :small_chance)) < Enum.count(values, &(&1 == :big_chance))
   end
 
-  property "one_of/1" do
-    check all int <- one_of([integer(1..5), integer(-1..-5//-1)]) do
-      assert int in 1..5 or int in -1..-5//-1
+  describe "one_of/1" do
+    property "picks one of the given generators" do
+      check all int <- one_of([integer(1..5), integer(-1..-5//-1)]) do
+        assert int in 1..5 or int in -1..-5//-1
+      end
+    end
+
+    # Regression test for https://github.com/whatyouhide/stream_data/issues/97.
+    property "shrinks towards earlier generators" do
+      check all size <- positive_integer(),
+                seed <- {integer(), integer(), integer()} do
+        {:error, result} =
+          one_of([:foo, {:bar, integer()}])
+          |> check_all(
+            [max_shrinking_steps: 100, initial_size: size, initial_seed: seed],
+            fn example -> {:error, example} end
+          )
+
+        assert result.shrunk_failure == :foo
+        assert result.nodes_visited < 100
+      end
     end
   end