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

Narrowing doesn't work on a typevar with a union bound #15631

Open
eltoder opened this issue Jul 9, 2023 · 4 comments · May be fixed by #15711
Open

Narrowing doesn't work on a typevar with a union bound #15631

eltoder opened this issue Jul 9, 2023 · 4 comments · May be fixed by #15711
Labels
bug mypy got something wrong

Comments

@eltoder
Copy link

eltoder commented Jul 9, 2023

Bug Report

not isinstance check fails to narrow type for a TypeVar with a union bound.

To Reproduce

from typing import TypeVar

T = TypeVar("T", bound=int | str)

def foo(x: T) -> None:
    if not isinstance(x, int):
        reveal_type(x)

https://mypy-play.net/?mypy=latest&python=3.11&gist=2be94d37f0b12222d62a7f40e59fb152

Expected Behavior

Revealed type is "builtins.str".

Actual Behavior

Revealed type is "T`-1".

Your Environment

  • Mypy version used: 1.4.1
  • Python version used: 3.10.6
@eltoder eltoder added the bug mypy got something wrong label Jul 9, 2023
@A5rocks
Copy link
Collaborator

A5rocks commented Jul 18, 2023

By the way, the expected behavior is not correct:

Imagine T is bound as some string subclass! If T were accepted as str then the following should probably be valid, correct?:

from typing import TypeVar

T = TypeVar("T", bound=int | str)

def foo(x: T) -> T:
    if not isinstance(x, int):
        return "h"
    else:
        # whatever
        return x

But that's not the case! The main problem here is typevar reveal_types cannot really express the narrowing as shown here. While mypy can do it under the hood (and you presumably have a case that shows this isn't the case?) that's not exactly what you're asking here!

Mind clarifying a bit?

@eltoder
Copy link
Author

eltoder commented Jul 18, 2023

Good point, my example is not 100% correct. Let me clarify and also add a second problem case.

  1. A problem from your example that already happens with a positive isinstance check:
from typing import TypeVar

T = TypeVar("T", bound=int | str)

def foo(x: T) -> T:
    if isinstance(x, int):
        reveal_type(x)
        return x
    else:
        return x

This produces:

typevar_narrow.py:7: note: Revealed type is "builtins.int"
typevar_narrow.py:8: error: Incompatible return value type (got "int", expected "T")  [return-value]

The type of x was narrowed to int and became incompatible with T even though the code is obviously correct. Here we did too much narrowing.

  1. A problem with not enough narrowing:
from typing import TypeVar

T = TypeVar("T", bound=int | str)

def foo(x: T) -> None:
    if not isinstance(x, int):
        bar(x)

def bar(x: str) -> None:
    pass

This produces

typevar_narrow.py:7: error: Argument 1 to "bar" has incompatible type "T"; expected "str"  [arg-type]

but we know the type should be (a subclass of) str, because of the bound on T.

To make it work in both cases, I think, rather than narrowing the type of x we should narrow the bound on T.

@brianschubert
Copy link
Collaborator

Duplicate of #15235

@brianschubert brianschubert marked this as a duplicate of #15235 Nov 14, 2024
@brianschubert brianschubert closed this as not planned Won't fix, can't repro, duplicate, stale Nov 14, 2024
@brianschubert brianschubert marked this as not a duplicate of #15235 Nov 14, 2024
@brianschubert
Copy link
Collaborator

brianschubert commented Nov 14, 2024

Actually, this issue is different from #15235. #15235 is only about the negative-narrowing behavior, whereas this issue is also about the positive-narrowing behavior. My bad, sorry for the noise!

@brianschubert brianschubert reopened this Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants