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

Tracking issue for overloads #5119

Closed
8 of 20 tasks
Michael0x2a opened this issue May 29, 2018 · 11 comments
Closed
8 of 20 tasks

Tracking issue for overloads #5119

Michael0x2a opened this issue May 29, 2018 · 11 comments
Assignees
Labels
meta Issues tracking a broad area of work topic-overloads

Comments

@Michael0x2a
Copy link
Collaborator

Michael0x2a commented May 29, 2018

This is an issue summarizing all remaining TODOs for the overloads overhaul.

Done

In-progress

Remaining todos:

Remaining todos; deprioritized

Issues that need triaging

@ilevkivskyi
Copy link
Member

Update docs to clarify what the new overload semantics are.

Just for myself, a simple summary of current logic:

  1. Definition site:
  • First signature args never overlap with second signature args (either type or arity) -> Always OK
  • First signature args are broader than second signature args -> Error
  • First signature args are narrower than second signature args -> OK if the return is also narrower, otherwise Error
  • There is a partial overlap between first and second signature args -> Error, but three exceptions where overlap is small/hard to express, so we still allow it: multiple inheritance, empty collections, descriptors.
  1. Call site:
  • We pick the first possible match (which would be also normally the narrowest due to above rules)
  • Also if there are two matches because of an allowed partial overlap, first signature wins
  • If there are two matches because of Any we return Any
  • We use simple union math if it allows match/gives more precise match.

@Michael0x2a did I miss something?

@Michael0x2a
Copy link
Collaborator Author

@ilevkivskyi -- No, I think that looks good. A few tweaks:

  • I would rephrase the third and fourth bullet points to read like this:

    • First signature args are narrower than or partially overlapping with second signature args -> OK if the return is also narrower, otherwise error.

      There are three exceptions where the overlap is small/hard to express, so we still allow it: multiple inheritance, empty collections, and descriptors.

    • First signature args are all partially overlapping with second args -> Error, even if the return is narrower. This would be safe if the return is narrower, but it's sloppy and probably worth warning about. (This currently isn't implemented.)

    Basically, we also need to check the return type when some of the args are narrower and some are partially overlapping.

    The last bullet point is strictly speaking unnecessary, but I believe we decided it would be a good idea because it could catch some sloppy code. (I should add this to my todo list, actually.)

  • Regarding definition sites in general: we pretend that strict optional is always enabled at that stage. Idk if that's a super important detail though, especially since strict-optional is the default now.

@ilevkivskyi
Copy link
Member

The last bullet point is strictly speaking unnecessary, but I believe we decided it would be a good idea because it could catch some sloppy code. (I should add this to my todo list, actually.)

Yes, I remember we discussed this, we can do this is mypy, but this doesn't need to be in spec/PEP 484 just to make the mental model more "compact".

@ilevkivskyi
Copy link
Member

@Michael0x2a what is your timeline for working on this? 0.620 release will be around June 29 (right after Python 3.7) and I will be the release manager. I will be glad to see better support for overloads in (since this is a big and long awaited feature). Do you think we can make it? If needed, I can review the PRs faster/devote more time to this.

@Michael0x2a
Copy link
Collaborator Author

Michael0x2a commented Jun 2, 2018

@ilevkivskyi: In short, I think I can have the overload TODOs and the bulk of the issues ready by then; I'm a bit less certain about operator methods.

Specifically:

  • I'm currently working on the first two TODOs. Progress has been a bit slow due to unrelated reasons, but I'm planning on having the first set of PRs for those ready sometime this upcoming week.

  • The third issue is a bit iffier, mostly because I haven't given much thought to operator methods. I think it'll be relatively trivial to add support for generics in a way that preserves the existing semantics; refactoring and removing unnecessary checks will probably require more thought. That said, I don't think this is as big of a todo, so I'm planning on deprioritizing this until overloads are solid.

  • I also briefly took a look at the overload-related issues on the tracker: I think we can close roughly half of the issues atm; resolving the other half will probably take up the rest of the month.

    I plan on stepping through the issues more carefully and closing the resolved ones once 0.610 is released. (I know the blog announcement likes to list issues that were closed and I don't want to muddy matters by closing a bunch of issues unrelated to that release.)

  • The other TODOs look either trivial or non-essential.

@ilevkivskyi
Copy link
Member

I think I can have the overload TODOs and the bulk of the issues ready by then;

OK, this is perfect.

The third issue is a bit iffier, mostly because I haven't given much thought to operator methods.

There is also a related issue that dunder code is quite old, most of it is not using checkmember functions, instead duplicating it in some special ways. I will open a dedicated issue for this now.

I think we can close roughly half of the issues atm;

This is great!

Michael0x2a added a commit to Michael0x2a/mypy that referenced this issue Jun 6, 2018
This commit addresses TODO 2 from python#5119
by adding support for detecting overloads with partially overlapping
arities.

It also refactors the `is_callable_compatible` method. Specifically,
this pull request...

1. Pulls out a lot of the logic for iterating over formal arguments into
   a helper method in CallableType.

2. Pulls out logic for handling varargs and kwargs outside of loops.

3. Rearranges some of the logic so we can return earlier slightly more
   frequently.
Michael0x2a added a commit that referenced this issue Jun 16, 2018
This commit addresses TODO 2 from #5119
by adding support for detecting overloads with partially overlapping
arities.

It also refactors the `is_callable_compatible` method. Specifically,
this pull request...

1. Pulls out a lot of the logic for iterating over formal arguments into
   a helper method in CallableType.

2. Pulls out logic for handling varargs and kwargs outside of loops.

3. Rearranges some of the logic so we can return earlier slightly more
   frequently.
@ilevkivskyi
Copy link
Member

If all arguments in the first overload are partially overlapping with the second, report an error (even if the first return type is narrower).

On a second thought, I think we can just skip this. Although it would flag some weird definitions, this is not technically type unsafe, and unlike the "... will be never matched" error (which is also technically not type unsafe, but is obvious) may only confuse users.

@ilevkivskyi
Copy link
Member

I just read again through the list and propose to focus on three things before the next release (apart from finishing the existing PRs):

  • Add better support for partially-overlapping types. We don't need to go really far here, just have a bit more complete implementation (btw there is a function in meet.py called is_overlapping_types you can maybe re-use some code from it). Also I would completely remove the plan to error even if returns are subtypes. IIRC I proposed this, but now I see that people aren't happy when mypy is prohibiting weird but safe code ("function doesn't return a value").
  • Improve error messages (mostly fix the "no overload variant matches", plus maybe few smaller improvements).
  • Improve interaction between dunders and overloads. I see you listed it in deprioritized, but I think there may be few low hanging fruits here. For example it is straight prohibited to override an overloaded dunder with another overload (even with the same type!) In my experience dunders are actually often overloaded and people can be very surprised.

@Michael0x2a
Copy link
Collaborator Author

@ilevkivskyi -- sounds good to me. I have a PR for partially-overlapping types that I'm working on right now. It still needs a loooot more testing, but hopefully it'll be ready for review during the next few days or so.

For example it is straight prohibited to override an overloaded dunder with another overload (even with the same type!) In my experience dunders are actually often overloaded and people can be very surprised.

Also sounds good -- I agree that we can probably find a few easy wins.

@ilevkivskyi
Copy link
Member

I will need to take a day of rest, I can review your PRs on Sunday morning.

@97littleleaf11 97littleleaf11 added the meta Issues tracking a broad area of work label Nov 12, 2021
@ichard26
Copy link
Collaborator

While I don't think every single item mentioned in this tracking issue has been finished, the vast majority is done and the remaining work can live as independent issues. The only thing that doesn't have an issue is general refactoring and clean up, would one be useful @Michael0x2a?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues tracking a broad area of work topic-overloads
Projects
None yet
Development

No branches or pull requests

5 participants
@Michael0x2a @97littleleaf11 @ilevkivskyi @ichard26 and others