Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inquisitor sometimes fails to report arguments as freely varying #3864

Closed
tybug opened this issue Jan 28, 2024 · 3 comments · Fixed by #4160 or #4162
Closed

Inquisitor sometimes fails to report arguments as freely varying #3864

tybug opened this issue Jan 28, 2024 · 3 comments · Fixed by #4160 or #4162
Assignees
Labels
enhancement it's not broken, but we want it to be better

Comments

@tybug
Copy link
Member

tybug commented Jan 28, 2024

Previously discussed in #3818 (comment).

>>> @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:

# Check for any previous examples that match the prefix and suffix,
# so we can skip if we found a passing example while shrinking.
if any(
seen.startswith(buffer[:start]) and seen.endswith(buffer[end:])
for seen in seen_passing_buffers
):
continue

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 (Migrate DataTree to the new IR #3818), but it may also be fine as is.
  2. Check against the final ConjectureResult (ConjectureResult.arg_slices?).
@tybug tybug added the enhancement it's not broken, but we want it to be better label Jan 28, 2024
@Zac-HD
Copy link
Member

Zac-HD commented Mar 31, 2024

I personally favor a DataTree-based analysis, which would also be a suitable base to revive #3624 to report on sub-argument parts such as elements of st.tuples(), arguments to st.builds(), etc.

@tybug
Copy link
Member Author

tybug commented Mar 31, 2024

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
Copy link
Member Author

tybug commented Nov 7, 2024

A heads up that I'm working on migrating the explain phase to the IR as my next task, on the off chance that it helps avoid duplicate work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement it's not broken, but we want it to be better
Projects
None yet
2 participants