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

mypy --no-implicit-reexport infers st.one_of(...) -> Any in 6.3.3+ #2884

Closed
sobolevn opened this issue Mar 1, 2021 · 8 comments · Fixed by #2885
Closed

mypy --no-implicit-reexport infers st.one_of(...) -> Any in 6.3.3+ #2884

sobolevn opened this issue Mar 1, 2021 · 8 comments · Fixed by #2885
Assignees
Labels
interop how to play nicely with other packages legibility make errors helpful and Hypothesis grokable

Comments

@sobolevn
Copy link
Member

sobolevn commented Mar 1, 2021

My CI failed, because of the mypy check.
For some reason the latest version did change the typing of st.one_of.

returns/contrib/hypothesis/containers.py:58: error: Returning Any from function declared to return "SearchStrategy[Any]"

Source: https://github.com/dry-python/returns/blob/master/returns/contrib/hypothesis/containers.py#L58

Link to the job: https://github.com/dry-python/returns/pull/839/checks?check_run_id=200189528

@Zac-HD
Copy link
Member

Zac-HD commented Mar 1, 2021

I'd expect this to be linked to hypothesis-python-6.3.2...hypothesis-python-6.3.3, where the implementation of st.one_of() was moved... though that really, really shouldn't have changed the inferred type.

Can you check 6.3.2 and 6.3.3 to confirm that it's one of those updates, and also report your Python and mypy versions?

@sobolevn
Copy link
Member Author

sobolevn commented Mar 1, 2021

Yes, sure!

Currently I have:

And it works.

Now, let's update hypothesis to 6.3.3

(.venv) ~/Documents/github/returns  master ✔                                         
» hypothesis --version
hypothesis, version 6.3.2

(.venv) ~/Documents/github/returns  master ✔                                         
» mypy returns
Success: no issues found in 112 source files

(.venv) ~/Documents/github/returns  master ✔                                         
» pip install -U 'hypothesis==6.3.3'
Collecting hypothesis==6.3.3
  Downloading hypothesis-6.3.3-py3-none-any.whl (358 kB)
     |████████████████████████████████| 358 kB 1.6 MB/s 
Requirement already satisfied, skipping upgrade: attrs>=19.2.0 in ./.venv/lib/python3.8/site-packages (from hypothesis==6.3.3) (20.3.0)
Requirement already satisfied, skipping upgrade: sortedcontainers<3.0.0,>=2.1.0 in ./.venv/lib/python3.8/site-packages (from hypothesis==6.3.3) (2.3.0)
Installing collected packages: hypothesis
  Attempting uninstall: hypothesis
    Found existing installation: hypothesis 6.3.2
    Uninstalling hypothesis-6.3.2:
      Successfully uninstalled hypothesis-6.3.2
Successfully installed hypothesis-6.3.3
WARNING: You are using pip version 20.2.4; however, version 21.0.1 is available.
You should consider upgrading via the '/Users/sobolev/Documents/github/returns/.venv/bin/python -m pip install --upgrade pip' command.

(.venv) ~/Documents/github/returns  master ✔                                         
» mypy returns                      
returns/contrib/hypothesis/containers.py:58: error: Returning Any from function declared to return "SearchStrategy[Any]"
Found 1 error in 1 file (checked 112 source files)

@sobolevn sobolevn changed the title st.one_of returns any in the latest release (6.3.4) st.one_of returns any in the latest releases (6.3.3 and 6.3.4) Mar 1, 2021
@Zac-HD
Copy link
Member

Zac-HD commented Mar 1, 2021

Huh, I think that this is because I moved the definition of one_of() from core.py to strategies.py, but it's imported into __init__ via core. Changing the import, for that and the other moved functions, might be enough to fix it... though I maintain that it shouldn't have broken.

What's worse is that we already have an automated test for this (note "Any" in params is SearchStrategy[Any] in the test) -

@pytest.mark.parametrize(
"val,expect",
[
("integers()", "int"),
("text()", "str"),
("integers().map(str)", "str"),
("booleans().filter(bool)", "bool"),
("lists(none())", "list[None]"),
("dictionaries(integers(), datetimes())", "dict[int, datetime.datetime]"),
("data()", "hypothesis.strategies._internal.core.DataObject"),
("none() | integers()", "Union[None, int]"),
# Ex`-1 stands for recursion in the whole type, i.e. Ex`0 == Union[...]
("recursive(integers(), lists)", "Union[list[Ex`-1], int]"),
# We have overloads for up to five types, then fall back to Any.
# (why five? JSON atoms are None|bool|int|float|str and we do that a lot)
("one_of(integers(), text())", "Union[int, str]"),
(
"one_of(integers(), text(), none(), binary(), builds(list))",
"Union[int, str, None, bytes, list[_T`1]]",
),
(
"one_of(integers(), text(), none(), binary(), builds(list), builds(dict))",
"Any",
),
],
)
def test_revealed_types(tmpdir, val, expect):
"""Check that Mypy picks up the expected `X` in SearchStrategy[`X`]."""
f = tmpdir.join(expect + ".py")
f.write(
"from hypothesis.strategies import *\n"
"s = {}\n"
"reveal_type(s)\n".format(val)
)
typ = get_mypy_analysed_type(str(f.realpath()), val)
assert typ == f"hypothesis.strategies._internal.strategies.SearchStrategy[{expect}]"
- and it's continued passing the whole time. I'm therefore reluctant to make further changes without also understanding enough to be confident that the test, or new test, would actually warn us in future.

@sobolevn sobolevn self-assigned this Mar 1, 2021
@sobolevn
Copy link
Member Author

sobolevn commented Mar 1, 2021

Looks like a job for me! 😆

@Zac-HD Zac-HD added interop how to play nicely with other packages legibility make errors helpful and Hypothesis grokable labels Mar 1, 2021
@sobolevn
Copy link
Member Author

sobolevn commented Mar 1, 2021

Ok, here's what's happening.

dry-python/returns has --no-implicit-reexport option enabled. While hypothesis does not use this option: https://github.com/HypothesisWorks/hypothesis/blob/master/mypy.ini#L1 That's the main reason we see a different type inference.

hypothesis@6.3.2 has one_of defined in core.py and re-exported in strategies/__init__.py via __all__. Which is fine.

hypothesis@6.3.3 has one_of defined in strategies.py. But! It is still re-exported via core.py in strategies/__init__.py (which just has runtime-only import, which is treated as implicit re-export by mypy). Which is not fine, since we lose explicit re-export in strategies -> core step.

Related python/mypy#7042

What are the possible solutions?

  1. Enable no-implicit-reexport for hypothesis, because this check has proven to be a good idea for libraries. Then I can fix all violations
  2. Only fix this particular case without any other changes - it will be easier but less reliable

@Zac-HD
Copy link
Member

Zac-HD commented Mar 1, 2021

  1. Enable no-implicit-reexport for `hypothesis, because this check has proven to be a good idea for libraries. Then I can fix all violations

Given that you've volunteered to fix the violations, I'm happy to go with this! Thanks @sobolevn 🤩

(presuming that it doesn't require unexpectedly ugly code beyond the changed imports)

@sobolevn
Copy link
Member Author

sobolevn commented Mar 1, 2021

Found 149 errors in 14 files (checked 95 source files)

😆 😭

@Zac-HD Zac-HD changed the title st.one_of returns any in the latest releases (6.3.3 and 6.3.4) mypy --no-implicit-reexport infers st.one_of(...) -> Any in 6.3.3+ Mar 1, 2021
@sobolevn
Copy link
Member Author

sobolevn commented Mar 4, 2021

I confirm that it works now: dry-python/returns#847

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

Successfully merging a pull request may close this issue.

2 participants