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

Change PEP 647 to allow for narrowing in the negative case #926

Closed
Zomatree opened this issue Nov 6, 2021 · 10 comments
Closed

Change PEP 647 to allow for narrowing in the negative case #926

Zomatree opened this issue Nov 6, 2021 · 10 comments
Labels
topic: feature Discussions about new features for Python's type annotations

Comments

@Zomatree
Copy link

Zomatree commented Nov 6, 2021

Currently PEP 647 doesnt narrow the type in the negative case, i feel like this should be changed to narrow in the negative case, currently due to it not narrowing its impossible to correctly type certain functions with typeguards fully, this came up in actual code a couple times, for example:

this example is used in conjuction with python/typeshed#5658

R_T = TypeVar("R_T")
P = ParamSpec("P")

async def maybe_coroutine(func: Callable[P, Union[R_T, Coroutine[Any, Any, R_T]]], *args: P.args, **kwargs: P.kwargs) -> R_T:
    value = func(*args, **kwargs)

    if isawaitable(value):
        value = await value

    return value

because value isnt narrowed to be R_T@maybe_coroutine outside the if statement its back to being a union even though it would never be awaitable because of the if statement, this feels like a massive downside and makes typeguard pretty useless for a lot of cases, forcing people to use cast or # type: ignore.

While this may cause a lot of controversy with changing a quite large part of how typeguards work, i feel like this is a needed change and a step in the right direction for making typing much more powerful and able to be properly integrated into more projects codebases without having to resort to ignoring or overwrites the type

@JelleZijlstra
Copy link
Member

cc @erictraut. I know Eric has said before that this was a conscious decision based on experience with TypeScript, but I don't recall the details and PEP 647 doesn't actually cover this in its "Rejected ideas" section. It would be useful to add an explicit rejection of the idea to the PEP.

I've definitely also felt the need for a TypeGuard that narrows in the negative case too.

@Zomatree
Copy link
Author

Zomatree commented Nov 6, 2021

I have no experience with typeguards in typescripts so if this is carried over from typescript i would like to know why and maybe potential ways around this or a better solution to this issue. But from my current knowledge, this is only a disadvantage.

@srittau srittau added the topic: feature Discussions about new features for Python's type annotations label Nov 6, 2021
@gvanrossum
Copy link
Member

IIRC the issue is that a typeguard function may reject objects that match the given type because their value doesn't match. Maybe I can make up an example.

def is_small_int(x: object) -> TypeGuard[int]:
    return isinstance(x) and 0 <= x < 256

def foo(x: int|str) -> str:
    if is_small_int(x):
        return str(x)
    elif isinstance(x, int):
        return hex(x)
    else:
        return repr(x)

Here the type checker cannot assume that the elif clause is unreachable just because is_small_int() returns false -- x could still be an int.

@Zomatree
Copy link
Author

Zomatree commented Nov 6, 2021

I see this as a non-issue, If you want to check the value of the int in your typeguarded function then the typeguard's value should also reflect that, in this case its perfectly reasonable to define the function to something like:

def is_small_int(x: object) -> TypeGuard[Literal[0, 1, 2, 3, 4, 5, ...]]:
    return isinstance(x) and 0 <= x < 256

for more complex checks that couldn't be described in the return type, you shouldn't be doing that in a typeguard or if this is a perfectly world where the python's type system was powerful enough then there shouldn't be anything that couldn't be described via the return type.

@erictraut
Copy link
Collaborator

Negative type narrowing cannot be safely applied for user-defined type guards. A user-defined type guard check indicates only that the value is of a given type. It is not type safe to assume that a type can be eliminated from a union if this function returns False.

I'll provide a couple of concrete examples. The first one is a flavor of a problem that bites me periodically in TypeScript, which does provide narrowing in the negative case when the input argument is a union. This is the reason why the TypeScript team suggested that they somewhat regret making the decision.

class Cat:
    color: str

class Dog:
    breed: str

Pet = Cat | Dog

def is_black_cat(val: Pet) -> TypeGuard[Cat]:
    return isinstance(val, Cat) and val.color == "black"

def filter_back_cats(pets: list[Pet]) -> list[Cat]:
    return [x for x in pets if is_black_cat(x)]

def print_pet_info(pet: Pet):
    if is_black_cat(pet):
        print("Found black cat!")
    elif isinstance(pet, Cat):
        # This would be incorrectly flagged as an unreachable piece of code
        # if is_black_cat() eliminated Cat from the union.
        print("Regular cat")
    else:
        print("Not a cat")

TypeScript is able to get the "correct" answer most of the time in the negative case because user-defined type guards in TypeScript require that the TypeGuard type (the "narrowed" type) is a proper subtype of the value type (the type of the input parameter). It's able to use this to determine which subtype of the input union type should be eliminated and which should be retained in the negative case.

When we were defining the rules for PEP 647, we decided that this limitation was too constraining for Python, and we relaxed this requirement. This makes the handling of the negative case even more fraught with problems.

Consider this example:

def is_str_list(val: Sequence[int | str | None]) -> TypeGuard[List[str]]:
    return isinstance(val, list) and all(isinstance(x, str) for x in val)

def func1(val: Sequence[str | int] | List[str | None]):
    if is_str_list(val):
        return

    # What is the type of `val` here?
    reveal_type(val)

Your assertion that user-defined type guards are "pretty useless for a lot of cases" without negative narrowing strikes me as a bit hyperbolic. In practice, I don't think this limitation is that big of a problem in most cases.

Significant thought and discussion went into this PEP during its design, and we concluded that this was a reasonable tradeoff.

In any event, the change you're proposing would be a breaking change at this point since Python 3.10 is released and people have started to use TypeGuard in its current form and assuming its currently-defined semantics.

@Zomatree
Copy link
Author

Zomatree commented Nov 8, 2021

While i can see the issue now, there are definitely ways to fix these issues, the one that comes to my mind is to introduce an Intersection type (#213) and do something similar to:

class BlackCatProto(Protocol):
    color: Literal["black"]

def is_black_cat(pet: Pet) -> TypeGuard[Cat & BlackCatProto]:
    return isinstance(pet, Cat) and pet.color == "black"

this is similar to how typescript does it as well

function isBlackCat(pet: Pet): pet is Cat & { color: "black" } {
  return pet instanceof Cat && v.color === "black";
}

@hmc-cs-mdrissi
Copy link

Intersection types are complex enough of a feature it is unlikely that supporting narrowing in negative case is reason to add intersection types. I expect intersection types to need there own pep/campaigning to happen along with clear choices as to are intersections nominal/structural/something else?

I think alternative approach for Negative narrowing is having second variant of TypeGuard like ExactTypeGuard[T] where the function being false implies the value is not type T while normal TypeGuards continue to only cover positive case. I mainly care about union negative narrowing logic as that's main time the issue appears for me and currently I work around it with casts as needed.

@Zomatree
Copy link
Author

Zomatree commented Nov 9, 2021

While I understand intersection types are complex, this is still another reason for adding them. People have been asking for an intersection type for quite a while and the more reasons for adding them the better.

I'm +1 on your idea of ExactTypeGuard that only affects unions as unions are the main case for why I want this changed.

@MajorDallas
Copy link

I also found TypeGuard's behavior surprising and unintuitive. For what I needed it for, not being able to narrow in the negative case also meant that I had to find another solution.

At least for me, the confusion arises from the examples above. In Eric's Cat example, for instance:

blackcat = Cat(color="black")
tabbycat = Cat(color="tabby")
reveal_type(blackcat)  # Cat
reveal_type(tabbycat)  # Cat
if not is_black_cat(tabbycat):
    print(type(tabbycat))  # still Cat

To me, inspecting the runtime value of color is not a "type" guard but a "state" guard. TypeGuard actually being a state guard is what made the behavior surprising, and I'm inclined to agree with it being "pretty useless for a lot of cases" in the static analysis context.

@erictraut
Copy link
Collaborator

I think this can be closed now that TypeIs has been added to the type system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: feature Discussions about new features for Python's type annotations
Projects
None yet
Development

No branches or pull requests

8 participants