[ty] more precise lazy scope place lookup #19932
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mtshiba
requested review from
carljm,
AlexWaygood,
sharkdp and
dcreager
as code owners
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
CodSpeed Instrumentation Performance ReportMerging #19932 will not alter performanceComparing Summary
|
CodSpeed WallTime Performance ReportMerging #19932 will not alter performanceComparing Summary
|
…hether to keep the lazy snapshot
mtshiba
force-pushed
the
lazy-scope-place-lookup2
branch
from
f785733 to
69f82c7
Compare
mtshiba
force-pushed
the
lazy-scope-place-lookup2
branch
from
69f82c7 to
4225c28
Compare
mtshiba
force-pushed
the
lazy-scope-place-lookup2
branch
from
04f57df to
5a07823
Compare
mtshiba
commented
Aug 25, 2025
mtshiba
commented
Aug 25, 2025
mtshiba
commented
Aug 25, 2025
carljm
reviewed
Aug 25, 2025
carljm
requested changes
Sep 3, 2025
There was a problem hiding this comment.
Thanks for your work on this, and sorry it took me a while to complete review! I think this is an important issue to solve, and this definitely improves the behavior. That said, I'm not convinced that the approach taken is really correct. I commented inline on the specific problematic point. Currently this PR marks snapshots as "incomplete" and then later tries to figure out how to "merge" that incomplete snapshot with all-reachable-bindings. This makes it quite difficult to figure out which of the "all-reachable" bindings should be considered vs ignored.
I think a more robust approach here might be to actually update the snapshot with new bindings, as we encounter new bindings after the snapshot was created.
crates/ty_python_semantic/src/semantic_index/use_def/place_state.rs
Outdated
Show resolved
Hide resolved
mtshiba
force-pushed
the
lazy-scope-place-lookup2
branch
from
81dbc63 to
de6e9db
Compare
mtshiba
force-pushed
the
lazy-scope-place-lookup2
branch
from
48eaf75 to
3fc81ab
Compare
second-ed
pushed a commit
to second-ed/ruff
that referenced
this pull request
Sep 9, 2025
## Summary This is a follow-up to astral-sh#19321. Now lazy snapshots are updated to take into account new bindings on every symbol reassignment. ```python def outer(x: A | None): if x is None: x = A() reveal_type(x) # revealed: A def inner() -> None: # lazy snapshot: {x: A} reveal_type(x) # revealed: A inner() def outer() -> None: x = None x = 1 def inner() -> None: # lazy snapshot: {x: Literal[1]} -> {x: Literal[1, 2]} reveal_type(x) # revealed: Literal[1, 2] inner() x = 2 ``` Closes astral-sh/ty#559. ## Test Plan Some TODOs in `public_types.md` now work properly. --------- Co-authored-by: Carl Meyer <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This is a follow-up to #19321.
This PR introduces the concept of "completeness" toEnclosingSnapshot, making lookup of nonlocal symbols more precise.Here's the new mechanism we'll add: bindings that appear before the lazy nested scope can be analyzed exactly for reachability and narrowing. We record them in enclosing snapshots, and if bindings appear after the lazy nested scope, instead of sweeping the snapshot, we mark it as "incomplete." Then, at load time, we analyze bindings before the nested scope exactly, and always treat bindings after the nested scope as bound.Now lazy snapshots are updated to take into account new bindings on every symbol reassignment.
Closes astral-sh/ty#559.
Test Plan
Some TODOs in
public_types.mdnow work properly.