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 on self does not narrow associated TypeVars #14425

Open
JosiahKane opened this issue Jan 10, 2023 · 13 comments
Open

TypeGuard on self does not narrow associated TypeVars #14425

JosiahKane opened this issue Jan 10, 2023 · 13 comments
Labels
bug mypy got something wrong

Comments

@JosiahKane
Copy link

I have a class which is generic and has some runtime marker for determining the underlying type. I would like to use that marker to guide MyPy in validating the implementation of the class. So if I have a TypeGuard which confirms the current object is generic in A (that is, TypeVar AB_T is bound to A), then it should be safe for the method which is annotated as returning an AB_T to return an A in that context.

What I'm finding instead is that the TypeGuard correctly tells MyPy that the object which knows itself to be Gen[AB_T] is specifically a Gen[A], but it does not make the link that AB_T is A. As such, it refuses to return an A.

I've tested the following example with MyPy 0.991 on Python 3.10

from typing import Generic, Literal, overload, TypeVar
from typing_extensions import TypeGuard

class A: ...
class B: ...
AB_T = TypeVar("AB_T", A, B)

class Gen(Generic[AB_T]):
    # These two overloads ensure that the marker matches the generic type
    @overload
    def __init__(self: "Gen[A]", should_be_A: Literal[True]) -> None:
        ...

    @overload
    def __init__(self: "Gen[B]", should_be_A: Literal[False]) -> None:
        ...

    def __init__(self: "Gen[AB_T]", should_be_A: bool) -> None:
        self._should_be_A = should_be_A

    def res(self) -> AB_T:
        # This should return A() for a Gen[A] and B() for a Gen[B]
        if Gen.gives_A(self): # This calling convention seems to be needed to narrow self
            reveal_type(self) # and it now correctly understands that self is a Gen[A]
            return A() # But this still complains about the scenario where self is a Gen[B]
        elif Gen.gives_B(self):
            reveal_type(self)
            res: AB_T = B() # This also complains about the scenario where AB_T is A, even though we know it is B.
            return res
        else:
            assert False

    def gives_A(self) -> "TypeGuard[Gen[A]]":
        return self._should_be_A

    def gives_B(self) -> "TypeGuard[Gen[B]]":
        return not self._should_be_A

Actual Behavior

generic_specialisation.py:26: note: Revealed type is "generic_specialisation.Gen[generic_specialisation.A]"
generic_specialisation.py:27: error: Incompatible return value type (got "A", expected "B")  [return-value]
generic_specialisation.py:29: note: Revealed type is "generic_specialisation.Gen[generic_specialisation.B]"
generic_specialisation.py:30: error: Incompatible types in assignment (expression has type "B", variable has type "A")  [assignment]
@JosiahKane JosiahKane added the bug mypy got something wrong label Jan 10, 2023
@JosiahKane
Copy link
Author

In the event that this complaint is actually desired behaviour because in the general case TypeVars interact with variance, inheritance, and all sorts of "But what if it is actually both types?" questions, it may be worth knowing that the following (ugly) variation passes without errors by bouncing the variable through a member variable on the class.

_res_return_type: AB_T
def res(self) -> AB_T:
    # This should return A() for a Gen[A] and B() for a Gen[B]
    if Gen.gives_A(self):
        reveal_type(self) # Again it now correctly understands that self is a Gen[A]
        self._res_return_type= A() # So the type of self._res_return_type is A, so this is a legal assignment
    ... # Case for B skipped for brevity
    return self._res_return_type # And here the type of self._res_return_type is understood to be AB_T

@A5rocks
Copy link
Contributor

A5rocks commented Jan 12, 2023

The TypeGuard PEP explicitly excludes narrowing self. I have a PR open that should warn on this behavior.

@JosiahKane
Copy link
Author

Thanks for the reply, @A5rocks. The only thing that I can see in PeP 647 is this point which near as I can tell explains why self.gives_A() doesn't act as a TypeGuard against self and why I needed to have Gen.gives_A(self) instead. It does say "If narrowing of self or cls is required, the value can be passed as an explicit argument to a type guard function."

My question, however, is whether when I have successfully (using the explicit argument approach) narrowed Self[T] to Self[A], other things in T like the return type of the current method should also be understood to be A.

It seems that either the answer is yes, and the [return-value] and [assignment] errors in my first post are false positives, or the answer is no and the indirection in my follow up post is a false negative.

@A5rocks
Copy link
Contributor

A5rocks commented Jan 12, 2023

Ah I see what you mean now. (FWIW I recommend using @staticmethod on your typeguard, and yes I was talking about just the calling method, not that you can't call a typeguard on self).

I think yours might be the same issue as:

from typing import TypeGuard, TypeVar, Generic, Any

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

class A(Generic[T]):
    pass

def tp_int(a: A[Any]) -> TypeGuard[A[int]]:
    return False

def tp_str(a: A[Any]) -> TypeGuard[A[str]]:
    return False

def f(a: A[T]) -> T:
    if tp_int(a):
        return 42
    elif tp_str(a):
        return "hi"
    else:
        assert False

No self narrowing needed! I do think this is a bug and that this could be fixed (ie remap T in the guards). Let's say the errors are false positives.

FWIW this case would probably (by my guess) have to be fixed first:

from typing import TypeVar

T = TypeVar("T")

def f(x: T) -> T:
    if isinstance(x, int):
        return 42
    else:
        return x

I looked for an issue and found #1539

Reading the justification for closing it, your case is actually working as expected:tm:

Actually nevermind, I think a big part of me being confused here is because while isinstance(x, T) means that x's type is a subtype of T, the typeguard will say that x's type is T. This behavior is foot-gunny (#11230) but nonetheless your code should work. I remain convinced this is a false positive.

@erictraut
Copy link

If you write a user-defined type guard function as an instance method, the self parameter is not considered the target of type narrowing. You need to provide at least one additional parameter beyond self. @JosiahKane, in your original example above, you have defined gives_A and gives_B as instance methods, but you haven't provided a parameter for a type narrowing target. That means these are invalid user-defined type guard methods. Mypy should arguably generate an error in this case. I've just filed an enhancement request in pyright to do the same.

If your intent is to test the value of a Gen[Any] object to determine whether it is either a Gen[A] or Gen[B], then you should either move your type guard functions outside of the class or change these methods to static methods within the class.

    @staticmethod
    def gives_A(obj: "Gen[Any]") -> "TypeGuard[Gen[A]]":
        return obj._should_be_A

    @staticmethod
    def gives_B(obj: "Gen[Any]") -> "TypeGuard[Gen[B]]":
        return not obj._should_be_A

If, for some reason, you really don't want to use a static method and want to stick with an instance method, then you will need to define a second parameter.

    def gives_A(self, obj: "Gen[Any]") -> "TypeGuard[Gen[A]]":
        return obj._should_be_A

And you would need to call it like this:

self.gives_A(self)

@A5rocks, you said "the typeguard will say that x's type is T". That's an incorrect interpretation. The type specified as the return type of a TypeGuard is expressed as a type annotation, and all type annotations implicitly mean "the type is a subtype of T". For example, if you specify that a function returns type float, that doesn't mean that the return value is necessarily a float; it could be an int. The same is true with TypeGuard[float].

@A5rocks
Copy link
Contributor

A5rocks commented Jan 12, 2023

Ah, my interpretation was based on how typeguards don't narrow to an intersection between the type it's guarded against and the type passed in, unlike normal type narrowing.

I see now how that was a wrong interpretation. I was confused at the time at how to handle the case described (assuming the typeguard is valid). Maybe this will become obvious once I think about it.

@JosiahKane
Copy link
Author

JosiahKane commented Jan 12, 2023

Thanks for the reply, @erictraut.

I understand the point about instance methods not narrowing self if used conventionally, which is why my code was written as Gen.gives_A(self) instead of self.gives_A(). I agree that it's then clearer to write gives_A as a static method, and I would certainly not object if my version had raised a suitable warning. However, that's still not really the core question, and doesn't change the observed behaviour since the reveal_type lines show that both versions narrow self.

The question is whether, having narrowed self as Gen[T] to Gen[A], MyPy should understand T to necessarily refer to A. If yes, then there is a false positive in complaining about returning A from a method which promises a T. If no, then there is a false negative in being able to return an A via the indirection of packing it into self, as illustrated in my second comment.

@ezyang
Copy link

ezyang commented Jan 29, 2024

FWIW this case would probably (by my guess) have to be fixed first:

from typing import TypeVar

T = TypeVar("T")

def f(x: T) -> T:
    if isinstance(x, int):
        return 42
    else:
        return x

I would quite like for this particular example to be fixed, it will make a lot of GADT-style code a lot simpler (today, I have to cast the return type back to T in order to get this to type check).

@hauntsaninja
Copy link
Collaborator

@ezyang the example you post is not type safe:

from typing import TypeVar

T = TypeVar("T")

def bad(x: T) -> T:
    if isinstance(x, int):
        return 42
    else:
        return x

class subint(int): ...

def takes_subint(x: subint):
    assert isinstance(x, subint)

takes_subint(bad(subint(0)))  # boom

@ezyang
Copy link

ezyang commented Jan 30, 2024

Well, how about the more precise:

from typing import TypeVar

T = TypeVar("T")

def f(x: T) -> T:
    if type(x) is int:
        return 42
    else:
        return x

@hauntsaninja
Copy link
Collaborator

That is safe (for non-generics)!

@ezyang
Copy link

ezyang commented Jan 31, 2024

I am too used to type systems that don't have subtyping 👹

ezyang added a commit to pytorch/pytorch that referenced this issue Jan 31, 2024
I was just playing around with improving the typing of symbolic_shapes. The PR is not "complete" but I in particular wanted to get feedback on whether or not people liked making ValueRanges Generic; it seems that distinguishing if you have an Expr ValueRange or a SympyBoolean ValueRange is a lot of trouble for downstream. Using TypeGuard, we can perform refinements on the generic parameter inside methods, although we still have to cast back to ValueRange[T] due to python/mypy#14425 (comment)

Signed-off-by: Edward Z. Yang <ezyangmeta.com>

[ghstack-poisoned]
ezyang added a commit to pytorch/pytorch that referenced this issue Jan 31, 2024
I was just playing around with improving the typing of symbolic_shapes. The PR is not "complete" but I in particular wanted to get feedback on whether or not people liked making ValueRanges Generic; it seems that distinguishing if you have an Expr ValueRange or a SympyBoolean ValueRange is a lot of trouble for downstream. Using TypeGuard, we can perform refinements on the generic parameter inside methods, although we still have to cast back to ValueRange[T] due to python/mypy#14425 (comment)

Signed-off-by: Edward Z. Yang <ezyangmeta.com>

[ghstack-poisoned]
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this issue Jan 31, 2024
I was just playing around with improving the typing of symbolic_shapes. The PR is not "complete" but I in particular wanted to get feedback on whether or not people liked making ValueRanges Generic; it seems that distinguishing if you have an Expr ValueRange or a SympyBoolean ValueRange is a lot of trouble for downstream. Using TypeGuard, we can perform refinements on the generic parameter inside methods, although we still have to cast back to ValueRange[T] due to python/mypy#14425 (comment)

Signed-off-by: Edward Z. Yang <[email protected]>

Pull Request resolved: #118529
Approved by: https://github.com/Skylion007
facebook-github-bot pushed a commit to pytorch/benchmark that referenced this issue Feb 1, 2024
Summary:
I was just playing around with improving the typing of symbolic_shapes. The PR is not "complete" but I in particular wanted to get feedback on whether or not people liked making ValueRanges Generic; it seems that distinguishing if you have an Expr ValueRange or a SympyBoolean ValueRange is a lot of trouble for downstream. Using TypeGuard, we can perform refinements on the generic parameter inside methods, although we still have to cast back to ValueRange[T] due to python/mypy#14425 (comment)

Signed-off-by: Edward Z. Yang <[email protected]>

X-link: pytorch/pytorch#118529
Approved by: https://github.com/Skylion007

Reviewed By: clee2000

Differential Revision: D53296779

Pulled By: ezyang

fbshipit-source-id: 95799914350e50aeedf1acad93d71b79cad827c8
ezyang added a commit to pytorch/pytorch that referenced this issue Feb 3, 2024
I was just playing around with improving the typing of symbolic_shapes. The PR is not "complete" but I in particular wanted to get feedback on whether or not people liked making ValueRanges Generic; it seems that distinguishing if you have an Expr ValueRange or a SympyBoolean ValueRange is a lot of trouble for downstream. Using TypeGuard, we can perform refinements on the generic parameter inside methods, although we still have to cast back to ValueRange[T] due to python/mypy#14425 (comment)

Signed-off-by: Edward Z. Yang <ezyangmeta.com>

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler

[ghstack-poisoned]
ezyang added a commit to pytorch/pytorch that referenced this issue Feb 3, 2024
I was just playing around with improving the typing of symbolic_shapes. The PR is not "complete" but I in particular wanted to get feedback on whether or not people liked making ValueRanges Generic; it seems that distinguishing if you have an Expr ValueRange or a SympyBoolean ValueRange is a lot of trouble for downstream. Using TypeGuard, we can perform refinements on the generic parameter inside methods, although we still have to cast back to ValueRange[T] due to python/mypy#14425 (comment)

Signed-off-by: Edward Z. Yang <ezyangmeta.com>

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler

[ghstack-poisoned]
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this issue Feb 4, 2024
I was just playing around with improving the typing of symbolic_shapes. The PR is not "complete" but I in particular wanted to get feedback on whether or not people liked making ValueRanges Generic; it seems that distinguishing if you have an Expr ValueRange or a SympyBoolean ValueRange is a lot of trouble for downstream. Using TypeGuard, we can perform refinements on the generic parameter inside methods, although we still have to cast back to ValueRange[T] due to python/mypy#14425 (comment)

Signed-off-by: Edward Z. Yang <[email protected]>

Pull Request resolved: #118529
Approved by: https://github.com/Skylion007
facebook-github-bot pushed a commit to pytorch/benchmark that referenced this issue Feb 6, 2024
Summary:
I was just playing around with improving the typing of symbolic_shapes. The PR is not "complete" but I in particular wanted to get feedback on whether or not people liked making ValueRanges Generic; it seems that distinguishing if you have an Expr ValueRange or a SympyBoolean ValueRange is a lot of trouble for downstream. Using TypeGuard, we can perform refinements on the generic parameter inside methods, although we still have to cast back to ValueRange[T] due to python/mypy#14425 (comment)

Signed-off-by: Edward Z. Yang <[email protected]>

X-link: pytorch/pytorch#118529
Approved by: https://github.com/Skylion007

Reviewed By: atalman

Differential Revision: D53436982

Pulled By: ezyang

fbshipit-source-id: a31a94137df69eae5ddc86396580655ff478b8a2
pytorch-bot bot pushed a commit to pytorch/pytorch that referenced this issue Feb 8, 2024
I was just playing around with improving the typing of symbolic_shapes. The PR is not "complete" but I in particular wanted to get feedback on whether or not people liked making ValueRanges Generic; it seems that distinguishing if you have an Expr ValueRange or a SympyBoolean ValueRange is a lot of trouble for downstream. Using TypeGuard, we can perform refinements on the generic parameter inside methods, although we still have to cast back to ValueRange[T] due to python/mypy#14425 (comment)

Signed-off-by: Edward Z. Yang <[email protected]>

Pull Request resolved: #118529
Approved by: https://github.com/Skylion007
pytorch-bot bot pushed a commit to pytorch/pytorch that referenced this issue Feb 8, 2024
I was just playing around with improving the typing of symbolic_shapes. The PR is not "complete" but I in particular wanted to get feedback on whether or not people liked making ValueRanges Generic; it seems that distinguishing if you have an Expr ValueRange or a SympyBoolean ValueRange is a lot of trouble for downstream. Using TypeGuard, we can perform refinements on the generic parameter inside methods, although we still have to cast back to ValueRange[T] due to python/mypy#14425 (comment)

Signed-off-by: Edward Z. Yang <[email protected]>

Pull Request resolved: #118529
Approved by: https://github.com/Skylion007
@ReSqAr
Copy link

ReSqAr commented May 27, 2024

I just came across this issue and was indeed very puzzled by the mypy errors. It would be really great if the idea can be expressed safely using type annotations. I first thought that the type narrowing on T wasn't happening at all. However mypy complains (perhaps surprisingly?) about all three cases here:

import typing

T = typing.TypeVar("T")


def func(x: T) -> T:
    if type(x) is int:
        return 42
    elif type(x) is str:
        return "foo"
    elif type(x) is float:
        return x  # returns as is
    else:
        return x

mypy's output is:

mypy_14425.py:8: error: Incompatible return value type (got "int", expected "T")  [return-value]
mypy_14425.py:10: error: Incompatible return value type (got "str", expected "T")  [return-value]
mypy_14425.py:12: error: Incompatible return value type (got "float", expected "T")  [return-value]

Version:

mypy --version
mypy 1.10.0 (compiled: yes)

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

No branches or pull requests

6 participants