-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Adds support for basic union math with overloads #4842
Adds support for basic union math with overloads #4842
Conversation
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.
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.
@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. |
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.
4edaacb
to
a5d181b
Compare
Superseded by #5084. |
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_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 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 ofAny
under 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.