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 inference of protocol against overloaded function #12227

Merged
merged 2 commits into from
Feb 22, 2022
Merged

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented Feb 21, 2022

We used to infer a callable in a protocol against all overload
items. This could result in incorrect results, if only one
of the overload items would actually match the protocol.
For example, we could get incorrect constraints from
an incompatible overload item.

Fix the issue by only considering the first matching overload
item.

This seems to help with protocols involving __getitem__.
In particular, this fixes regressions related to
SupportsLenAndGetItem, which is used for random.choice.

We used to infer a callable in a protocol against all overload
items. This could result in incorrect results, if only one
of the overload items would actually match the protocol.

Fix the issue by only considering the first matching overload
item.

This seems to help with protocols involving `__getitem__`.
In particular, this fixes regressions related to
`SupportsLenAndGetItem`, which is used for `random.choice`.
@github-actions

This comment has been minimized.

@JukkaL JukkaL marked this pull request as draft February 21, 2022 15:39
@JukkaL
Copy link
Collaborator Author

JukkaL commented Feb 21, 2022

It looks like there are some regressions. I will have a look.

@github-actions

This comment has been minimized.

@JukkaL JukkaL marked this pull request as ready for review February 21, 2022 16:58
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.

Makes sense, although I wouldn't mind more tests where the protocol contains an overloaded def.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Feb 22, 2022

Makes sense, although I wouldn't mind more tests where the protocol contains an overloaded def.

I started adding tests, and it turns out you had a very good point -- overloads in protocols seem to be pretty poorly tested and still have some issues. However, I don't want to block the next mypy public release on fixing the remaining issues. I'll create a follow-up issue about this and merge this now, since this fixes a regression.

@JukkaL JukkaL closed this Feb 22, 2022
@JukkaL JukkaL reopened this Feb 22, 2022
@JukkaL JukkaL merged commit a8b6d6f into master Feb 22, 2022
@JukkaL JukkaL deleted the mypy-regression-1 branch February 22, 2022 13:56
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants