Skip to content

[ty] Suppress diagnostics in statically unreachable nested scopes #23849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
charliermarsh wants to merge 2 commits into
base: main
Choose a base branch
from
+39 −7
Open

[ty] Suppress diagnostics in statically unreachable nested scopes #23849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef4caf0
[ty] Suppress diagnostics in statically unreachable nested scopes
charliermarsh Mar 10, 2026
137b350
Revert changes to empty-body diagnostic
sharkdp Mar 12, 2026
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
2 changes: 0 additions & 2 deletions crates/ty_python_semantic/resources/mdtest/attributes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,8 +623,6 @@ class C:
self.c = c
if False:
def set_e(self, e: str) -> None:
# TODO: Should not emit this diagnostic
# error: [unresolved-attribute]
self.e = e

# TODO: this would ideally be `Unknown | Literal[1]`
Expand Down
15 changes: 10 additions & 5 deletions crates/ty_python_semantic/resources/mdtest/function/return_type.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,12 +187,14 @@ elif TYPE_CHECKING:
def j() -> str: ...

else:
def j_() -> str: ... # error: [empty-body]
def j():
raise NotImplementedError
Copy link
Contributor

@sharkdp sharkdp Mar 12, 2026

Choose a reason for hiding this comment

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

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).


if False:
pass
elif not TYPE_CHECKING:
def k_() -> str: ... # error: [empty-body]
def k() -> str:
raise NotImplementedError

else:
def k() -> str: ...
Expand Down Expand Up @@ -224,19 +226,22 @@ if typing.TYPE_CHECKING:
def o() -> str: ...

if not typing.TYPE_CHECKING:
def p() -> str: ... # error: [empty-body]
def p() -> str:
raise NotImplementedError

if compat.sub.sub.TYPE_CHECKING:
def q() -> str: ...

if not compat.sub.sub.TYPE_CHECKING:
def r() -> str: ... # error: [empty-body]
def r() -> str:
raise NotImplementedError

if t.TYPE_CHECKING:
def s() -> str: ...

if not t.TYPE_CHECKING:
def t() -> str: ... # error: [empty-body]
def t() -> str:
raise NotImplementedError
```

## Conditional return type
Expand Down
23 changes: 23 additions & 0 deletions crates/ty_python_semantic/resources/mdtest/unreachable.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,29 @@ if False:
print(x)
```

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.

```toml
[environment]
python-version = "3.14"
```

```py
from typing import TYPE_CHECKING

class NonCallable:
pass

if not TYPE_CHECKING:
def _(non_callable: NonCallable):
non_callable()

if False:
def _(non_callable: NonCallable):
non_callable()
```

### Type annotations

Silencing of diagnostics also works for type annotations, even if they are stringified:
Expand Down
6 changes: 6 additions & 0 deletions crates/ty_python_semantic/src/types/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,12 @@ impl<'db, 'ctx> LintDiagnosticGuardBuilder<'db, 'ctx> {
if ctx.is_in_multi_inference() {
return None;
}
// 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;
}
Comment on lines +445 to +450
Copy link
Contributor

@sharkdp sharkdp Mar 12, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@carljm carljm Mar 12, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@sharkdp sharkdp Mar 12, 2026

Choose a reason for hiding this comment

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

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" + 1

Sure, 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?

Copy link
Member

@AlexWaygood AlexWaygood Mar 12, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Member

@AlexWaygood AlexWaygood Mar 12, 2026

Choose a reason for hiding this comment

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

I think there might actually already be cases where we (accidentally?) suppress some of those diagnostics in unreachable regions, though, IIRC

Copy link
Contributor

@carljm carljm Mar 12, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@carljm carljm Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@sharkdp sharkdp Mar 13, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Member

@AlexWaygood AlexWaygood Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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


Some((severity, source))
}
Expand Down
Loading