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

Introduce error category [unsafe-overload]. #16060

Closed
randolf-scholz opened this issue Sep 6, 2023 · 3 comments · Fixed by #16061
Closed

Introduce error category [unsafe-overload]. #16060

randolf-scholz opened this issue Sep 6, 2023 · 3 comments · Fixed by #16061

Comments

@randolf-scholz
Copy link
Contributor

Feature

Specifically, consider this example from the documentation [mypy-play]:

from typing import overload, Union

@overload
def unsafe_func(x: int) -> int: ...  # [misc] overlap with incompatible return types
@overload
def unsafe_func(x: object) -> str: ...
def unsafe_func(x: object) -> Union[int, str]:
    if isinstance(x, int):
        return 42
    else:
        return "some string"

Pitch

misc is too generic and makes it difficult to specifically disable this kind of error. I propose to give it a specific name like unsafe-overload.

Due to the absence of Intersection and Not, this check raises false positives in certain code, for example:

@overload
def foo(x: Vector) -> Vector: ...
@overload
def foo(x: Iterable[Vector]) -> list[Vector]: ...
def foo(x):
   if isinstance(x, Vector):
      return x
   return list(map(foo, x))

Where Vector implements __iter__. Obviously, what we really mean here is:

@overload
def foo(x: Vector) -> Vector: ...
@overload
def foo(x: Iterable[Vector] & Not[Vector]) -> list[Vector]: ...
def foo(x):
   if isinstance(x, Vector):
      return x
   return list(map(foo, x))

Which is actually implicit by the ordering of overloads. The latter would not raise unsafe-overload, since Iterable[Vector] & Not[Vector] is not a subtype of Vector. But since it can't be expressed in the current type system, there needs to be a simple way to silence this check.

@JelleZijlstra
Copy link
Member

Seems fine to me, please submit a PR. In general we should minimize use of the "misc" error code.

One concern is about backward incompatibility: users who use # type: ignore[misc] will have to change their code. Not sure if we have a stance about that.

@randolf-scholz
Copy link
Contributor Author

So if misc stands for uncategorized errors, then possible type: ignore[misc] should cover any other error code, but emit a warning if it covers an error with a code different from misc?

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 6, 2023

One concern is about backward incompatibility: users who use # type: ignore[misc] will have to change their code. Not sure if we have a stance about that.

We now have a feature where we can make new errors codes "subcodes" of other codes, so that people's existing type: ignore comments don't break. Am example is the newish method-assign error code. @randolf-scholz, I recommend that you update your PR to use a similar approach.

(This change is a great suggestion by the way, thanks for taking it on.)

JelleZijlstra pushed a commit that referenced this issue Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants