[ty] Correctly handle calls to functions marked as returning Never / NoReturn

[abhijeetbodas2001]

Summary

ty does not understand that calls to functions which have been annotated as having a return type of Never / NoReturn are terminal.

This PR fixes that, by adding new reachability constraints when call expressions are seen. If the call expression evaluates to Never, the code following it will be considered to be unreachable. Note that, for adding these constraints, we only consider call expressions at the statement level, and that too only inside function scopes. This is because otherwise, the number of such constraints becomes too high, and evaluating them later on during type inference results in a major performance degradation.

Fixes astral-sh/ty#180

Test Plan

New mdtests.

Ecosystem changes

This PR removes the following false-positives:

• "Function can implicitly return None, which is not assignable to ...".
• "Name foo used when possibly not defind" - because the branch in which it is not defined has a NoReturn call, or when foo was imported in a try, and the except had a NoReturn call.

[abhijeetbodas2001]

This isn't ready for review yet, but I wanted a mypy_primer run, and a place to write notes.

[AlexWaygood]

the crash in mypy_primer is unrelated; I've filed #18336 to fix it

[abhijeetbodas2001]

Does seem related now. Investigating.

[AlexWaygood]

Does seem related now. Investigating.

Hmm, I'm not so sure -- I feel like assertions from inside Salsa (a third-party library we use) should probably never trigger, no matter what we do in ty 😄 there could very well be a bug in this PR as well, but I think there's probably a bug in Salsa here too

[AlexWaygood]

yeah, you're running into salsa-rs/salsa#831

[MichaReiser]

yeah, you're running into salsa-rs/salsa#831

Which means that this PR introduces deeply nested cycles. We may want to take a closer look if they can be avoided.

[carljm]

The assertion error in cycle.rs is a known issue with cycle iteration in Salsa that @MichaReiser has already been looking at. I suspect the only relation to this diff is that it can cause more cycles, particularly with deferred name resolution.

[abhijeetbodas2001]

Thanks for the explanation.
This is also timing out when analyzing https://github.com/astral-sh/ruff/blob/main/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH210.py via run_corpus_tests, but I cannot reproduce the long run-time if I copy the contents from that file and run it as an mdtest. Interesting.

[AlexWaygood]

The corpus tests run in a pretty different way to our mdtests. Our mdtests try to emulate how ty would run if you executed it from the command line. But the corpus tests attempt to probe the type of each sub-expression in a file's AST, which is somewhat different to what ty does when it's run from the command line. (Many subexpressions can actually end up being irrelevant when ty is being run from the command line, so it skips analyzing them altogether.)

[carljm]

This is the extra step that runs for corpus tests. We currently don't have an easy way to run that on an arbitrary code file from CLI, but it would be a good thing to add.

[abhijeetbodas2001]

Is closing+re-opening the PR good enough, or would a rebase on main be required?

[AlexWaygood]

Is closing+re-opening the PR enough good enough, or would a rebase on main be required?

Closing+reopening should be sufficient. GitHub CI on PRs doesn't run directly on the PR branch -- it runs on the result of a merge commit of the PR branch with the target branch.

[codspeed-hq]

CodSpeed Instrumentation Performance Report

Merging #18333 will degrade performances by 7.96%

_{Comparing abhijeetbodas2001:never (c9c7890) with main (411cccb)}

Summary

❌ 1 (👁 1) regressions
✅ 38 untouched benchmarks

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
👁	hydra-zen	779.6 ms	847 ms	-7.96%

[sharkdp]

@abhijeetbodas2001 I saw that you just rebased this on top of main after the reachability constraints PR was merged. Let me know if you have any questions or need help with this.

[carljm]

I think this PR was sort of waiting for astral-sh/ty#210, to see if it significantly reduces the number of these new constraints that we need to evaluate in practice.

But given that the latest codspeed results here are showing <5% cold-check regression, we might be able to consider merging this without even waiting for that.

[abhijeetbodas2001]

@abhijeetbodas2001 I saw that you just rebased this on top of main after the reachability constraints PR was merged. Let me know if you have any questions or need help with this.

Thanks! I haven't gotten to investigating the outstanding failures here. I'll do that and get back.

[carljm]

Thanks @sharkdp for the further exploration. I think it makes sense to start with the statement-level-only version, and consider expanding it carefully if/as we see issues crop up with more deeply nested calls returning Never.

[sharkdp]

@abhijeetbodas2001 Feel free to cherry-pick 2eae76b onto this branch.

[carljm]

I do still think it's also worth exploring the idea that when resolving imports, we ignore some kinds of reachability constraints (call-Never constraints for sure, maybe also assertion constraints?) that are particularly likely to accumulate at end-of-scope for almost any symbol. I think that should dramatically reduce the number of these constraints we have to evaluate, and in particular, the number of them we have to evaluate in third-party code, that may trigger further type inference we could otherwise avoid entirely.

An alternative version of cutting scope here that would be even simpler to experiment with, but would I think have similar effects, would be to never add these call-Never constraints at all in non-function scopes. (Today, non-function scopes are the only ones where we use end-of-scope symbol resolution). I think this way of cutting scope might more closely match what other type checkers do and don't support? At least pyright doesn't ever seem to respect Never-returning calls as a constraint when inferring types of class attributes or imported names. (But within a function, it does respect Never-returning calls that aren't top-level statements.)

[abhijeetbodas2001]

I've addressed the review comments from above. Major changes are:

• This now restricts these new constraints to be added only at statement-level calls (thanks @sharkdp!), and only in function scopes.

• When required, evaluating the constraints will fall-back to inferring the full call expression. As part of this, the constraint now stores references to both the callable, and the call expression.

• Because we now add these new constraints only inside function scopes, nox and streamlit no longer panic, like they used to when we had module-level constraints.

  So I've moved them back to `good.txt`.

  

Marking this as ready for another review.

[carljm]

This looks great to me! Really excellent work.