-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Fix type argument inference for overloaded functions with explicit self types (Fixes #14943). #14975
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
Closed
tyralla
wants to merge
10
commits into
python:master
from
tyralla:fix/protocols_overloads_and_self_types
Closed
Fix type argument inference for overloaded functions with explicit self types (Fixes #14943). #14975
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
4b1fe1c
Fix type argument inference for overloaded functions with explicit se…
tyralla f354153
revert first attempt
tyralla 589e0c6
try filtering in `bind_self` instead
tyralla 9c107b3
black
tyralla b143796
flake
tyralla 6b1573f
Simplify `checker.TypeChecker.bind_and_map_method` and remove "xfail"…
tyralla f1414f2
Add `testSelfTypeOverrideCompatibilitySelfTypeVar` (not my work: take…
tyralla 6e5476a
ignore_type_vars=False for testing
tyralla 3097ac2
Also filter externally defined self types (preliminary approach)
tyralla 33a1f96
reverse ignore_type_vars=False
tyralla File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next
Next commit
Fix type argument inference for overloaded functions with explicit se…
…lf 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`.
- Loading branch information
commit 4b1fe1c6f9d75a852c666b4110abec6aa0c4552c
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
ignore_type_vars
option tois_subtype
, which seems like a huge change for such a specific problem to me.bind_self
would be simple but requires changing its signature and eventually many code sections that usebind_self
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).