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

Type Narrowing failure in 0.981 #13800

Open
gholden-wavefin opened this issue Oct 3, 2022 · 10 comments · May be fixed by #14390
Open

Type Narrowing failure in 0.981 #13800

gholden-wavefin opened this issue Oct 3, 2022 · 10 comments · May be fixed by #14390
Assignees
Labels

Comments

@gholden-wavefin
Copy link

I'm seeing an Incompatible return value type error in 0.981 that wasn't present in previous versions of mypy.

To Reproduce

from typing import TypeVar

T = TypeVar("T", dict, float)

def method(x: T) -> T:
    if isinstance(x, dict):
        return {}
    else:
        return 0.0

Expected Behavior

% mypy --version
mypy 0.971 (compiled: yes)
% mypy test-mypy-error.py
Success: no issues found in 1 source file

Actual Behavior

% mypy --version
mypy 0.981 (compiled: yes)
% mypy test-mypy-error.py
[Success: no issues found in 1 source file](test-mypy-error.py:7: error: Incompatible return value type (got "Dict[<nothing>, <nothing>]", expected "float"))

Your Environment
mypy version 0.981
python version 3.9.13

@gholden-wavefin gholden-wavefin added the bug mypy got something wrong label Oct 3, 2022
@hauntsaninja
Copy link
Collaborator

Interesting, mypy_primer -p ~/dev/mypy_primer/test.py --bisect --new v0.981 --old v0.971 --debug bisects this to #13386

@hauntsaninja
Copy link
Collaborator

Looks like it's from python/typeshed#8465, cc @sobolevn

@sobolevn
Copy link
Member

sobolevn commented Oct 3, 2022

Very strange 😨
How can __hash__ affect this?

@hauntsaninja
Copy link
Collaborator

Yeah, it's pretty strange, but it's true. Fixes itself on master if you add __hash__ back to float. These dunders...

@sobolevn
Copy link
Member

sobolevn commented Oct 3, 2022

It does not feel right. It looks like it exposes some other bug. I will have a look.

@sobolevn sobolevn self-assigned this Oct 3, 2022
@sobolevn
Copy link
Member

sobolevn commented Oct 4, 2022

So, I am diving into it.

First of all, I was able to reproduce this problem.
Secondly, this works:

from typing import TypeVar

T = TypeVar("T", dict, int)

def method(x: T) -> T:
    if isinstance(x, dict):
        return {}
    else:
        return 0

Also, set and list work.
But, float and str does not work.

At this point I am sure that this is a bug.

Revealed types

Let's reveal types:

from typing import TypeVar

T = TypeVar("T", dict, str)

def method(x: T) -> T:
    if isinstance(x, dict):
        reveal_type(x)
        return {}
    else:
        reveal_type(x)
        return 'a'

Output:

ex.py:7: note: Revealed type is "builtins.dict[Any, Any]"
ex.py:7: note: Revealed type is "ex.<subclass of "dict" and "str">"
ex.py:8: error: Incompatible return value type (got "Dict[<nothing>, <nothing>]", expected "str")  [return-value]
ex.py:10: note: Revealed type is "builtins.str"

Looks like Revealed type is "ex.<subclass of "dict" and "str">" should not ever happen.

What about types that do work?

from typing import TypeVar

T = TypeVar("T", dict, int)

def method(x: T) -> T:
    if isinstance(x, dict):
        reveal_type(x)
        return {}
    else:
        reveal_type(x)
        return 1

Outputs:

» mypy ex.py --show-traceback
ex.py:7: note: Revealed type is "builtins.dict[Any, Any]"
ex.py:10: note: Revealed type is "builtins.int"
Success: no issues found in 1 source file

So, let's find why the intersection of two instances is created in the first place 🤔

.intersect_instances

This happens, because self.intersect_instances((v, t), ctx) works this way:

Intersecting: builtins.int builtins.dict[Any, Any]
Result: None

Intersecting: builtins.str builtins.dict[Any, Any]
Result: ex.<subclass of "dict" and "str">

@headtr1ck
Copy link

Possibly related: #13956
Even though this fails also for previous mypy versions.

@emirkmo
Copy link

emirkmo commented Nov 24, 2022

I am not sure if this is related but I wanted to add another data point on 0.991

@classmethod
def from_df(cls, df: pd.DataFrame | pd.Series) -> "Sites":
   if isinstance(df, pd.Series):
      return cls.from_sitemap(cast(SiteDict, df.to_dict()))
   assert isinstance(df, pd.DataFrame)  # for mypy type failure
   df.rename({df.columns[0]: "name"}, axis=1, inplace=True)

Error:

error: No overload variant of "rename" of "Series" matches argument types "Dict[Union[str, bytes, date, timedelta, int, float, complex, Any], str]", "int", "bool"  [call-overload]
                df.rename({df.columns[0]: "name"}, axis=1, inplace=True)
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Without the assert isinstance(df, pd.DataFrame) # for mypy type failure Mypy falsely warns that Series.rename does not have the proper overload, but df should already be narrowed to a pd.DataFrame by that point. Pylance/pyright gets it right.

Environment:
mypy 0.991 (compiled: yes)
Python 3.10.6

Options:
--allow-redefinition --allow-untyped-globals --ignore-missing-imports --implicit-reexport --enable-incomplete-feature=Unpack

@tyralla
Copy link
Contributor

tyralla commented Jan 3, 2023

Are we sure the initial report reveals a bug or is this behaviour by design? If I am right, we can consider it only a bug if the order of the type variable constraints matters. Then Mypy should behave as follows:

from typing import TypeVar

class A: ...
class B: v = 1
class C(A, B): ...

T1 = TypeVar("T1", A, B)

def f1(x: T1) -> T1:
    if isinstance(x, A):
        return A()  # no error
    return B()

T2 = TypeVar("T2", B, A)

def f2(x: T2) -> T2:
    if isinstance(x, A):
        return A()  # error: Incompatible return value type (got "A", expected "B")  [return-value]
    return B()

f1(C()).v  # error: "A" has no attribute "v"  [attr-defined]
f2(C()).v

Are there any promises that Mypy or other type checkers prioritise the first over the second type variable constraint when estimating return types in the context of multiple inheritance?

If we agree that order matters (and if I do not miss other potential problems here), adjusting Mypy should be manageable with reasonable effort.

@tyralla
Copy link
Contributor

tyralla commented Jan 4, 2023

It seems to be a known issue. I found the following test case (added by @Michael0x2a):

[case testIsInstanceAdHocIntersectionGenericsWithValuesDirectReturn]
# flags: --warn-unreachable
from typing import TypeVar

class A:
    attr: int
class B:
    attr: int
class C:
    attr: str

T1 = TypeVar('T1', A, B)
def f1(x: T1) -> T1:
    if isinstance(x, A):
        # The error message is confusing, but we indeed do run into problems if
        # 'x' is a subclass of A and B
        return A()   # E: Incompatible return value type (got "A", expected "B")
    else:
        return B()

T2 = TypeVar('T2', B, C)
def f2(x: T2) -> T2:
    if isinstance(x, B):
        # In contrast, it's impossible for a subclass of "B" and "C" to
        # exist, so this is fine
        return B()
    else:
        return C()
[builtins fixtures/isinstance.pyi]

tyralla added a commit to tyralla/mypy that referenced this issue Jan 4, 2023
… `isinstance`, constrained type variables and multiple inheritance (fixes python#13800).

The idea is to make a union of the current expanded return type and the previously expanded return types if the latter are among the bases of intersections generated by `isinstance` checks.
JelleZijlstra pushed a commit to python/typeshed that referenced this issue Jul 9, 2023
…10426)

Reverts #8465
Fixes #10424
Closes #10425

#8465 caused regressions: see #10424 and python/mypy#13800. Since it didn't fix any known problems (just some stylistic nits that we had), let's just revert the PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
7 participants