Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add guide to type narrowing #1798
Changes from 1 commit
0a0cc10
56bb86d
22999d3
4892b5e
1a970ff
71d0497
c89d178
596f4fc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
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)
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
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"