[ty] Fix nested global and nonlocal lookups through forwarding scopes#24279
[ty] Fix nested global and nonlocal lookups through forwarding scopes#24279charliermarsh merged 3 commits intomainfrom
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 86.61%. The percentage of expected errors that received a diagnostic held steady at 81.56%. The number of fully passing files held steady at 70/132. |
Memory usage reportMemory usage unchanged ✅ |
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-await |
40 | 0 | 0 |
invalid-return-type |
1 | 0 | 0 |
| Total | 41 | 0 | 0 |
Changes in flaky projects detected. Raw diff output excludes flaky projects; see the HTML report for details.
|
(I haven't worked on the bindings stuff before; spent a decent chunk of time trying to understand this but apologies if I'm way off here.) |
2ac5133 to
9931a8c
Compare
| A nested function should still resolve a name in the module scope if the enclosing function marks it | ||
| `global`, even if that function also has a conditional assignment to the name: | ||
|
|
||
| ```py | ||
| PULL_SUMS: list[float] = [] | ||
|
|
||
| def bandit(foo: int) -> None: | ||
| global PULL_SUMS | ||
|
|
||
| if foo == 0: | ||
| PULL_SUMS = [] | ||
| return | ||
|
|
||
| def bar(arm: int) -> None: | ||
| PULL_SUMS[arm] | ||
| ``` | ||
|
|
||
| A simpler scalar case should also keep falling back to the module global: | ||
|
|
||
| ```py | ||
| x: int = 1 | ||
|
|
||
| def outer(flag: bool) -> None: | ||
| global x | ||
|
|
||
| if flag: | ||
| x = 2 | ||
| return | ||
|
|
||
| def inner() -> None: | ||
| y: int = x | ||
| ``` |
There was a problem hiding this comment.
Not a big deal, but I don't think there's value in including both of these tests. I would include only the latter one, since it's simpler. The issue here clearly has to do with simply resolving the name to the right scope, not to do with the particular type of the name.
| let is_forwarding_symbol = enclosing_place_expr | ||
| .as_symbol() | ||
| .is_some_and(|symbol| symbol.is_global() || symbol.is_nonlocal()); | ||
| let stores_visible_bindings = !is_forwarding_symbol |
There was a problem hiding this comment.
I find the name stores_visible_bindings confusing here, because we will set it to true for any symbol that isn't a forwarding symbol, regardless of what bindings it stores.
I think this would be clearer if we removed the !is_forwarding_symbol component here (so stores_visible_bindings really is just what it says), and then added that clause down below in the if statement (so replace !stores_visible_bindings below with (is_forwarding_symbol && !stores_visible_bindings))
There was a problem hiding this comment.
I'm considering rewriting this block so that we don't have to compute these eagerly (since they'll often be redundant given the boolean logic)...
There was a problem hiding this comment.
LLVM may just optimize this away though.
9931a8c to
e31dd7d
Compare
* main: (35 commits) Store definition indexes as u32 (#24307) Avoid re-using symbol in RUF024 fix (#24316) [ty] Add materialization to `Divergent` type (#24255) [ty] Make `Divergent` a top-level type variant (#24252) [ty] Fix nested global and nonlocal lookups through forwarding scopes (#24279) Fetch the cargo-dist binary directly instead of using the installer (#24258) [ty] Fix panic on `list[Annotated[()]]` (#24303) Don't measure the AST deallocation time in parser benchmarks (#24301) Enable CodSpeed's memory benchmarks for simulation benchmarks (#24298) Upgrade imara-diff to 0.2.0 (#24299) [ty] Represent `InitVar` as a special form internally, not a class (#24248) `RUF067`: Allow dunder-named assignments in non-strict mode [`pyupgrade`] UP018 should detect more unnecessarily wrapped literals (UP018) (#24093) [ty] Remove unused `system.glob` method (#24300) [ty] Reject functional TypedDict with mismatched name (#24295) Update Rust crate arc-swap to v1.9.0 (#24292) [ty] Remove unused `@Todo(Functional TypedDicts)` (#24297) Update CodSpeedHQ/action action to v4.12.1 (#24290) Update taiki-e/install-action action to v2.69.6 (#24293) Update Rust crate toml to v1.0.7 (#24289) ...
Summary
Given the motivating example:
Before this change, in
bar, we'd first look at thebanditscope, but becausebandithadPULL_SUMS = [], we stopped walking outward to the module scope, and never saw the top-levelPULL_SUMS: "list[float]" = [].Now, if the binding in the scope is just an unbound placeholder for a
nonlocalorglobal, we keep walking outwards.Closes astral-sh/ty#3157.