Add type hints to the public API, with generic Strategy type #1253

Zac-HD · 2018-04-24T14:35:40Z

Does not quite close #200 (next time though! 🎉)

I won't recite the whole motivation here - suffice to say that at least some people like static typing, and so I'd like to let the type-checker find some of their obvious misuses of Hypothesis.

Things not obvious from review:

Mypy doesn't support arity-specific overloads, which would be useful for tuples.
The disallow_untyped_decorators option is indispensable if you don't want a a decorator to erase the type hints on a function.

I would also like to write some tests using reveal_type to check that Mypy does in fact infer the expected type for various expressions. This may go in a later PR, to keep things reviewable - I've confirmed most of this manually, so automated tests would really be for tooling or code regressions.

Zac-HD · 2018-04-25T14:26:22Z

check-shellcheck was fixed in #1255, so the build is passing now 😄

Ready for review!

Zac-HD · 2018-05-02T11:23:53Z

Ping @HypothesisWorks/hypothesis-python-contributors for review!

kxepal · 2018-05-02T11:45:21Z

LGFM.

@Zac-HD
I'm sorry for being late for the mypy party, but do you have a references with explanation or why did you pick comment-based typing instead of stubs (just curious, though I think I understand why) and what is the if False hack origins from? That one looks pretty weird for the first sight.

Zac-HD · 2018-05-02T12:25:18Z

Do you have a references with explanation or why did you pick comment-based typing instead of stubs (just curious, though I think I understand why)

I picked comments over stubs for a couple of reasons.

Distribution is harder to screw up. Stub files introduce a whole new wrinkle for downstream packagers to deal with, while they're already distributing Hypothesis with comments! Writing the build rule to include a single empty file (py.typed) is much easier for us too.
Locality of reference. It's much more difficult to edit a function signature but forget the type hints when they're on adjacent lines, and easier to catch in review if it happens anyway. Hopefully this will make the transition easier if or when we ditch Python 2 and use the inline annotation syntax.
IMO it's easier to extend incrementally to a few functions or even arguments at a time if you're using comments.

What is the if False hack origins from? That one looks pretty weird for the first sight.

There's a constant typing.TYPE_CHECKING (ie False at runtime) which is designed to guard imports. Luckily Mypy also accepts a literal False, saving us some silly juggling of imports wherever we need to import a type. There's no deep logic to it, just the Mypy devs recognising that this was a useful pattern.

kxepal · 2018-05-02T12:42:56Z

Thank you!

Zac-HD · 2018-05-08T00:21:36Z

Ping @DRMacIver for review - it's been almost two weeks!

I'd really like to get this merged soon so I can focus on PyCon preparation 😄

DRMacIver · 2018-04-26T09:50:05Z

hypothesis-python/src/hypothesis/searchstrategy/strategies.py

+
+except ImportError:  # pragma: no cover
+    Ex = 'key'  # type: ignore
+    Generic = {Ex: object}  # type: ignore


I actual cracked up at this hack.

(It's a perfectly reasonable solution but for some reason struck me as very funny)

DRMacIver · 2018-04-26T10:00:54Z

hypothesis-python/src/hypothesis/strategies.py

+    pass  # pragma: no cover
+
+
+@overload  # noqa: F811  # redefinition of sampled_from


Maybe we should just turn off this warning in config? I'm not sure it gives us much of use and these noqas are annoying.

Maybe my tendency to never-weaken-default-settings is going too far here, yes.

DRMacIver · 2018-05-08T07:31:45Z

hypothesis-python/docs/details.rst

+up our type hints.
+
+.. note::
+    Hypothesis' type hints may make breaking changes between minor releases.


Should the paragraphs below be part of this note? They qualify it, so I think it makes sense to have them logically grouped.

It also might be nice to make clear that we do plan to stabilise it eventually (but not before the main typing module becomes non-provisional)

Expanding note, yes. IMO the plans to stabilise are implicit, and I'd rather leave them that way until we have some more experience and understanding of how often they break over time.

DRMacIver · 2018-05-08T07:34:38Z

hypothesis-python/docs/details.rst

+
+.. doctest::
+
+    >>> from hypothesis.strategies import SearchStrategy


Does this doctest really add much? It seems weird in context as it's only asserting something about the dynamic subtyping behaviour in a section where we talk about how the only valid use case of this type is static type checking.

...I'll take it out, you're right.

History: this is how I found the problem with unannotated decorators erasing type hints. More on that below...

DRMacIver · 2018-05-08T07:36:37Z

hypothesis-python/docs/details.rst

+- ``integers()`` is of type ``SearchStrategy[int]``.
+- ``lists(integers())`` is of type ``SearchStrategy[List[int]]``.
+- ``SearchStrategy[bool]`` is a subtype of ``SearchStrategy[int]``,
+  because ``bool`` is a subtype of ``int``.


nit: bool being a subtype of int is weird enough that I'd probably prefer a different example. Maybe just that it's a subtype of SearchStrategy[Any]?

DRMacIver · 2018-05-08T07:37:05Z

hypothesis-python/docs/details.rst

+Please do not inherit from, compare to, or otherwise use the
+:class:`~hypothesis.strategies.SearchStrategy` in any way outside of type
+hints.  This is a private, internal interface that is *only* available for
+type checking, and may be changed or removed in any minor release.


I think that's a bit strong. We can probably commit to keeping the name around without a major release bump.

DRMacIver · 2018-05-08T07:41:33Z

hypothesis-python/src/hypothesis/core.py

-def given(*given_arguments, **given_kwargs):
-    # type: (*SearchStrategy, **SearchStrategy) -> Callable
+def given(
+    *given_arguments,  # type: Union[SearchStrategy, UniqueIdentifier]


Doesn't need to be as part of this PR, but it would be nice to tighten up these types to be more specific about which UniqueIdentifier is permitted here, if only so the UniqueIdentifier type doesn't appear in our public APIs.

I'll include that too; it's pretty easy to toss in class InferType(UniqueIdentifier): ..., infer = InferType('infer') (better name welcome), and edit the hints.

Zac-HD · 2018-05-08T08:34:27Z

Note on release plans: as this stands, there are no automated tests that will catch regressions in the type hints as seen by downstream users. With some distance from the initial implementation, that makes me uncomfortable - I'm pretty sure that means that they'll break soon, or simply aren't working at all yet.

In the interest of reviewable diffs, I therefore propose to replace the docs+release commit with a smaller one that does another non-user-facing patch, and then do a follow-up PR later this week with tests and the public release of type annotations. Sound good?

Zac-HD · 2018-05-08T14:23:05Z

Updated for another @DRMacIver review 😄

DRMacIver · 2018-05-08T15:25:33Z

hypothesis-python/src/hypothesis/utils/conventions.py

@@ -27,5 +27,14 @@ def __repr__(self):
        return self.identifier


-infer = UniqueIdentifier(u'infer')
+class DefaultValueType(UniqueIdentifier):
+    # Using subclasses so they can be distinguished by e.g. Mypy


nit: This comment should probably go at the top level as it applies to two classes, not just this one.

(so Mypy can tell you're using the wrong one)

Zac-HD force-pushed the typed branch 5 times, most recently from 115f439 to e7be891 Compare April 25, 2018 13:23

DRMacIver mentioned this pull request Apr 25, 2018

Fix shellcheck install #1255

Merged

DRMacIver reviewed May 8, 2018

View reviewed changes

Zac-HD added 3 commits May 8, 2018 21:11

Make SearchStrategy generic in the type of examples

26f0da5

Add other public API type hints

5410b79

Patch note: nothing public *yet*

52f2793

Zac-HD force-pushed the typed branch 4 times, most recently from 5c021f3 to f03a034 Compare May 8, 2018 13:03

DRMacIver approved these changes May 8, 2018

View reviewed changes

Create unique types for UniqueIdentifiers

ddfb6e6

(so Mypy can tell you're using the wrong one)

Zac-HD force-pushed the typed branch from f03a034 to ddfb6e6 Compare May 8, 2018 23:36

Zac-HD merged commit 661fd78 into HypothesisWorks:master May 9, 2018

Zac-HD deleted the typed branch May 9, 2018 00:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type hints to the public API, with generic Strategy type #1253

Add type hints to the public API, with generic Strategy type #1253

Zac-HD commented Apr 24, 2018 •

edited

Zac-HD commented Apr 25, 2018 •

edited

Zac-HD commented May 2, 2018

kxepal commented May 2, 2018

Zac-HD commented May 2, 2018

kxepal commented May 2, 2018

Zac-HD commented May 8, 2018

DRMacIver Apr 26, 2018

DRMacIver Apr 26, 2018

Zac-HD May 8, 2018

DRMacIver May 8, 2018

Zac-HD May 8, 2018

DRMacIver May 8, 2018

Zac-HD May 8, 2018

DRMacIver May 8, 2018

DRMacIver May 8, 2018

DRMacIver May 8, 2018

Zac-HD May 8, 2018

Zac-HD commented May 8, 2018

Zac-HD commented May 8, 2018

DRMacIver May 8, 2018

		pass # pragma: no cover


		@overload # noqa: F811 # redefinition of sampled_from


		.. doctest::

		>>> from hypothesis.strategies import SearchStrategy

Add type hints to the public API, with generic Strategy type #1253

Add type hints to the public API, with generic Strategy type #1253

Conversation

Zac-HD commented Apr 24, 2018 • edited

Zac-HD commented Apr 25, 2018 • edited

Zac-HD commented May 2, 2018

kxepal commented May 2, 2018

Zac-HD commented May 2, 2018

kxepal commented May 2, 2018

Zac-HD commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD commented May 8, 2018

Zac-HD commented May 8, 2018

Choose a reason for hiding this comment

Zac-HD commented Apr 24, 2018 •

edited

Zac-HD commented Apr 25, 2018 •

edited