Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions crates/ty_python_semantic/resources/mdtest/loops/while_loop.md
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,34 @@ while random():
x
```

### Statically unreachable `del` branches don't poison cyclic loopback

This creates a loop-header cycle through `if x`, but the `del x` branch should still disappear once
the loopback type settles:

```py
def random() -> bool:
return False

x = 1
while random():
if x:
x = 1
else:
del x
reveal_type(x) # revealed: Literal[1]
```

Comparison-guarded `del` branches should also disappear once the loopback type settles:

```py
x = 1
while x < 10:
if x == 4:
del x
reveal_type(x) # revealed: Literal[1]
```

### Bindings in a loop are possibly-unbound after the loop

```py
Expand Down Expand Up @@ -441,6 +469,43 @@ while random():
reveal_type(y) # revealed: Literal[1, 2]
```

### Monotonic widening can keep stale loopback bindings reachable

```py
def random() -> bool:
return False

x = 0
while random():
# TODO: This should reveal `Literal[0]`.
reveal_type(x) # revealed: Literal[0, 2]
if x == 1:
x = 2
```

### Conditional unpacking and loop exits converge normally

This reduced example from issue #3057 used to panic with "too many cycle iterations":

```py
def fetch(req) -> tuple:
return (True, None)

def paginate():
bookmark = None
while True:
if bookmark is None:
req = None
else:
req = bookmark
ok, next_bookmark = fetch(req)
if not ok:
return
bookmark = next_bookmark
if bookmark is None or bookmark == 0:
break
```

### Loop bodies that are guaranteed to execute at least once

TODO: We should be able to see when a loop body is guaranteed to execute at least once. However,
Expand Down
8 changes: 1 addition & 7 deletions crates/ty_python_semantic/src/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1181,7 +1181,6 @@ fn loop_header_reachability_impl<'db>(
let loop_header = get_loop_header(db, loop_header_definition.loop_token());
let place = loop_header_definition.place();

let mut has_defined_bindings = false;
let mut deleted_reachability = Truthiness::AlwaysFalse;
let mut reachable_bindings = FxIndexSet::default();

Expand All @@ -1202,7 +1201,6 @@ fn loop_header_reachability_impl<'db>(
def, definition,
"loop headers only include bindings from within the loop"
);
has_defined_bindings = true;
reachable_bindings.insert(ReachableLoopBinding {
definition: def,
narrowing_constraint: live_binding.narrowing_constraint,
Expand All @@ -1221,7 +1219,6 @@ fn loop_header_reachability_impl<'db>(
}

LoopHeaderReachability {
has_defined_bindings,
deleted_reachability,
reachable_bindings,
}
Expand All @@ -1230,8 +1227,6 @@ fn loop_header_reachability_impl<'db>(
/// Result of [`loop_header_reachability`]: pre-computed reachability info for loop-back bindings.
#[derive(Debug, Clone, PartialEq, Eq, salsa::Update, get_size2::GetSize)]
pub(crate) struct LoopHeaderReachability<'db> {
/// Whether any reachable loop-back binding is a defined binding.
pub(crate) has_defined_bindings: bool,
pub(crate) deleted_reachability: Truthiness,
/// Reachable loop-back bindings that are not `del`s.
pub(crate) reachable_bindings: FxIndexSet<ReachableLoopBinding<'db>>,
Expand All @@ -1247,7 +1242,6 @@ impl<'db> LoopHeaderReachability<'db> {
reachable_bindings.extend(self.reachable_bindings);

LoopHeaderReachability {
has_defined_bindings: self.has_defined_bindings,
deleted_reachability: self.deleted_reachability,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it would be equivalent to change this to self.has_defined_bindings || previous.has_defined_bindings? Do we also need to update deleted_reachability here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't want to do that because it would over-estimate unboundness, though I can't come up with a test impacted by changing that.

Copy link
Copy Markdown
Member

@ibraheemdev ibraheemdev Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this diff:

- deleted_reachability: self.deleted_reachability,
+ deleted_reachability: self.deleted_reachability.or(previous.deleted_reachability),

The following example emits a warning:

x = 1
# warning[possibly-unresolved-reference]:
while x < 10:
    # warning[possibly-unresolved-reference]:
    if x == 4:
        del x

So I think leaving it is correct (though it is possible it may prevent cycle convergence in some cases, but if we don't have a reproduction we can leave it for now). As @carljm points out, I think this means that your change also has the same problem of over-estimating the definedness of bindings within the loop, but I think it's fine to land for now to fix the cycle panics.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added both tests, thank you!

reachable_bindings,
}
Expand Down Expand Up @@ -1392,7 +1386,7 @@ fn place_from_bindings_impl<'db>(
// that none of them loop-back. In that case short-circuit, so that we don't
// produce an `Unknown` fallback type, and so that `Place::Undefined` is still a
// possibility below.
if !loop_header.has_defined_bindings {
if loop_header.reachable_bindings.is_empty() {
return None;
}
} else {
Expand Down
Loading