Skip to content

Add reusable type for @composite draw argument#3069

Merged
Zac-HD merged 9 commits intoHypothesisWorks:masterfrom
ROpdebee:add-composite-draw-arg-type-hint
Aug 27, 2021
Merged

Add reusable type for @composite draw argument#3069
Zac-HD merged 9 commits intoHypothesisWorks:masterfrom
ROpdebee:add-composite-draw-arg-type-hint

Conversation

@ROpdebee
Copy link
Copy Markdown
Contributor

I've split this off from #3068 to keep each PR self-contained and because this is perhaps a bit more of a controversial change. Also related to #3005.

In short, this change adds and exports a type, DrawFn that people using type annotations can use to annotate the draw function provided by @composite. This goal of this new type is similar to DataObject, with one big difference being that this one exists solely for typing and doesn't serve any functional goals. Its main benefit is that it's reusable and a single point of definition of the interface that can be evolved if hypothesis evolves, which is much better than everyone copying over the suggested typevars from #3005 into their own files.

If anyone can suggest a better name for this type, I'd be happy to change it!

A note on the implementation: I've implemented this as a class rather than a typevar (as suggested in #3005), since the draw function seems to take an optional argument label and it isn't possible to specify that with just a typing.Callable alone. Callback protocols do support this, but typing.Protocol has only landed in the stdlib since Python 3.8. Instead, this class acts like a protocol without actually inheriting from one. Its sole purpose is to describe something which, when called with a SearchStrategy[Ex] and an optional label, returns an Ex. We never have to use this within hypothesis itself, it's purely a hint/tool for the user to use in their wrapped function annotation.

A note for future maintenance: If you ever decide to make this an actual typing.Protocol, make sure not to make the protocol itself generic, just the __call__ method. So, class DrawFn(Protocol): ..., not class DrawFn(Protocol[Ex]): .... The latter would require providing a type argument whenever the DrawFn type is used in an annotation (with strict mypy settings), the former does not.

Also, although PEP 612 will allow for better typing of @composite, I'm not sure whether type checkers would then be able to infer the parameter types of the decorated functions. The PEP provides an example regarding Concatenate, and there, the concatenated parameter (Request) is still annotated in the wrapped function. Moreover, strict settings for type checkers might continue to require annotations on each parameter, regardless of whether they can somehow infer the type. Therefore, I don't think this contribution would become obsolete once 3.10 becomes the minimum supported version.

An example (adapted from the linked issue):

@st.composite
def foo(draw: st.DrawFn) -> st.SearchStrategy[Tuple[int, int]]:
    s = draw(st.text())
    i = draw(st.integers())
    t = draw(st.tuples(st.text(), st.integers()))
    reveal_type(s)  # str
    reveal_type(i)  # int
    reveal_type(t)  # tuple[str, int]
    
    return (i, t[1])

This type-checks correctly and without errors under the strictest mypy settings (excluding --disallow-any-decorated).

Similarly to #3068, I've pondered on the appropriate version bump type, and I feel like minor is more appropriate here than in #3068. If I may weigh in on the release strategy for these, I'd suggest releasing both PRs as a single release rather than each one individually.

Copy link
Copy Markdown
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ruben - this looks good to me, and I really appreciate your detailed writeup!

  • I agree with your analysis of why a Callable typevar is unsatisfactory. While I prefer not to add public API, it's clearly justified in this case.
  • IMO we should make it a Protocol though, at least on py38+ - see below.
  • I agree with your analysis of the interaction with PEP-612.
  • This is a minor release, independent of #3068 - we like to keep versions atomic.
  • I'd usually also suggest adding your name to the contributors list, but we can leave that for now to avoid merge conflicts.

It would be great to add a test for this to https://github.com/HypothesisWorks/hypothesis/blob/master/whole-repo-tests/test_type_hints.py, along the lines of "mypy runs on this code without errors"; after that I think we'll be ready to merge!

Comment thread hypothesis-python/src/hypothesis/strategies/_internal/core.py Outdated
Comment thread hypothesis-python/src/hypothesis/strategies/_internal/core.py Outdated
Copy link
Copy Markdown
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! One tiny formatting issue, then we'll merge 🚀

Comment thread hypothesis-python/RELEASE.rst Outdated
@ROpdebee
Copy link
Copy Markdown
Contributor Author

ROpdebee commented Aug 26, 2021

@Zac-HD I just noticed that in the docs, the DrawFn class is showing up as DrawFn(*args, **kwargs). I think it's because CPython adds a fake __init__ to protocols. It's a bit pedantic, but we could try to get rid of these through a fake constructor of our own. If we create our own fake constructor, we could also prevent users from accidentally instantiating this class on < 3.8. Edit: It doesn't work, CPython still overwrites the initializer for direct subclasses (not for indirect ones). However, explicitly raising an error on < 3.8 still seems like a good idea.

Comment thread hypothesis-python/src/hypothesis/strategies/_internal/core.py
Comment thread hypothesis-python/src/hypothesis/strategies/_internal/core.py Outdated
ROpdebee and others added 4 commits August 26, 2021 15:11
Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>
Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>
@ROpdebee
Copy link
Copy Markdown
Contributor Author

ROpdebee commented Aug 26, 2021

I've excluded the pre-3.8 initializer from coverage. It is covered by a test on Python versions where we use our own Protocol (3.6, 3.7), but not under >= 3.8 since the constructor is overwritten, and the coverage runner uses 3.8.

Also apologies for hammering CI and the force-pushes, for some reason the build.sh script refuses to work properly on my system and I can't be bothered trying to figure out why. Actually, I know why, it's openssl, but I don't particularly feel like wasting 5 hours figuring out how to fix it (again) 😅

@ROpdebee
Copy link
Copy Markdown
Contributor Author

ROpdebee commented Aug 26, 2021

Hmm, flaky test?

@Zac-HD Zac-HD merged commit f19e57c into HypothesisWorks:master Aug 27, 2021
@Zac-HD
Copy link
Copy Markdown
Member

Zac-HD commented Aug 27, 2021

Thanks again @ROpdebee! Now released as Hypothesis 6.16.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants