Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update to the Overloads chapter #1839
base: main
Are you sure you want to change the base?
Update to the Overloads chapter #1839
Changes from 1 commit
bab532e
de81026
5ca254e
33c819f
f993b28
660295c
06c86f1
831945b
3906a12
bb8fe09
cce3879
7591a4d
91d4adc
69d6d4a
e13dbbe
eed0815
57495db
27f1c79
535075f
8875a4a
cb04dd6
5eabe53
484b03c
ac3b70e
87377ed
f7bf384
4936ac1
e0e0b8a
cc748d3
17d3e15
02f0652
f5bee93
169fa58
c041484
c10a72d
8a98eae
5d22e8d
98f36e8
67c1675
d414386
f4293e8
e835221
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JelleZijlstra, @JukkaL, I want to confirm that you are OK with the proposed definition of "partially overlapping overloads" in light of the response @ilevkivskyi posted to this mypy issue. I think Ivan's point is worth considering, although I'm not sure how to write the specification for "cases that could fool the type checker into choosing the wrong overload". Intuitively, I know what he means here, but a precise spec might be difficult to formulate. It looks from this historical comment that mypy contains quite a few rules and various exceptions to those rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does pyright currently check overlapping overloads?
The text already contains a number of special cases (
__get__
, ignoring multiple inheritance, etc.). Also, it seems to me that overlap is a somewhat isolated issue; the rules we set out for evaluating overload calls still work even if some overloads overlap. It might be better then to leave the spec vague, saying that type checkers may choose to flag some unsafe overlaps, without trying to come up with precise rules.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation is complicated. It leverages assignability rules but internally sets a flag that changes some of the normal assignability behaviors. For example, normally
Any
is assignable to every other type, but when this flag is set,Any
is not assignable to any type (even itself). There are a number of other changes in the algorithm, but they attempt to implement the behavior described in this PR.If we don't have agreement on the desired behavior, then I agree that it's best to keep the spec loose here. Actually, I'm not sure it makes sense to discuss it at all in the spec if we are not in agreement on the intended behavior. So maybe we should just delete that entire section and the corresponding conformance tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph felt unclear. I had to read it multiple times to understand the intent. Maybe reword this, or include some motivation why we have this rule, so that this can be understood easily without peeking at the following paragraph?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a correctness requirement for the return type here to be assumed to be
Any
? It seems to me that it would also be valid for a type-checker to use the union of all the ambiguous matching overloads. I would prefer for the specification not to prevent that option. (No type checker currently does this.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing stubs (including typeshed, numpy, pandas, and others) assume that the result will be
Any
in this case, so I don't think this is something we can change at this point. An earlier version of pyright generated a union, and it resulted in many false positive errors and lots of unhappy users. I think it's important for the spec to specifyAny
here so stub authors can rely on the behavior.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, ok, that's useful info, thank you. If you happen to know of any old pyright issues where unhappy users surfaced these problems with the union behavior, I would be curious to take a look at some real world cases relying on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I maintain the stubs of NumPy and SciPy, and I know those stubs better than I care to admit, but this doesn't ring a bell for me. Do you have an example?
Because if so, then I'd be more than willing to change that. I'd like to avoid having to deal with
Any
if I can (and not only becauseset("Any") < set("Annoying")
).