List possible overload variants when none match #5296

Michael0x2a · 2018-06-29T05:22:34Z

Resolves #672 and supersedes #5054.

Currently, if a user calls an overloaded function and mypy is unable to find a match, mypy will display display a warning like 'No overload variant of "func" matches argument types" with no additional context.

This pull request will list several matching variants in addition to that message. If possible, mypy will attempt to show only overloads that have a compatible number of arguments. The number of overloads shown is always capped at a maximum of 2.

This pull request does not attempt to report a custom error when...

Union-math fails. Rationale: the pending PR will change how unions are handled dramatically enough to the point where any error handling here would be pointless.
Mypy is able to infer what signature the user most likely meant by looking at the erased types. Rationale: I attempted to implement this feature but was unable to make it consistently work without adding several ugly hacks around how mypy records errors so decided to defer implementing this feature.

Resolves python#672 and supercedes python#5054. Currently, if a user calls an overloaded function and mypy is unable to find a match, mypy will display display a warning like 'No overload variant of "func" matches argument types" with no additional context. This pull request will list several matching variants in addition to that message. If possible, mypy will attempt to show only overloads that have a compatible number of arguments. The number of overloads shown is always capped at a maximum of 2. This pull request does *not* attempt to report a custom error when... 1. Union-math fails. Rationale: the pending PR will change how unions are handled dramatically enough to the point where any error handling here would be pointless. 2. Mypy is able to infer what signature the user most likely meant by looking at the erased types. Rationale: I attempted to implement this feature but was unable to make it consistently work without adding several ugly hacks around how mypy records errors so decided to defer implementing this feature.

ilevkivskyi · 2018-06-29T13:18:21Z

Sorry, it looks like I will not be able to review today :-( Probably on Saturday evening.

Michael0x2a · 2018-06-29T13:35:46Z

@ilevkivskyi -- no worries, take your time :)

If you also want to prioritize landing the other overload PRs first, I'm fine with that. This PR feels like the kind of thing that'll cause a bunch of merge conflicts, so maybe it makes sense to just defer reviewing this so I can fix the conflicts all in one go instead of distributing them through the other PRs.

Michael0x2a · 2018-06-29T13:36:02Z

Up to you though, of course.

ilevkivskyi

Thanks! Looks good to me. There is one minor comment, feel free to merge when it is easier for you.

ilevkivskyi · 2018-07-01T14:44:53Z

mypy/messages.py

                        offset: int, max_items: int) -> None:
-        for item in tp.items()[:max_items]:
+        if len(targets) == 0:


if not targets: would be more idiomatic.

Michael0x2a · 2018-07-01T23:53:49Z

@gvanrossum -- before I merge this in, did you want to take a quick look? I know you said that the error messages were too noisy in the previous incarnation of this PR. If you still feel that way, there are probably one or two things I could try doing to make the error message shorter.

gvanrossum

OK, since you asked, I still have some quibbles with the output. I trust Ivan reviewed the algorithmic part of the code adequately.

gvanrossum · 2018-07-02T19:07:03Z

mypy/messages.py

+        shown = min(max_items, len(targets))
+        max_available = len(func.items())
+        if shown < max_available:
+            self.note('<{} more overload(s) not shown>'.format(max_available - shown),


Apart form the uglines around plurals, it seems there are two possible reasons why not all overloads are shown:

because they don't match the number of arguments (or the keywords or whatever)

because the sheer number exceeds max_items

I think it would be good to differentiate between these in the messages somehow.

gvanrossum · 2018-07-02T19:07:55Z

mypy/messages.py

-        for item in tp.items()[:max_items]:
+        if not targets:
+            targets = func.items()
+        for item in targets[:max_items]:
            self.note('@overload', context, offset=2 * offset)


The extra line with @overload just doubles the verbosity without adding any information. I think it's okay to leave it out.

@overload

This commit responds to the latest code review. It... 1. Handles plurals. 2. Removes the '@overload' text to help save space. 3. Modifies the 'some overloads not shown' message so it instead displays one of two variants: if we aren't showing all matching variants, we display: <X more matching overloads not shown, out of N total overloads> This happens if there are more then 2 matching overloads. If we do show all matching variants but hide non-matching overloads, we display: <X more non-matching overloads not shown> 4. Moves all of the new overload display functionality back to a new method named `pretty_overload_matches` and reverts `pretty_overload` back to the original logic. (The new logic was growing to be different enough from the previous logic that a split seemed like the cleanest approach.)

Michael0x2a · 2018-07-04T09:46:16Z

@gvanrossum and/or @ilevkivskyi -- how's this?

The new format should handle plurals correctly and will display a message that looks like this if there are more then 2 overloads that have matching types/arity/whatever:

Possible overload variants:
    def foo(x: int) -> int
    def foo(x: str) -> str
    <2 more matching overloads not shown, out of 5 total overloads>

...or like this if we're suppressing only non-matching overloads:

Possible overload variants:
    def foo(x: int) -> int
    def foo(x: str) -> str
    <1 more non-matching overload not shown>

...and of course, like this if we we were able to display all the variants:

Possible overload variants:
    def foo(x: int) -> int
    def foo(x: str) -> str

(I guess since we cut the number of lines shown in half by removing @overload, we could also increase the max number of variants shown to 3 or 4 or something. I don't particularly care either way though.)

ilevkivskyi

Thanks! Looks even better now.

ilevkivskyi · 2018-07-05T14:24:16Z

mypy/messages.py

+        if shown < max_available:
+            left = max_available - shown
+            msg = '<{} more overload{} not shown>'.format(left, plural_s(left))
+            self.note(msg, context, offset=2 * offset)


All changes in this function are unnecessary now, except plural_s usage.

ilevkivskyi · 2018-07-05T14:26:36Z

mypy/messages.py

+        assert shown <= max_matching <= max_available
+        if shown < max_matching <= max_available:
+            left = max_matching - shown
+            msg = '<{} more matching overload{} not shown, out of {} total overloads>'.format(


I would rather say this as {} more matching and {} non-matching overload{} not shown. But this is still not perfect because the overloads aren't "matching" (as the error message above says). Anyway, I think it is not so important to delay merging this PR, so I will go ahead unless someone proposes better wording.

ilevkivskyi · 2018-07-05T21:47:16Z

@Michael0x2a Will you have time to make minor final updates here before I cut the release branch tomorrow morning? (If not it is also OK.)

Michael0x2a · 2018-07-05T22:00:37Z

@ilevkivskyi -- most likely. I'm planning on leaving work + finishing dinner in roughly 5 hours or so and can start making any necessary changes after that.

I guess the timing might be a little tight, depending on whether you mean morning Dublin time vs Pacific time, but eh, whatever.

gvanrossum

I'm personally happy at this point, though I'm not sure how Ivan feels. If you're going to iterate on this more, here's one more idea that will surely drive you nuts: why replace one line that has useful info with another line that says "<1 more not shown>"? It might make more sense to just display the extra overload, and only start suppressing overloads when the excess is 2 or more. (Then again maybe the overload is much longer than what fits on a line, so take this with a grain of salt. Also it's probably not worth the extra complexity in the code, and you are probably tired of me.)

Michael0x2a · 2018-07-06T03:58:10Z

@gvanrossum and @ilevkivskyi -- Mkay, I reverted the changes to pretty_overload (apart from some tweaks to make pluralization tidier), swapped out the word "matching" for "similar" for the first error message, and added the tweak Guido suggested (it ended up being pretty simple to implement in the end).

I'll merge this in once the tests pass (unless there are any last-minute objections, I guess).

Michael0x2a added 2 commits June 28, 2018 22:19

Fix missed test cases

67d8e7d

ilevkivskyi mentioned this pull request Jun 29, 2018

Better error messages when calling overloaded function with invalid types. #5054

Closed

ilevkivskyi approved these changes Jul 1, 2018

View reviewed changes

Address code review

c363f62

gvanrossum reviewed Jul 2, 2018

View reviewed changes

Merge branch 'master' into overload-better-error-messages

2f73a18

Michael0x2a mentioned this pull request Jul 4, 2018

Release 0.620 planning #5314

Closed

ilevkivskyi self-assigned this Jul 4, 2018

ilevkivskyi approved these changes Jul 5, 2018

View reviewed changes

gvanrossum approved these changes Jul 6, 2018

View reviewed changes

Respond to the 3rd? 4th? 5th? code review(s)

873521b

JelleZijlstra approved these changes Jul 6, 2018

View reviewed changes

Michael0x2a merged commit 624a94f into python:master Jul 6, 2018

Michael0x2a deleted the overload-better-error-messages branch July 6, 2018 04:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List possible overload variants when none match #5296

List possible overload variants when none match #5296

Michael0x2a commented Jun 29, 2018

ilevkivskyi commented Jun 29, 2018

Michael0x2a commented Jun 29, 2018

Michael0x2a commented Jun 29, 2018

ilevkivskyi left a comment

ilevkivskyi Jul 1, 2018

Michael0x2a commented Jul 1, 2018

gvanrossum left a comment

gvanrossum Jul 2, 2018

gvanrossum Jul 2, 2018

Michael0x2a commented Jul 4, 2018

ilevkivskyi left a comment

ilevkivskyi Jul 5, 2018

ilevkivskyi Jul 5, 2018

ilevkivskyi commented Jul 5, 2018

Michael0x2a commented Jul 5, 2018

gvanrossum left a comment

Michael0x2a commented Jul 6, 2018

List possible overload variants when none match #5296

List possible overload variants when none match #5296

Conversation

Michael0x2a commented Jun 29, 2018

ilevkivskyi commented Jun 29, 2018

Michael0x2a commented Jun 29, 2018

Michael0x2a commented Jun 29, 2018

ilevkivskyi left a comment

Choose a reason for hiding this comment

ilevkivskyi Jul 1, 2018

Choose a reason for hiding this comment

Michael0x2a commented Jul 1, 2018

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum Jul 2, 2018

Choose a reason for hiding this comment

gvanrossum Jul 2, 2018

Choose a reason for hiding this comment

Michael0x2a commented Jul 4, 2018

ilevkivskyi left a comment

Choose a reason for hiding this comment

ilevkivskyi Jul 5, 2018

Choose a reason for hiding this comment

ilevkivskyi Jul 5, 2018

Choose a reason for hiding this comment

ilevkivskyi commented Jul 5, 2018

Michael0x2a commented Jul 5, 2018

gvanrossum left a comment

Choose a reason for hiding this comment

Michael0x2a commented Jul 6, 2018