RFC: Allow registration of tuples #3782
Conversation
Previously, if `tuple` was registered, the registered strategy would be ignored. Now it is actually used. Note that `tuple[...]` cannot be registered because genericized types are not allowed to be registered in general, so a user must register `tuple` with a function that takes the type args and returns a strategy appropriate to the particular genericized type at hand.
|
well it worked on my machine .... |
There was a problem hiding this comment.
It looks like dropping some obsolete compatibility code will make this easier - let me know if you'd like me to push that, since I'm probably the only person who remembers how it worked. I think all the tests pass on Python 3.10 and later, which is a good sign!
Otherwise, see comments below and don't forget to write a RELEASE.rst note 😄
| @@ -118,6 +118,14 @@ def test_lookup_overrides_defaults(typ): | |||
| assert st.from_type(typ).example() is not sentinel | |||
|
|
|||
|
|
|||
| def test_lookup_registered_tuple(): | |||
| sentinel = object() | |||
| typ = tuple[int] | |||
There was a problem hiding this comment.
This will fail on Python 3.8, because builtin generics only became subscriptable in Python 3.9. The solution is to move this test to hypothesis-python/tests/cover/test_lookup_py39.py 🙂
| # you can register types with all generic parameters | ||
| List[T], | ||
| Sequence[T], | ||
| tuple[T], |
There was a problem hiding this comment.
from typing import Tuple here; and it'd be worth testing both Tuple[T] and Tuple[T, ...].
We may need to wrap it in pytest.param(Tuple[T], marks=pytest.mark.xfail(sys.version_info[:2] <= (3, 9), reason="stdlib changes") to fix the other CI failures though, because it's treated as a parametrized generic type on those older versions.
| @@ -1227,25 +1227,6 @@ def _from_type(thing: Type[Ex]) -> SearchStrategy[Ex]: | |||
| # refactoring it's hard to do without creating circular imports. | |||
| from hypothesis.strategies._internal import types | |||
|
|
|||
| def as_strategy(strat_or_callable, thing): | |||
There was a problem hiding this comment.
I'd prefer to avoid moving this, since we won't need it for a registered function to resolve tuples.
There was a problem hiding this comment.
Don't we still need it for tuples though? As I understand it sanity checks the user registration. Like we might take it for granted that the user registration ought to be callable, but even if it's confirmed to be callable, as_strategy still does other checks to ensure that the returned strategy is formed correctly, and it feels right to invoke as_strategy to perform those checks rather than to copy the code.
I could try leaving as_strategy in core.py, and then if a circular dependency comes up when I invoke it in types.py, I could try finding some way to resolve that. I did not actually verify that this causes a circular dependency, I just assumed that it would, so I could go check that. But if I don't use as_strategy at all, then I guess I'd have to copy some of its checks into types.py, which I could do if you want but doesn't seem like the best solution to me.
| if get_origin(thing) == tuple or isinstance( | ||
| thing, getattr(typing, "TupleMeta", ()) | ||
| ): |
There was a problem hiding this comment.
It turns out that the TupleMeta case hasn't been necessary since we dropped support for Python 3.6, which means we can move this logic to a new @register'd resolve_tuple() function below. That will treat tuple as if it's any other generic type, and should simplify things considerably!
There was a problem hiding this comment.
Currently, I'm just pre-registering lambda _: NotImplmented, rather than going the route i mentioned earlier of pre-registering the default tuple resolution logic. Are you suggesting I go back to that option? And you're saying that some logic in from_typing_type will handle tuples, if we delete the tuple special casing from from_typing_type?
I'm looking at from_typing_type and the only place where it checks global_type_lookup is at
hypothesis/hypothesis-python/src/hypothesis/strategies/_internal/types.py
Lines 389 to 393 in 019a9ef
but that check requires
is_generic_type(k) which I assume is not true for k == tuple?
|
|
Wow - #3785 took me a few attempts, but it looks like dropping the obsolete special cases was all we needed to fix! |
Previously, if
tuplewas registered, the registered strategy would be ignored. Now it is actually used. Note thattuple[...]cannot be registered because genericized types are not allowed to be registered in general, so a user must registertuplewith a function that takes the type args and returns a strategy appropriate to the particular genericized type at hand.