[ty] Refactor if narrowing by ericmarkmartin · Pull Request #18392 · astral-sh/ruff

ericmarkmartin · 2025-05-30T19:29:24Z

Summary

Refactor the semantic index builder’s If statement analysis. This change also lets us detect more unreachable code paths.

Test Plan

Add new tests to unreachable.md
Update existing tests

carljm · 2025-05-30T22:18:23Z

Looks like there are still some failing tests here? Let me know if any of these are things you'd like a second pair of eyes on how to address; otherwise I'll mark this as draft, go ahead and put it back up for review when CI is green.

ericmarkmartin · 2025-05-31T21:40:05Z

Looks like there are still some failing tests here? Let me know if any of these are things you'd like a second pair of eyes on how to address; otherwise I'll mark this as draft, go ahead and put it back up for review when CI is green.

Looks like we're green now, but mypy primer seems unhappy (sorry if that means I shouldn't have opened it, but I'm not sure how to proceed with primer failures)

sharkdp · 2025-06-04T08:04:27Z

Thank you for working on this!

Looks like we're green now, but mypy primer seems unhappy

The mypy_primer run was failing due to a network error. I re-ran it now and it fails with

fatal[panic] Panicked at crates/ty_python_semantic/src/symbol.rs:927:21 when checking `/tmp/mypy_primer/projects/trio/src/trio/_core/__init__.py`: `internal error: entered unreachable code: If we have at least one declaration, the scope-start should not be definitely visible`

This might be related to astral-sh/ty#365 (comment). I am planning to fix that bug as soon as #18041 is merged (which makes significant changes to the semantic index builder, and has a much higher impact). I suggest that we wait with this PR until these two things have been resolved.

carljm · 2025-06-06T01:30:00Z

crates/ty_python_semantic/resources/mdtest/narrow/type.md

 def _[T](x: A | B):
    if type(x) is A[str]:
-        reveal_type(x)  # revealed: (A[int] & A[Unknown]) | (B & A[Unknown])
+        # TODO: previous was revealed: (A[int] & A[Unknown]) | (B & A[Unknown])


I think the conclusion that this is unreachable code is actually correct -- type(x) will never return a GenericAlias at runtime, so type(x) is A[str] can never be true, only type(x) is A can be.

This test probably warrants some expansion, but it doesn't need to happen in this PR. We have an issue open for improving how we handle narrowing against generic types.

carljm · 2025-06-06T01:30:56Z

crates/ty_python_semantic/resources/mdtest/overloads.md

+    # TODO: false positive due to unreachable, causing @overload to be typed at
+    # Never


We intend to generally silence diagnostics in unreachable code, which should take care of this

carljm · 2025-06-06T02:05:35Z

crates/ty_python_semantic/src/semantic_index/builder.rs

+                    let v_constraint = self.record_visibility_constraint(predicate);
+                    self.visit_body(clause_body);
+                    self.record_visibility_constraint_id(v_constraint);


As we discussed, it definitely looks weird that we have to record the visibility constraint both before and after. I'm going to take a closer look to try to understand why that is.

carljm · 2025-06-06T02:11:10Z

Not able to reproduce the mypy-primer error running this locally. Will push a rebase and see if it repros in CI.

codspeed-hq · 2025-06-06T02:17:19Z

CodSpeed Performance Report

Merging #18392 will degrade performances by 4.14%

_{Comparing ericmarkmartin:refactor-if-narrowing (4af0119) with main (cb8246b)}

Summary

❌ 1 regressions
✅ 33 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
❌	`ty_micro[many_string_assignments]`	65.7 ms	68.6 ms	-4.14%

carljm · 2025-06-06T02:24:39Z

Hmm, well, it definitely repros here. That's odd. This is what I'm running locally:

➜ uvx \
            --from="git+https://github.com/hauntsaninja/mypy_primer@01a7ca325f674433c58e02416a867178d1571128" \
            mypy_primer --repo /Users/carlmeyer/projects/ruff \
            --type-checker ty \
            --old main \
            --new refactor-if-narrowing \
            --project-selector "/(trio)\$" \
            --output diff \
            --debug

carljm · 2025-06-06T02:33:56Z

Ah, figured it out.

The failing file is src/trio/_core/__init__.py, which contains this code:

# Windows imports
if sys.platform == "win32":
    from ._run import (
        current_iocp,
        monitor_completion_key,
        readinto_overlapped,
        register_with_iocp,
        wait_overlapped,
        write_overlapped,
    )
# Kqueue imports
elif sys.platform != "linux" and sys.platform != "win32":
    from ._run import current_kqueue, monitor_kevent, wait_kevent

And the failure repros locally if I run with --python-platform="linux". Otherwise locally my platform defaults to darwin, and the issue doesn't appear.

Since the repro is an if, not a loop with break, it seems clear this is an issue introduced by this PR; it's not astral-sh/ty#365 (comment)

carljm · 2025-06-10T20:33:06Z

@sharkdp is now working on a larger refactor of reachability and visibility constraints, we should revisit this after that's done and see what is still needed/relevant here. Thanks for the work on this PR, it helped demonstrate the need for this refactor.

@carljm

## Summary * Completely removes the concept of visibility constraints. Reachability constraints are now used to model the static visibility of bindings and declarations. Reachability constraints are *much* easier to reason about / work with, since they are applied at the beginning of a branch, and not applied retroactively. Removing the duplication between visibility and reachability constraints also leads to major code simplifications [^1]. For an overview of how the new constraint system works, see the updated doc comment in `reachability_constraints.rs`. * Fixes a [control-flow modeling bug (panic)](astral-sh/ty#365) involving `break` statements in loops * Fixes a [bug where](astral-sh/ty#624) where `elif` branches would have wrong reachability constraints * Fixes a [bug where](astral-sh/ty#648) code after infinite loops would not be considered unreachble * Fixes a panic on the `pywin32` ecosystem project, which we should be able to move to `good.txt` once this has been merged. * Removes some false positives in unreachable code because we infer `Never` more often, due to the fact that reachability constraints now apply retroactively to *all* active bindings, not just to bindings inside a branch. * As one example, this removes the `division-by-zero` diagnostic from astral-sh/ty#443 because we now infer `Never` for the divisor. * Supersedes and includes similar test changes as #18392 closes astral-sh/ty#365 closes astral-sh/ty#624 closes astral-sh/ty#642 closes astral-sh/ty#648 ## Benchmarks Benchmarks on black, pandas, and sympy showed that this is neither a performance improvement, nor a regression. ## Test Plan Regression tests for: - [x] astral-sh/ty#365 - [x] astral-sh/ty#624 - [x] astral-sh/ty#642 - [x] astral-sh/ty#648 [^1]: I'm afraid this is something that @carljm advocated for since the beginning, and I'm not sure anymore why we have never seriously tried this before. So I suggest we do *not* attempt to do a historical deep dive to find out exactly why this ever became so complicated, and just enjoy the fact that we eventually arrived here. --------- Co-authored-by: Carl Meyer <[email protected]>

sharkdp · 2025-06-17T07:56:36Z

I'm going to close this, now that #18621 has been merged. @ericmarkmartin thank you for your work here.

ericmarkmartin requested review from AlexWaygood, carljm, dcreager and sharkdp as code owners May 30, 2025 19:29

MichaReiser changed the title ~~Refactor if narrowing~~ [ty] Refactor if narrowing May 30, 2025

MichaReiser added the ty Multi-file analysis & type inference label May 30, 2025

carljm marked this pull request as draft May 30, 2025 22:18

ericmarkmartin marked this pull request as ready for review May 31, 2025 21:40

carljm reviewed Jun 6, 2025

View reviewed changes

ericmarkmartin added 7 commits June 5, 2025 19:09

wip

9aad75d

if refactor

f07a955

add elif test case

81d5411

add visibility constraints before _and_ after visiting if clause bodies

548524a

resolve overloads.md error

14eb433

label more types as Never in tests

328d877

remove extraneous test

4af0119

carljm force-pushed the refactor-if-narrowing branch from d282919 to 4af0119 Compare June 6, 2025 02:11

sharkdp mentioned this pull request Jun 11, 2025

[ty] Reachability constraints #18621

Merged

4 tasks

AlexWaygood removed their request for review June 16, 2025 22:35

sharkdp closed this Jun 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

[ty] Refactor if narrowing#18392

[ty] Refactor if narrowing#18392
ericmarkmartin wants to merge 7 commits intoastral-sh:mainfrom
ericmarkmartin:refactor-if-narrowing

ericmarkmartin commented May 30, 2025 •

edited

Loading

Uh oh!

carljm commented May 30, 2025

Uh oh!

ericmarkmartin commented May 31, 2025 •

edited

Loading

Uh oh!

sharkdp commented Jun 4, 2025

Uh oh!

carljm Jun 6, 2025

Uh oh!

carljm Jun 6, 2025

Uh oh!

carljm Jun 6, 2025

Uh oh!

carljm commented Jun 6, 2025

Uh oh!

codspeed-hq bot commented Jun 6, 2025

Uh oh!

carljm commented Jun 6, 2025

Uh oh!

carljm commented Jun 6, 2025 •

edited

Loading

Uh oh!

carljm commented Jun 10, 2025

Uh oh!

sharkdp commented Jun 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		# TODO: false positive due to unreachable, causing @overload to be typed at
		# Never

Comments

Conversation

ericmarkmartin commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

carljm commented May 30, 2025

Uh oh!

ericmarkmartin commented May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharkdp commented Jun 4, 2025

Uh oh!

carljm Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

carljm Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

carljm Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

carljm commented Jun 6, 2025

Uh oh!

codspeed-hq bot commented Jun 6, 2025

CodSpeed Performance Report

Merging #18392 will degrade performances by 4.14%

Summary

Benchmarks breakdown

Uh oh!

carljm commented Jun 6, 2025

Uh oh!

carljm commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carljm commented Jun 10, 2025

Uh oh!

sharkdp commented Jun 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ericmarkmartin commented May 30, 2025 •

edited

Loading

ericmarkmartin commented May 31, 2025 •

edited

Loading

carljm commented Jun 6, 2025 •

edited

Loading