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

Bounded/constrained type variables don't shadow Any in overloads #5506

Merged
merged 4 commits into from
Aug 26, 2018

Conversation

ilevkivskyi
Copy link
Member

Fixes #4227

The problem is that after unification with Any type variables can also become Any. I am aware that this may introduce (rare) cases when we don't detect never-matched overload. However, since this error does not introduce any unsafety, false negatives are clearly better than false positives.

@ilevkivskyi
Copy link
Member Author

Just to clarify, the main complain in the original issue is a valid mypy error, but the subsequently appeared error about never-matching overload is bogus. And it is what I try to fix in this PR.

Copy link
Collaborator

@Michael0x2a Michael0x2a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay on this PR. I initially wasn't sure if this PR was correct or if I was comfortable with how this ended up handling Anys, so ended up arguing with myself for a good part of this past week.

In any case, I think I'm largely on board now, apart from one proposed change: can we add the following test case?

from typing import TypeVar, Any, overload

T = TypeVar('T')

@overload
def foo(x: T) -> T: ...
@overload
def foo(x: Any) -> Any: ...

@overload
def bar(x: T) -> T: ...
@overload
def bar(x: Any) -> int: ...

Your PR makes both functions type-check with no errors, which I think is probably correct. It'd be good to have a test that definitively confirms whether this is ok or not, though.

Once we do, I think we can probably merge this in.


A little more broadly though, I do have a somewhat vaguer concern about this PR -- it seems that with this Pr, we're handling 'Any' and overloads in two different things. Once by doing a limited form of type erasure on the LHS, and once by adding a special case within 'is_more_precise'.

I wonder if it might be worth consolidating these two into one somehow, or taking a more comprehensive view?

In particular, it seems to me that it ought to be safe for the last overload variants to use Any as a fallback -- we want the Anys on the RHS to never be considered more precise then the corresponding types on the LHS.

So, rather then adjusting the typevars on the left, what if we adjusted the Anys on the right?

E.g. we could maybe replace all 'Anys' on the right with some special type that's the exact inverse: some type that is never equal to, a super type of, or a subtype of any other type?

Or maybe we could adjust the behavior of is_more_precise in some clever way? Or go the other direction and find some way of removing is_more_precise and just use is_proper_subtype instead? Not sure.

That said, I don't think either of us have the bandwidth to really investigate this idea though, so I propose we just go ahead and merge in this PR (after adding the suggested test case above).

I just wanted to bring up this idea/concern because I'm getting a vague feeling we're doing something a little suboptimal here -- both here and in my partial overlaps PR. (I hope that makes sense.)

@ilevkivskyi
Copy link
Member Author

It'd be good to have a test that definitively confirms whether this is ok or not, though.
Once we do, I think we can probably merge this in.

Could you please clarify what do you mean by "definitely confirms"? Do you want to find a call to foo that will chose the second overload? I think there is no such call. But as we discussed before, this is fine because the whole scope of the error in question is not type safety, but just detecting "suspicious" code (like the notorious Function does not return a value error). In my experience, people are totally fine if mypy doesn't detect the error, but the are totally not fine when mypy gives a false positive in such situation.

A little more broadly though, I do have a somewhat vaguer concern about this PR -- it seems that with this Pr, we're handling 'Any' and overloads in two different things. Once by doing a limited form of type erasure on the LHS, and once by adding a special case within 'is_more_precise'.

I don't think the additional special case in is_more_precise is only for overloads, but anyway, I think the core obstacle on the way to simpler solution is that is_proper_subtype(Any, Any) returns True. I tried to change this, but it looks like some other unrelated test cases depend on this. I added a comment about this.

So, rather then adjusting the typevars on the left, what if we adjusted the Anys on the right?
E.g. we could maybe replace all 'Anys' on the right with some special type that's the exact inverse: some type that is never equal to, a super type of, or a subtype of any other type?

I think this will be an even more ad-hoc solution.

Or maybe we could adjust the behavior of is_more_precise in some clever way?
Or go the other direction and find some way of removing is_more_precise and just use is_proper_subtype instead? Not sure.

I tried several combinations of directions/existing subtype-like functions, and I didn't find anything satisfactory. This is why I went with an ad-hoc solution.

@Michael0x2a
Copy link
Collaborator

Could you please clarify what do you mean by "definitely confirms"? Do you want to find a call to foo that will chose the second overload?

Ah, sorry -- that was poorly worded. I meant to say that I think it's correct that mypy reports no errors with the test cases I included. However, I could see a future PR (probably one that I'd write, TBH) making mypy report errors in cases like those by accident. So, I wanted those test cases added to this PR to guard against that happening.

@ilevkivskyi
Copy link
Member Author

So, I wanted those test cases added to this PR to guard against that happening.

OK, np. I added the test in the last commit above.

@ilevkivskyi
Copy link
Member Author

@Michael0x2a OK, so does this mean the PR can be merged now? Could you please take a look again?

@Michael0x2a Michael0x2a merged commit 1d06efa into python:master Aug 26, 2018
@Michael0x2a
Copy link
Collaborator

Yup, everything looks good to me -- I went ahead and merged.

(I still have vague misgivings about how we're handling Any in general, but it's not obvious to me what to do about it/agree that this PR looks like the best approach so far.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants