-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[ty] basic narrowing on attribute and subscript expressions #17643
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
|
a9ee6ed
to
cfa79a2
Compare
Thank you for working on this!
I think it's not just a matter of additional narrowing. It looks to me like, on this branch, we can infer incorrect types by not considering attribute assignments: from typing import reveal_type
class C:
x: int | None = None
c = C()
if c.x is None:
c.x = 1
reveal_type(c.x) # this branch: None I'm afraid this is something that we need to fix before we can consider merging this. It also leads to some new false positives in the ecosystem checks. Please let me know if you need any help with this. |
cfa79a2
to
9db9728
Compare
This comment was marked as outdated.
This comment was marked as outdated.
## Summary This PR partially solves astral-sh/ty#164 (derived from #17643). Currently, the definitions we manage are limited to those for simple name (symbol) targets, but we expand this to track definitions for attribute and subscript targets as well. This was originally planned as part of the work in #17643, but the changes are significant, so I made it a separate PR. After merging this PR, I will reflect this changes in #17643. There is still some incomplete work remaining, but the basic features have been implemented, so I am publishing it as a draft PR. Here is the TODO list (there may be more to come): * [x] Complete rewrite and refactoring of documentation (removing `Symbol` and replacing it with `Place`) * [x] More thorough testing * [x] Consolidation of duplicated code (maybe we can consolidate the handling related to name, attribute, and subscript) This PR replaces the current `Symbol` API with the `Place` API, which is a concept that includes attributes and subscripts (the term is borrowed from Rust). ## Test Plan `mdtest/narrow/assignment.md` is added. --------- Co-authored-by: David Peter <[email protected]> Co-authored-by: Carl Meyer <[email protected]>
7417ba6
to
0c8996c
Compare
The https://github.com/tornadoweb/tornado/blob/master/tornado/iostream.py#L947-L984 Upon investigation, it appears that this is in fact a known bug. I minimized the repro code and got: while True:
...
if not size:
break
try:
...
self._total_write_done_index += num_bytes
except BlockingIOError:
break
...
if index > self._total_write_done_index:
... I think this is another example of astral-sh/ty#365. |
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.
Thank you very much — this looks great.
Unfortunately, I think I found some cases that are not yet handled correctly when we combine assignments to attributes/objects with narrowing.
crates/ty_python_semantic/resources/mdtest/narrow/complex_target.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/narrow/complex_target.md
Outdated
Show resolved
Hide resolved
…et.md Co-authored-by: David Peter <[email protected]>
…n the assignments
I can take care of fixing the merge conflicts 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.
Thank you very much!
Summary
This PR closes astral-sh/ty#164.
This PR introduces a basic type narrowing mechanism for attribute/subscript expressions.
Member accesses, int literal subscripts, string literal subscripts are supported (same as mypy and pyright).
Advanced narrowing mechanisms, such as the following, are not implemented in this PR (and will be future works):
narrowing by overwriting assignments[ty] type narrowing by attribute/subscript assignments #18041Test Plan
New test cases are added to
mdtest/narrow/complex_target.md
.