[ty] Suppress diagnostics in statically unreachable nested scopes#23849
Open
charliermarsh wants to merge 2 commits intomainfrom
Open
[ty] Suppress diagnostics in statically unreachable nested scopes#23849charliermarsh wants to merge 2 commits intomainfrom
charliermarsh wants to merge 2 commits intomainfrom
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 85.29%. The percentage of expected errors that received a diagnostic held steady at 78.13%. The number of fully passing files held steady at 64/132. |
|
Memory usage reportMemory usage unchanged ✅ |
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-key |
0 | 302 | 0 |
invalid-await |
40 | 0 | 0 |
unused-type-ignore-comment |
15 | 0 | 0 |
unresolved-attribute |
0 | 14 | 0 |
invalid-type-arguments |
0 | 13 | 0 |
invalid-type-form |
0 | 13 | 0 |
empty-body |
0 | 12 | 0 |
unknown-argument |
0 | 10 | 0 |
call-non-callable |
0 | 9 | 0 |
invalid-parameter-default |
0 | 8 | 0 |
invalid-argument-type |
0 | 7 | 0 |
invalid-return-type |
1 | 5 | 0 |
invalid-assignment |
0 | 4 | 0 |
too-many-positional-arguments |
0 | 4 | 0 |
unsupported-dynamic-base |
0 | 3 | 0 |
unsupported-operator |
0 | 2 | 0 |
invalid-legacy-type-variable |
0 | 1 | 0 |
not-iterable |
0 | 1 | 0 |
unused-awaitable |
0 | 1 | 0 |
| Total | 56 | 409 | 0 |
charliermarsh
force-pushed
the
charlie/unreachable
branch
from
f847285 to
85b1132
Compare
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager,
ibraheemdev and
sharkdp
as code owners
Member
Author
|
I... think we want this behavior? |
Member
|
@sharkdp has probably opinions on this |
sharkdp
approved these changes
Mar 12, 2026
Comment on lines
+467
to
+468
| This also applies to deferred annotations on Python 3.14+, which otherwise make the unreachable | ||
| scope look more "real" during body checking: |
Contributor
There was a problem hiding this comment.
Maybe
Suggested change
| This also applies to deferred annotations on Python 3.14+, which otherwise make the unreachable | |
| scope look more "real" during body checking: | |
| This also applies to deferred annotations on Python 3.14+, which are resolved from the | |
| perspective of the end of the scope, which may not be part of the unreachable section. |
Comment on lines
+445
to
+450
| // Nested scopes in statically unreachable branches should not produce body diagnostics. | ||
| if !semantic_index(ctx.db, ctx.file) | ||
| .is_scope_reachable(ctx.db, ctx.scope.file_scope_id(ctx.db)) | ||
| { | ||
| return None; | ||
| } |
Contributor
There was a problem hiding this comment.
I think this is the actual change that we want.
The empty-body rule was moved from function body inference to deferred function-definition validation to ensure that it keeps running (which seems to be intended) even for unreachable code.
I looked at those test cases and I would argue that we want a different behavior there. This might have been intended in the past, but we've since come to the conclusion that we may be required to silence all diagnostics in definitely-unreachable sections. So I'll push a change to this PR that removes most of the changes and only leaves this part. Instead, I'm going to modify the test cases.
Contributor
There was a problem hiding this comment.
If this is in reference to if not TYPE_CHECKING, I'm not sure that we should extrapolate from that to "all definitely unreachable sections" of code. I think we probably will need to suppress all errors in if not TYPE_CHECKING sections, and I think there are arguments for that that don't apply to other unreachable sections.
Contributor
There was a problem hiding this comment.
If this is in reference to
if not TYPE_CHECKING, I'm not sure that we should extrapolate from that to "all definitely unreachable sections" of code.
Yes, I was referring to some recent discussions related to python/typing#2210. It's possible that those lines above are too big of a hammer (because they make no such distinction), but I'm less and less convinced that it's worth spending a lot of effort to emit diagnostics for code like
if False:
"a" + 1Sure, this line would definitely lead to a runtime error if it was reachable, but is that diagnostic really that useful? No other type checker makes an effort. And it is a reasonable developer/debugging use case to put an early return in a large function to silence diagnostics in "everything that comes below" (apart from the future "this is unreachable" graying out LSP feature / lint rule).
I probably shouldn't have said "we've since come to the conclusion" — the statement above was just based on some discussion with @AlexWaygood this morning.
What are some concrete scenarios where we would definitely want to emit a diagnostic in definitely-unreachable sections of code?
Member
There was a problem hiding this comment.
The only diagnostic which I would really like us to continue to try hard to emit in unreachable code is reveal_type diagnostics -- it's very confusing for users if a reveal_type call silently causes us not to emit a revealed-type diagnostic, especially if they have warnings about unreachable code turned off. Maybe assert_type and assert_never calls fall into a similar bucket there?
Member
There was a problem hiding this comment.
I think there might actually already be cases where we (accidentally?) suppress some of those diagnostics in unreachable regions, though, IIRC
Contributor
There was a problem hiding this comment.
I think there might actually already be cases where we (accidentally?) suppress some of those diagnostics in unreachable regions, though, IIRC
This doesn't ring a bell for me -- any further memory of in which cases you recall this happening, or why?
Contributor
There was a problem hiding this comment.
We did have user feedback way back in #15797 (comment) from a couple users that the desired behavior, at least for some kinds of unreachable code, for some developers, was "full diagnostics". I think it's clear that "full diagnostics" is not possible without false positives, but that does at least suggest that creating unreachable code intentionally during development, for other reasons, and still wanting full diagnostics on it, is also a valid developer use case.
Overall though, I don't think this matters too much either way and I'm happy for us to do whatever is simplest. (Up until now, I don't think we have gone to extra effort to preserve some diagnostics in unreachable code, I think it was the behavior that fell out naturally, and suppressing required extra work.) If we need to suppress all diagnostics in if not TYPE_CHECKING, then "simplest" is probably to suppress in all unreachable code, even if it doesn't follow as a strict requirement that we handle them the same.
Contributor
There was a problem hiding this comment.
I think there might actually already be cases where we (accidentally?) suppress some of those diagnostics in unreachable regions, though, IIRC
This doesn't ring a bell for me -- any further memory of in which cases you recall this happening, or why?
I think the current state is still accurately described in this mdtest section: https://github.com/astral-sh/ruff/blob/main/crates/ty_python_semantic/resources/mdtest/unreachable.md#no-incorrect-diagnostics-in-unreachable-code: "[…] we do not attempt to provide the full set of diagnostics in unreachable sections. In fact, a large number of diagnostics are suppressed in unreachable code, simply due to the fact that we infer Never for most of the symbols."
On top of this, Alex is right in the sense that there are some special diagnostics that are simply "turned off" in unreachable sections (to be more precise, we turn them off if the AST node on which we would emit the diagnostic is unreachable). For example, we turn off unresolved-reference here. See also this mdtest and the ones following.
We did have user feedback way back in #15797 (comment) from a couple users that the desired behavior, at least for some kinds of unreachable code, for some developers, was "full diagnostics". I think it's clear that "full diagnostics" is not possible without false positives, but that does at least suggest that creating unreachable code intentionally during development, for other reasons, and still wanting full diagnostics on it, is also a valid developer use case.
👍
Overall though, I don't think this matters too much either way and I'm happy for us to do whatever is simplest.
Okay
(Up until now, I don't think we have gone to extra effort to preserve some diagnostics in unreachable code, I think it was the behavior that fell out naturally, and suppressing required extra work.)
Yeah, maybe. I think it might have been easier to keep track of reachability constraints for all AST nodes (or for basic blocks, for performance reasons) and to simply suppress all diagnostics in unreachable code. I agree it's not like it was super hard to arrive at the current state, and maybe it's what fell out most naturally, but we did quite a few adjustments on top of "everything will be fine because we infer Never everywhere". To give one one additional example, consider something like
if sys.version_info >= (3, 11):
import tomllib
# …If we wouldn't track reachability constraints for that import node in order to explicitly silence the unresolved-import diagnostic in unreachable sections, this would lead to a false positive when checking on lower Python versions.
So in conclusion, I think it's reasonable to try and make an effort to further patch little things like astral-sh/ty#2891 by dedicated means when they come up. But if we agree that it "doesn't matter too much either way", maybe the easier way would be the big hammer of silencing all diagnostics in unreachable code. This PR is somewhere weirdly in between these two approaches (in that it only swings the big hammer for nested scopes inside unreachable sections), so maybe we should indeed spend a bit more time discussing this before making a decision here.
Member
There was a problem hiding this comment.
I think there might actually already be cases where we (accidentally?) suppress some of those diagnostics in unreachable regions, though, IIRC
This doesn't ring a bell for me -- any further memory of in which cases you recall this happening, or why?
Claude found a repro for me:
from typing import reveal_type, assert_type, assert_never
def after_return():
x: int = 1
return
reveal_type(x) # expected: info diagnostic showing type
# actual: nothing
def always_false_branch():
x: int = 1
if False:
reveal_type(x) # expected: info diagnostic
# actual: nothing
def exhaustive_narrowing(x: int | str):
if isinstance(x, int):
return 1
elif isinstance(x, str):
return 2
else:
reveal_type(x) # expected: revealed type `Never`
# actual: nothing
assert_type(x, int) # expected: type-assertion-failure
# actual: nothing
assert_never(x)The issue is that in unreachable regions, the symbols reveal_type, assert_never and assert_type are all actually inferred as having type Never right now. So even the final assert_never call looks like it's working as expected... but it's actually not at all -- the function itself is being inferred as Never, so we're not inferring a call to the function typing.assert_never at all here. https://play.ty.dev/f2163845-feab-4be8-8423-29d461adb33f
note that reveal_type actually works much better... if you don't import it at all... https://play.ty.dev/3dc64038-1ee7-451f-a855-7b70f25c4150
sharkdp
force-pushed
the
charlie/unreachable
branch
from
85b1132 to
137b350
Compare
sharkdp
reviewed
Mar 12, 2026
| else: | ||
| def j_() -> str: ... # error: [empty-body] | ||
| def j(): | ||
| raise NotImplementedError |
Contributor
There was a problem hiding this comment.
The change to the code is not really necessary, but think this better reflects how someone would actually write this code. They would use the if TYPE_CHECKING branch to provide a type signature, and then use the else branch for the actual implementation (that may or may not have caused some diagnostics, if it were not in a non-TYPE_CHECKING block).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stop emitting body diagnostics from nested scopes defined under statically unreachable branches such as
if Falseandif not TYPE_CHECKING.This fixes false positives like
call-non-callablein unreachable functions, including the Python 3.14 deferred-annotation case. Theempty-bodyrule was moved from function body inference to deferred function-definition validation to ensure that it keeps running (which seems to be intended) even for unreachable code.Closes astral-sh/ty#2891.