hypothesis icon indicating copy to clipboard operation
hypothesis copied to clipboard

Inquisitor sometimes fails to report arguments as freely varying

Open tybug opened this issue 1 year ago • 2 comments

Previously discussed in https://github.com/HypothesisWorks/hypothesis/pull/3818#discussion_r1467092386.

>>> @given(st.booleans(), st.booleans(), st.lists(st.none()), st.booleans(), st.booleans())
>>> def f(a, b, c, d, e):
        assert not (b and d)
>>> f()

Falsifying example: test_inquisitor_comments_basic_fail_if_either(
    # The test always failed when commented parts were varied together.
    a=False,
    b=True,
    c=[],  # or any other generated value
    d=True,
    e=False,  # or any other generated value
)

should also mark a as freely varying.

The issue is here: https://github.com/HypothesisWorks/hypothesis/blob/626d45dd2cf8d4021e370d54b2cc4dcfab7eac4e/hypothesis-python/src/hypothesis/internal/conjecture/shrinker.py#L530-L536

where we are trying to be efficient by looking for buffers that prove an arg cannot vary freely, because we saw a buffer where the arg was the only thing that varied and the buffer went from interesting to valid. But this condition may have false positives if the size of the arg buffer changed size and happened to match a buffer where additional things varied.

To solve this, we'll need to check against a more structured representation of inputs than the buffer. We could:

  1. Check against the DataTree representation, which encodes the tree structure of calls. This may run into the same buffer-size-changed problem unless/until the DataTree is migrated to the ir (#3818), but it may also be fine as is.
  2. Check against the final ConjectureResult (ConjectureResult.arg_slices?).

tybug avatar Jan 28 '24 00:01 tybug

I personally favor a DataTree-based analysis, which would also be a suitable base to revive https://github.com/HypothesisWorks/hypothesis/pull/3624 to report on sub-argument parts such as elements of st.tuples(), arguments to st.builds(), etc.

Zac-HD avatar Mar 31 '24 04:03 Zac-HD

Yup, I think the correct path forward here is clearly migrating inquisitor to the ir, which should resolve this in passing. Said migration could very likely be worked on at any point in parallel with the rest of the migration. I'll get to it at some point by necessity, of course 🙂 but it's not on my immediate task list if someone wanted to beat me to it.

I've added the explain phase migration as a task to #3921.

tybug avatar Mar 31 '24 05:03 tybug