Repository navigation
Adds support for basic union math with overloads - #4842
Closed
Michael0x2a wants to merge 3 commits into
Closed
Michael0x2a wants to merge 3 commits into
Michael0x2a wants to merge 3 commits into
Conversation
ilevkivskyi
reviewed
Apr 5, 2018
Member
There was a problem hiding this comment.
It is a good piece of work, thanks! It would be god to have more time to review this, but now I have just one brief comment about tests: You don't have any tests wth generic functions and methods of generic classes. There might be some corner cases when type variables are involved.
Collaborator
Author
|
@ilevkivskyi -- yep, will do. I plan on adding more tests either later today or over the weekend -- since this pull request does change the semantics of overloads slightly, I just wanted to make sure it wasn't going to be rejected out-of-hand first :) No rush on reviewing -- it looks like people are pretty busy so I don't mind waiting. |
Michael0x2a
force-pushed
the
modify-overloads-to-support-basic-union-math
branch
from
April 11, 2018 15:30
1ebd4c4 to
4edaacb
Compare
This commit adds support for very basic and simple union math when calling overloaded functions, resolving python#4576. As a side effect, this change also fixes a bug where calling overloaded functions can sometimes silently infer a return type of 'Any' and slightly modifies the semantics of how mypy handles overlaps in overloaded functions. Details on specific changes made: 1. The new algorithm works by modifying checkexpr.overload_call_targets to return all possible matches, rather then just one. We start by trying the first matching signature. If there was some error, we (conservatively) attempt to union all of the matching signatures together and repeat the typechecking process. If it doesn't seem like it's possible to combine the matching signatures in a sound way, we end and just output the errors we obtained from typechecking the first match. The "signature-unioning" code is currently deliberately very conservative. I figured it was better to start small and attempt to handle only basic cases like python#1943 and relax the restrictions later as needed. For more details on this algorithm, see the comments in checkexpr.union_overload_matches. 2. This change incidentally resolves any bugs related to how calling an overloaded function can sometimes silently infer a return type of Any. Previously, if a function call caused an overload to be less precise then a previous one, we gave up and returned a silent Any. This change removes this case altogether and only infers Any if either (a) the caller arguments explicitly contains Any or (b) if there was some error. For example, see python#3295 and python#1322 -- I believe this pull request touches on and maybe resolves (??) those two issues. 3. As a result, this caused a few errors in mypy where code was relying on this "silently infer Any" behavior -- see the changes in checker.py and semanal.py. Both files were using expressions of the form `zip(*iterable)`, which ended up having a type of `Any` under the old algorithm. The new algorithm will instead infer `Iterable[Tuple[Any, ...]]` which actually matches the stubs in typeshed. 4. Many of the attrs tests were also relying on the same behavior. Specifically, these changes cause the attr stubs in `test-data/unit/lib-stub` to no longer work. It seemed that expressions of the form `a = attr.ib()` were evaluated to 'Any' not because of a stub, but because of the 'silent Any' bug. I couldn't find a clean way of fixing the stubs to infer the correct thing under this new behavior, so just gave up and removed the overloads altogether. I think this is fine though -- it seems like the attrs plugin infers the correct type for us anyways, regardless of what the stubs say. If this pull request is accepted, I plan on submitting a similar pull request to the stubs in typeshed. 4. This pull request also probably touches on python/typing#253. We still require the overloads to be written from the most narrow to general and disallow overlapping signatures. However, if a *call* now causes overlaps, we try the "union" algorithm described above and default to selecting the first matching overload instead of giving up.
Michael0x2a
force-pushed
the
modify-overloads-to-support-basic-union-math
branch
from
April 23, 2018 22:13
4edaacb to
a5d181b
Compare
Collaborator
Author
|
Superseded by #5084. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This commit adds support for very basic and simple union math when calling overloaded functions, resolving #4576.
One thing led to another, and this ended up accidentally fixing or touching on several different overload-related issues. In particular, I believe this pull request:
Although this change is functional and mergeable, I was planning on polishing it more -- adding more tests, fleshing out the union math behavior, etc.
However, I think these are sort of big changes and wanted to check in and make sure this pull request is actually welcome/is a good idea. If not, let me know, and I'd be happy to abandon it.
Details on specific changes made:
The new algorithm works by modifying
checkexpr.overload_call_targetsto return all possible matches, rather then just one.We start by trying the first matching signature. If there was some error, we (conservatively) attempt to union all of the matching signatures together and repeat the typechecking process.
If it doesn't seem like it's possible to combine the matching signatures in a sound way, we end and just output the errors we obtained from typechecking the first match.
The "signature-unioning" code is currently deliberately very conservative. I figured it was better to start small and attempt to handle only basic cases like os.open doesn't accept Union[bytes, Text] #1943 and relax the restrictions later as needed. For more details on this algorithm, see the comments in
checkexpr.union_overload_matches.This change incidentally resolves any bugs related to how calling an overloaded function can sometimes silently infer a return type of Any. Previously, if a function call caused an overload to be
less precise then a previous one, we gave up and returned a silent Any.
This change removes this case altogether and only infers Any if either (a) the caller arguments explicitly contains Any or (b) if there was some error.
For example, see Should have a way to annotate an Optional carryover method. #3295 and Any incorrectly inferred for a call to an overloaded function #1322 -- I believe this pull request touches on and maybe resolves (??) those two issues.
As a result, this caused a few errors in mypy where code was relying on this "silently infer Any" behavior -- see the changes in checker.py and semanal.py. Both files were using expressions of the form
zip(*iterable), which ended up having a type ofAnyunder the old algorithm. The new algorithm will instead inferIterable[Tuple[Any, ...]]which actually matches the stubs in typeshed.Many of the attrs tests were also relying on the same behavior. It seemed that expressions of the form
a = attr.ib()were evaluated to 'Any' not because of the definitions in the stubs, but because of the 'silent Any' bug.I couldn't find a clean way of fixing the stubs to infer the correct thing under this new behavior, so just gave up and removed the overloads altogether. I think this is fine though -- it seems like the attrs plugin infers the correct type for us anyways, regardless of what the stubs say.
If this pull request is accepted, I plan on submitting a similar pull request to the stubs in typeshed.
This pull request also probably touches on Clarify what @overload means typing#253. We still require the overloads to be written from the most narrow to general and disallow overlapping signatures.
However, if a call now causes overlaps, we try the "union" algorithm described above and default to selecting the first matching overload instead of giving up.