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

Add partial overload checks #5476

Merged
merged 13 commits into from Aug 27, 2018

Conversation

Projects
None yet
2 participants
@Michael0x2a
Collaborator

Michael0x2a commented Aug 14, 2018

This pull request adds more robust support for detecting partially overlapping types. Specifically, it detects overlaps with...

  1. TypedDicts
  2. Tuples
  3. Unions
  4. TypeVars
  5. Generic types containing variations of the above.

It also swaps out the code for detecting overlaps with operators and removes some associated (and now unused) code.

This pull request builds on top of #5474 and #5475 -- once those two PRs are merged, I'll rebase this diff if necessary.

This pull request also supercedes #5475 -- that PR contains basically the same code as these three PRs, just smushed together.

@Michael0x2a Michael0x2a requested a review from ilevkivskyi Aug 14, 2018

@Michael0x2a

This comment has been minimized.

Show comment
Hide comment
@Michael0x2a

Michael0x2a Aug 14, 2018

Collaborator

@ilevkivskyi -- So, just to summarize the status of all these PRs in one place...

The following PRs are independent from one another and can be reviewed in any order:

  1. #5474 (Generalizing subtype caches)
  2. #5475 (Refactoring reversible operators)
  3. #5321 (Clean up old overload logic -- this one is low priority)

This PR builds on top of code from 1 and 2. Once those two PRs are merged in, I'll rebase this PR to shrink the diff size.

I've left #5280, the old PR for adding partial overlaps, open for now since it has some extra context/probably includes some feedback that I've forgotten to incorporate. Feel free to close it whenever though if you want to minimize clutter -- I can re-find it pretty easily.

I've tested this PR on our internal codebases -- so far, I believe this PR introduces one regression that I've as of yet been unable to reproduce and fix. (Basically, the code is asserting some field is None, and mypy gets confused for some reason).

I'll continue working on tracking that final regression and fixing it over the next few days while I'm waiting on your code reviews, but if you have time to take a look/can figure out how to produce a smaller repo, I'd be extremely grateful.

(And sorry for dumping all of this code on you all of a sudden -- if you want me to break up the above PRs into even smaller ones, let me know, and I'll be happy to do it.)

Collaborator

Michael0x2a commented Aug 14, 2018

@ilevkivskyi -- So, just to summarize the status of all these PRs in one place...

The following PRs are independent from one another and can be reviewed in any order:

  1. #5474 (Generalizing subtype caches)
  2. #5475 (Refactoring reversible operators)
  3. #5321 (Clean up old overload logic -- this one is low priority)

This PR builds on top of code from 1 and 2. Once those two PRs are merged in, I'll rebase this PR to shrink the diff size.

I've left #5280, the old PR for adding partial overlaps, open for now since it has some extra context/probably includes some feedback that I've forgotten to incorporate. Feel free to close it whenever though if you want to minimize clutter -- I can re-find it pretty easily.

I've tested this PR on our internal codebases -- so far, I believe this PR introduces one regression that I've as of yet been unable to reproduce and fix. (Basically, the code is asserting some field is None, and mypy gets confused for some reason).

I'll continue working on tracking that final regression and fixing it over the next few days while I'm waiting on your code reviews, but if you have time to take a look/can figure out how to produce a smaller repo, I'd be extremely grateful.

(And sorry for dumping all of this code on you all of a sudden -- if you want me to break up the above PRs into even smaller ones, let me know, and I'll be happy to do it.)

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Aug 14, 2018

Collaborator

I've left #5280, the old PR for adding partial overlaps, open for now since it has some extra context/probably includes some feedback that I've forgotten to incorporate. Feel free to close it whenever though if you want to minimize clutter -- I can re-find it pretty easily.

I would prefer to close it, just to not get lost :-)

(And sorry for dumping all of this code on you all of a sudden -- if you want me to break up the above PRs into even smaller ones, let me know, and I'll be happy to do it.)

No problem at all, thanks for working on this. The PR sizes are just right for my taste.

(Btw, there is already a merge conflict, you can probably rebase after merging the subtype cache.)

Collaborator

ilevkivskyi commented Aug 14, 2018

I've left #5280, the old PR for adding partial overlaps, open for now since it has some extra context/probably includes some feedback that I've forgotten to incorporate. Feel free to close it whenever though if you want to minimize clutter -- I can re-find it pretty easily.

I would prefer to close it, just to not get lost :-)

(And sorry for dumping all of this code on you all of a sudden -- if you want me to break up the above PRs into even smaller ones, let me know, and I'll be happy to do it.)

No problem at all, thanks for working on this. The PR sizes are just right for my taste.

(Btw, there is already a merge conflict, you can probably rebase after merging the subtype cache.)

Add support for partially overlapping types
This pull request adds more robust support for detecting partially
overlapping types. Specifically, it detects overlaps with...

1. TypedDicts
2. Tuples
3. Unions
4. TypeVars
5. Generic types containing variations of the above.

It also swaps out the code for detecting overlaps with operators and
removes some associated (and now unused) code.

This pull request builds on top of #5474
and #5475 -- once those two PRs are
merged, I'll rebase this diff if necessary.

This pull request also supercedes #5475 --
that PR contains basically the same code as these three PRs, just
smushed together.
@Michael0x2a

This comment has been minimized.

Show comment
Hide comment
@Michael0x2a

Michael0x2a Aug 16, 2018

Collaborator

@ilevkivskyi -- the rebase is done now, fyi

Collaborator

Michael0x2a commented Aug 16, 2018

@ilevkivskyi -- the rebase is done now, fyi

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Aug 16, 2018

Collaborator

OK, thanks! It is a bit late here. I will finish this tomorrow morning.

Collaborator

ilevkivskyi commented Aug 16, 2018

OK, thanks! It is a bit late here. I will finish this tomorrow morning.

@ilevkivskyi

Thanks, looks good! Here are few comments.

Show outdated Hide outdated mypy/checker.py Outdated
Show outdated Hide outdated mypy/checker.py Outdated
Show outdated Hide outdated mypy/checker.py Outdated
Show outdated Hide outdated mypy/meet.py Outdated
Show outdated Hide outdated mypy/meet.py Outdated
Show outdated Hide outdated mypy/meet.py Outdated
if out is None:
pass
return out
[builtins fixtures/isinstance.pyi]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 17, 2018

Collaborator

TBH I am not sure what these two tests test. Maybe add a note or somehow modify them so that it is more clear what is a potential failure?

@ilevkivskyi

ilevkivskyi Aug 17, 2018

Collaborator

TBH I am not sure what these two tests test. Maybe add a note or somehow modify them so that it is more clear what is a potential failure?

Show outdated Hide outdated test-data/unit/check-overloading.test Outdated
Show outdated Hide outdated mypy/meet.py Outdated
Show outdated Hide outdated mypy/meet.py Outdated
@Michael0x2a

This comment has been minimized.

Show comment
Hide comment
@Michael0x2a

Michael0x2a Aug 26, 2018

Collaborator

@ilevkivskyi -- ok, sorry for the delay! I can confirm that this is now (finally) ready for a second look, and can also confirm that it triggers no new errors in our internal codebases.

Specific changes I made:

  1. Removed debug prints and comments
  2. Consolidated the two "unsafe overlapping overload" checks into just a single one, and removed the extraneous whitespace.
  3. Finally tracked down and fixed a bug related to Nones, isinstance checks, and no-strict-optional. In sort, the bug was that I was inconsistently filtering out NoneType when in non-strict-optional mode.
  4. Created an issue (#5510) to track the extra error message that we generate when turning a generic overload function into a method.
  5. Added a comment explaining why we don't care about variance in is_overlapping_types.
  6. Added a comment to the two extra test cases I added to check-isinstance.test explaining what they do.

I've also left a few responses to your comments up above.

Collaborator

Michael0x2a commented Aug 26, 2018

@ilevkivskyi -- ok, sorry for the delay! I can confirm that this is now (finally) ready for a second look, and can also confirm that it triggers no new errors in our internal codebases.

Specific changes I made:

  1. Removed debug prints and comments
  2. Consolidated the two "unsafe overlapping overload" checks into just a single one, and removed the extraneous whitespace.
  3. Finally tracked down and fixed a bug related to Nones, isinstance checks, and no-strict-optional. In sort, the bug was that I was inconsistently filtering out NoneType when in non-strict-optional mode.
  4. Created an issue (#5510) to track the extra error message that we generate when turning a generic overload function into a method.
  5. Added a comment explaining why we don't care about variance in is_overlapping_types.
  6. Added a comment to the two extra test cases I added to check-isinstance.test explaining what they do.

I've also left a few responses to your comments up above.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Aug 27, 2018

Collaborator

Thanks for the updates! I will make another pass of review tomorrow (most likely).

Collaborator

ilevkivskyi commented Aug 27, 2018

Thanks for the updates! I will make another pass of review tomorrow (most likely).

@ilevkivskyi

Looks good now modulo the TODO comment I mentioned, feel free to merge after adding it.

Btw, there is a merge conflict, could you please double check this is still OK with internal codebases after fixing the conflict (just in case).

@Michael0x2a

This comment has been minimized.

Show comment
Hide comment
@Michael0x2a

Michael0x2a Aug 27, 2018

Collaborator

@ilevkivskyi -- it looks like everything checks out internally.

Before I merge though, I do have one final concern: while I can confirm this diff doesn't introduce any new errors, it's still possible that this diff will start making mypy mark previously reachable branches as being unreachable.

How big of a concern is this/do you think this is something we should try testing for first in some way?

Collaborator

Michael0x2a commented Aug 27, 2018

@ilevkivskyi -- it looks like everything checks out internally.

Before I merge though, I do have one final concern: while I can confirm this diff doesn't introduce any new errors, it's still possible that this diff will start making mypy mark previously reachable branches as being unreachable.

How big of a concern is this/do you think this is something we should try testing for first in some way?

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Aug 27, 2018

Collaborator

How big of a concern is this/do you think this is something we should try testing for first in some way?

I think this is OK. I am not too concerned about rare false negatives, we are already aware of the problem, and the issues are filed. I think you can go ahead and merge this.

Collaborator

ilevkivskyi commented Aug 27, 2018

How big of a concern is this/do you think this is something we should try testing for first in some way?

I think this is OK. I am not too concerned about rare false negatives, we are already aware of the problem, and the issues are filed. I think you can go ahead and merge this.

@Michael0x2a Michael0x2a merged commit dd70710 into python:master Aug 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Michael0x2a Michael0x2a deleted the Michael0x2a:improve-partial-overload-checks branch Aug 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment