[ty] make `del x` force local resolution of `x` in the current scope #19389

oconnor663 · 2025-07-16T18:22:49Z

Updated: The preferred approach here is to keep the SemanticIndex simple (del of any name marks that name "bound" in the current scope) and to move complexity to type inference (free variable resolution stops when it finds a binding, unless that binding is declared nonlocal). As part of this change, free variable resolution will now union the types it finds as it walks in enclosing scopes. This approach is still incomplete, because it doesn't consider inner scopes or sibling scopes, but it improves the common case.

crates/ty_python_semantic/resources/mdtest/del.md

github-actions · 2025-07-16T18:27:10Z

`mypy_primer` results

No ecosystem changes detected ✅
No memory usage changes detected ✅

github-actions · 2025-07-16T18:31:59Z

`ecosystem-analyzer` results

No changes detected ✅
Full report with detailed diff

carljm

Thanks! I think this is a behavior improvement and I'm ok with landing it as is, but it seems to me that our approach to nonlocal might not quite be fully there yet (discussed inline).

crates/ty_python_semantic/src/semantic_index/builder.rs

crates/ty_python_semantic/resources/mdtest/del.md

crates/ty_python_semantic/src/types/infer.rs

Fixes astral-sh/ty#769. The preferred approach here is to keep the SemanticIndex simple (`del` of any name marks that name "bound" in the current scope) and to move complexity to type inference (free variable resolution stops when it finds a binding, unless that binding is declared `nonlocal`). As part of this change, free variable resolution will now union the types it finds as it walks in enclosing scopes. This approach is still incomplete, because it doesn't consider inner scopes or sibling scopes, but it improves the common case.

oconnor663 · 2025-07-17T23:17:21Z

Ok @carljm, I've reworked this PR and updated the description up top. As you suggested, I've moved the nonlocal + del case into infer.rs, to keep the complexity out of the Semantic Index. I've also added a UnionBuilder to the scope walking loop, along with some new test cases for that. It...seems to work :-D

oconnor663 · 2025-07-17T23:18:17Z

crates/ty_python_semantic/src/types/infer.rs

+                        // doesn't mark it `nonlocal`. The name is resolved, and we won't consider
+                        // any scopes outside of this one.
+                        return if let Some(type_) = nonlocal_union_builder.try_build() {
+                            Place::Type(type_, local_boundness).into()


I'm fuzzy on the difference between Boundness::PossiblyUnbound and Place::Unbound, so please look extra close at what I'm doing here with local_boundness and tell me if this makes sense.

oconnor663 · 2025-07-17T23:42:39Z

The updated ecosystem analysis has 4 new diagnostics that I think are ~~false positives~~ (update: maybe not?) to do with function parameters in an enclosing scope, so I'm pretty sure I've screwed something up. Will track it down and add another test case.

Update: Ok, I think this is a minimized version of the new failures:

def f():
    x: int = 1
    def g():
        if not isinstance(x, int):
            print(x)  # [unresolved-reference]: Name `x` used when not defined <-- only on this PR, not main

I think what's happening is that my new type unioning, maybe plus the way narrow_place_with_applicable_constraints is used in the same loop, leads to the conclusion that the type is...I'm not sure...uninhabited? It's not obvious to me that this is wrong, but I don't understand it very well. Notably, there are no diagnostics for this simpler example:

x: int = 1
if not isinstance(x, int):
    print(x)

oconnor663 · 2025-07-18T18:16:20Z

Ok, I've added a bool to distinguish "narrowing constraints result in Never" from "we never encountered any definitions, so UnionBuilder::build returns Never". The ecosystem report is now clean. This PR has Carl's green checkmark from before that fix, but I'd love to get eyes from at least one other reviewer who's familiar with this unioning / narrowing / boundness machinery. Does it look like I'm doing the right thing here? Am I missing any useful helpers or duplicating anything that I shouldn't be? Etc.

ruff/crates/ty_python_semantic/src/types/infer.rs

Lines 6117 to 6142 in 8db7db3

    
           if !enclosing_place.is_marked_nonlocal() { 
        
               // We've reached a function-like scope that marks this name bound or 
        
               // declared but doesn't mark it `nonlocal`. The name is therefore resolved, 
        
               // and we won't consider any scopes outside of this one. 
        
               // 
        
               // A corner case we need to consider here is the `del` keyword, which 
        
               // marks a name bound in its scope (shadowing enclosing scopes if the name 
        
               // isn't `nonlocal`) but doesn't actually provide a binding, e.g.: 
        
               // ```py 
        
               // x = 1 
        
               // def foo(): 
        
               //     print(x)  # error: [unresolved-reference] "Name `x` used when not defined" 
        
               //     if False: 
        
               //         del x 
        
               // ``` 
        
               // In that case we haven't added any types to the `UnionBuilder`, and 
        
               // `.build()` would return `Type::Never`. But we need to distinguish that 
        
               // from other cases that lead to `Type::Never`, like `if` conditions with 
        
               // unsatisfiable type constraints, and we don't want to report 
        
               // unbound/unresolved errors on those cases. The `found_some_definition` 
        
               // flag keeps track of this. 
        
               return if found_some_definition { 
        
                   Place::Type(nonlocal_union_builder.build(), local_boundness).into() 
        
               } else { 
        
                   Place::Unbound.into() 
        
               };

carljm · 2025-07-18T20:34:00Z

crates/ty_python_semantic/resources/mdtest/scopes/nonlocal.md

        x = "hello"  # error: [invalid-assignment] "Object of type `Literal["hello"]` is not assignable to `int`"
 ```

+## The types of `nonlocal` binding get unioned


These tests are great! Very clear.

* main: [ty] Avoid secondary tree traversal to get call expression for keyword arguments (astral-sh#19429) [ty] Add goto definition to playground (astral-sh#19425) [ty] Add support for `@warnings.deprecated` (astral-sh#19376) [ty] make `del x` force local resolution of `x` in the current scope (astral-sh#19389) # Conflicts: # crates/ty_ide/src/goto.rs

* main: (25 commits) [ty] Sync vendored typeshed stubs (#19461) [ty] Extend tuple `__len__` and `__bool__` special casing to also cover tuple subclasses (#19289) [ty] bump docstring-adder pin (#19458) [ty] Disallow assignment to `Final` class attributes (#19457) Update dependency ruff to v0.12.4 (#19442) Update pre-commit hook astral-sh/ruff-pre-commit to v0.12.4 (#19443) Update rui314/setup-mold digest to 702b190 (#19441) Update taiki-e/install-action action to v2.56.19 (#19448) Update Rust crate strum_macros to v0.27.2 (#19447) Update Rust crate strum to v0.27.2 (#19446) Update Rust crate rand to v0.9.2 (#19444) Update Rust crate serde_json to v1.0.141 (#19445) Fix `unreachable` panic in parser (#19183) [`ruff`] Support byte strings (`RUF055`) (#18926) [ty] Avoid second lookup for `infer_maybe_standalone_expression` (#19439) [ty] Implemented "go to definition" support for import statements (#19428) [ty] Avoid secondary tree traversal to get call expression for keyword arguments (#19429) [ty] Add goto definition to playground (#19425) [ty] Add support for `@warnings.deprecated` (#19376) [ty] make `del x` force local resolution of `x` in the current scope (#19389) ...

oconnor663 requested review from AlexWaygood, carljm, dcreager and sharkdp as code owners July 16, 2025 18:22

oconnor663 added the ty Multi-file analysis & type inference label Jul 16, 2025

oconnor663 commented Jul 16, 2025

View reviewed changes

crates/ty_python_semantic/resources/mdtest/del.md Outdated Show resolved Hide resolved

oconnor663 added the ecosystem-analyzer label Jul 16, 2025

carljm approved these changes Jul 16, 2025

View reviewed changes

crates/ty_python_semantic/src/semantic_index/builder.rs Outdated Show resolved Hide resolved

crates/ty_python_semantic/resources/mdtest/del.md Outdated Show resolved Hide resolved

crates/ty_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved

oconnor663 force-pushed the del_mark_bound branch from e9cac19 to 14f5744 Compare July 17, 2025 23:11

oconnor663 force-pushed the del_mark_bound branch from 14f5744 to 8a926bb Compare July 17, 2025 23:13

oconnor663 commented Jul 17, 2025

View reviewed changes

oconnor663 added ecosystem-analyzer and removed ecosystem-analyzer labels Jul 17, 2025

remove a redundant test case

b7a322e

sharkdp removed their request for review July 18, 2025 07:55

distinguish "narrowed to Never" from "we never encountered a definition"

8db7db3

oconnor663 added ecosystem-analyzer and removed ecosystem-analyzer labels Jul 18, 2025

carljm added 2 commits July 18, 2025 14:37

a few adjustments

d5ecd27

another comment tweak

b2503ec

carljm approved these changes Jul 18, 2025

View reviewed changes

carljm merged commit e9a64e5 into main Jul 18, 2025
37 checks passed

carljm deleted the del_mark_bound branch July 18, 2025 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ty] make `del x` force local resolution of `x` in the current scope #19389

[ty] make `del x` force local resolution of `x` in the current scope #19389

Uh oh!

oconnor663 commented Jul 16, 2025 •

edited

Loading

Uh oh!

Uh oh!

github-actions bot commented Jul 16, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jul 16, 2025 •

edited

Loading

Uh oh!

carljm left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oconnor663 commented Jul 17, 2025

Uh oh!

oconnor663 Jul 17, 2025

Uh oh!

oconnor663 commented Jul 17, 2025 •

edited

Loading

Uh oh!

oconnor663 commented Jul 18, 2025 •

edited

Loading

Uh oh!

carljm Jul 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[ty] make del x force local resolution of x in the current scope #19389

[ty] make del x force local resolution of x in the current scope #19389

Uh oh!

Conversation

oconnor663 commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

github-actions bot commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ecosystem-analyzer results

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oconnor663 commented Jul 17, 2025

Uh oh!

oconnor663 Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

oconnor663 commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oconnor663 commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carljm Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[ty] make `del x` force local resolution of `x` in the current scope #19389

[ty] make `del x` force local resolution of `x` in the current scope #19389

oconnor663 commented Jul 16, 2025 •

edited

Loading

github-actions bot commented Jul 16, 2025 •

edited

Loading

`mypy_primer` results

github-actions bot commented Jul 16, 2025 •

edited

Loading

`ecosystem-analyzer` results

oconnor663 commented Jul 17, 2025 •

edited

Loading

oconnor663 commented Jul 18, 2025 •

edited

Loading