[ty] report unused bindings as unnecessary hint diagnostics#23305
[ty] report unused bindings as unnecessary hint diagnostics#23305MichaReiser merged 22 commits intoastral-sh:mainfrom
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 86.59%. The percentage of expected errors that received a diagnostic held steady at 80.96%. The number of fully passing files held steady at 68/132. |
|
Memory usage reportSummary
Significant changesClick to expand detailed breakdownprefect
sphinx
trio
flake8
|
Don't seem related but maybe I messed up somewhere, will review tomorrow |
|
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? |
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. |
|
(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?) |
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-await |
40 | 0 | 0 |
invalid-return-type |
1 | 0 | 0 |
| Total | 41 | 0 | 0 |
Changes in flaky projects detected. Raw diff output excludes flaky projects; see the HTML report for details.
|
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) |
carljm
left a comment
There was a problem hiding this comment.
Thanks, this is looking pretty nice!
Main semantic issue I see currently is that we wrongly report a closure-captured binding as unused; unfortunately this may not be trivial to properly fix.
Simplest way to fix this (might be fine for now, results only in false negatives) is to consider all bindings "used" if their scope contains any nested scopes at all.
More precise way is to consider a binding used only if a nested scope actually references its Place.
Co-authored-by: Micha Reiser <micha@reiser.io>
93a16a0 to
183cff0
Compare
| if !parsed.errors().is_empty() { | ||
| return Vec::new(); | ||
| } |
There was a problem hiding this comment.
I think I asked this before. What's the reason for bailing early if there are syntax errors? It results in "flickering" while transitioning between valid and invalid syntax (which is always when typing)
There was a problem hiding this comment.
I was conservative and assumed there could be false positives but now after reviewing particular test cases it seems fine and event better not to have it, I added two tests related to that as well
There was a problem hiding this comment.
The test for notebooks has now also slightly different assertion
51a2da1
crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs
Outdated
Show resolved
Hide resolved
| || function_is_overload_declaration | ||
| || method_has_stub_body | ||
| || *skip_unused_parameters_for_override.get_or_insert_with(|| { | ||
| method_name_exists_in_superclass(db, index, &parsed, file_scope_id) |
There was a problem hiding this comment.
This check still allows false positives in cases where a superclass method may have a simpler implementation that doesn't use all parameters, but the parameters are still needed for use by some subclass overrides. If we want to avoid these false positives, then we can only consider parameters unused if the method or class is final.
On the other hand, it seems like pylance doesn't care about either kind of false positive. It will happily mark a parameter as unused even if it is part of the signature of an override method. So if we want to follow pylance's lead, we could also not care about that case, and eliminate a bunch of extra code here.
There was a problem hiding this comment.
@carljm I addressed your other suggestions. On this point, I'm also fine with removing this suppression and simplifying the code if we want to align with Pylance. Since this overlaps with @MichaReiser's earlier concern as well, I will wait for both of your views before changing it.
There was a problem hiding this comment.
With the most recent changes here this looks merge-ready to me, but I'm curious for @MichaReiser thoughts on this. Should we follow pylance, keep things simple, and mark parameters unused regardless of override possibilities? Is there a strong rationale for avoiding the false positives only in the "unused in override" case, but not in the "unused in base method" case (as the PR currently does)?
There was a problem hiding this comment.
unused in base method" case (as the PR currently does)?
I don't think this is intentional
Given that these are only hints, having false positives does very little harm. But we should probably revisit this if we decide to promote those checks to lint rules where it would become annoying.
So I'm fine removing the extra complexity but I also don't see much harm in having them. @denyszhak I let you make the final call. Let me know when it's ready to merge.
There was a problem hiding this comment.
Yes, that wasn't intentional. Since you both seemed fine with the simpler behavior, I removed that suppression for consistency. If we revisit this later, I think it should be addressed more holistically, since doing it properly would likely require more sophisticated logic and doesn't seem worth the complexity for a hint level feature.
There was a problem hiding this comment.
@MichaReiser I pushed the change if you can give it a final look before the merge
carljm
left a comment
There was a problem hiding this comment.
I only re-reviewed the unused_bindings.rs module, but it looks good to me now.
Just one question above for @MichaReiser
|
Thank you @denyszhak for your persistent and solid work on this PR! |
|
Awesome work. Thank you so much. |
* main: (40 commits) [ty] resolve union-likes in emitting union attribute errors (#24263) [ty] Improve support for `Callable` type context (#23888) [ty] Propagate type context through `await` expressions (#24256) [`pyflakes`] Flag annotated variable redeclarations as `F811` in preview mode (#24244) [ty] Preserve `Divergent` when materializing recursive aliases (#24245) Fix W391 fixes for consecutive empty notebook cells (#24236) [flake8-bugbear] Clarify RUF071 fix safety for non-path string comparisons (#24149) [ty] Ban type qualifiers in PEP-695 type aliases (#24242) [ty] Include keyword-prefixed symbols in completions for attributes (#24232) [ty] Add tests for TypedDict method overloads on unions (#24230) [ty] report unused bindings as unnecessary hint diagnostics (#23305) Remove unused `non_root` variable (#24238) Extend F507 to flag %-format strings with zero placeholders (#24215) [`flake8-simplify`] Suppress `SIM105` for `except*` before Python 3.12 (#23869) Ignore pre-initialization references in SIM113 (#24235) Parenthesize expression in RUF050 fix (#24234) Publish playgrounds using the `release-playground` environment (#24223) [ty] Fix instance-attribute lookup in methods of protocol classes (#24213) [ty] Used shared expression cache during generic call inference (#24219) [ty] make `Type::BoundMethod` include instances of same-named methods bound to a subclass (#24039) ...
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