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

Remove unnecessary overloads from re #6762

Closed
wants to merge 5 commits into from

Conversation

jpy-git
Copy link
Contributor

@jpy-git jpy-git commented Dec 30, 2021

This PR follows on from the discussion here where @AlexWaygood highlighted that overloads are unnecessary when a function has the same return type and therefore simple union-types should be used instead. This PR combines these unnecessary overloads in re.pyi and for Pattern in typing.pyi.

Additionally I spotted that Pattern.split should have a return type of list[AnyStr | Any] rather than list[AnyStr]:
image
(re.split is already correct)

stdlib/re.pyi Outdated Show resolved Hide resolved
@jpy-git jpy-git mentioned this pull request Dec 30, 2021
@github-actions

This comment has been minimized.

srittau
srittau previously approved these changes Dec 30, 2021
@srittau
Copy link
Collaborator

srittau commented Dec 30, 2021

The primer output looks a bit concerning.

@srittau srittau dismissed their stale review December 30, 2021 18:47

primer output

@github-actions

This comment has been minimized.

@Akuli Akuli changed the title Remove unnecessary overrides from re Remove unnecessary overloads from re Dec 30, 2021
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/strings/object_array.py:216: error: Value of type variable "AnyStr" of "compile" cannot be "Union[str, Pattern[Any], Any]"  [type-var]
+ pandas/core/array_algos/replace.py:88: error: Value of type variable "AnyStr" of "search" cannot be "Union[str, Pattern[Any], Any]"  [type-var]

sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/util/docstrings.py:20:22: error: Value of type variable "AnyStr" of "compile" cannot be "Union[str, Pattern[Any], Any]"
+ sphinx/util/docstrings.py: note: In function "separate_metadata":
+ sphinx/util/docstrings.py:39:30: error: Value of type "Union[str, Pattern[Any], Any]" is not indexable
+ sphinx/testing/util.py: note: In function "assert_re_search":
+ sphinx/testing/util.py:34:12: error: Value of type variable "AnyStr" of "search" cannot be "Union[str, Pattern[Any], Any]"
+ sphinx/testing/util.py: note: In function "assert_not_re_search":
+ sphinx/testing/util.py:39:8: error: Value of type variable "AnyStr" of "search" cannot be "Union[str, Pattern[Any], Any]"

@AlexWaygood
Copy link
Member

Well, I can't say I really understand why a union-type causes mypy to produce these false-positives when overloads don't. It doesn't make much sense to me.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Dec 30, 2021

AnyStr is a type variable, so mypy might be struggling to solve T | Pattern[T] == str | Pattern[Any]. I'm not convinced this isn't a mypy bug (e.g. unions can cause ambiguity when solving type vars, Any is a tricky type, value restricted type vars are tricky), but mypy's confusion is at least understandable. Would be worth checking how other type checkers deal with this.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Dec 30, 2021

Came back to this after eating a lunch, I'm now fairly sure this is a mypy bug, but I'd still be curious to see if other type checkers do the right thing.

from typing import Any, TypeVar, Union

T = TypeVar("T")
V = TypeVar("V", str, bytes)

def check_t(x: T | list[T]) -> T: ...
def check_v(x: V | list[V]) -> V: ...

x_str: str
x_list_any: list[Any]
x_str_list_any: str | list[Any]

# mypy does the correct thing here

reveal_type(check_t(x_str))  # str
reveal_type(check_t(x_list_any))  # list[Any] | Any
reveal_type(check_t(x_str_list_any))  # str | list[Any] | Any
reveal_type(check_v(x_str))  # str

# but maybe not here

# E: Value of type variable "V" of "check_v" cannot be "Union[List[Any], Any]"
# N: Revealed type is "Union[builtins.list[Any], Any]"
reveal_type(check_v(x_list_any))
# E: Value of type variable "V" of "check_v" cannot be "Union[str, List[Any], Any]"
# N: Revealed type is "Union[builtins.str, builtins.list[Any], Any]"
reveal_type(check_v(x_str_list_any))

Looking at the output, my guess is that mypy isn't taking value restrictions into account when solving constraints and only using value restriction as a check later. That is, list[Any] cannot match the T part of T | list[T] because it would violate the value restriction.

@jpy-git
Copy link
Contributor Author

jpy-git commented Dec 30, 2021

What I find a bit weird about these mypy_primer errors is that the changes in this PR are basically the same as what we added for regex in #6713.

If we take this error from the mypy_primer output:
sphinx/util/docstrings.py:20:22: error: Value of type variable "AnyStr" of "compile" cannot be "Union[str, Pattern[Any], Any]
It happens on this line:
https://github.com/sphinx-doc/sphinx/blob/eed0730b4ba3bd2fbd34f2d6ab555ba876c77717/sphinx/util/docstrings.py#L20
but if we repeat that test but using regex instead, which has similar union typing pattern: AnyStr | _regex.Pattern[AnyStr], we don't see any errors as expected:

from docutils.parsers.rst.states import Body
import regex

s = Body.patterns["field_marker"]
reveal_type(s)
p = regex.compile(s)
reveal_type(p)

>>> mypy test.py
test2.py:5: note: Revealed type is "Any"
test2.py:7: note: Revealed type is "regex._regex.Pattern[Any]"

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Dec 31, 2021

That's probably related to your setup, e.g. looks like you might be missing docutils-stubs (note, different from types-docutils) or custom typeshed dir. I can repro with your test.py just fine.
The code in my previous comment is also a self contained repro (replace list -> Pattern, V -> AnyStr to make it resemble the original case)

@jpy-git
Copy link
Contributor Author

jpy-git commented Dec 31, 2021

Found this comment that seems to be referring to it being a known issue with mypy not being able to resolve T | Cls[T] and hence the overloads:
#5470 (comment)

Wasn't able to track down a corresponding issue in mypy though

@hauntsaninja
Copy link
Collaborator

I opened python/mypy#11880 to track this. As I mentioned, the issue seems clearly related to value restriction

@srittau srittau added the status: deferred Issue or PR deferred until some precondition is fixed label Jan 2, 2022
@srittau
Copy link
Collaborator

srittau commented Jan 2, 2022

I marked this as "deferred" for now, until the mypy problems are sorted out. PR seems good otherwise.

AlexWaygood added a commit to AlexWaygood/typeshed that referenced this pull request Jan 2, 2022
The semantics of this proposed PR should be identical to the existing stub. I'm curious, though, to see whether primer shows false-positives like those we saw in python#6762
@AlexWaygood AlexWaygood mentioned this pull request Jan 2, 2022
@JelleZijlstra
Copy link
Member

I'm not hopeful that the mypy bug will be resolved any time soon. It doesn't seem like this PR fixes any concrete problem, so can we just close it?

@Akuli Akuli closed this Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: deferred Issue or PR deferred until some precondition is fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants