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

Trying to resolve or register 'metadata' like typing.Annotated should be an error #2978

Closed
Zac-HD opened this issue May 25, 2021 · 12 comments
Closed
Assignees
Labels
legibility make errors helpful and Hypothesis grokable

Comments

@Zac-HD
Copy link
Member

Zac-HD commented May 25, 2021

This was prompted by seeing PEP 655, which proposes a Required and NotRequired "type" analogous to typing.Annotated - that is, the correct way to resolve these types is to "see through" them to the actual type underneath.

(those might be added in 3.11; there's already implementations in typing_extensions and support from mypy)

However, from_type(typing.Annotated) or from_type(typing.Annotated) should be an explicit error - there's nothing wrapped that we could look at. For this issue, we should:

  • carefully consider the typing module docs and create a full list of 'metadata types' to ban;
    • Off the top of my head: Annotated, ClassVar, Final, ... already exist.
      Python 3.10 will add more, including TypeGuard, Concatenate, TypeAlias, and maybe others.
  • Test that we "see through" all such types (iirc currently just Annotated)
  • Make it an error to register them, or to resolve them without a wrapped type - from the sdtlib typing module or typing_extensions package
@Zac-HD Zac-HD added the legibility make errors helpful and Hypothesis grokable label May 25, 2021
@sobolevn
Copy link
Member

sobolevn commented Aug 23, 2021

Make it an error to register them

I have a question about this case with Annotated. Let's consider this example:

def my_factory(thing):
   if isinstance(thing.__origin__, int):
       return st.integers(**thing.__metadata__)
   raise ...

st.register_type_strategy(Annotated, my_factory)

So, this example can users help to write something like def func(number: Annotated[int, min=1, max=99]):
Banning st.register_type_strategy(Annotated, ...) will completely ruin this potentially cool use-case.

@Zac-HD
Copy link
Member Author

Zac-HD commented Aug 24, 2021

Just confirming, your only concern is registering bare Annotated?


Currently we 'see through' Annotated, as specified by the standard library docs. I agree that doing smarter things with annotated types is desirable; once there's a consensus on how to interpret particular strategy-relevant arguments to Annotated, I'm very happy to support them.

However I think that st.register_type_strategy() is unsuitable, and I want a concrete use-case before committing to any user-facing API. It's going to be more complicated than your example too, because Annotated[int, min=1, max=99] is a SyntaxError - you have to use positional args (technically index by a tuple), so most of the ideas I've seen refer to using custom types.

@sobolevn
Copy link
Member

Sorry about Annotated[int, min=1, max=3], I've meant something like this (type-checks):

from typing_extensions import Annotated

class HypothesisStrategyArgs(object):
    def __init__(self, *args, **kwargs) -> None:
        self.args = args
        self.kwargs = kwargs

MyInt = Annotated[int, HypothesisStrategyArgs(min=1, max=3)]

def my_func(num: MyInt) -> int:
    ...

It looks like an amazing API to me personally.

@Zac-HD
Copy link
Member Author

Zac-HD commented Aug 24, 2021

It's going to be really, really hard to pass **kwargs through into the strategies correctly, on the level of "completely redesigning the whole way strategies are registered for types because now you can only ever register functions which might take arbitrary kwargs". It also has the downside of requiring you to have Hypothesis available outside your test environment.

However... if we're accepting that, it would be easy to support Annotated[int, st.integers(1, 3)] - just scan through the __args__, and return the first strategy we see (using the existing from_type(__args__[0]) logic otherwise). Want to write a PR for the latter? I don't mind committing to this as public API because there's it's the only reasonable interpretation of annotating a type with a SearchStrategy object.

@sobolevn
Copy link
Member

Yes, I will create a PR soon! 👍

@sobolevn sobolevn self-assigned this Aug 27, 2021
@sobolevn
Copy link
Member

sobolevn commented Sep 1, 2021

Some questions I got while working on this:

  1. What should we do when Annotated types are nested? Like in this example:
PositiveInt = Annotated[int, ...]
PositiveOddInt = Annotated[PositiveInt, ...]

We can go into nested type or just ignore them. The other question is: it looks like PositiveOddInt should win, but it is the last one, not the first one.

  1. How can we (and even should we?) register callable strategies with Annotated? Will something that can be called, has one arg and returns SearchStrategy work for us? (I don't think that calling random functions is a good idea)

@Zac-HD
Copy link
Member Author

Zac-HD commented Sep 2, 2021

  1. The usual semantics is to flatten all the annotations. After that, I think we should use the last (ie outer-most) SearchStrategy object that we find.
  2. In this case I think it has to be a SearchStrategy object; a callable returning SearchStrategy is somewhat more ambiguous. We can always change that later!

@sobolevn
Copy link
Member

I can forbid using TypeAlias, ParamSpec, Concatenate, and TypeGuard.
So, .from_type(TypeAlias) would be a readable user-facing error.

Does this sound right? 🤔

@Zac-HD
Copy link
Member Author

Zac-HD commented Dec 12, 2021

Yes 👍

I'd make register_type_strategy(TypeAlias, ...) an error too, since instantiating it just doesn't make sense.

@sobolevn
Copy link
Member

sobolevn commented Jan 7, 2022

Working on ClassVar!

@sobolevn
Copy link
Member

sobolevn commented Jan 9, 2022

TypeGuard is a bit different. We should automatically resolve TypeGuard as bool in cases like def (some: object) -> TypeGuard[...]:.

Should we allow to register it? Interesting question. I think that there might be cases when TypeGuard can be used together with some runtime-contracts. So, I might think of cases when TypeGuard can be registered as a strategy that relies on the input.

@Zac-HD do you agree? 🙂

@Zac-HD
Copy link
Member Author

Zac-HD commented Jan 11, 2022

There are definitely runtime uses, so I think we should allow it (and maybe even register it by default!). However, it's only valid if parametrized, so we should allow e.g. TypeGuard[int] but not bare TypeGuard, and therefore allow registering a function but not a strategy - e.g. lambda tg: st.booleans(), but raising InvalidArgument if tg is bare.

The trickier part is if we upgrade st.functions() to infer the returns strategy, which I've been meaning to do for a while. In this case, should the generated function actually be a correct typeguard? I tend to think "no", because if you wanted that why are you using Hypothesis to generate random functions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legibility make errors helpful and Hypothesis grokable
Projects
None yet
Development

No branches or pull requests

2 participants