[ty] Fix loop-header reachability cycles in conditional unpacking

[charliermarsh]

Summary

Closes astral-sh/ty#3057.

Closes astral-sh/ty#3104.

[astral-sh-bot]

Typing conformance results

No changes detected ✅

Current numbers

The percentage of diagnostics emitted that were expected errors held steady at 85.38%. The percentage of expected errors that received a diagnostic held steady at 78.70%. The number of fully passing files held steady at 64/132.

[astral-sh-bot]

Memory usage report

Memory usage unchanged ✅

[astral-sh-bot]

ecosystem-analyzer results

No diagnostic changes detected ✅

Full report with detailed diff (timing results)

[ibraheemdev]

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?

[charliermarsh]

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.

[ibraheemdev]

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.

[charliermarsh]

Added both tests, thank you!

[carljm]

This might be the unavoidable cost of fixing these cycles (I'd have to think harder about what else we could do to avoid it), but Codex observes that this branch means we now infer Literal[0, 2] for x = 0; while random(): reveal_type(x); if x == 1: x = 2, whereas on main we correctly reveal Literal[0].

Of course, no other type checker is able to reveal Literal[0] here, which is another indication that this regression might be acceptable.

[charliermarsh]

I'll need to dig deeper to figure out if there's a fix that would retain the current behavior, but we might want to err on the side of merging this since we now have two panic reports.

[ibraheemdev]

Happy to land this for now to fix the panics, but it would be helpful to add @carljm's example as a regression test.