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

Fix type argument inference for overloaded functions with explicit self types (Fixes #14943). #14975

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

tyralla
Copy link
Contributor

@tyralla tyralla commented Mar 29, 2023

Fix type argument inference for overloaded functions with explicit self types (Fixes #14943).

When finding no compatible callable right away, try to let method ConstraintBuilderVisitor.find_matching_overload_item return the first overloaded function with the suitable self type.

Checked by test case TestOverloadedMethodWithExplictSelfTypes.

…lf types (Fixes python#14943).

When finding no compatible callable right away, try to let method `ConstraintBuilderVisitor.find_matching_overload_item` return the first overloaded function with the suitable self type.

Checked by test case `TestOverloadedMethodWithExplictSelfTypes`.
@AlexWaygood AlexWaygood requested a review from JukkaL March 29, 2023 15:46
@AlexWaygood AlexWaygood mentioned this pull request Apr 4, 2023
2 tasks
# Try to return the first item with the correct self type (fixes issue 14943).
for item in items:
if isinstance(item.definition, FuncDef) and isinstance(item.definition.type, CallableType):
if item.bound_args and item.definition.type.arg_types:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The definition attribute is only supposed to be used for error messages, and it's not guaranteed to be set. It cannot be reliably used for type checking. It's not serialized, in particular, but there may well be other contexts where it's lost that I'm not aware of.

Also, the bound_args attribute is unfortunately not documented (or the documentation seems incorrect). Again, I'm not sure if we can expect it to be always set.

Here's an alternative approach that might work. When we bind the self type to an overloaded method (in mypy.typeops.bind_self), what about filtering the overload items at that point to those matching the self type (only when explicit self types are used)? This filtering would happen in an earlier stage, and I believe we have access to all the necessary information without using undocumented or inconsistently set attributes. The result of the filtering could be a non-overloaded callable type or an overloaded type with fewer items.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback. Using two fragile attributes in five new lines - what a negative hit rate...

Your suggestion targets code sections I did not encounter yet. My first naive implementation of your suggestion solved the original issue but broke many other things. So far, I could not find any information to distinguish implicit and explicit self types within bind_self. Is there some obvious way I am missing?

I would take a second, more thorough try within the next few days. Please do not hesitate to fix this yourself if you already have a good solution in mind and if you want that issue fixed before the 1.2 release.

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 tried to filter within bind_self as suggested. All tests pass (on my computer). Let's see what Mypy primer says.

I am still not overly happy with my approach for the following reasons:

  1. I added the ignore_type_vars option to is_subtype, which seems like a huge change for such a specific problem to me.
  2. I found no way to distinguish explicit self types from implicit self types.
  3. The result is still always an overloaded type (changing this within bind_self would be simple but requires changing its signature and eventually many code sections that use bind_self).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3. The result is still always an overloaded type (changing this within bind_self would be simple but requires changing its signature and eventually many code sections that use bind_self).

I played around with returning a single non-overloaded callable for a few minutes. Maybe it's not a good idea because it changes the signatures in error reports and notes. On the other hand, I did not encounter a case where an overloaded type with a single item caused a problem (but I did not actively search for such a problem).

@github-actions

This comment has been minimized.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

(drive by comment, will look further when I get time)
Hmm, I'd added logic like this in #14017 and later moved it to bind_and_map_method in #14882. Maybe there's some unification to be had

… from `testSelfTypeOverrideCompatibilityTypeVar`
@tyralla
Copy link
Contributor Author

tyralla commented Apr 14, 2023

Hmm, I'd added logic like this in #14017 and later moved it to bind_and_map_method in #14882. Maybe there's some unification to be had

Your tip seems helpful. I could remove the filtering logic you implemented in checker.TypeChecker.bind_and_map_method without breaking any tests. Also, testSelfTypeOverrideCompatibilityTypeVar, previously marked by "xfail", now passes. However, this benefit still comes at the cost of the ignore_type_vars approach (which might eventually shadow some actual bugs?).

@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
Collaborator

Yeah, I'm not sure about the ignore_type_vars

See this other related PR I opened here: #15045 In this case I was able to replace type vars by their upper bound

@github-actions

This comment has been minimized.

@tyralla
Copy link
Contributor Author

tyralla commented Apr 15, 2023

I added the test case you introduced in #15045 for testing (I can remove it later). It passes with the current ignore_type_vars approach.

For further testing, I also disabled the ignore_type_vars option (not committed). This results only in problems for the test cases testSelfTypeRestrictedMethodOverloadInit and testSelfTypeRestrictedMethodOverloadInitFallBacks, which check the following modifications of the self type:

class C(Generic[T]):
    @overload
    def __init__(self: C[T], item: T, use_tuple: Literal[False]) -> None: ...
    @overload
    def __init__(self: C[Tuple[T, ...]], item: T, use_tuple: Literal[True]) -> None: ...

Maybe assuming general TypeVar compatibility in the given context is not such a bad thing because Tuple[T, ...]] could be replaced by arbitrarily complex expressions. And actual bugs (e.g. because T is bound to something else) should be found elsewhere. Or can you think of an example where the ignore_type_vars approach would not remove an overloaded item with a well-defined explicit self type that does not fit the current "original type"?

@hauntsaninja
Copy link
Collaborator

I think that test case is buggy, there was some discussion at #14729 (comment) and python/typing#1340 (comment)

@tyralla
Copy link
Contributor Author

tyralla commented Apr 15, 2023

Interesting. If we would disallow such self type definitions, the ignore_type_vars option would hopefully become obsolete. I commit this change to see the results on the primer.

@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/series.py:3330: error: Signature of "sort_values" incompatible with supertype "NDFrame"  [override]
+ pandas/core/series.py:3330: note:      Superclass:
+ pandas/core/series.py:3330: note:          @overload
+ pandas/core/series.py:3330: note:          def sort_values(self, *, axis: Union[int, Literal['index', 'columns', 'rows']] = ..., ascending: Union[bool, Sequence[bool]] = ..., inplace: Literal[False] = ..., kind: str = ..., na_position: str = ..., ignore_index: bool = ..., key: Optional[Callable[[Series], Union[Series, Union[Union[ExtensionArray, ndarray[Any, Any]], Index, Series]]]] = ...) -> Series
+ pandas/core/series.py:3330: note:          @overload
+ pandas/core/series.py:3330: note:          def sort_values(self, *, axis: Union[int, Literal['index', 'columns', 'rows']] = ..., ascending: Union[bool, Sequence[bool]] = ..., inplace: Literal[True], kind: str = ..., na_position: str = ..., ignore_index: bool = ..., key: Optional[Callable[[Series], Union[Series, Union[Union[ExtensionArray, ndarray[Any, Any]], Index, Series]]]] = ...) -> None
+ pandas/core/series.py:3330: note:          @overload
+ pandas/core/series.py:3330: note:          def sort_values(self, *, axis: Union[int, Literal['index', 'columns', 'rows']] = ..., ascending: Union[bool, Sequence[bool]] = ..., inplace: bool = ..., kind: str = ..., na_position: str = ..., ignore_index: bool = ..., key: Optional[Callable[[Series], Union[Series, Union[Union[ExtensionArray, ndarray[Any, Any]], Index, Series]]]] = ...) -> Optional[Series]
+ pandas/core/series.py:3330: note:      Subclass:
+ pandas/core/series.py:3330: note:          @overload
+ pandas/core/series.py:3330: note:          def sort_values(self, *, axis: Union[int, Literal['index', 'columns', 'rows']] = ..., ascending: Union[bool, int, Sequence[bool], Sequence[int]] = ..., inplace: Literal[False] = ..., kind: str = ..., na_position: str = ..., ignore_index: bool = ..., key: Optional[Callable[[Series], Union[Series, Union[Union[ExtensionArray, ndarray[Any, Any]], Index, Series]]]] = ...) -> Series
+ pandas/core/series.py:3330: note:          @overload
+ pandas/core/series.py:3330: note:          def sort_values(self, *, axis: Union[int, Literal['index', 'columns', 'rows']] = ..., ascending: Union[bool, int, Sequence[bool], Sequence[int]] = ..., inplace: Literal[True], kind: str = ..., na_position: str = ..., ignore_index: bool = ..., key: Optional[Callable[[Series], Union[Series, Union[Union[ExtensionArray, ndarray[Any, Any]], Index, Series]]]] = ...) -> None

pandas-stubs (https://github.com/pandas-dev/pandas-stubs)
- pandas-stubs/core/indexes/range.pyi:87: error: Unused "type: ignore" comment

cwltool (https://github.com/common-workflow-language/cwltool)
- cwltool/main.py:964: error: Unused "type: ignore[arg-type]" comment

mongo-python-driver (https://github.com/mongodb/mongo-python-driver)
- bson/son.py: note: At top level:
- bson/son.py:118: error: Unused "type: ignore" comment

@tyralla
Copy link
Contributor Author

tyralla commented Apr 15, 2023

The ignore_type_vars=False experiment showed that type_object_type_from_function also required some adjustment, as it relies on pre-determined self types. Many problems reported by the primer (at least those related to defaultdict) were due to filtering the overloaded items but not their pre-determined self types. I fixed this, but so far likely not in the best possible style.

However, some other findings gave me the impression that the ignore_type_vars approach might really be okay, so I reactivated it. Everything else I tried had its edges but no advantages I am aware of.

NicolasT pushed a commit to NicolasT/gptsum that referenced this pull request Aug 27, 2023
This requires an explicit `cast` of `shutil.copyfileobj` to work-around
a typeshed/Mypy issue.

Bumps [mypy](https://github.com/python/mypy) from 1.1.1 to 1.4.1.
- [Commits](python/mypy@v1.1.1...v1.4.1)

---
updated-dependencies:
- dependency-name: mypy
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

See: python/mypy#15031
See: https://github.com/python/mypy/pull/14902/files#diff-363460977156fcfda748f21565484fe1d5862edf2823e9784cbf34d1e52ff2f2
See: python/mypy#14975

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Nicolas Trangez <[email protected]>
NicolasT pushed a commit to NicolasT/gptsum that referenced this pull request Aug 27, 2023
This requires an explicit `cast` of `shutil.copyfileobj` to work-around
a typeshed/Mypy issue.

Bumps [mypy](https://github.com/python/mypy) from 1.1.1 to 1.4.1.
- [Commits](python/mypy@v1.1.1...v1.4.1)

---
updated-dependencies:
- dependency-name: mypy
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

See: python/mypy#15031
See: https://github.com/python/mypy/pull/14902/files#diff-363460977156fcfda748f21565484fe1d5862edf2823e9784cbf34d1e52ff2f2
See: python/mypy#14975

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Nicolas Trangez <[email protected]>
NicolasT pushed a commit to NicolasT/gptsum that referenced this pull request Aug 27, 2023
This requires an explicit `cast` of `shutil.copyfileobj` to work-around
a typeshed/Mypy issue.

Bumps [mypy](https://github.com/python/mypy) from 1.1.1 to 1.4.1.
- [Commits](python/mypy@v1.1.1...v1.4.1)

---
updated-dependencies:
- dependency-name: mypy
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

See: python/mypy#15031
See: https://github.com/python/mypy/pull/14902/files#diff-363460977156fcfda748f21565484fe1d5862edf2823e9784cbf34d1e52ff2f2
See: python/mypy#14975

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Nicolas Trangez <[email protected]>
JukkaL added a commit that referenced this pull request Oct 3, 2024
JukkaL added a commit that referenced this pull request Oct 3, 2024
Fix type argument inference for overloaded functions with explicit self
types. Filter out the overload items based on the declared and actual
types of self. The implementation is best effort and does the filtering
only in simple cases, to reduce the risk of regressions (primarily
performance, but I worry also about infinite recursion). I added a fast
path for the typical case, since without it the filtering was quite
expensive.

Note that the overload item filtering already worked in many contexts.
This only improves it in specific contexts -- at least when inferring
generic protocol compatibility.

This is a more localized (and thus lower-risk) fix compared to #14975
(thanks @tyralla!). #14975 might still be a good idea, but I'm not
comfortable merging it now, and I want a quick fix to unblock the mypy
1.12 release.

Fixes #15031. Fixes #17863.

Co-authored by @tyralla.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot infer type argument from overloaded function with explicit self types
3 participants