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

use get_origin when looking up type registration #3762

Closed
wants to merge 1 commit into from

Conversation

nickcollins
Copy link
Contributor

This is only an RFC, this will break tests and is used to demonstrate which tests will break if we make this oversimplistic change

This is only an RFC, this will break tests and is used to demonstrate which tests will break if we make this oversimplistic change
Comment on lines +1314 to 1315
if get_origin(thing) in types._global_type_lookup:
return as_strategy(types._global_type_lookup[thing], thing)
Copy link
Member

Choose a reason for hiding this comment

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

This definitely won't work, because we're then accessing a key which in many cases isn't in the dict. I think the baseline you're trying for is more like "if the thing itself isn't registered, check whether the origin is:

        if res := types._global_type_lookup.get(thing):
            return as_strategy(res, thing)
        if res := types._global_type_lookup.get(get_origin(thing)):
            return as_strategy(res, thing)

although I expect this will still fail in many cases, e.g. it looks like Type[ForwardRef(...)] is sometimes a thing but the type resolver doesn't know about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems a common issue with the test failures is the origin being Typing.Type, or something like that.
Notably, we have:

1031 @given(st.from_type(typing.Type[typing.Union["str", "int"]]))
1032 def test_resolves_type_of_union_of_forwardrefs_to_builtins(x):
1033     assert x in (str, int)

where it seems that hypothesis is special casing from_type when the origin is Type; it seems as though Type and all parameterizations of it are automatically handled by Hypothesis, without requiring any registration? So if we try to lookup the registration of a parameterized Type by origin, we get whatever is registered for Type, thus eliding the special casing and get the wrong answer?

I tried

1315         if thing in types._global_type_lookup:                                         
1316             return as_strategy(types._global_type_lookup[thing], thing)                
1317         elif not thing_origin is type and thing_origin in types._global_type_lookup:   
1318             return as_strategy(types._global_type_lookup[thing_origin], thing)

and that prevented most of the test failures, though there are still plenty more

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we special-case type somewhere, rather than going through the usual lookup logic? That was the core of a recent problem for tuple, at least.

@given(st.from_type(typing.Type[typing.Union["str", "int"]]))
def test_resolves_type_of_union_of_forwardrefs_to_builtins(x):
    assert x in (str, int)

This test just seems wrong though - it should use a union of the types (type[str] | type[int]) to get this result, not type of the unions (type[str | int]). As much as I hate changing behavior, this would be a bugfix and this OK without a major version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Do you know the recent problem with tuple you were mentioning? No worries if you don't, but I thought I might take a look at it because I think any additional context I have would help me better understand the right approach here.
  1. I see what you're saying about that particular test being wrong - that seems like a distinct issue to me? I pulled up that test as a somewhat arbitrary example of a test that was failing before but which (IIRC) is now working with my latest change. So maybe we should open a separate issue for fixing that thing? LMK what you think.

  2. When I have time I'll keep working on the remaining test failures. Hopefully they won't be too tricky. For now I'll assume that the latest change proposal works well enough for whatever special-casing may be happening with type, and then add other checks in the conditional to take care of issues cropping up in the remaining tests that are failing, if that makes sense to you.

Copy link
Member

Choose a reason for hiding this comment

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

  1. I think I was remembering a mixture of your register_type_strategy doesn't seem to work on parameterized generic types #3750 and also Incompatible Sequence types generated after register_type_strategy() #3767 - there are several PRs linked to the latter.

  2. That test is indeed a distinct thing to deal with, but no need to open a tracking issue, I've got a local fix and will push that this evening.

  3. Sounds good; just remember to step back and reevaluate whether it's the right approach if you get stuck or end up making many many changes to pass the last few tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been digging into this some more and i realized that I may have misunderstood some things. The fix above isn't really appropriate, as I've now realized that generics for custom classes (e.g. MyGeneric[int]) work just fine, and so the problem I brought up with this issue is really only a problem with stuff like tuples, that require special casing (I think you alluded to this previously, but I didn't understand much about the codebase back then). That said, I see that the solution to the problem will be in the special casing for tuples, probably something related to from_typing_type as you suggested previously). However, I was also wondering for the sake of my own general understanding, where are regular generics handled? When I look at the code, there doesn't seem to be a place in in _internal/core/_from_type where MyGeneric[int] is handled. Also, when I put print statements in _from_type, none of them ever trigger when I do a test for MyGeneric[int]. Yet, the test clearly works, so is from_type handling the generics in some other chunk of code, somewhere else? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I think from_typing_type should probably be named from_generic_type, but unclear it's worth changing now - the concepts are a bit confused, not just the name.

That's invoked from here and iirc that's where we handle user-defined generic classes.

@nickcollins
Copy link
Contributor Author

this turned out to be the wrong approach, the issue #3750 was fixed via a different PR

@nickcollins nickcollins closed this Nov 5, 2023
@nickcollins nickcollins deleted the patch-1 branch November 5, 2023 02:52
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.

None yet

2 participants