Improve Step 5 of overload call evaluation.#2250
Improve Step 5 of overload call evaluation.#2250rchen152 wants to merge 10 commits intopython:mainfrom
Conversation
Makes two modifications to step 5 of overload call evaluation: 1. When materializing arguments to check whether we can eliminate overloads, skip arguments that have the same parameter type in all overloads. 2. When we still have multiple overloads after the materialization filter, try to find a return type that all materializations of all other return types are assignable to. Updates the conformance tests as well. All type checkers already do (1). Mypy and pyrefly pass the conformance test for (2). I believe mypy has a heuristic that approximates the new rule; pyrefly implements it exactly.
|
I think it's preferable to keep PRs updating type-checker versions separate from PRs implementing conformance suite and spec changes. |
docs/spec/overload.rst
Outdated
| - All possible :term:`materializations <materialize>` of the argument's type are | ||
| assignable to the corresponding parameter type, or | ||
| - The parameter types corresponding to this argument in all of the remaining overloads | ||
| are :term:`equivalent`. |
There was a problem hiding this comment.
I think this is correct but there's some subtlety in the wording that we should cover in the test cases:
- "The parameter types corresponding to this argument". That may be a very different parameter in each of the overloads; e.g. maybe one takes
*argsand the other has explicit arguments. So my reading is that to check this, you have to create for each overload a mapping between argument and parameter type, and consult that. What is the parameter type corresponding to an unpacked argument, though? - "the remaining overloads". So we only check this overload and all following overloads, not ones above it?
There was a problem hiding this comment.
Great points, thanks!
- I added a clarification about unpacked arguments and some more tests.
- Ah, I meant all of the candidate overloads that are remaining at the beginning of Step 5, not just the overloads following the one that we're checking. I replaced "remaining overloads" with "candidate overloads" - hopefully that's clearer.
Point taken, sorry! I was being a bit sloppy because I didn't want to open two PRs XD Version update here: #2254 |
* "remaining" overloads => "candidate" overloads * moar tests
|
Thanks for the feedback! I believe I've addressed all the comments. |
|
Thanks Rebecca for looking into this. I think we really need changes here. Please give me a few days to respond before you call for a vote. I have a few more general thoughts. |
davidhalter
left a comment
davidhalter
left a comment
There was a problem hiding this comment.
Thanks a lot for this PR. I feel like this is taking overloads in a much better direction. Projects like scipy-stubs will benefit a lot from this.
I think we can (and probably should) still add AnyOf in the future, but the improved return types already fix most of the issues I have had with the current spec.
Sorry for taking so much time for such a small review, but it took me many hours to understand Mypy's code around overloads good enough to make sure we don't omit something.
| for all remaining overloads are :term:`equivalent`, proceed to step 6. | ||
| they should be replaced with their solved types. Eliminate every overload for | ||
| which there exists a :term:`materialization <materialize>` of another | ||
| overload's return type that is not assignable to this overload's return type. |
There was a problem hiding this comment.
This is clever. I like this much better than Mypy's type erasure (where list[int] and list[str] become list[Any]), because this generalizes better. I think the sentence is a bit hard to understand - at least for me - but I doubt I would be able to come up with something more understandable.
There was a problem hiding this comment.
Agreed that this phrasing is clunky, but I spent a while wordsmithing it, and this was the best I could come up with :/
There was a problem hiding this comment.
I think this can be phrased more simply in terms of "top materialization" (the union of all possible materializations of a gradual type), but that's not a term that has been formally defined in the spec yet, so that would just introduce a different problem :)
No worries! Part of the process is waiting at least a week for feedback anyways. Thanks for checking mypy's code. |
5 participants
Makes two modifications to step 5 of overload call evaluation:
Updates the conformance tests as well. All type checkers already do (1). Mypy and pyrefly pass the conformance test for (2). I believe mypy has a heuristic that approximates the new rule; pyrefly implements it exactly.