[ty] report unused bindings as unnecessary hint diagnostics#23305
Open
denyszhak wants to merge 10 commits intoastral-sh:mainfrom
Open
[ty] report unused bindings as unnecessary hint diagnostics#23305denyszhak wants to merge 10 commits intoastral-sh:mainfrom
denyszhak wants to merge 10 commits intoastral-sh:mainfrom
Conversation
denyszhak
commented
Feb 16, 2026
Typing conformance resultsNo changes detected ✅ |
|
Memory usage reportSummary
Significant changesClick to expand detailed breakdownprefect
sphinx
trio
flake8
|
Contributor
Author
Don't seem related but maybe I messed up somewhere, will review tomorrow |
Contributor
|
Thanks for working on this! I have no opinion on the LSP side (that'd be @BurntSushi or @dhruvmanila or @MichaReiser), but in terms of how this data is collected: I suspect it could be done with less repeated work and less added code (and thus less potential for independent bugs / ty disagreeing with itself) by doing it in Did you explore the possibility of such an implementation at all? |
Contributor
We currently have some non-determinism, so it could be that. I just added ecosystem-analyzer tag, the ecosystem analyzer will tell you which projects have flaky results. |
Contributor
|
(Assigning myself here at least on the topic of how we collect the data; maybe someone from LSP team can also assign yourself as reviewer on the LSP-side implementation?) |
|
Contributor
Author
|
Contributor
Author
Repeated/duplicated code in traversal was my main concern before shipping, I though about alternatives but not the exact option you suggesting. I treated the semantic index as something to query rather than looking at what it already computes. (closed this pr accidentally so I reopened it back) |
Contributor
Author
Right, that's what I meant here #23305 (comment) and I can submit a follow up PR with this change if we are ok with leaving as if in the current one |
Member
It seems fine to me to do this since it's only bringing our internal representation more in line with what the LSP specification provides: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnosticSeverity |
denyszhak
commented
Feb 19, 2026
faee0e6 to
af6bd5e
Compare
|
af6bd5e to
aba2679
Compare
Contributor
Author
@dhruvmanila I addressed this in |
carljm
previously requested changes
Feb 25, 2026
Contributor
carljm
left a comment
There was a problem hiding this comment.
I reviewed only the ty_python_semantic changes (well I did make one observation in ty_server). Looking generally good, some updates needed I think.
crates/ty_server/src/server/api/requests/workspace_diagnostic.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Carl Meyer <carl@oddbird.net>
a5c3cb5 to
ae41dcb
Compare
ae41dcb to
82016f1
Compare
Contributor
|
I'm happy with the semantic side of this. Thanks @denyszhak ! Removing my assignment and my review request. Hoping either @BurntSushi or @dhruvmanila (or @MichaReiser once he's back) can pick this up and make sure the LSP/diagnostics side of it is good. |
Member
|
I can take a look at this tomorrow. Thanks for the ping @carljm! |
BurntSushi
requested changes
Feb 26, 2026
Member
BurntSushi
left a comment
There was a problem hiding this comment.
I think there are a few things in this PR that I'm uncertain about. In particular:
- I'm less certain now about whether we want to add a new "hint" level. And especially adding that to our configuration schema.
- Adding these hints as a diagnostic that is more like a side-channel to our existing diagnostics is concerning to me. This means it exists outside of how we normally suppress or configure diagnostics, which leads to a worse UX IMO.
| Valid severities are: | ||
|
|
||
| * `ignore`: Disable the rule. | ||
| * `hint`: Enable the rule and create a hint diagnostic. |
Member
There was a problem hiding this comment.
Hmm, I am having second thoughts on this. pyright does not have a "hint" level setting: https://code.visualstudio.com/docs/python/settings-reference
And note that even though we have an "info" level setting, it does not appear in the docs here.
I guess maybe we should wait for @MichaReiser to come back to review this bit.
Can you say why you can't use the "info" level here instead?
Contributor
Author
There was a problem hiding this comment.
There’s a UX difference between Hint and Info, so this is mostly about how noisy we want this to be.
In my local VSCode setup:
Hint + Unnecessary shows dimming only
Info + Unnecessary shows dimming and underline (more visually invasive)
So I used pyright as the reference and kept Hint for unused-code dimming (pyright also uses Hint for unreachable or deprecated tagged diagnostics):
https://github.com/microsoft/pyright/blob/main/packages/pyright-internal/src/languageServerBase.ts#L1547
Member
There was a problem hiding this comment.
Yeah, but the pyright docs don't note "hint" as a diagnostic level. So it might be using "hint" without actually publishing it as part of their configuration schema.
I think we'll want to wait for @MichaReiser to review this.
Contributor
Author
There was a problem hiding this comment.
@BurntSushi Right. the initial implementation was intentionally side-wired for UX only, not integrated into the normal lint/diagnostic pipeline.
Thanks for the feedback!
I’ll wait for your and @MichaReiser’s guidance on the preferred direction, and in the meantime I’ll address your other comments that are independent of that decision.
Member
There was a problem hiding this comment.
This is a cool feature! Thanks for working on it.
I think the right design here heavily depends on our goal:
- Is this an IDE-only feature that we don't want to expose on the CLI, then using Diagnostics feels wrong. Instead,
ty_servershould append the LSP-diagnostics manually where necessary - Is this a re-implementation of Ruff's F821 lint rule that can also be enabled on the CLI like any other rule? If so, we should add a new rule code and decide on a default severity (that the server might decide to override).
Reading through the PR, my impression is that we tried to accomplish one but using some of the infrastructure of 2.
To keep things simple, I'm leaning towards aiming for 1. but we should implement it without using ruff_db::Diagnostic. Instead, we should inject the additional LSP diagnostics in publish_diagnostic and the workspace diagnostic handlers. We can always pull this code out and create a separate lint rule if we decide to go for 2. in the future.
Contributor
Author
There was a problem hiding this comment.
Thanks for review! Sounds good to me, I will get this updated soon
Contributor
There was a problem hiding this comment.
Tbh I don't think it's a good idea to follow pyright's example here. It doesn't have a hint category but that makes it much less flexible for users who may want to turn them off per rule, or have hints for rules where a diagnostic tag (unreachable, unused or deprecated) doesn't apply.
Its also worth noting that pretty much every editor other than vscode renders these hints in a much less subtle way (basically the same as information), for example neovim. So users will want more configurability over these hints than you may think
Adding ahint diagnostic level was the solution I went with to address the issues many users had with pyright's diagnostic tags and i'd recommend the same thing for ty
(Kinda unrelated but maybe worth mentioning that astral-sh/ty#1074 is another use case for hints)
crates/ty_server/src/server/api/requests/workspace_diagnostic.rs
Outdated
Show resolved
Hide resolved
| @@ -24,6 +25,8 @@ use crate::system::{AnySystemPath, file_to_url}; | |||
| use crate::{DIAGNOSTIC_NAME, Db, DiagnosticMode}; | |||
| use crate::{PositionEncoding, Session}; | |||
|
|
|||
| const UNUSED_BINDING_CODE: &str = "unused-binding"; | |||
Member
There was a problem hiding this comment.
Hmmm, this seems concerning to me. Why is this defined outside of where we typically define lints in ty? And in particular, how would one disable this lint via configuration?
Contributor
Author
There was a problem hiding this comment.
Same rationale as in my comment below, I initially treated this as editor UX-only dimming, so I kept it outside the normal lint flow.
I agree with you it's not ideal to keep it hanging here, I will move this.
| @@ -325,7 +328,10 @@ pub(super) fn compute_diagnostics( | |||
| return None; | |||
| }; | |||
|
|
|||
| let diagnostics = db.check_file(file); | |||
| let mut diagnostics = db.check_file(file); | |||
Member
There was a problem hiding this comment.
Maybe a dumb question, but why aren't these diagnostics returned as part of db.check_file? Is it because we don't want to show them on the CLI? This feels non-ideal to me because it creates a subtle inconsistency with how diagnostics are generated between the CLI and LSP.
Contributor
Author
There was a problem hiding this comment.
That was my original intent. I treated this as an editor UX signal (dimming) rather than a normal lint category, so I initially kept it outside the lint pipeline and CLI path.
I agree centralized generation through the lint pipeline is better here. The open question is presentation. Should CLI display this rule, or should we filter it similarly to Pyright’s unused-code UX diagnostics? My concern with CLI is that It can be a low-signal UX noise that can drown actionable type errors/warnings.
Contributor
There was a problem hiding this comment.
Emitting a diagnostic seems very useful, and could also be used in astral-sh/ty#2829.
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
8 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.
Fixes astral-sh/ty#2607
Summary
Add unused-binding dimming in ty via diagnostics (Hint + DiagnosticTag::Unnecessary) so VS Code fades unused locals even without Pylance. VS Code default fading for “unused” is diagnostics-driven, so this PR uses diagnostics, not semantic tokens.
The unused-binding detection now lives in
ty_python_semanticand is consumed byty_server, with local-scope to avoid false positives for now. Cros-modile/class reference will be addressed in a follow up PR because it requires a bit more thinking and effort. Unused-binding analysis centralized in semantic (unused_bindings) so other IDE surfaces can reuse it later (for example emit semantic info).Test Plan
Added unit tests in unused_bindings.rs
Added/updated e2e diagnostics coverage in pull_diagnostics.rs
Manually tested in VSCode