[ty] report unreachable code as unnecessary hint diagnostics

[denyszhak]

Closes astral-sh/ty#784

Summary

Implements dimming for unreachable code in LSP-capable editors. Unreachable ranges are collected from the semantic index and emitted as DiagnosticTag::UNNECESSARY hints.

Test Plan

Unit tests coverage, e2e test coverage and manual testing

[denyszhak]

It's a draft for now with only two types of unreachable code but I need to think slightly more to map it to more precise types: structural, static, maybe type related before sending for review.

[astral-sh-bot]

Typing conformance results

No changes detected ✅

Current numbers

The percentage of diagnostics emitted that were expected errors held steady at 87.94%. The percentage of expected errors that received a diagnostic held steady at 83.36%. The number of fully passing files held steady at 79/133.

[astral-sh-bot]

Memory usage report

Memory usage unchanged ✅

[astral-sh-bot]

ecosystem-analyzer results

No diagnostic changes detected ✅

Flaky changes detected. This PR summary excludes flaky changes; see the HTML report for details.

Full report with detailed diff (timing results)

[denyszhak]

It's a draft for now with only two types of unreachable code but I need to think slightly more to map it to more precise types: structural, static, maybe type related before sending for review.

I ended up keeping only two categories for now. Splitting further into structural/static/type-analysis-style categories is possible in principle, but with the current reachability plumbing that would require extra classification logic at this layer mainly to refine the user-facing message. The current implementation still covers the full set of unreachable cases, what’s intentionally deferred is finer-grained messaging. I didn’t want to add more categorization logic unless we can align it with ty’s actual reachability model rather than approximate pyright’s labels.

[MichaReiser]

This is very cool. Thank you for working on it.

I only have a few suggestions around wording, documentation, and test structure. This is otherwise good to go.

[sharkdp]

Only certain simple patterns like if False have a constraint of ALWAYS_FALSE. There are more complex patterns like if 1 + 1 == 3 or if isinstance("test", str) which need type inference for evaluation. It looks like we are categorizing these as CurrentAnalysis here.

I have not reviewed the rest of the code, so maybe this is not problematic. But it might be slightly confusing for users?

(I was always expecting that we would have to do the opposite to achieve this categorization: to match on a restricted set of patterns like if sys.version_info >= (3, 10) which we would know depend on the current user settings; and then categorize everything else as Unconditional)

[denyszhak]

What you say is to flip this thing to have like two types of unreachable: Configuration and Unconditional and we have let's say is_configuration_expr and everything that evaluate as config goes into this bucket and everything else is Unconditional?

This sounds good actually, I was slightly confused in what separate would be best.

[denyszhak]

I think this is actually the correct way for UX, I will update this

[sharkdp]

Yes. It would be good to understand which of these failure cases is more problematic:

• an "unconditionally unreachable" statement that is falsely being labeled as "unreachable due to config settings"
• a statement that is "unreachable due to config settings" that is falsely being labeled as "unconditionally unreachable"

We should probably answer that before we make any changes here.

[denyszhak]

Second one sounds worse for me. Labeling config-dependent code as unconditionally unreachable would be a stronger claim than we can justify, since that code may still be reachable on another Python version or platform. The other direction is still a loss of precision, but less misleading. So I think the safer approach is to keep the configuration bucket narrow and only use for explicitly recognized config patterns, and make the fallback bucket the regular generic unreachable case rather than “unconditional”.

wdyt?

[MichaReiser]

What about "Code is unreachable (may depend on your current environment and settings)"?

We could change the message of the Unconditional to code is _always_ unreachable, to make the distinction clearer.

The Conditional unreachable could also use a info subdiagnostic to say that it depneds on the current environment and settings, instead of using parentheses.

[denyszhak]

For Unconditional I changed to "Code is always unreachable" and for CurrentAnalysis to "Code is unreachable\nThis may depend on your current environment and settings" which results in a good UX to me?

I kept CurrentAnalysis over Conditional, the name Conditional paired with Unconditional creates a false symmetry implying the code is "sometimes reachable", which isn't accurate. It's still unreachable under the current analysis. "Contextual" could also be the option.

[carljm]

Agree with everything said above. One additional note I'd make (just for future) is that we also support a case like this:

if sys.version_info >= (3, 11):
    NEW_ENOUGH = True
else:
    NEW_ENOUGH = False

or more complex cases, where even checks against specific patterns we might recognize can end up "laundered" into a boolean value whose static truthiness we will recognize, but not its configuration-dependent origin.

I think this is just reinforcing @sharkdp's point that we can't really hope to reliably distinguish "unreachable for configuration-dependent reasons" and "always unreachable, detected by complex type inference but actually independent of configuration".

Worth noting that rust-analyzer doesn't seem to attempt this distinction at all -- even explicitly configuration-gated code sections still show up as an unreachable-code diagnostic in my editor.

[MichaReiser]

I'd be fine not to make this distinction.

[sharkdp]

I like what we have now. If we show the stronger "Code is always unreachable" message, we can be certain that the code is actually unreachable under all circumstances (unless there's a bug in ty's reachability analysis).

And conversely, showing the "This may depend on your current environment and settings" message seems potentially helpful in cases where it does apply; and not actively harmful in cases where it doesn't.