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

Argument copying technique in LazyStrategy does not preserve custom list subtypes #1017

Closed
JoshRosen opened this issue Dec 10, 2017 · 4 comments · Fixed by #1019
Closed

Argument copying technique in LazyStrategy does not preserve custom list subtypes #1017

JoshRosen opened this issue Dec 10, 2017 · 4 comments · Fixed by #1019
Labels
question not sure it's a bug? questions welcome

Comments

@JoshRosen
Copy link

JoshRosen commented Dec 10, 2017

I encountered an unexpected behavior when attempting to test a third-party library which makes use of custom list subtypes. Here's an example which reproduces the behavior:

Consider a class which inherits from the built-in Python list type and adds additional methods:

class MyList(list):
    def custom_method(self):
        return "result"
    
    def __copy__(self):
        return MyList(list(self))
    
    def __deepcopy__(self, table):
        return self.__copy__()

Because I've overridden both __copy__ and __deepcopy__, copying this list preserves its type:

>>> from copy import copy, deepcopy
>>> my_list = MyList([])
>>> type(copy(my_list))
<class '__main__.MyList'>
>>> type(deepcopy(my_list))
<class '__main__.MyList'>

Let's say that I want to have a strategy which is parameterized by an instance of this class. This works as expected for some strategies:

>>> from hypothesis import strategies as st
>>> type(st.just(my_list).example())
<class '__main__.MyList'>

However, I noticed that the argument type is not preserved when I use a composite strategy:

>>> @st.composite
... def my_strategy(draw, my_list):
...     return my_list
...
>>> type(my_strategy(my_list).example())
<type 'tuple'>

I believe that this behavior is due to how arguments are copied in LazyStrategy:

https://github.com/HypothesisWorks/hypothesis-python/blob/3ee500943938d60a8a97b7d3d948522d65f23e84/src/hypothesis/searchstrategy/lazy.py#L88

Each argument is being copied by tupelize(), which is defined as

def tupleize(x):
    if isinstance(x, (tuple, list)):
        return tuple(x)
    else:
        return x

I'm not sure whether it would be safe to replace tupelize with copy here: converting lists to tuples here guards against mutation from both code inside of the composite strategy's body as well as the code which calls / constructs the strategy, so safely using copy here might also require additional copy calls elsewhere to guard against mutation during/after invocation.

I'm able to work around this behavior by wrapping my argument in an outer list (e.g. [my_list]) and unpacking the argument in my composite strategy.

I'm therefore not blocked by this behavior but I found it confusing and figured it might be worth reporting / documenting.

@Zac-HD Zac-HD added enhancement it's not broken, but we want it to be better question not sure it's a bug? questions welcome and removed enhancement it's not broken, but we want it to be better labels Dec 10, 2017
@Zac-HD
Copy link
Member

Zac-HD commented Dec 10, 2017

Thanks for the report Josh - it's fantastic to get so much detail.

I'll need to have a closer look when I have more time, but I think this is working as intended to avoid the mutable-default-argument problem - having arguments mutate between examples would break example shrinking, along with other things. If that's the case we should probably disallow mutable arguments instead of changing them though!

Can you show a larger example of where this has come up? I think a concrete example would help me understand what you want to do 😄

@DRMacIver
Copy link
Member

No I don't think this is intended behaviour. I can't remember what the argument handling logic in lazy strategy does exactly, but as far as I remember it's meant mostly as an optimization with the intention of unwrapping nested strategies. It shouldn't have user visible behaviour other than that

@JoshRosen
Copy link
Author

JoshRosen commented Dec 10, 2017

For more context on how this somewhat convoluted-looking example cropped up: I am in the process of incrementally porting a legacy third-party domain-specific randomized testing harness to use Hypothesis for test case data generation so that I can benefit from Hypothesis's example database and example shrinking (the port is going well so far and has already uncovered a new bug in the system under test).

The existing test framework being ported has several internal data structures which have this pattern where they extend from list to add several list filtering methods (if this existing library was written in Scala then I suspect they would have used implicits to factor out these common list filtering methods and would just pass around immutable Seqs). A slightly more concrete example is something like this (in psuedocode):

@st.composite
def build_typed_thing(draw, types_list):
    // [...]
    condition = ...
    type_of_thing = draw(st.sampled_from(types_list.valid_types(condition)))
    // [... other generator calls ...]
    return thing(type_of_thing, ...)

where types_list is some custom list subtype whose sole purpose is to add the .valid_types(condition) filtering method.

I'm not actually a fan of this pattern and my planned workaround is to refactor these wrapper classes so that they don't directly extend from list (I'm pretty sure they can simply contain a list and can simply extend the collections.Sequence abstract base class).

I'm reporting this mostly just to aid in searchability (keywords: tuple, copy, type) in case anyone else hits the same issue because this was slightly tricky to debug.

Regarding the mutable-default-argument problem: tupelize helps here, but it's only guarding against mutation of lists. For more general safety, it seems like you'd also want to copy other mutable arguments here.

@Zac-HD
Copy link
Member

Zac-HD commented Dec 11, 2017

Digging into the history of lazy.py, it looks like this is the remnant of a Hypothesis-1-era attempt to fix broken reprs of elements of tuples in argument signatures. I think we can therefore just delete it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question not sure it's a bug? questions welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants