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

Unable to narrow unions including EllipsisType or NoneType using the identity of their singleton #13117

Closed
tungol opened this issue Jul 13, 2022 · 6 comments · Fixed by #13157
Labels
feature good-second-issue topic-type-narrowing Conditional type narrowing / binder

Comments

@tungol
Copy link
Contributor

tungol commented Jul 13, 2022

Bug Report

Variables whose type is a union that includes types.EllipsisType can't be narrowed using if foo is Ellipsis. That's unexpected to me, because Ellipsis is documented as the only instance of EllipsisType.

Similar but slightly different issues occur with NoneType and NotImplementedType.

To Reproduce

Minimal reproducing case:

from types import EllipsisType


def working(foo: EllipsisType | int) -> int:
    if isinstance(foo, EllipsisType):
        # Revealed type is "builtins.ellipsis"  (seems to understand the singleton relationship here)
        foo = 1
    reveal_type(foo)  # Revealed type is "builtins.int"
    return foo  # no error

def not_working(foo: EllipsisType | int) -> int:
    if foo is Ellipsis:
        reveal_type(foo)  # Revealed type is "Union[builtins.ellipsis, builtins.int]" (why is int still here?)
        foo = 1
    reveal_type(foo)  # Revealed type is "Union[builtins.ellipsis, builtins.int]"
    return foo  # error: Incompatible return value type (got "Union[ellipsis, int]", expected "int")

Mypy appears to believe that there might be values whose type is EllipsisType but which don't have the same identity as Ellipsis. I don't think that's possible.

Similar things happen with None and NoneType:

from types import NoneType


def working(foo: NoneType | int) -> int:
    if isinstance(foo, NoneType):
        reveal_type(foo)  # Revealed type is "types.NoneType"  (interesting difference from Ellipsis)
        foo = 1
    reveal_type(foo)  # Revealed type is "builtins.int"
    return foo  # no error


def not_working(foo: NoneType | int) -> int:
    if foo is None:
        reveal_type(foo)  # Doesn't show any output (?)
        foo = 1
    reveal_type(foo)  # Revealed type is "Union[types.NoneType, builtins.int]"
    return foo  # Incompatible return value type (got "Union[NoneType, int]", expected "int")

NotImplemented and NotImplementedType pass without error, but appear to do so for the wrong reasons:

from types import NotImplementedType


def working(foo: NotImplementedType | int) -> int:
    if isinstance(foo, NotImplementedType):
        reveal_type(foo)  # Revealed type is "builtins._NotImplementedType"
        foo = 1
    reveal_type(foo)  # Revealed type is "Union[builtins._NotImplementedType, builtins.int]"
    return foo  # no error ??


def also_working(foo: NotImplementedType | int) -> int:
    if foo is NotImplemented:
        reveal_type(foo)  # Revealed type is "Union[builtins._NotImplementedType, builtins.int]"
        foo = 1
    reveal_type(foo)  # Revealed type is "Union[builtins._NotImplementedType, builtins.int]"
    return foo  # no error ??

Your Environment

  • Mypy version used: mypy 0.961 (compiled: yes)
  • Mypy command-line flags: none
  • Mypy configuration options from mypy.ini (and other config files): none
  • Python version used: python Python 3.10.4
  • Operating system and version: macOS 10.15.7
@tungol tungol added the bug mypy got something wrong label Jul 13, 2022
@JelleZijlstra JelleZijlstra added feature topic-type-narrowing Conditional type narrowing / binder and removed bug mypy got something wrong labels Jul 13, 2022
@tungol tungol changed the title Ellipsis is not treated as the only instance of EllipsisType Unable to narrow unions including EllipsisType or NoneType using the identity of their singleton Jul 14, 2022
@tungol
Copy link
Contributor Author

tungol commented Jul 14, 2022

I tested with bool. It almost works correctly, but it's a little over-eager. Checking for either True or False will narrow out both boolean values:

def test_bool(foo: bool | int) -> int:
    if foo is True:
        reveal_type(foo)  # Revealed type is "Literal[True]"
        foo = 1

    reveal_type(foo)  # Revealed type is "builtins.int"
    return foo  # no error

@JelleZijlstra
Copy link
Member

The bool | int case is probably because bool is a subclass of int, and sometimes mypy simplifies the union and throws away bool. If you type foo as bool | str instead, the types are as expected.

@tungol
Copy link
Contributor Author

tungol commented Jul 15, 2022

Ah, that makes sense. So the relevant parallel case for boolean literals is something like this:

def test_bool(foo: bool | str) -> str:
    if foo is True:
        reveal_type(foo)  # Revealed type is "Literal[True]"
        foo = "1"
    reveal_type(foo)  # Revealed type is "Union[Literal[False], builtins.str]"
    if foo is False:
        reveal_type(foo)  # Revealed type is "Literal[False]"
        foo = "1"
    reveal_type(foo)  # Revealed type is "builtins.str"
    return foo  # no error

which works like I'd expect it to.

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 16, 2022

NoneType should be rejected by mypy (#11288) so it doesn't need to be special cased. If using just None, things already work.

Summarizing the above discussion, these cases are relevant:

  • EllipsisType / Ellipsis
  • NotImplementedType / NotImplemented

I think that the implementation will have to touch find_isinstance_check_helper (or the functions that it calls) in mypy.checker.

vlaci pushed a commit to vlaci/mypy that referenced this issue Jul 16, 2022
After this change, narrowing to `Ellipsis` works similarly with
regards to narrowing as `None` in `Optional`s

It would be a good followup refactor to delegate some of the logic
from `is_singleton_type` to the actual mypy types so they could decide
for themselves if they are representing singleton objects

Fixes: python#13117
Co-authored-by: Zsolt Cserna <cserna.zsolt@gmail.com>
@vlaci
Copy link
Contributor

vlaci commented Jul 16, 2022

I think, the NotImplemented case works as intended: it is implemented similarly as Any, so it can be a substitute for all types.

vlaci pushed a commit to vlaci/mypy that referenced this issue Jul 17, 2022
After this change, narrowing to `Ellipsis` works similarly with
regards to narrowing as `None` in `Optional`s

It would be a good followup refactor to delegate some of the logic
from `is_singleton_type` to the actual mypy types so they could decide
for themselves if they are representing singleton objects

Fixes: python#13117
Co-authored-by: Zsolt Cserna <cserna.zsolt@gmail.com>
ilevkivskyi pushed a commit that referenced this issue Jul 17, 2022
#13157)

After this change, narrowing to `Ellipsis` works similarly with
regards to narrowing as `None` in `Optional`s

It would be a good followup refactor to delegate some of the logic
from `is_singleton_type` to the actual mypy types so they could decide
for themselves if they are representing singleton objects

Fixes: #13117
Co-authored-by: Zsolt Cserna <cserna.zsolt@gmail.com>

Co-authored-by: László Vaskó <laszlo.vasko@onekey.com>
Co-authored-by: Zsolt Cserna <cserna.zsolt@gmail.com>
vlaci pushed a commit to vlaci/mypy that referenced this issue Jul 17, 2022
This greatly reduces complexity as each type can determine for
themselves if they are representing a singleton.

`get_enum_values` has to be moved as well, as it is used by
`get_singleton_type`.

Relates-to: python#13117
ilevkivskyi pushed a commit that referenced this issue Jul 17, 2022
This greatly reduces complexity as each type can determine for
themselves if they are representing a singleton.

`get_enum_values` has to be moved as well, as it is used by
`get_singleton_type`.

Relates-to: #13117

Co-authored-by: László Vaskó <laszlo.vasko@onekey.com>
@tungol
Copy link
Contributor Author

tungol commented Jul 18, 2022

Nice! Thanks, @vlaci !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature good-second-issue topic-type-narrowing Conditional type narrowing / binder
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants