reportIncompatibleMethodOverride and typevartuples

[mehdigmira]

Describe the bug

It seems to me reportIncompatibleMethodOverride is not working as expected when using typevartuples.

To Reproduce

# pyright: reportIncompatibleMethodOverride=true

from typing import Generic, TypeVar

from typing_extensions import TypeVarTuple, Unpack

Ts = TypeVarTuple("Ts")
T = TypeVar("T")


class Parent(Generic[Unpack[Ts]]):
    def run(self, *args: Unpack[Ts]) -> None:
        ...


class Child(Parent[int]):
    def run(self, *args: str) -> None:  # this is not reported as an error, i would expect it to
        ...


class Parent2(Generic[T]):
    def run(self, *args: T) -> None:
        ...


class Child2(Parent2[int]):
    def run(self, *args: str) -> None:  # this is reported as an error, works as expected
        ...


class Parent3(Generic[Unpack[Ts]]):
    def run(self, *args: Unpack[tuple[Unpack[Ts], int]]) -> None:
        ...


class Child3(Parent3[int]):
    def run(self, arg1: int, arg2: int) -> None:  # this is reported as an error, it should not IMO
        ...

Expected behavior
Child should be reported as an error. Child3 should not.

VS Code extension or command-line
pyright 1.1.277

[mehdigmira]

@erictraut why isn't this considered as a bug ?

[erictraut]

This is considered an enhancement request because reportIncompatibleMethodOverride doesn't check all combinations of parameter types today. You're requesting that its logic be expanded to handle some cases that it wasn't originally designed to detect.

[mehdigmira]

I've looked and debugged the algorithm in validateOverrideMethodInternal.

validating *args

As I understand it, the logic when validating positional arguments is:

• If override has less positional args than base class, this should be flagged as an error, unless override has *args

I think we can make this a little bit more strict by changing the rule to:

• If override has less positional args than base class, this should be flagged as an error, unless override has *args and the type of *args if compatible with the positional args of base method that are not implemented by override.

class Parent:
     def my_method41(self, a: int, b: str) -> None:
         ...

     def my_method42(self, a: int, b: str) -> None:
         ...

class Child:
     # this should not generate an error because args is of the right type
     def my_method41(self, a: int, *args: str) -> None:
         ...

     # this should generate an error because args doesn't have the right type
     def my_method42(self, a: int, *args: int) -> None:
         ...

Unpack

I think there is a bug, in the way unpacked args are handled.

class Parent(Generic[Unpack[Ts]]):
    def method_1(self, *args: Unpack[Ts]) -> None:
        ...

    def method_2(self, *args: Unpack[tuple[Unpack[Ts]]]) -> None:
        ...


class Child(Parent[int]):
    def method_1(self, arg1: int) -> None:  # this is not reported as an error. It makes sense.
        ...

    def method_2(self, arg1: int) -> None:  # this is reported as an error, although method_1 are strictly equivalent
        ...

I think this is because Parent.method_1 parameters (baseType.details.parameters) are evaluated as

{category: 0, name: 'self', hasDefault: false, defaultValueExpression: undefined, defaultType: undefined, …}
{category: 0, name: '__p1', isNameSynthesized: true, type: {…}, hasDeclaredType: true}

while
Parent.method_2 parameters are evaluated as

{category: 0, name: 'self', hasDefault: false, defaultValueExpression: undefined, defaultType: undefined, …}
{category: 1, name: 'args', hasDefault: false, defaultValueExpression: undefined, defaultType: undefined, …}

If you're OK with this approach, I can submit a PR for the first point, and try to find a fix for item n°2 (though i need to look closer to understand why unpacked and typevartuples seem to be handled by different parts of the code)

[erictraut]

I'd prefer that you don't make changes to this code. This is one of the more complex pieces of code in pyright, so I'd prefer to do it myself. It will take me longer to review and verify a PR than it will to implement it myself.

I'll get to this eventually, but I don't consider it a high priority because it's an enhancement request and a false negative.

[mehdigmira]

Alright ! The second point does seem a bit tricky. The first is fairly straightforward from what I've seen.

I understand you consider the enhancement part low prio. But I still believe there is a bug in the second part that I described :). It's a false positive, and not a false negative.

[erictraut]

One of the (many) complexities that PEP 646 added was that it broke the invariant that positional-only parameters must always precede parameters that can be used as keyword-only parameters. For example, def func(a: int, /, b: int, *args: *tuple[int]). In this case, a is a positional-only parameter, b is a positional+keyword parameter, and the unpacked int is another positional-only parameter. This led to the false positive you reported above. I've addressed this issue, and the fix will be included in the next release.

This doesn't yet address the enhancement request part. There are reasons why the *args override check is incomplete. There are legitimate (type-safe) cases where an exact parameter match is not required. Any enhancement that proposes to make this check more strict will need to consider these legitimate cases, and if the stricter check creates too many false positive errors in internal Microsoft code bases, I will not be inclined to add it.

[mehdigmira]

Thanks for the quick fix !
For the enhancement part, I was thinking of smth like mehdigmira@8eb7415#diff-e8178cbc8c6f5764a3b368c69c7aad9132af94c62d25559c433c80d58e9cf51dR23798

[erictraut]

This will be addressed in the next release. I used your code largely unchanged. Thanks for the contribution.

[erictraut]

This is included in pyright 1.1.286, which I just published. It will also be included in a future version of pylance.