Skip to content

Reachability check seems to consider isinstance(obj, Hashable) as always True #11799

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

Closed
haxtibal opened this issue Dec 20, 2021 · 6 comments · Fixed by #11802
Closed

Reachability check seems to consider isinstance(obj, Hashable) as always True #11799

haxtibal opened this issue Dec 20, 2021 · 6 comments · Fixed by #11802
Labels
bug mypy got something wrong topic-type-narrowing Conditional type narrowing / binder

Comments

@haxtibal
Copy link

Running mypy --warn-unreachable (version 0.920) against the following snippet gives a false "unreachable statement" error.

from collections.abc import Hashable
from typing import List, Tuple, Union

def wrap_hash(obj: Union[Tuple[int], List[int]]):
    if isinstance(obj, Hashable):
        return hash(obj)
    return hash(id(obj))  # error: Statement is unreachable  [unreachable]

wrap_hash((0,)) # reaches 1st return
wrap_hash([0])  # reaches 2nd return

We can even exchange the Union annotation for a single unhashable type like List or Dict, and still get the same unreachable error. Looks like isinstance(obj, Hashable) is just considered True for any type.

Is this a mypy bug? Can we add some hints into the example code so that mypy would figure out correct reachability?

@haxtibal haxtibal added the bug mypy got something wrong label Dec 20, 2021
@sobolevn
Copy link
Member

The problem is that __hash__ is defined on object as a real method.

reveal_type(object.__hash__)  # N: Revealed type is "def (self: builtins.object) -> builtins.int"

We had this problem and discussion several times before.
Maybe we can try to change it to None | def (self: builtins.object) -> builtins.int?
CC @JelleZijlstra

@JelleZijlstra
Copy link
Member

We already have __hash__: None on list in typeshed. Why would object have to change?

@sobolevn
Copy link
Member

I don't know how we can represent this idea in types, but: some object subtypes are hashable (def (self: builtins.object) -> builtins.int) and some are not (None). But, object defines that all subclasses are hashable by defining __hash__ as def (self: builtins.object) -> builtins.int.

But, on the other hand, we know that object itself is hashable. Only its subtypes can be unhashable.

@JelleZijlstra
Copy link
Member

But mypy already correctly errors when a subclass of a class that fulfills a protocol doesn't fulfill it:

from typing import Protocol

class X(Protocol):
    def foo(self, a: int) -> str:
        pass
    
class A:
    def foo(self, a: int) -> str:
        return ""
        
class B(A):
    def foo(self) -> str:  # type: ignore[override]
        return ""
        
a: X = A()
b: X = B()  # E: Incompatible types in assignment (expression has type "B", variable has type "X")

Why is Hashable different?

@sobolevn
Copy link
Member

sobolevn commented Dec 20, 2021

Looks like we have a bug in mypy 🤔
For some reason mypy treats Hashable as plain object.

1 NameExpr(Hashable [typing.Hashable])
TypeInfo(
  Name(typing.Hashable)
  Bases(builtins.object)
  Mro(typing.Hashable, builtins.object)
  Names(
    __hash__)
  DeclaredMetaclass(abc.ABCMeta)
  MetaclassType(abc.ABCMeta))
2 def () -> builtins.object

I will look into it.

@sobolevn
Copy link
Member

I hope that #11802 will fix it! 🙏

@JelleZijlstra JelleZijlstra added the topic-type-narrowing Conditional type narrowing / binder label Mar 21, 2022
JelleZijlstra pushed a commit that referenced this issue Mar 24, 2022
When this piece of code was checked:

```python
from typing import Awaitable, Hashable, Union, Tuple, List

obj: Union[Tuple[int], List[int]]
if isinstance(obj, Hashable):
    reveal_type(obj)
```

Mypy revealed that `Hashable` is `() -> object`. It happened, because [`is_subtype(explicit_type, default_ret_type, ignore_type_params=True)`](https://github.com/python/mypy/blob/56684e43a14e3782409c47e99bb47191d631a3de/mypy/typeops.py#L130) was `True`, where `explicit_type=object` and `default_ret_type=Hashable`. It happened because `object` has `__hash__` method.

The only thing that popped out of my head is to simply exclude protocols from this condition.
I guess that we might double check protocols with `__new__` and `__init__` to be sure. But, I am not able to think of proper test cases for this. Any ideas? Or is my single test good enough?

I am adding `pythoneval` test, because of the complexity in how `Hashable` is defined and analyzed in real life.

Closes #11799
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-type-narrowing Conditional type narrowing / binder
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants