-
Notifications
You must be signed in to change notification settings - Fork 258
Add guide to type narrowing #1798
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
Conversation
Some of the text derives from the "How to teach this" section of PEP 742. The asyncio example is based on a sample written by Liz King in https://discuss.python.org/t/problems-with-typeis/55410.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this! Looks good overall. I added a couple of minor suggestions. Feel free to use or ignore as you see fit.
docs/source/type_narrowing.rst
Outdated
In addition to narrowing local variables, type checkers usually also support | ||
narrowing instance attributes and sequence members, such as | ||
``if x.some_attribute is not None`` or ``if x[0] is not None``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably not mention this. I believe there are many situations where other type checkers will happily understand an instance attribute or sequence/dictionary lookup as being narrowed, but Pyre will not, because it cannot safely determine that the stored value has not been reset to a new value by some other function in between two accesses of that value. Moreover, I believe even the more limited narrowing that Pyre allows is not strictly type-safe if you have multithreaded code.
In the name of practicality, I personally prefer the approach mypy and pyright have taken here. But it's an area in which type checkers differ, and mypy's/pyright's behaviour isn't strictly safe. So I think it's perhaps better to leave it unsaid here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this shouldn't be mentioned. It's something users commonly need and ask for. While it is somewhat more obviously unsafe than some other narrowing patterns, there's few kinds of type narrowing that are 100% safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pyre seems to do type narrowing here too:
Correct, but if you insert a single intermediate function call, it decides that it can no longer assume that the attribute wasn't modified in between the two accesses by the intermediate function call, and no longer does the type narrowing: https://pyre-check.org/play?input=%23%20pyre-strict%0A%0Afrom%20typing%20import%20*%0A%0Afrom%20dataclasses%20import%20dataclass%0A%0A%40dataclass%0Aclass%20X%3A%0A%20%20%20%20a%3A%20int%20%7C%20None%0A%0Adef%20do_something_else()%20-%3E%20None%3A%0A%20%20%20%20pass%0A%0Adef%20f(x%3A%20X)%20-%3E%20None%3A%0A%20%20%20%20if%20x.a%20is%20not%20None%3A%0A%20%20%20%20%20%20%20%20do_something_else()%0A%20%20%20%20%20%20%20%20reveal_type(x.a)%0A%20%20%20%20%20%20%20%20print(x.a%20%2B%201)%0A%20%20%20%20print(x.a%20%2B%201)
I'm not sure why this shouldn't be mentioned.
Well, I just worry about documenting behaviour here that isn't standardised between all major type checkers. It might be pretty confusing for users of type checkers with differing behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text is already pretty clear that the exact type narrowing behaviors differ among type checkers, but I'll add another line to clarify that.
I think it's important to mention this behavior because it's a common pattern in practice, so people are likely to run into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think the extra line definitely helps
Thanks Alex! Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Thanks for writing it up.
docs/source/type_narrowing.rst
Outdated
are useful if you want to reuse a more complicated check in multiple places, or | ||
you use a check that the type checker doesn't understand. In these cases, you | ||
can define a ``TypeIs`` or ``TypeGuard`` function to perform the check and allow type checkers | ||
to use it to narrow the type of a variable. Among the two, ``TypeIs`` usually |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for only two items, I would usually prefer "Between" over "Among"
to use it to narrow the type of a variable. Among the two, ``TypeIs`` usually | |
to use it to narrow the type of a variable. Between the two, ``TypeIs`` usually |
Oh, it might also be useful to mention |
docs/source/type_narrowing.rst
Outdated
# Incorrect: does not return True for all ints | ||
def is_positive_int(x: object) -> TypeIs[int]: | ||
return isinstance(x, int) and x > 0 | ||
|
||
# Incorrect: returns True for some non-ints | ||
def is_real_number(x: object) -> TypeIs[int]: | ||
return isinstance(x, (int, float)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two examples would also be good to have in the TypeIs
documentation at docs.python.org. Also, the TypeIs
and TypeGuard
docs could link to this document when it gets merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with or without the changes suggested by others.
@carljm good call, added a section on |
Please move to the guides directory when resolving conflicts! |
Some of the text derives from the "How to teach this" section of PEP 742.
The asyncio example is based on a sample written by Liz King in
https://discuss.python.org/t/problems-with-typeis/55410.