[ty] improve lazy scope place lookup #19321
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
Contributor
|
mtshiba
force-pushed
the
lazy-scope-narrowing
branch
from
96967f3 to
6fb1747
Compare
mtshiba
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Contributor
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
possibly-unbound-attribute |
0 | 123 | 27 |
unresolved-attribute |
101 | 2 | 6 |
call-non-callable |
0 | 47 | 0 |
invalid-argument-type |
0 | 20 | 13 |
unsupported-operator |
0 | 5 | 20 |
unused-ignore-comment |
3 | 0 | 0 |
invalid-assignment |
0 | 2 | 0 |
invalid-return-type |
0 | 2 | 0 |
missing-argument |
0 | 1 | 0 |
no-matching-overload |
0 | 1 | 0 |
redundant-cast |
1 | 0 | 0 |
too-many-positional-arguments |
1 | 0 | 0 |
unresolved-reference |
0 | 1 | 0 |
| Total | 106 | 204 | 66 |
Contributor
Author
|
The changes in this PR seem to generate new false positive warnings like this: def f(cond: bool):
if cond:
foo = 1
def g():
if cond:
# warning: [possibly-unresolved-reference]
fooThis may be a separate work, but it shows the need for a "proper" boundness analysis of nonlocal symbols (we currently assume the boundness of |
MichaReiser
reviewed
Jul 14, 2025
MichaReiser
reviewed
Jul 14, 2025
sharkdp
approved these changes
Jul 16, 2025
Contributor
sharkdp
left a comment
sharkdp
left a comment
There was a problem hiding this comment.
This looks really great, thank you!
I would appreciate if someone with a better understanding of lazy/eager scopes could review the semantic index changes here.
Did you look more closely at the ecosystem impact? I understand that there might be quite a few new (true and false) positives because we infer a less-permissive type in many cases now (since we don't union with Unknown anymore), which probably adds a lot of noise to the ecosystem results. It might still be worth going over some of them to see if there are any unexpected things popping up.
crates/ty_python_semantic/resources/mdtest/narrow/conditionals/nested.md
Outdated
Show resolved
Hide resolved
…/nested.md Co-authored-by: David Peter <sharkdp@users.noreply.github.com>
mtshiba
force-pushed
the
lazy-scope-narrowing
branch
from
6ac318c to
508823f
Compare
Contributor
Author
A typical error added by this PR is something like Other undesirable errors (warnings) are caused by the lack of proper boundness analysis of nonlocal symbols, as mentioned above. One thing that bothers me is that the ecosystem-analyzer results report a |
Contributor
The mypy-primer comment is truncated if it becomes too long. This new error does show up in mypy-primer, at https://github.com/astral-sh/ruff/actions/runs/16325751556/job/46115311715?pr=19321#step:6:1477
I think assuming a nonlocal symbol (accessed from a lazy nested scope) is always bound is probably the right thing to do, at least for now. If we do that, it shouldn't cause false positive
This should just be replacing a previous |
mtshiba
force-pushed
the
lazy-scope-narrowing
branch
from
028325e to
fc11d9c
Compare
Contributor
Author
I see. I checked and it seems that the new error correctly captures the mistake in the code.
Okay. For now, I've reverted the behavior to treat all nonlocal symbols as bound.
Yes, I checked that all the newly added |
Co-authored-by: Carl Meyer <carl@oddbird.net>
mtshiba
force-pushed
the
lazy-scope-narrowing
branch
from
4d72a9c to
34c21c4
Compare
… the presence of attribute or subscript assignments
carljm
approved these changes
Jul 25, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This looks good to me. I think as a follow-up we will want to further improve this so that it looks for reassignments after the definition of the nested scope rather than any reassignments at all. Then we would be able to handle the case described in astral-sh/ty#559
Also, @oconnor663 is working on some changes to the semantic index so that it calculates target scopes for nonlocal references (as a fixup pass); I think that may allow simplifying some of the code here. (We decided to do this for now instead of the bigger change to deferred walking of nested scopes.)
I would like to leave it up to @MichaReiser whether to merge this before or after he merges #19497, since that's a much bigger PR and more prone to accumulating merge conflicts.
crates/ty_python_semantic/resources/mdtest/narrow/conditionals/nested.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/narrow/conditionals/nested.md
Outdated
Show resolved
Hide resolved
Member
|
Either way is fine with me. The change in |
Member
|
I'll go ahead and merge this. |
AlexWaygood
pushed a commit
that referenced
this pull request
Jul 25, 2025
Co-authored-by: David Peter <sharkdp@users.noreply.github.com> Co-authored-by: Carl Meyer <carl@oddbird.net> Co-authored-by: Micha Reiser <micha@reiser.io>
oconnor663
reviewed
Aug 1, 2025
| let key = EnclosingSnapshotKey { | ||
| enclosing_scope, | ||
| enclosing_place: place_id, | ||
| nested_scope, |
Contributor
There was a problem hiding this comment.
Was this maybe intended to be nested_scope: ancestor_scope_id instead? I think as written the loop body does the same lookup repeatedly, instead of doing a different lookup for each ancestor, but I don't understand this code well enough yet to know what sort of test case would demonstrate the difference.
Contributor
Author
There was a problem hiding this comment.
Consider the following situation: there is a nested scope (child scope) and a further nested scope (grandchild scope) within an enclosing scope. If both scopes (child/grandchild) are lazy scopes, the snapshots of the enclosing scope's state for the two scopes should be exactly the same (or, if narrowing is performed within the child scope, the snapshot of the enclosing scope seen by the grandchild will be more detailed). Therefore, the snapshot only needs to be taken once in the innermost lazy scope. In other words, the operation to obtain the snapshot can be performed outside the loop, but since it is not a particularly expensive operation, leaving it in the loop should not cause any significant performance issues.
Contributor
Author
There was a problem hiding this comment.
The need for the loop here is because if no snapshot is found and there is a lazy scope between the enclosing scope and the nested scope, EnclosingSnapshotResult::NoLongerInEagerContext must be returned.
Contributor
There was a problem hiding this comment.
Gotcha, thanks for clarifying.
This was referenced Aug 4, 2025
carljm
pushed a commit
that referenced
this pull request
Aug 5, 2025
) ## Summary This is a follow-up to #19321. If we try to access a class variable before it is defined, the variable is looked up in the global scope, rather than in any enclosing scopes. Closes astral-sh/ty#875. ## Test Plan New tests in `narrow/conditionals/nested.md`.
carljm
pushed a commit
that referenced
this pull request
Aug 5, 2025
## Summary This is a follow-up to #19321. Narrowing constraints introduced in a class scope were not applied even when they can be applied in lazy nested scopes. This PR fixes so that they are now applied. Conversely, there were cases where narrowing constraints were being applied in places where they should not, so it is also fixed. ## Test Plan Some TODOs in `narrow/conditionals/nested.md` are now work correctly.
carljm
added a commit
that referenced
this pull request
Sep 8, 2025
## Summary This is a follow-up to #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 <carl@astral.sh>
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 <carl@astral.sh>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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 PR improves place (symbol) lookup in lazy scopes.
This PR has two main changes:
Unknown([ty] Infer nonlocal types as unions of all reachable bindings #18750), but if the defined scope is private (e.g., function scope), we can excludeUnknown(Unknownis only necessary when the symbol is writable from the outside), so we will do so.closes astral-sh/ty#259
closes astral-sh/ty#659
Test Plan
New tests in
narrow/conditionals/nested.md.