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

TypeGuard applied outside of it's scope #2852

Closed
ikamensh opened this issue Jan 12, 2022 · 14 comments
Closed

TypeGuard applied outside of it's scope #2852

ikamensh opened this issue Jan 12, 2022 · 14 comments
Labels
as designed Not a bug, working as intended

Comments

@ikamensh
Copy link
Contributor

ikamensh commented Jan 12, 2022

Hello, below is a suspected bug / inconsistent behavior I couldn't understand.

In the code below, a value is passed through to a submethod with same typing annotation, but an error is generated.

Code:

    def __init__(
        self,
        low: Union[SupportsFloat, np.ndarray],
       ...
    ):
        if shape is not None:
            shape = tuple(shape)
        elif not np.isscalar(low):
            shape = low.shape  # type: ignore
        elif not np.isscalar(high):
            shape = high.shape  # type: ignore
        else:
            raise ValueError(
                "shape must be provided or inferred from the shapes of low or high"
            )
        low = _broadcast(low, dtype, shape, inf_sign="-")  # error


def _broadcast(
    value: Union[SupportsFloat, np.ndarray],
    ...
) -> np.ndarray:
        ...

Error:

/home/runner/work/gym/gym/gym/spaces/box.py
  /home/runner/work/gym/gym/gym/spaces/box.py:53:32 - error: Argument of type "SupportsFloat | ndarray[Unknown, Unknown] | generic | bool | int | float | complex | str | bytes | memoryview" cannot be assigned to parameter "value" of type "SupportsFloat | ndarray[Unknown, Unknown]" in function "_broadcast"
    Type "SupportsFloat | ndarray[Unknown, Unknown] | generic | bool | int | float | complex | str | bytes | memoryview" cannot be assigned to type "SupportsFloat | ndarray[Unknown, Unknown]"
      Type "generic" cannot be assigned to type "SupportsFloat | ndarray[Unknown, Unknown]"
        "generic" is incompatible with protocol "SupportsFloat"
          "__float__" is not present
        "generic" is incompatible with "ndarray[Unknown, Unknown]"
      Type "complex" cannot be assigned to type "SupportsFloat | ndarray[Unknown, Unknown]"
        "complex" is incompatible with protocol "SupportsFloat"
          "__float__" is not present (reportGeneralTypeIssues)
1 error, 0 warnings, 0 infos 

CI:

Run jakebailey/pyright-action@v1
  with:
    version: 1.1.204
    python-platform: Linux
    python-version: 3.7
    no-comments: true
    lib: false
    warnings: false
  env:
    PYRIGHT_VERSION: 1.1.204
    pythonLocation: /opt/hostedtoolcache/Python/3.10.1/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.10.1/x64/lib

Complete source code: https://github.com/boschresearch/gym/blob/df82bdc27bc877bcaf98aecb203bc07fbea19ceb/gym/spaces/box.py#L53

Full CI log:
https://github.com/openai/gym/runs/4789425067?check_suite_focus=true

@JelleZijlstra
Copy link
Contributor

Does np.isscalar return a TypeGuard? I suspect that's affecting the type afterwards.

@ikamensh
Copy link
Contributor Author

I've looked it up, you are right isscalar in the latest numpy is using TypeGuard. https://github.com/numpy/numpy/blob/4adc87dff15a247e417d50f10cc4def8e1c17a03/numpy/core/numeric.pyi#L575

Yet the line where the problem is reported is outside of any if / else conditioned on a typeguard.

@ikamensh
Copy link
Contributor Author

ikamensh commented Jan 12, 2022

Update: this error only occurs when following code is present before the statement where error occurs:

if shape is not None:
  shape = tuple(shape)
elif not np.isscalar(low):
  shape = low.shape  # type: ignore
elif not np.isscalar(high):
  shape = high.shape  # type: ignore
else:
  raise ValueError(
      "shape must be provided or inferred from the shapes of low or high"
  )

I've updated the initial issue description to include this code.

@erictraut
Copy link
Collaborator

Thanks for the bug report. When reporting problems like this, it's really helpful if you can produce a minimal, self-contained code sample.

In this particular case, the problem comes down to this:

from typing import SupportsFloat, Union
import numpy as np

def func(val: np.generic):
    x: Union[SupportsFloat, np.ndarray] = val

The issue is that np.generic isn't compatible with either the type SupportsFloat or np.ndarray.

I think pyright is correct in generating an error here.

If you think this should not generate a type error, please consider reporting it to the numpy maintainers.

@erictraut erictraut added the as designed Not a bug, working as intended label Jan 12, 2022
@ikamensh
Copy link
Contributor Author

I disagree, because I don't see how in my case low becomes of type np.generic. It's passed as SupportsFloat | np.ndarray and is interpreted as possibly np.generic only after following block of code:

        if shape is not None:
            shape = tuple(shape)
        elif not np.isscalar(low):
            shape = low.shape  # type: ignore
        elif not np.isscalar(high):
            shape = high.shape  # type: ignore
        else:
            raise ValueError(
                "shape must be provided or inferred from the shapes of low or high"
            )

Removing following lines, which deal with another variable alltogether, removes the error:

        elif not np.isscalar(high):
            shape = high.shape  # type: ignore

I will try to produce more minimal example, but I don't think the example you have provided captures this problem.

@erictraut
Copy link
Collaborator

The np.isscalar function is defined as a user-defined type guard that uses the TypeGuard mechanism defined in PEP 647. (I'm the author of that PEP, BTW.)

def isscalar(element: object) -> TypeGuard[
    generic | bool | int | float | complex | str | bytes | memoryview
]: ...

This is telling the type checker that the value passed in should be "narrowed" to type generic | bool | int | float | complex | str | bytes | memoryview if the function returns True. Note that generic is included in this union.

@JelleZijlstra
Copy link
Contributor

Yes, this is a case where we'd want TypeGuard to narrow the Union instead of setting the type to exactly the TypeGuard argument.

@ikamensh
Copy link
Contributor Author

ikamensh commented Jan 12, 2022

Thank you for this PEP, it's very much appreciated. I think I have decent grasp of what TypeGuard currently does.

Here is a minimal example, having nothing to do with numpy, that demonstrates the issue:

from typing import TypeGuard


def tg(x) -> TypeGuard[str | int]:
    return isinstance(x, (str, int))


def foo(y: str) -> None:

    if 1 % 2:
        pass
    elif tg(y):
        print("CASE 1")  # Type is narrowed here, but it's not important for this example
    else:
        raise Exception("Invalid case")

    # Since we always take 1%2 branch, y is still a `str`
    z: str = y  # bla.py:19:14 - error: Expression of type "str | int" cannot be assigned to declared type "str"

@erictraut
Copy link
Collaborator

Yeah, the example above is an incorrect usage of TypeGuard for the reason you pointed out.

@ikamensh ikamensh changed the title ... | generic | ... cannot be assigned to parameter TypeGuard applied outside of it's scope Jan 12, 2022
@ikamensh
Copy link
Contributor Author

The PEP647 describes TypeGuard functionality:

_T = TypeVar("_T")

def is_two_element_tuple(val: Tuple[_T, ...]) -> TypeGuard[Tuple[_T, _T]]:
    return len(val) == 2

def func(names: Tuple[str, ...]):
    if is_two_element_tuple(names):
        reveal_type(names)  # Tuple[str, str]
    else:
        reveal_type(names)  # Tuple[str, ...]
    # MY ADDITION
    reveal_type(names)  # Expected: Tuple[str, ...]

What is not mentioned is what happens outside of if/else. I'm assuming nothing changes outside of if/else? The example above, when run with pyright, shows:

  bla.py:10:21 - info: Type of "names" is "Tuple[str, str]"
  bla.py:12:21 - info: Type of "names" is "Tuple[str, ...]"
  bla.py:14:17 - info: Type of "names" is "Tuple[str, str] | Tuple[str, ...]"

So any conditional with a typeguard in it is designed to change the type in the remainder of the code block?

@erictraut
Copy link
Collaborator

Yes, all type narrowing always extends beyond the conditioned block. This is not specific to user-defined type guards.

Consider the following example:

def func(x: object):
    if not isinstance(x, str):
        return
    reveal_type(x) # str

@ikamensh
Copy link
Contributor Author

ikamensh commented Jan 13, 2022

This mechanic is clear when there is a return / raise under if, without another way to reach the later code. In the example you give it could have as well been under an else clause.

This problem appears specific to user-defined type guards though. Following has no errors in typecheck:

def foo(y: str) -> None:

    if 1 % 2:
        pass
    elif isinstance(y, (str, int)):
        print("CASE 1")  # Type is narrowed here, but it's not important for this example
    else:
        raise Exception("Invalid case")

    # Since we always take 1%2 branch, y is still a `str`
    z: str = y  # good

Wrapping the isinstance check in a type guard creates an error (example copied from above):

from typing import TypeGuard


def tg(x) -> TypeGuard[str | int]:
    return isinstance(x, (str, int))


def foo(y: str) -> None:

    if 1 % 2:
        pass  # This branch is always taken, type guard doesn't matter actually
    elif tg(y):
        print("CASE 1")  # Type is narrowed here, but it's not important for this example
    else:
        raise Exception("Invalid case")

    # Since we always take 1%2 branch, y is still a `str`
    z: str = y  # bla.py:19:14 - error: Expression of type "str | int" cannot be assigned to declared type "str"

In summary, TypeGuard broadening the type beyond original together with this type broadening being taken beyond definitive if/else clauses seems undesirable to me.

@erictraut
Copy link
Collaborator

Normally "narrowing" requires you to transform a type into a more specific subtype of the original. This was too limiting for TypeGuard for reasons explained in the [Rejected Ideas section of PEP 647[(https://www.python.org/dev/peps/pep-0647/#enforcing-strict-narrowing). To accommodate the broad range of use cases that we were targeting, we needed to allow for more flexibility. This allows TypeGuard to "broaden" or otherwise transform types in ways that would be unsafe if used incorrectly. There was much discussion and debate about this during the PEP review process, but we ultimately decided that TypeGuard was an advanced feature and we would need to trust that it wouldn't be abused.

I can assure you that it is working the way it is intended here. There is not a bug in pyright (or in mypy, which will produce the same behavior). The bug is apparently in the np.isscalar declaration, which is inappropriately indicating that the type of the input value should be narrowed to a generic.

@erictraut
Copy link
Collaborator

Please refer to this active discussion about extensions to the existing TypeGuard to better handle the use case above. Input is welcome.

jkterry1 pushed a commit to openai/gym that referenced this issue Jan 24, 2022
* typing in spaces.Box and spaces.Discrete

* adds typing to dict and tuple spaces

* Typecheck all spaces

* Explicit regex to include all files under space folder

* Style: use native types and __future__ annotations

* Allow only specific strings for Box.is_bounded args

* Add typing to changes from #2517

* Remove Literal as it's not supported by py3.7

* Use more recent version of pyright

* Avoid name clash for type checker

* Revert "Avoid name clash for type checker"

This reverts commit 1aaf3e0.

* Ignore the error. It's reported as probable bug at microsoft/pyright#2852

* rebase and add typing for `_short_repr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as designed Not a bug, working as intended
Projects
None yet
Development

No branches or pull requests

3 participants