[red-knot] fix narrowing in nested scopes#17630
Conversation
|
carljm
left a comment
There was a problem hiding this comment.
Thank you for this PR! This is a very valuable feature, and it is clearly impactful in the ecosystem checks -- from what I can tell, it removes a lot of false positives.
I do have some questions about the implementation; it feels very "layered on top", as if it tries to only add new code and avoid touching existing code, and it seems like this results in some avoidable duplication (both of logic, and of real runtime work).
crates/red_knot_python_semantic/resources/mdtest/narrow/conditionals/nested.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/narrow/conditionals/nested.md
Outdated
Show resolved
Hide resolved
866e55f to
13d58d2
Compare
CodSpeed Performance ReportMerging #17630 will not alter performanceComparing Summary
|
carljm
left a comment
There was a problem hiding this comment.
Looking good! It still seems to me like we are doing more work than necessary here, and if we do less we may be able to reduce the regression.
crates/red_knot_python_semantic/resources/mdtest/narrow/conditionals/nested.md
Show resolved
Hide resolved
crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs
Outdated
Show resolved
Hide resolved
3243b02 to
dda49b6
Compare
|
@mtshiba I rebased your PR passed the Red Knot renaming. Make sure to pull before making new commits. |
…use` have a value only at a class scope
|
Thank you, this looks great! I thought of one bug while reviewing, but I went ahead and just fixed it, along with some other cleanups. (The bug was that in a class scope, although we do need to preserve the narrowing constraints that applied to "unbound", even if "unbound" becomes not-visible, new narrowing checks after unbound becomes not-visible do not apply, as they aren't narrowing the outer-scope value we will see in nested scopes, they are narrowing only the local bindings we don't see.) After rebasing on top of the |
This PR fixes astral-sh#17595. New test cases are added to `mdtest/narrow/conditionals/nested.md`. --------- Co-authored-by: Carl Meyer <[email protected]>
Summary
This PR fixes #17595.
Test Plan
New test cases are added to
mdtest/narrow/conditionals/nested.md.