Fix function is_protocol_implementation for quite similar protocol classes

[tyralla]

Fixes #9771

from typing import Protocol, TypeVar, Union

T1 = TypeVar("T1", covariant=True)
T2 = TypeVar("T2")

class P1(Protocol[T1]):
    def b(self) -> int: ...
    def a(self, other: "P1[T2]") -> T1: ...

class P2(Protocol[T1]):
    def a(self, other: Union[P1[T2], "P2[T2]"]) -> T1: ...

Method infer_constraints_from_protocol_members of class ConstraintBuilderVisitor raises an assertion for the above code when we pass P2 as instance and P1 as template to it (inline comment: The above is safe since at this point we know that 'instance' is a subtype of (erased) 'template', therefore it defines all protocol members).

So, the actual error happens before. Method visit_instance of class ConstraintBuilderVisitor should never apply the method infer_constraints_from_protocol_members on P1 and P2. It nevertheless does so due to function is_protocol_implementation indicating P2 actually implements P1.

In my naive understanding, the problem seems to lie in the recursive approach underlying function is_protocol_implementation. This function first analyses method a to check if P2 is a subclass of P1. Therefore, it needs to check if P1 is a subclass of P2. This can result in a situation where the "'assuming' structural subtype matrix" is already filled with P1 and P2. is_protocol_implementation then returns True (for avoiding infinite recursion in other cases) without checking any other members, which is incorrect for the given case.

My simple solution is first to check if all members of P1 (the potential supertype) define a subset of P2 (the potential subtype). If this does not hold, we do not need to perform the complex recursive approach and can return False immediately. Maybe not the most general solution, but it works on the additional test testTwoUncomfortablyIncompatibleProtocolsWithoutRunningInIssue9771 (I can't think of a better name...) and seems not to impact any other tests.

[github-actions]

Diff from mypy_primer, showing the effect of this PR on open source code:

pandas (https://github.com/pandas-dev/pandas.git)
+ pandas/core/frame.py:6045: error: Incompatible types in assignment (expression has type "Iterable[Any]", variable has type "Union[Hashable, Sequence[Hashable], None]")  [assignment]
+ pandas/core/frame.py:6054: error: Unsupported right operand type for in ("Union[Index, Hashable]")  [operator]


black (https://github.com/psf/black.git)
+ src/black/__init__.py:307:43: error: Argument 1 to "__call__" of "_lru_cache_wrapper" has incompatible type "Iterable[str]"; expected "Hashable"

[JelleZijlstra]

I'm not sure how, but this PR actually fixes a more basic bug. This code passes on master:

from typing import Hashable, Iterable

def f(x: Hashable) -> None:
    pass

def g(x: Iterable[str]) -> None:
    f(x)

But with your PR it correctly produces lruc.py:20: error: Argument 1 to "f" has incompatible type "Iterable[str]"; expected "Hashable". The error mypy-primer found in Black above alerted me to this.

[hauntsaninja]

I think Jelle's fixed issue is explained by:

• mypy/mypy/subtypes.py

  Line 598 in 9ab622c

  method = info.get_method(name)

  finds that Iterable does have a __hash__ method in its MRO.
• The protocol_members attribute doesn't look at object

  mypy/mypy/nodes.py

  Line 2497 in fffbe88

  for base in self.mro[:-1]: # we skip "object" since everyone implements it

I'm not the most familiar with this code, but your write up makes sense, this passes tests and fixes a crash multiple people have run into, so I'll merge soon if no one else chimes in. Thanks! :-)

[JelleZijlstra]

I feel like we should add an explicit test for the Hashable/Iterable case to make sure it doesn't regress.

[tyralla]

I tried to add the following regression test you suggested:

[case testHashable]

from typing import Hashable, Iterable

def f(x: Hashable) -> None:
    pass

def g(x: Iterable[str]) -> None:
    f(x)   # E: Argument 1 to "f" has incompatible type "Iterable[str]"; expected "Hashable"

[typing fixtures/typing-full.pyi]

Additional content of typing-full.pyi:

@runtime_checkable
class Hashable(Protocol, metaclass=ABCMeta):
    @abstractmethod
    def __hash__(self) -> int: pass

However, this test passes both with and without the PR. Am I missing something?

[hauntsaninja]

I'm guessing object doesn't have __hash__ in the default builtins test fixture

[tyralla]

Yes, you are right. Defining a fixture for such an essential feature did not come into my mind.

I added the minimal stub file object_hashable.pyi. Now the test actually fails after uncommenting the additional check that I introduced into is_protocol_implementation.

[github-actions]

Diff from mypy_primer, showing the effect of this PR on open source code:

pandas (https://github.com/pandas-dev/pandas.git)
+ pandas/core/frame.py:6045: error: Incompatible types in assignment (expression has type "Iterable[Any]", variable has type "Union[Hashable, Sequence[Hashable], None]")  [assignment]
+ pandas/core/frame.py:6054: error: Unsupported right operand type for in ("Union[Index, Hashable]")  [operator]

[JelleZijlstra]

Thanks for adding the test!

[github-actions]

Diff from mypy_primer, showing the effect of this PR on open source code:

pandas (https://github.com/pandas-dev/pandas.git)
+ pandas/core/frame.py:6045: error: Incompatible types in assignment (expression has type "Iterable[Any]", variable has type "Union[Hashable, Sequence[Hashable], None]")  [assignment]
+ pandas/core/frame.py:6054: error: Unsupported right operand type for in ("Union[Index, Hashable]")  [operator]

[hauntsaninja]

Thank you!