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

Incorrect @overload overlaps detection with TypeVar(infer_variance=True) #8860

Closed
minmax opened this issue Aug 29, 2024 · 3 comments
Closed
Labels
as designed Not a bug, working as intended bug Something isn't working

Comments

@minmax
Copy link

minmax commented Aug 29, 2024

pyright 1.1.378

After upgrading from version 377, pyright found 1 error in my codebase, but I think it's a bug.

from typing import Generic
from typing import Literal
from typing import Protocol
from typing import overload

from typing_extensions import TypeVar


T = TypeVar("T")

T_infer = TypeVar("T_infer", infer_variance=True)


class Field(Generic[T]):
    pass


class TypedEnumFieldProtocol(Protocol[T_infer]):
    # Overload 1 for "__new__" overlaps overload 2
    # and returns an incompatible type (reportOverlappingOverload)

    @overload
    def __new__(cls, null: Literal[False] = ...) -> Field[T_infer]: ...

    @overload
    def __new__(cls, null: Literal[True] = ...) -> Field[T_infer | None]: ...

If i add infer_variance=True to T, the error will go away.

@minmax minmax added the bug Something isn't working label Aug 29, 2024
@erictraut
Copy link
Collaborator

The new behavior that you're seeing is correct. The reason you didn't see it previously was because of a bug in older versions of pyright.

The problem here is that you have overlapping overloads that return incompatible types. If you were to call the __new__ function with no arguments, you would match both overloads because they both have a default argument. But both overloads return incompatible types if the type parameter T is invariant. If T is covariant (which is what will be computed if you add infer_variance=True), then Field[T_infer | None] is a subtype of Field[T_infer]. This makes the overlapping overloads type safe.

You can fix the overlapping overload in your code by removing the = ... in the second overload.

@erictraut erictraut added the as designed Not a bug, working as intended label Aug 29, 2024
@erictraut erictraut closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2024
@minmax
Copy link
Author

minmax commented Aug 29, 2024

The error also goes away if I swap the methods.

    @overload
    def __new__(cls, null: Literal[True] = ...) -> Field[T_infer | None]: ...

    @overload
    def __new__(cls, null: Literal[False] = ...) -> Field[T_infer]: ...

Unfortunately null=True | False resolution was broken for all non nullable fields of sub classed of django.db.models.Model (from django_types).
I can't remove the default value from null, because there are other preceding kreyword-only arguments in the real class.
It looks like there is no longer a mechanism left to correctly type this particular place for the old django.

e.g.:

 def __new__(  # type: ignore [misc]
        cls,
        verbose_name: str | None = ...,
        name: str | None = ...,
        auto_now: bool = ...,
        auto_now_add: bool = ...,
        primary_key: bool = ...,
        max_length: int | None = ...,
        unique: bool = ...,
        blank: bool = ...,
        null: Literal[False] = ...,
        db_index: bool = ...,
        default: _DT | Callable[[], _DT] | None = ...,
        editable: bool = ...,
        auto_created: bool = ...,
        serialize: bool = ...,
        unique_for_date: str | None = ...,
        unique_for_month: str | None = ...,
        unique_for_year: str | None = ...,
        choices: Iterable[
            tuple[_DT, str] | tuple[str, Iterable[tuple[_DT, str]]]
        ] = ...,
        help_text: str = ...,
        db_column: str | None = ...,
        db_comment: str | None = ...,
        db_tablespace: str | None = ...,
        validators: Iterable[_ValidatorCallable] = ...,
        error_messages: _ErrorMessagesToOverride | None = ...,
    ) -> DateTimeField[datetime]: ...

For context: Field from example it is a minimized version of django.db.models.fields.Field from django_types.

@minmax
Copy link
Author

minmax commented Aug 29, 2024

I found a solution, if you add * before arguments, then you don't have to specify the default value for any subsequent arguments.

    def __new__(
        cls,
        *,
        verbose_name: str | None = ...,
        null: Literal[True],
    ):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as designed Not a bug, working as intended bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants