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

Add guide to type narrowing #1798

Merged
merged 8 commits into from
Jul 19, 2024
Merged

Conversation

JelleZijlstra
Copy link
Member

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.

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.
Copy link
Collaborator

@erictraut erictraut 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 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 Show resolved Hide resolved
docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
Comment on lines 26 to 28
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``.
Copy link
Member

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.

Copy link
Member Author

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: 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%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%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. 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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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

docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
JelleZijlstra and others added 3 commits July 14, 2024 15:15
Thanks Alex!

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

@carljm carljm 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 good to me! Thanks for writing it up.

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
Copy link
Member

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"

Suggested change
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

docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
@carljm
Copy link
Member

carljm commented Jul 15, 2024

Oh, it might also be useful to mention __subclasscheck__ and __instancecheck__ in combination with isinstance guards as another source of unsafety in narrowing?

docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
docs/source/type_narrowing.rst Outdated Show resolved Hide resolved
Comment on lines 98 to 105
# 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))

Copy link
Collaborator

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.

Copy link
Collaborator

@srittau srittau left a 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.

@srittau
Copy link
Collaborator

srittau commented Jul 15, 2024

I'd like to get #1802 (and transitively #1801) in first, so this guide can start off at the right URL.

@JelleZijlstra
Copy link
Member Author

@carljm good call, added a section on isinstance().

@srittau
Copy link
Collaborator

srittau commented Jul 15, 2024

Please move to the guides directory when resolving conflicts!

@JelleZijlstra JelleZijlstra merged commit 28fa515 into python:main Jul 19, 2024
5 checks passed
@JelleZijlstra JelleZijlstra deleted the typenarrow branch July 19, 2024 03:56
@JelleZijlstra JelleZijlstra restored the typenarrow branch September 10, 2024 23:39
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.

6 participants