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

Support TypeGuard (PEP 647) #9865

Merged
merged 27 commits into from
Jan 18, 2021
Merged

Support TypeGuard (PEP 647) #9865

merged 27 commits into from
Jan 18, 2021

Conversation

gvanrossum
Copy link
Member

While PEP 647 is still in draft mode, this is (or if it isn't will soon be) a full implementation.

This depends on a typeshed PR: python/typeshed#4879.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty reasonable to me!

Even though it's not in the direct path of changes, might be worth adding a test for interactions with the walrus operator.

Also, I'm guessing higher order use of type guarded functions doesn't work? E.g. something like:

def filter(fn: Callable[[T1], TypeGuard[T2]], it: Iterable[T1]) -> Iterator[T2]: ...

reveal_type(filter(...))

mypy/checker.py Outdated
and isinstance(node.callee.node, SYMBOL_FUNCBASE_TYPES)
and isinstance(node.callee.node.type, CallableType)
and node.callee.node.type.type_guard is not None):
if len(node.args) < 1: # TODO: Is this an error?
Copy link
Collaborator

@hauntsaninja hauntsaninja Dec 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me, it's similar to what happens with if isinstance() + you have a TODO in your tests to error at the definition site for functions that don't take args

- walrus
- higher-order functions
@gvanrossum
Copy link
Member Author

Also, I'm guessing higher order use of type guarded functions doesn't work?

Hm, it doesn't, but it really should. Unfortunately that's going to require a different approach, and I'm not sure how to do it. There are an awful lot of places where a Callable's ret_type attribute is used, and I'm hesitant to try and audit them all to see whether I should redirect the code to the type_guard attribute or not.

@hauntsaninja
Copy link
Collaborator

I don't have a general answer for you, but maybe add something to this line and you get lucky and it mostly just works?

res.extend(infer_constraints(template.ret_type, cactual.ret_type,

@gvanrossum
Copy link
Member Author

I don't have a general answer for you, but maybe add something to this line and you get lucky and it mostly just works?

res.extend(infer_constraints(template.ret_type, cactual.ret_type,

Thanks, right on the nose. It was this and a bit in expandtype.py.

@gvanrossum
Copy link
Member Author

gvanrossum commented Jan 1, 2021 via email

@gvanrossum gvanrossum requested a review from JukkaL January 4, 2021 04:47
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! Overall looks good -- the changes are clean and localized. This may not be a very commonly needed feature, but in cases where this is useful the current alternatives are all quite bad.

I did a quick review pass and left suggestions about additional test cases. I'll read the PEP in detail and play around with the implementation, and I'll do another round of review afterwards.

def is_point(a: object) -> TypeGuard[points.Point]: pass
[file points.py]
class Point: pass
[builtins fixtures/tuple.pyi]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea about additional tests (sorry if some of these already exist):

  • Test a type guard function with a non-empty body. Check that a non-boolean return value is flagged as an error.
  • Test a type guard that narrows down Dict[str, object] to a TypedDict.
  • Test using type guard with an assert.
  • Test using two type guards with 'or', such as if is_int(x) or is_float(x):.
  • Test narrowing down from Any using a type guard.

@@ -270,6 +270,16 @@ def copy_modified(self, *,
self.line, self.column)


class TypeGuardType(Type):
"""Only used by find_instance_check() etc."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add link to the PEP in the docstring.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 7, 2021

A few more ideas about tests:

  • Test that if not is_int(x) does not narrow Union[int, str] to str (also in the else block of if is_int(x)).
  • Test a type guard implemented as a class method.

@gvanrossum
Copy link
Member Author

gvanrossum commented Jan 7, 2021 via email

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 7, 2021

Two more things that may be worth testing:

  • Overloaded function where one item/all items have a type guard.
  • Decorated type guard function (decorator has type T -> T).

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 8, 2021

More possible things to test:

  • If a method is a type guard, do we need to ensure that any overrides in subclasses are type guards (and compatible type guards)?
  • If we have a higher-order function that takes a type guard, test that the passed function is a suitable type guard (currently it looks like we can pass an arbitrary function that returns bool).

(It's also okay to create follow-up issues for some of these if they don't work currently.)

These mostly come from Jukka's suggestions.
Most pass; 4 are currently being skipped:

[case testTypeGuardWithKeywordArgsSwapped-skip]
is_float(b=1, a=a) incorrectly asserts that b is a float

[case testTypeGuardWithStarArgsTuple-skip]
[case testTypeGuardWithStarArgsList-skip]
Horrible things happen with *args

[case testTypeGuardOverload-skip]
A plain bool function is accepted by a higher-order function
(overload) requiring a type guard

[case testTypeGuardMethodOverride-skip]
A plain bool function in a subclass incorrectly is allowed to
override a type guard in the base class
@gvanrossum
Copy link
Member Author

I added tests for all your suggestions (the "overload" test doubles for overloading and higher-order-functions).

This found several problems -- for now I'm just skipping those 5 tests. I'll look into fixing them but I might take your offer of filing issues for some of them.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! Looks good now. Left a few minor comments, feel free to merge once you've addressed them (or decided that they are not worth it).

test-data/unit/check-typeguard.test Outdated Show resolved Hide resolved
cc = filter(is_int, a)
reveal_type(cc) # N: Revealed type is 'typing.Iterator[builtins.int]'
# TODO: Make this work -- this matches the third overload instead of the second
dd = filter(non_tg, a)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you switch the order of the second and third overload items?

mypy/expandtype.py Outdated Show resolved Hide resolved
@gvanrossum
Copy link
Member Author

Thanks, I will merge after the test pass. I hope this can be added to the 0.800 release. (Even if it's not perfect, it shouldn't change anything for users who don't use TypeGuard.)

The main significant issue is that overloads using a type guard as higher-order function don't work right. I'll file a separate issue for that.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 21, 2021

Sorry, this will miss 0.800, but we are hoping to make another release fairly soon after 0.800 that switches to modular typeshed and this will be included

@mortoray
Copy link

This may not be a very commonly needed feature, but in cases where this is useful the current alternatives are all quite bad.

For reference, custom type guards are extremely useful in any testing library as well as any data validation and/or file loading. In those domains type validation functions are frequently used. I think we'll find this feature being used everywhere once available.

I'll be happy when this is available!

JelleZijlstra added a commit that referenced this pull request May 21, 2021
In python/typeshed#5473, I tried to switch a number of `inspect` functions to use the new `TypeGuard` functionality. Unfortunately, mypy-primer found a number of crashes in third-party libraries in places where a TypeGuard function was ANDed together with some other check. Examples:

- https://github.com/sphinx-doc/sphinx/blob/4.x/sphinx/util/inspect.py#L252
- https://github.com/sphinx-doc/sphinx/blob/4.x/sphinx/ext/coverage.py#L212
- https://github.com/streamlit/streamlit/blob/develop/lib/streamlit/elements/doc_string.py#L105

The problems trace back to the decision in #9865 to make TypeGuardType not inherit from ProperType: in various conditions that are more complicated than a simple `if` check, mypy wants everything to become a ProperType. Therefore, to fix the crashes I had to make TypeGuardType a ProperType and support it in various visitors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants