From b4c8f2560487515857512404d4302e77b63456ec Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Mon, 16 Feb 2026 01:21:35 +0100 Subject: [PATCH 01/22] ty: report unused bindings as unnecessary hint diagnostics --- crates/ty_python_semantic/src/types.rs | 1 + .../src/types/unused_bindings.rs | 413 ++++++++++++++++++ .../ty_server/src/server/api/diagnostics.rs | 139 +++++- .../api/requests/workspace_diagnostic.rs | 31 +- .../ty_server/tests/e2e/pull_diagnostics.rs | 56 +++ ...used_binding_has_unnecessary_hint_tag.snap | 29 ++ ...space_reports_unused_binding_hint_tag.snap | 35 ++ 7 files changed, 677 insertions(+), 27 deletions(-) create mode 100644 crates/ty_python_semantic/src/types/unused_bindings.rs create mode 100644 crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__unused_binding_has_unnecessary_hint_tag.snap create mode 100644 crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__workspace_reports_unused_binding_hint_tag.snap diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index 0656296c085b6..fda88ba9ddea1 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -137,6 +137,7 @@ mod type_alias; mod typed_dict; mod typevar; mod unpacker; +pub mod unused_bindings; mod variance; mod visitor; diff --git a/crates/ty_python_semantic/src/types/unused_bindings.rs b/crates/ty_python_semantic/src/types/unused_bindings.rs new file mode 100644 index 0000000000000..ef85904a40f44 --- /dev/null +++ b/crates/ty_python_semantic/src/types/unused_bindings.rs @@ -0,0 +1,413 @@ +use crate::semantic_index::scope::ScopeKind; +use crate::{Db, SemanticModel}; +use ruff_db::parsed::parsed_module; +use ruff_python_ast::visitor::source_order::{ + SourceOrderVisitor, walk_expr, walk_parameter, walk_parameter_with_default, walk_pattern, + walk_stmt, +}; +use ruff_python_ast::{self as ast}; +use ruff_text_size::{Ranged, TextRange}; + +fn is_dunder_name(name: &str) -> bool { + name.len() > 4 && name.starts_with("__") && name.ends_with("__") +} + +fn should_mark_unnecessary(scope_kind: ScopeKind, name: &str) -> bool { + if name.starts_with('_') || matches!(name, "self" | "cls") || is_dunder_name(name) { + return false; + } + + // Keep this local-scope only to avoid false positives for bindings that can + // be observed or referenced indirectly from module/class contexts. + match scope_kind { + ScopeKind::Function | ScopeKind::Lambda | ScopeKind::Comprehension => true, + ScopeKind::Module | ScopeKind::Class | ScopeKind::TypeParams | ScopeKind::TypeAlias => { + false + } + } +} + +/// Check whether a symbol is unused within its containing scope and should be marked as unnecessary. +/// Returns `Some(true)` if unused and should be marked, `Some(false)` otherwise, or `None` if the symbol cannot be found. +fn is_symbol_unnecessary_in_scope( + model: &SemanticModel<'_>, + scope_node: ast::AnyNodeRef<'_>, + name: &str, +) -> Option { + let file = model.file(); + let file_scope = model.scope(scope_node)?; + let index = crate::semantic_index::semantic_index(model.db(), file); + let scope = index.scope(file_scope); + + if !should_mark_unnecessary(scope.kind(), name) { + return Some(false); + } + + let place_table = index.place_table(file_scope); + let symbol_id = place_table.symbol_id(name)?; + let symbol = place_table.symbol(symbol_id); + + // Global and nonlocal assignments target bindings from outer scopes. + // Treat them as externally managed to avoid false positives here. + if symbol.is_global() || symbol.is_nonlocal() { + return Some(false); + } + + Some(!symbol.is_used()) +} + +#[derive(Debug, Clone, Eq, PartialEq, Hash)] +pub struct UnusedBinding { + pub range: TextRange, + pub name: String, +} + +#[salsa::tracked(returns(ref))] +pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec { + let parsed = parsed_module(db, file).load(db); + let model = SemanticModel::new(db, file); + + let mut collector = UnusedBindingCollector::new(&model); + collector.visit_body(parsed.suite()); + collector + .unused_bindings + .sort_unstable_by_key(|binding| binding.range.start()); + collector + .unused_bindings + .dedup_by_key(|binding| binding.range); + + collector.unused_bindings +} + +struct UnusedBindingCollector<'db> { + model: &'db SemanticModel<'db>, + unused_bindings: Vec, + in_target_creating_definition: bool, +} + +impl<'db> UnusedBindingCollector<'db> { + fn new(model: &'db SemanticModel<'db>) -> Self { + Self { + model, + unused_bindings: Vec::new(), + in_target_creating_definition: false, + } + } + + fn add_unused_binding(&mut self, range: TextRange, name: &str) { + self.unused_bindings.push(UnusedBinding { + range, + name: name.to_string(), + }); + } + + fn mark_pattern_binding_if_unused(&mut self, name: &ast::Identifier) { + if let Some(true) = is_symbol_unnecessary_in_scope( + self.model, + ast::AnyNodeRef::from(name), + name.id.as_str(), + ) { + self.add_unused_binding(name.range(), name.id.as_str()); + } + } + + fn with_target_creating_definition(&mut self, f: impl FnOnce(&mut Self)) { + let prev = self.in_target_creating_definition; + self.in_target_creating_definition = true; + f(self); + self.in_target_creating_definition = prev; + } +} + +impl SourceOrderVisitor<'_> for UnusedBindingCollector<'_> { + fn visit_stmt(&mut self, stmt: &ast::Stmt) { + match stmt { + ast::Stmt::Assign(assignment) => { + self.with_target_creating_definition(|this| { + for target in &assignment.targets { + this.visit_expr(target); + } + }); + + self.visit_expr(&assignment.value); + } + ast::Stmt::AnnAssign(assignment) => { + self.with_target_creating_definition(|this| { + this.visit_expr(&assignment.target); + }); + + self.visit_expr(&assignment.annotation); + if let Some(value) = &assignment.value { + self.visit_expr(value); + } + } + ast::Stmt::For(for_stmt) => { + self.with_target_creating_definition(|this| { + this.visit_expr(&for_stmt.target); + }); + + self.visit_expr(&for_stmt.iter); + self.visit_body(&for_stmt.body); + self.visit_body(&for_stmt.orelse); + } + ast::Stmt::With(with_stmt) => { + for item in &with_stmt.items { + self.visit_expr(&item.context_expr); + if let Some(expr) = &item.optional_vars { + self.with_target_creating_definition(|this| { + this.visit_expr(expr); + }); + } + } + + self.visit_body(&with_stmt.body); + } + ast::Stmt::Try(try_stmt) => { + self.visit_body(&try_stmt.body); + for handler in &try_stmt.handlers { + match handler { + ast::ExceptHandler::ExceptHandler(except_handler) => { + if let Some(expr) = &except_handler.type_ { + self.visit_expr(expr); + } + if let Some(name) = &except_handler.name + && let Some(true) = is_symbol_unnecessary_in_scope( + self.model, + ast::AnyNodeRef::from(except_handler), + name.id.as_str(), + ) + { + self.add_unused_binding(name.range(), name.id.as_str()); + } + self.visit_body(&except_handler.body); + } + } + } + self.visit_body(&try_stmt.orelse); + self.visit_body(&try_stmt.finalbody); + } + _ => walk_stmt(self, stmt), + } + } + + fn visit_expr(&mut self, expr: &ast::Expr) { + match expr { + ast::Expr::Name(name) => { + if self.in_target_creating_definition + && name.ctx.is_store() + && let Some(true) = is_symbol_unnecessary_in_scope( + self.model, + ast::AnyNodeRef::from(name), + name.id.as_str(), + ) + { + self.add_unused_binding(name.range(), name.id.as_str()); + } + walk_expr(self, expr); + } + ast::Expr::Named(named) => { + self.with_target_creating_definition(|this| { + this.visit_expr(&named.target); + }); + + self.visit_expr(&named.value); + } + _ => walk_expr(self, expr), + } + } + + fn visit_pattern(&mut self, pattern: &ast::Pattern) { + match pattern { + ast::Pattern::MatchAs(pattern_as) => { + if let Some(nested_pattern) = &pattern_as.pattern { + self.visit_pattern(nested_pattern); + } + if let Some(name) = &pattern_as.name { + self.mark_pattern_binding_if_unused(name); + } + } + ast::Pattern::MatchMapping(pattern_mapping) => { + for (key, nested_pattern) in + pattern_mapping.keys.iter().zip(&pattern_mapping.patterns) + { + self.visit_expr(key); + self.visit_pattern(nested_pattern); + } + if let Some(rest_name) = &pattern_mapping.rest { + self.mark_pattern_binding_if_unused(rest_name); + } + } + ast::Pattern::MatchStar(pattern_star) => { + if let Some(rest_name) = &pattern_star.name { + self.mark_pattern_binding_if_unused(rest_name); + } + } + _ => walk_pattern(self, pattern), + } + } + + fn visit_comprehension(&mut self, comprehension: &ast::Comprehension) { + self.with_target_creating_definition(|this| { + this.visit_expr(&comprehension.target); + }); + + self.visit_expr(&comprehension.iter); + for if_clause in &comprehension.ifs { + self.visit_expr(if_clause); + } + } + + fn visit_parameter(&mut self, parameter: &ast::Parameter) { + if let Some(true) = is_symbol_unnecessary_in_scope( + self.model, + ast::AnyNodeRef::from(parameter), + parameter.name.id.as_str(), + ) { + self.add_unused_binding(parameter.name.range(), parameter.name.id.as_str()); + } + walk_parameter(self, parameter); + } + + fn visit_parameter_with_default(&mut self, parameter_with_default: &ast::ParameterWithDefault) { + if let Some(true) = is_symbol_unnecessary_in_scope( + self.model, + ast::AnyNodeRef::from(parameter_with_default), + parameter_with_default.name().id.as_str(), + ) { + self.add_unused_binding( + parameter_with_default.name().range(), + parameter_with_default.name().id.as_str(), + ); + } + walk_parameter_with_default(self, parameter_with_default); + } +} + +#[cfg(test)] +mod tests { + use super::unused_bindings; + use crate::db::tests::TestDbBuilder; + use ruff_db::files::system_path_to_file; + + fn collect_unused_names(source: &str) -> anyhow::Result> { + let db = TestDbBuilder::new() + .with_file("/src/main.py", source) + .build()?; + let file = system_path_to_file(&db, "/src/main.py").unwrap(); + let mut names = unused_bindings(&db, file) + .iter() + .map(|binding| binding.name.clone()) + .collect::>(); + names.sort(); + Ok(names) + } + + #[test] + fn captures_safe_local_binding_kinds() -> anyhow::Result<()> { + let source = r#"def f(): + used_assign, dead_assign = (1, 2) + print(used_assign) + + for used_loop, dead_loop in [(1, 2)]: + print(used_loop) + + with open("x") as dead_with: + pass + + try: + 1 / 0 + except Exception as dead_exc: + pass + + if (dead_walrus := 1): + pass + + [1 for dead_comp in range(3)] + [ok_comp for ok_comp, dead_comp2 in [(1, 2)]] + + match {"x": 1, "y": 2}: + case {"x": used_match, **dead_rest}: + print(used_match) + case [used_star, *dead_star] as dead_as: + print(used_star) +"#; + + let names = collect_unused_names(source)?; + assert_eq!( + names, + vec![ + "dead_as", + "dead_assign", + "dead_comp", + "dead_comp2", + "dead_exc", + "dead_loop", + "dead_rest", + "dead_star", + "dead_walrus", + "dead_with", + ] + ); + Ok(()) + } + + #[test] + fn skips_module_class_placeholder_and_dunder_bindings() -> anyhow::Result<()> { + let source = r#"_module_dead = 1 + +class C: + __private_dead = 1 + + def method(self): + local_dead = 1 + _ = 2 + __dunder__ = 3 + return 0 +"#; + + let names = collect_unused_names(source)?; + assert_eq!(names, vec!["local_dead"]); + Ok(()) + } + + #[test] + fn skips_global_and_nonlocal_assignments() -> anyhow::Result<()> { + let source = r#"global_value = 0 + +def mutate_global(): + global global_value + global_value = 1 + local_dead = 1 + +def outer(): + captured = 0 + + def inner(): + nonlocal captured + captured = 1 + + inner() + return captured +"#; + + let names = collect_unused_names(source)?; + assert_eq!(names, vec!["local_dead"]); + Ok(()) + } + + #[test] + fn captures_unused_function_and_lambda_parameters() -> anyhow::Result<()> { + let source = r#"def fn(used, dead, _ignored, __dunder__): + return used + +def fn_defaults(a, b=1, *, c=2, d): + return a + d + +lam = lambda x, y, z=1: x + z +"#; + + let names = collect_unused_names(source)?; + assert_eq!(names, vec!["b", "c", "dead", "y"]); + Ok(()) + } +} diff --git a/crates/ty_server/src/server/api/diagnostics.rs b/crates/ty_server/src/server/api/diagnostics.rs index c687b65fd8e2f..516268187affa 100644 --- a/crates/ty_server/src/server/api/diagnostics.rs +++ b/crates/ty_server/src/server/api/diagnostics.rs @@ -9,6 +9,7 @@ use lsp_types::{ use ruff_diagnostics::Applicability; use ruff_text_size::Ranged; use rustc_hash::FxHashMap; +use ty_python_semantic::types::unused_bindings::unused_bindings; use ruff_db::diagnostic::{Annotation, Severity, SubDiagnostic}; use ruff_db::files::{File, FileRange}; @@ -24,29 +25,40 @@ use crate::system::{AnySystemPath, file_to_url}; use crate::{DIAGNOSTIC_NAME, Db, DiagnosticMode}; use crate::{PositionEncoding, Session}; +const UNUSED_BINDING_CODE: &str = "unused-binding"; + #[derive(Debug)] pub(super) struct Diagnostics { items: Vec, + unnecessary_items: Vec, encoding: PositionEncoding, file_or_notebook: File, } +#[derive(Debug, Clone, Eq, PartialEq, Hash)] +pub(super) struct UnnecessaryBindingDiagnostic { + range: ruff_text_size::TextRange, + name: String, +} + impl Diagnostics { /// Computes the result ID for `diagnostics`. /// /// Returns `None` if there are no diagnostics. pub(super) fn result_id_from_hash( diagnostics: &[ruff_db::diagnostic::Diagnostic], + unnecessary_items: &[UnnecessaryBindingDiagnostic], ) -> Option { - if diagnostics.is_empty() { + if diagnostics.is_empty() && unnecessary_items.is_empty() { return None; } - // Generate result ID based on raw diagnostic content only + // Generate result ID based on raw diagnostic content only. let mut hasher = DefaultHasher::new(); - // Hash the length first to ensure different numbers of diagnostics produce different hashes + // Hash lengths first to ensure different numbers of diagnostics produce different hashes. diagnostics.hash(&mut hasher); + unnecessary_items.hash(&mut hasher); Some(format!("{:016x}", hasher.finish())) } @@ -55,7 +67,7 @@ impl Diagnostics { /// /// Returns `None` if there are no diagnostics. pub(super) fn result_id(&self) -> Option { - Self::result_id_from_hash(&self.items) + Self::result_id_from_hash(&self.items, &self.unnecessary_items) } pub(super) fn to_lsp_diagnostics( @@ -95,25 +107,59 @@ impl Diagnostics { .push(lsp_diagnostic); } + for unnecessary in &self.unnecessary_items { + let Some((url, lsp_diagnostic)) = to_lsp_unnecessary_diagnostic( + db, + self.file_or_notebook, + unnecessary, + self.encoding, + ) else { + continue; + }; + + let Some(url) = url else { + tracing::warn!("Unable to find notebook cell"); + continue; + }; + + cell_diagnostics + .entry(url) + .or_default() + .push(lsp_diagnostic); + } + LspDiagnostics::NotebookDocument(cell_diagnostics) } else { - LspDiagnostics::TextDocument( - self.items - .iter() - .filter_map(|diagnostic| { - Some( - to_lsp_diagnostic( - db, - diagnostic, - self.encoding, - client_capabilities, - global_settings, - )? - .1, - ) - }) - .collect(), - ) + let mut diagnostics: Vec = self + .items + .iter() + .filter_map(|diagnostic| { + Some( + to_lsp_diagnostic( + db, + diagnostic, + self.encoding, + client_capabilities, + global_settings, + )? + .1, + ) + }) + .collect(); + + diagnostics.extend(self.unnecessary_items.iter().filter_map(|unnecessary| { + Some( + to_lsp_unnecessary_diagnostic( + db, + self.file_or_notebook, + unnecessary, + self.encoding, + )? + .1, + ) + })); + + LspDiagnostics::TextDocument(diagnostics) } } } @@ -326,14 +372,33 @@ pub(super) fn compute_diagnostics( }; let diagnostics = db.check_file(file); + let unnecessary_items = if db.project().should_check_file(db, file) { + unnecessary_binding_diagnostics(db, file) + } else { + Vec::new() + }; Some(Diagnostics { items: diagnostics, + unnecessary_items, encoding, file_or_notebook: file, }) } +pub(super) fn unnecessary_binding_diagnostics( + db: &ProjectDatabase, + file: File, +) -> Vec { + unused_bindings(db, file) + .iter() + .map(|binding| UnnecessaryBindingDiagnostic { + range: binding.range, + name: binding.name.clone(), + }) + .collect() +} + /// Converts the tool specific [`Diagnostic`][ruff_db::diagnostic::Diagnostic] to an LSP /// [`Diagnostic`]. pub(super) fn to_lsp_diagnostic( @@ -448,6 +513,38 @@ pub(super) fn to_lsp_diagnostic( )) } +pub(super) fn to_lsp_unnecessary_diagnostic( + db: &dyn Db, + file: File, + unnecessary: &UnnecessaryBindingDiagnostic, + encoding: PositionEncoding, +) -> Option<(Option, Diagnostic)> { + let location = unnecessary + .range + .to_lsp_range(db, file, encoding)? + .to_location(); + + let (range, url) = match location { + Some(location) => (location.range, Some(location.uri)), + None => (lsp_types::Range::default(), None), + }; + + Some(( + url, + Diagnostic { + range, + severity: Some(DiagnosticSeverity::HINT), + tags: Some(vec![DiagnosticTag::UNNECESSARY]), + code: Some(NumberOrString::String(UNUSED_BINDING_CODE.into())), + code_description: None, + source: Some(DIAGNOSTIC_NAME.into()), + message: format!("Binding `{}` is unused", unnecessary.name), + related_information: None, + data: None, + }, + )) +} + /// Converts an [`Annotation`] to a [`DiagnosticRelatedInformation`]. fn annotation_to_related_information( db: &dyn Db, diff --git a/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs b/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs index 54d323a3bd9a2..912c103a32c28 100644 --- a/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs +++ b/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs @@ -21,7 +21,10 @@ use ty_project::{ProgressReporter, ProjectDatabase}; use crate::PositionEncoding; use crate::capabilities::ResolvedClientCapabilities; use crate::document::DocumentKey; -use crate::server::api::diagnostics::{Diagnostics, to_lsp_diagnostic}; +use crate::server::api::diagnostics::{ + Diagnostics, UnnecessaryBindingDiagnostic, to_lsp_diagnostic, to_lsp_unnecessary_diagnostic, + unnecessary_binding_diagnostics, +}; use crate::server::api::traits::{ BackgroundRequestHandler, RequestHandler, RetriableRequestHandler, }; @@ -252,13 +255,15 @@ impl ProgressReporter for WorkspaceDiagnosticsProgressReporter<'_> { state.report_progress(&self.work_done); } + let unnecessary = unnecessary_binding_diagnostics(db, file); + // Don't report empty diagnostics. We clear previous diagnostics in `into_response` // which also handles the case where a file no longer has diagnostics because // it's no longer part of the project. - if !diagnostics.is_empty() { + if !diagnostics.is_empty() || !unnecessary.is_empty() { state .response - .write_diagnostics_for_file(db, file, diagnostics); + .write_diagnostics_for_file(db, file, diagnostics, &unnecessary); } state.response.maybe_flush(); @@ -281,7 +286,7 @@ impl ProgressReporter for WorkspaceDiagnosticsProgressReporter<'_> { let response = &mut self.state.get_mut().unwrap().response; for (file, diagnostics) in by_file { - response.write_diagnostics_for_file(db, file, &diagnostics); + response.write_diagnostics_for_file(db, file, &diagnostics, &[]); } response.maybe_flush(); } @@ -371,6 +376,7 @@ impl<'a> ResponseWriter<'a> { db: &ProjectDatabase, file: File, diagnostics: &[Diagnostic], + unnecessary: &[UnnecessaryBindingDiagnostic], ) { let Some(url) = file_to_url(db, file) else { tracing::debug!("Failed to convert file path to URL at {}", file.path(db)); @@ -392,7 +398,7 @@ impl<'a> ResponseWriter<'a> { .map(|doc| i64::from(doc.version())) .ok(); - let result_id = Diagnostics::result_id_from_hash(diagnostics); + let result_id = Diagnostics::result_id_from_hash(diagnostics, unnecessary); let previous_result_id = self.previous_result_ids.remove(&key).map(|(_url, id)| id); @@ -425,6 +431,19 @@ impl<'a> ResponseWriter<'a> { }) .collect::>(); + let mut lsp_diagnostics = lsp_diagnostics; + lsp_diagnostics.extend(unnecessary.iter().filter_map(|diagnostic| { + Some( + to_lsp_unnecessary_diagnostic( + db, + file, + diagnostic, + self.position_encoding, + )? + .1, + ) + })); + WorkspaceDocumentDiagnosticReport::Full(WorkspaceFullDocumentDiagnosticReport { uri: url, version, @@ -477,7 +496,7 @@ impl<'a> ResponseWriter<'a> { .ok() .map(|doc| i64::from(doc.version())); - let new_result_id = Diagnostics::result_id_from_hash(&[]); + let new_result_id = Diagnostics::result_id_from_hash(&[], &[]); let report = match new_result_id { Some(new_id) if new_id == previous_result_id => { diff --git a/crates/ty_server/tests/e2e/pull_diagnostics.rs b/crates/ty_server/tests/e2e/pull_diagnostics.rs index dc749a046d684..54c34dbaaa9de 100644 --- a/crates/ty_server/tests/e2e/pull_diagnostics.rs +++ b/crates/ty_server/tests/e2e/pull_diagnostics.rs @@ -40,6 +40,62 @@ def foo() -> str: Ok(()) } +#[test] +fn unused_binding_has_unnecessary_hint_tag() -> Result<()> { + let _filter = filter_result_id(); + + let workspace_root = SystemPath::new("src"); + let foo = SystemPath::new("src/foo.py"); + let foo_content = "\ +def foo(): + x = 1 + return 0 +"; + + let mut server = TestServerBuilder::new()? + .with_workspace(workspace_root, None)? + .with_file(foo, foo_content)? + .enable_pull_diagnostics(true) + .build() + .wait_until_workspaces_are_initialized(); + + server.open_text_document(foo, foo_content, 1); + let diagnostics = server.document_diagnostic_request(foo, None); + + assert_compact_json_snapshot!(diagnostics); + + Ok(()) +} + +#[test] +fn workspace_reports_unused_binding_hint_tag() -> Result<()> { + let _filter = filter_result_id(); + + let workspace_root = SystemPath::new("src"); + let foo = SystemPath::new("src/foo.py"); + let foo_content = "\ +def foo(): + x = 1 + return 0 +"; + + let mut server = TestServerBuilder::new()? + .with_workspace( + workspace_root, + Some(ClientOptions::default().with_diagnostic_mode(DiagnosticMode::Workspace)), + )? + .with_file(foo, foo_content)? + .enable_pull_diagnostics(true) + .build() + .wait_until_workspaces_are_initialized(); + + let diagnostics = server.workspace_diagnostic_request(None, None); + + assert_compact_json_snapshot!(diagnostics); + + Ok(()) +} + #[test] fn on_did_open_diagnostics_off() -> Result<()> { let _filter = filter_result_id(); diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__unused_binding_has_unnecessary_hint_tag.snap b/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__unused_binding_has_unnecessary_hint_tag.snap new file mode 100644 index 0000000000000..dc6fe5610a8d2 --- /dev/null +++ b/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__unused_binding_has_unnecessary_hint_tag.snap @@ -0,0 +1,29 @@ +--- +source: crates/ty_server/tests/e2e/pull_diagnostics.rs +expression: diagnostics +--- +{ + "kind": "full", + "resultId": "[RESULT_ID]", + "items": [ + { + "range": { + "start": { + "line": 1, + "character": 4 + }, + "end": { + "line": 1, + "character": 5 + } + }, + "severity": 4, + "code": "unused-binding", + "source": "ty", + "message": "Binding `x` is unused", + "tags": [ + 1 + ] + } + ] +} diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__workspace_reports_unused_binding_hint_tag.snap b/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__workspace_reports_unused_binding_hint_tag.snap new file mode 100644 index 0000000000000..2e056552f399f --- /dev/null +++ b/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__workspace_reports_unused_binding_hint_tag.snap @@ -0,0 +1,35 @@ +--- +source: crates/ty_server/tests/e2e/pull_diagnostics.rs +expression: diagnostics +--- +{ + "items": [ + { + "kind": "full", + "uri": "file:///src/foo.py", + "version": null, + "resultId": "[RESULT_ID]", + "items": [ + { + "range": { + "start": { + "line": 1, + "character": 4 + }, + "end": { + "line": 1, + "character": 5 + } + }, + "severity": 4, + "code": "unused-binding", + "source": "ty", + "message": "Binding `x` is unused", + "tags": [ + 1 + ] + } + ] + } + ] +} From 34f5f976631e3d5b55da9de078fd88439b5e714e Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Mon, 16 Feb 2026 02:01:04 +0100 Subject: [PATCH 02/22] ty: guard analysis when errors reported like during partial assignment --- crates/ty_python_semantic/src/types/unused_bindings.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/ty_python_semantic/src/types/unused_bindings.rs b/crates/ty_python_semantic/src/types/unused_bindings.rs index ef85904a40f44..c21989889e181 100644 --- a/crates/ty_python_semantic/src/types/unused_bindings.rs +++ b/crates/ty_python_semantic/src/types/unused_bindings.rs @@ -65,6 +65,10 @@ pub struct UnusedBinding { #[salsa::tracked(returns(ref))] pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec { let parsed = parsed_module(db, file).load(db); + if !parsed.errors().is_empty() || !parsed.unsupported_syntax_errors().is_empty() { + return Vec::new(); + } + let model = SemanticModel::new(db, file); let mut collector = UnusedBindingCollector::new(&model); From 3a4f0ee8cf409f3a7709c9876863f41d6da81824 Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Tue, 17 Feb 2026 00:51:58 +0100 Subject: [PATCH 03/22] ty: refactor to derive unused-binding diagnostics from use-def map --- .../src/semantic_index/use_def.rs | 55 +++- .../src/types/unused_bindings.rs | 304 +++++------------- 2 files changed, 120 insertions(+), 239 deletions(-) diff --git a/crates/ty_python_semantic/src/semantic_index/use_def.rs b/crates/ty_python_semantic/src/semantic_index/use_def.rs index 7e0f60c38396a..3bbea72613783 100644 --- a/crates/ty_python_semantic/src/semantic_index/use_def.rs +++ b/crates/ty_python_semantic/src/semantic_index/use_def.rs @@ -307,6 +307,11 @@ pub(crate) struct UseDefMap<'db> { /// this represents the implicit "unbound"/"undeclared" definition of every place. all_definitions: IndexVec>, + /// A bitset-like map indicating whether each binding definition has at least one use. + /// + /// This uses the same index as `all_definitions`. + used_bindings: IndexVec, + /// Array of predicates in this scope. predicates: Predicates<'db>, @@ -394,6 +399,14 @@ pub(crate) enum ApplicableConstraints<'map, 'db> { } impl<'db> UseDefMap<'db> { + pub(crate) fn all_definitions_with_usage( + &self, + ) -> impl Iterator, bool)> + '_ { + self.all_definitions + .iter_enumerated() + .map(|(id, &state)| (id, state, self.used_bindings[id])) + } + pub(crate) fn bindings_at_use( &self, use_id: ScopedUseId, @@ -895,6 +908,11 @@ pub(super) struct UseDefMapBuilder<'db> { /// Append-only array of [`DefinitionState`]. all_definitions: IndexVec>, + /// Tracks whether each binding definition has at least one use. + /// + /// Uses the same index as `all_definitions`. + used_bindings: IndexVec, + /// Builder of predicates. pub(super) predicates: PredicatesBuilder<'db>, @@ -947,6 +965,7 @@ impl<'db> UseDefMapBuilder<'db> { pub(super) fn new(is_class_scope: bool) -> Self { Self { all_definitions: IndexVec::from_iter([DefinitionState::Undefined]), + used_bindings: IndexVec::from_iter([false]), predicates: PredicatesBuilder::default(), reachability_constraints: ReachabilityConstraintsBuilder::default(), bindings_by_use: IndexVec::new(), @@ -964,6 +983,13 @@ impl<'db> UseDefMapBuilder<'db> { } } + fn push_definition_state(&mut self, state: DefinitionState<'db>) -> ScopedDefinitionId { + let def_id = self.all_definitions.push(state); + let used_id = self.used_bindings.push(false); + debug_assert_eq!(def_id, used_id); + def_id + } + pub(super) fn mark_unreachable(&mut self) { self.reachability = ScopedReachabilityConstraintId::ALWAYS_FALSE; @@ -1031,7 +1057,7 @@ impl<'db> UseDefMapBuilder<'db> { self.bindings_by_definition .insert(binding, bindings.clone()); - let def_id = self.all_definitions.push(DefinitionState::Defined(binding)); + let def_id = self.push_definition_state(DefinitionState::Defined(binding)); let place_state = match place { ScopedPlaceId::Symbol(symbol) => &mut self.symbol_states[symbol], ScopedPlaceId::Member(member) => &mut self.member_states[member], @@ -1279,9 +1305,7 @@ impl<'db> UseDefMapBuilder<'db> { place: ScopedPlaceId, declaration: Definition<'db>, ) { - let def_id = self - .all_definitions - .push(DefinitionState::Defined(declaration)); + let def_id = self.push_definition_state(DefinitionState::Defined(declaration)); let place_state = match place { ScopedPlaceId::Symbol(symbol) => &mut self.symbol_states[symbol], @@ -1311,9 +1335,7 @@ impl<'db> UseDefMapBuilder<'db> { ) { // We don't need to store anything in self.bindings_by_declaration or // self.declarations_by_binding. - let def_id = self - .all_definitions - .push(DefinitionState::Defined(definition)); + let def_id = self.push_definition_state(DefinitionState::Defined(definition)); let place_state = match place { ScopedPlaceId::Symbol(symbol) => &mut self.symbol_states[symbol], ScopedPlaceId::Member(member) => &mut self.member_states[member], @@ -1347,7 +1369,7 @@ impl<'db> UseDefMapBuilder<'db> { } pub(super) fn delete_binding(&mut self, place: ScopedPlaceId) { - let def_id = self.all_definitions.push(DefinitionState::Deleted); + let def_id = self.push_definition_state(DefinitionState::Deleted); let place_state = match place { ScopedPlaceId::Symbol(symbol) => &mut self.symbol_states[symbol], ScopedPlaceId::Member(member) => &mut self.member_states[member], @@ -1397,6 +1419,21 @@ impl<'db> UseDefMapBuilder<'db> { // as the live bindings for this use. let new_use = self.bindings_by_use.push(bindings); debug_assert_eq!(use_id, new_use); + + for live_binding in bindings.iter() { + let definition_id = live_binding.binding; + + if definition_id.is_unbound() { + continue; + } + + if matches!( + self.all_definitions[definition_id], + DefinitionState::Defined(_) + ) { + self.used_bindings[definition_id] = true; + } + } } pub(super) fn record_range_reachability(&mut self, range: TextRange) { @@ -1565,6 +1602,7 @@ impl<'db> UseDefMapBuilder<'db> { pub(super) fn finish(mut self) -> UseDefMap<'db> { self.all_definitions.shrink_to_fit(); + self.used_bindings.shrink_to_fit(); self.symbol_states.shrink_to_fit(); self.member_states.shrink_to_fit(); self.reachable_symbol_definitions.shrink_to_fit(); @@ -1657,6 +1695,7 @@ impl<'db> UseDefMapBuilder<'db> { UseDefMap { all_definitions: self.all_definitions, + used_bindings: self.used_bindings, predicates: self.predicates.build(), reachability_constraints: self.reachability_constraints.build(), interned_bindings, diff --git a/crates/ty_python_semantic/src/types/unused_bindings.rs b/crates/ty_python_semantic/src/types/unused_bindings.rs index c21989889e181..645f99aa3ad3e 100644 --- a/crates/ty_python_semantic/src/types/unused_bindings.rs +++ b/crates/ty_python_semantic/src/types/unused_bindings.rs @@ -1,11 +1,14 @@ +//! Collects unused local bindings for IDE-facing diagnostics. +//! +//! This intentionally reports only function-, lambda-, and comprehension-scope bindings. +//! Module and class bindings can be observed indirectly (e.g., imports, attribute access), so +//! reporting them here risks false positives without cross-file/reference analysis. + +use crate::semantic_index::definition::{DefinitionKind, DefinitionState}; +use crate::semantic_index::place::ScopedPlaceId; use crate::semantic_index::scope::ScopeKind; -use crate::{Db, SemanticModel}; +use crate::{Db, semantic_index}; use ruff_db::parsed::parsed_module; -use ruff_python_ast::visitor::source_order::{ - SourceOrderVisitor, walk_expr, walk_parameter, walk_parameter_with_default, walk_pattern, - walk_stmt, -}; -use ruff_python_ast::{self as ast}; use ruff_text_size::{Ranged, TextRange}; fn is_dunder_name(name: &str) -> bool { @@ -17,8 +20,6 @@ fn should_mark_unnecessary(scope_kind: ScopeKind, name: &str) -> bool { return false; } - // Keep this local-scope only to avoid false positives for bindings that can - // be observed or referenced indirectly from module/class contexts. match scope_kind { ScopeKind::Function | ScopeKind::Lambda | ScopeKind::Comprehension => true, ScopeKind::Module | ScopeKind::Class | ScopeKind::TypeParams | ScopeKind::TypeAlias => { @@ -27,33 +28,21 @@ fn should_mark_unnecessary(scope_kind: ScopeKind, name: &str) -> bool { } } -/// Check whether a symbol is unused within its containing scope and should be marked as unnecessary. -/// Returns `Some(true)` if unused and should be marked, `Some(false)` otherwise, or `None` if the symbol cannot be found. -fn is_symbol_unnecessary_in_scope( - model: &SemanticModel<'_>, - scope_node: ast::AnyNodeRef<'_>, - name: &str, -) -> Option { - let file = model.file(); - let file_scope = model.scope(scope_node)?; - let index = crate::semantic_index::semantic_index(model.db(), file); - let scope = index.scope(file_scope); - - if !should_mark_unnecessary(scope.kind(), name) { - return Some(false); - } - - let place_table = index.place_table(file_scope); - let symbol_id = place_table.symbol_id(name)?; - let symbol = place_table.symbol(symbol_id); - - // Global and nonlocal assignments target bindings from outer scopes. - // Treat them as externally managed to avoid false positives here. - if symbol.is_global() || symbol.is_nonlocal() { - return Some(false); - } - - Some(!symbol.is_used()) +fn should_consider_definition(kind: &DefinitionKind<'_>) -> bool { + matches!( + kind, + DefinitionKind::NamedExpression(_) + | DefinitionKind::Assignment(_) + | DefinitionKind::AnnotatedAssignment(_) + | DefinitionKind::For(_) + | DefinitionKind::Comprehension(_) + | DefinitionKind::VariadicPositionalParameter(_) + | DefinitionKind::VariadicKeywordParameter(_) + | DefinitionKind::Parameter(_) + | DefinitionKind::WithItem(_) + | DefinitionKind::MatchPattern(_) + | DefinitionKind::ExceptHandler(_) + ) } #[derive(Debug, Clone, Eq, PartialEq, Hash)] @@ -69,222 +58,75 @@ pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec { - model: &'db SemanticModel<'db>, - unused_bindings: Vec, - in_target_creating_definition: bool, -} - -impl<'db> UnusedBindingCollector<'db> { - fn new(model: &'db SemanticModel<'db>) -> Self { - Self { - model, - unused_bindings: Vec::new(), - in_target_creating_definition: false, - } - } + let index = semantic_index::semantic_index(db, file); + let mut unused = Vec::new(); - fn add_unused_binding(&mut self, range: TextRange, name: &str) { - self.unused_bindings.push(UnusedBinding { - range, - name: name.to_string(), - }); - } + for scope_id in index.scope_ids() { + let file_scope = scope_id.file_scope_id(db); + let scope = index.scope(file_scope); - fn mark_pattern_binding_if_unused(&mut self, name: &ast::Identifier) { - if let Some(true) = is_symbol_unnecessary_in_scope( - self.model, - ast::AnyNodeRef::from(name), - name.id.as_str(), + if !matches!( + scope.kind(), + ScopeKind::Function | ScopeKind::Lambda | ScopeKind::Comprehension ) { - self.add_unused_binding(name.range(), name.id.as_str()); + continue; } - } - fn with_target_creating_definition(&mut self, f: impl FnOnce(&mut Self)) { - let prev = self.in_target_creating_definition; - self.in_target_creating_definition = true; - f(self); - self.in_target_creating_definition = prev; - } -} + let place_table = index.place_table(file_scope); + let use_def_map = index.use_def_map(file_scope); -impl SourceOrderVisitor<'_> for UnusedBindingCollector<'_> { - fn visit_stmt(&mut self, stmt: &ast::Stmt) { - match stmt { - ast::Stmt::Assign(assignment) => { - self.with_target_creating_definition(|this| { - for target in &assignment.targets { - this.visit_expr(target); - } - }); - - self.visit_expr(&assignment.value); - } - ast::Stmt::AnnAssign(assignment) => { - self.with_target_creating_definition(|this| { - this.visit_expr(&assignment.target); - }); - - self.visit_expr(&assignment.annotation); - if let Some(value) = &assignment.value { - self.visit_expr(value); - } - } - ast::Stmt::For(for_stmt) => { - self.with_target_creating_definition(|this| { - this.visit_expr(&for_stmt.target); - }); - - self.visit_expr(&for_stmt.iter); - self.visit_body(&for_stmt.body); - self.visit_body(&for_stmt.orelse); - } - ast::Stmt::With(with_stmt) => { - for item in &with_stmt.items { - self.visit_expr(&item.context_expr); - if let Some(expr) = &item.optional_vars { - self.with_target_creating_definition(|this| { - this.visit_expr(expr); - }); - } - } + for (_, state, is_used) in use_def_map.all_definitions_with_usage() { + let DefinitionState::Defined(definition) = state else { + continue; + }; - self.visit_body(&with_stmt.body); - } - ast::Stmt::Try(try_stmt) => { - self.visit_body(&try_stmt.body); - for handler in &try_stmt.handlers { - match handler { - ast::ExceptHandler::ExceptHandler(except_handler) => { - if let Some(expr) = &except_handler.type_ { - self.visit_expr(expr); - } - if let Some(name) = &except_handler.name - && let Some(true) = is_symbol_unnecessary_in_scope( - self.model, - ast::AnyNodeRef::from(except_handler), - name.id.as_str(), - ) - { - self.add_unused_binding(name.range(), name.id.as_str()); - } - self.visit_body(&except_handler.body); - } - } - } - self.visit_body(&try_stmt.orelse); - self.visit_body(&try_stmt.finalbody); + if is_used { + continue; } - _ => walk_stmt(self, stmt), - } - } - fn visit_expr(&mut self, expr: &ast::Expr) { - match expr { - ast::Expr::Name(name) => { - if self.in_target_creating_definition - && name.ctx.is_store() - && let Some(true) = is_symbol_unnecessary_in_scope( - self.model, - ast::AnyNodeRef::from(name), - name.id.as_str(), - ) - { - self.add_unused_binding(name.range(), name.id.as_str()); - } - walk_expr(self, expr); - } - ast::Expr::Named(named) => { - self.with_target_creating_definition(|this| { - this.visit_expr(&named.target); - }); + let ScopedPlaceId::Symbol(symbol_id) = definition.place(db) else { + continue; + }; - self.visit_expr(&named.value); - } - _ => walk_expr(self, expr), - } - } + let symbol = place_table.symbol(symbol_id); + let name = symbol.name().as_str(); - fn visit_pattern(&mut self, pattern: &ast::Pattern) { - match pattern { - ast::Pattern::MatchAs(pattern_as) => { - if let Some(nested_pattern) = &pattern_as.pattern { - self.visit_pattern(nested_pattern); - } - if let Some(name) = &pattern_as.name { - self.mark_pattern_binding_if_unused(name); - } + if !should_mark_unnecessary(scope.kind(), name) { + continue; } - ast::Pattern::MatchMapping(pattern_mapping) => { - for (key, nested_pattern) in - pattern_mapping.keys.iter().zip(&pattern_mapping.patterns) - { - self.visit_expr(key); - self.visit_pattern(nested_pattern); - } - if let Some(rest_name) = &pattern_mapping.rest { - self.mark_pattern_binding_if_unused(rest_name); - } + + // Global and nonlocal assignments target bindings from outer scopes. + // Treat them as externally managed to avoid false positives here. + if symbol.is_global() || symbol.is_nonlocal() { + continue; } - ast::Pattern::MatchStar(pattern_star) => { - if let Some(rest_name) = &pattern_star.name { - self.mark_pattern_binding_if_unused(rest_name); - } + + let kind = definition.kind(db); + if !should_consider_definition(kind) { + continue; } - _ => walk_pattern(self, pattern), - } - } - fn visit_comprehension(&mut self, comprehension: &ast::Comprehension) { - self.with_target_creating_definition(|this| { - this.visit_expr(&comprehension.target); - }); + let range = match kind { + DefinitionKind::ExceptHandler(handler) => { + let Some(name) = &handler.node(&parsed).name else { + continue; + }; + name.range() + } + _ => kind.target_range(&parsed), + }; - self.visit_expr(&comprehension.iter); - for if_clause in &comprehension.ifs { - self.visit_expr(if_clause); + unused.push(UnusedBinding { + range, + name: name.to_string(), + }); } } - fn visit_parameter(&mut self, parameter: &ast::Parameter) { - if let Some(true) = is_symbol_unnecessary_in_scope( - self.model, - ast::AnyNodeRef::from(parameter), - parameter.name.id.as_str(), - ) { - self.add_unused_binding(parameter.name.range(), parameter.name.id.as_str()); - } - walk_parameter(self, parameter); - } + unused.sort_unstable_by_key(|binding| binding.range.start()); + unused.dedup_by_key(|binding| binding.range); - fn visit_parameter_with_default(&mut self, parameter_with_default: &ast::ParameterWithDefault) { - if let Some(true) = is_symbol_unnecessary_in_scope( - self.model, - ast::AnyNodeRef::from(parameter_with_default), - parameter_with_default.name().id.as_str(), - ) { - self.add_unused_binding( - parameter_with_default.name().range(), - parameter_with_default.name().id.as_str(), - ); - } - walk_parameter_with_default(self, parameter_with_default); - } + unused } #[cfg(test)] From 683ee63d02ba2e941d6080c89a5b2779cb76ddf0 Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Tue, 17 Feb 2026 01:10:02 +0100 Subject: [PATCH 04/22] ty: tiny readability improvements --- .../src/semantic_index/definition.rs | 13 +++++++ .../src/types/unused_bindings.rs | 35 ++++++++----------- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/crates/ty_python_semantic/src/semantic_index/definition.rs b/crates/ty_python_semantic/src/semantic_index/definition.rs index 26c570e6458c8..2705e2101e0d3 100644 --- a/crates/ty_python_semantic/src/semantic_index/definition.rs +++ b/crates/ty_python_semantic/src/semantic_index/definition.rs @@ -918,6 +918,19 @@ impl DefinitionKind<'_> { } } + /// Returns the best target range for binding-oriented diagnostics. + /// + /// Most definition kinds use [`Self::target_range`], but some kinds require + /// special handling to point at the exact bound name. + pub(crate) fn binding_name_range(&self, module: &ParsedModuleRef) -> Option { + match self { + DefinitionKind::ExceptHandler(handler) => { + handler.node(module).name.as_ref().map(|name| name.range()) + } + _ => Some(self.target_range(module)), + } + } + /// Returns the [`TextRange`] of the entire definition. pub(crate) fn full_range(&self, module: &ParsedModuleRef) -> TextRange { match self { diff --git a/crates/ty_python_semantic/src/types/unused_bindings.rs b/crates/ty_python_semantic/src/types/unused_bindings.rs index 645f99aa3ad3e..2f5991f13ac4e 100644 --- a/crates/ty_python_semantic/src/types/unused_bindings.rs +++ b/crates/ty_python_semantic/src/types/unused_bindings.rs @@ -9,7 +9,7 @@ use crate::semantic_index::place::ScopedPlaceId; use crate::semantic_index::scope::ScopeKind; use crate::{Db, semantic_index}; use ruff_db::parsed::parsed_module; -use ruff_text_size::{Ranged, TextRange}; +use ruff_text_size::TextRange; fn is_dunder_name(name: &str) -> bool { name.len() > 4 && name.starts_with("__") && name.ends_with("__") @@ -62,18 +62,19 @@ pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec Vec Vec Vec { - let Some(name) = &handler.node(&parsed).name else { - continue; - }; - name.range() - } - _ => kind.target_range(&parsed), }; unused.push(UnusedBinding { @@ -123,7 +118,7 @@ pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec Date: Tue, 17 Feb 2026 01:22:26 +0100 Subject: [PATCH 05/22] ty: use method pointer for except-handler binding range --- crates/ty_python_semantic/src/semantic_index/definition.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ty_python_semantic/src/semantic_index/definition.rs b/crates/ty_python_semantic/src/semantic_index/definition.rs index 2705e2101e0d3..f713dc82b5637 100644 --- a/crates/ty_python_semantic/src/semantic_index/definition.rs +++ b/crates/ty_python_semantic/src/semantic_index/definition.rs @@ -925,7 +925,7 @@ impl DefinitionKind<'_> { pub(crate) fn binding_name_range(&self, module: &ParsedModuleRef) -> Option { match self { DefinitionKind::ExceptHandler(handler) => { - handler.node(module).name.as_ref().map(|name| name.range()) + handler.node(module).name.as_ref().map(Ranged::range) } _ => Some(self.target_range(module)), } From 8fc2ebd9614a2987597605a7600c69f630b89219 Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Thu, 19 Feb 2026 23:23:35 +0100 Subject: [PATCH 06/22] ty: improve unused-binding capture for enclosed scope and more tests --- .../src/semantic_index/builder.rs | 65 +++++++++++++ .../src/semantic_index/use_def.rs | 50 +++++++--- .../src/types/unused_bindings.rs | 97 ++++++++++++++++--- 3 files changed, 189 insertions(+), 23 deletions(-) diff --git a/crates/ty_python_semantic/src/semantic_index/builder.rs b/crates/ty_python_semantic/src/semantic_index/builder.rs index c91afbed4b1ef..e33c8a858e353 100644 --- a/crates/ty_python_semantic/src/semantic_index/builder.rs +++ b/crates/ty_python_semantic/src/semantic_index/builder.rs @@ -557,6 +557,70 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { }); } + fn resolve_nested_reference_scope( + &self, + nested_scope: FileScopeId, + name: &str, + ) -> Option { + VisibleAncestorsIter::new(&self.scopes, nested_scope) + .skip(1) + .find_map(|(scope_id, _)| { + let place_table = &self.place_tables[scope_id]; + let symbol_id = place_table.symbol_id(name)?; + let symbol = place_table.symbol(symbol_id); + (symbol.is_bound() || symbol.is_declared()).then_some(scope_id) + }) + } + + /// Marks bindings in enclosing scopes as used when a nested scope resolves a reference to them. + /// + /// This reuses enclosing-snapshot data so lazy scopes account for later reassignments that can + /// also reach the nested reference. + fn mark_captured_bindings_used(&mut self) { + let mut resolved_scopes_by_nested_symbol = + FxHashMap::<(FileScopeId, ScopedSymbolId), Option>::default(); + + let mut snapshots_to_mark = Vec::new(); + + for (&key, &snapshot_id) in &self.enclosing_snapshots { + let ScopedPlaceId::Symbol(enclosing_symbol_id) = key.enclosing_place else { + continue; + }; + + let enclosing_symbol = + self.place_tables[key.enclosing_scope].symbol(enclosing_symbol_id); + let nested_place_table = &self.place_tables[key.nested_scope]; + + let Some(nested_symbol_id) = + nested_place_table.symbol_id(enclosing_symbol.name().as_str()) + else { + continue; + }; + + let nested_symbol = nested_place_table.symbol(nested_symbol_id); + if !nested_symbol.is_used() || nested_symbol.is_local() || nested_symbol.is_global() { + continue; + } + + let resolved_scope = *resolved_scopes_by_nested_symbol + .entry((key.nested_scope, nested_symbol_id)) + .or_insert_with(|| { + self.resolve_nested_reference_scope( + key.nested_scope, + enclosing_symbol.name().as_str(), + ) + }); + + if resolved_scope == Some(key.enclosing_scope) { + snapshots_to_mark.push((key.enclosing_scope, snapshot_id)); + } + } + + for (scope_id, snapshot_id) in snapshots_to_mark { + self.use_def_maps[scope_id].mark_enclosing_snapshot_bindings_used(snapshot_id); + } + } + fn pop_scope(&mut self) -> FileScopeId { self.try_node_context_stack_manager.exit_scope(); @@ -1651,6 +1715,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { // Pop the root scope self.pop_scope(); self.sweep_nonlocal_lazy_snapshots(); + self.mark_captured_bindings_used(); assert!(self.scope_stack.is_empty()); assert_eq!(&self.current_assignments, &[]); diff --git a/crates/ty_python_semantic/src/semantic_index/use_def.rs b/crates/ty_python_semantic/src/semantic_index/use_def.rs index 3bbea72613783..8d167b8861a07 100644 --- a/crates/ty_python_semantic/src/semantic_index/use_def.rs +++ b/crates/ty_python_semantic/src/semantic_index/use_def.rs @@ -1420,20 +1420,24 @@ impl<'db> UseDefMapBuilder<'db> { let new_use = self.bindings_by_use.push(bindings); debug_assert_eq!(use_id, new_use); - for live_binding in bindings.iter() { - let definition_id = live_binding.binding; + let binding_definition_ids = bindings + .iter() + .map(|live_binding| live_binding.binding) + .collect::>(); + self.mark_definition_ids_used(binding_definition_ids.into_iter()); + } - if definition_id.is_unbound() { - continue; - } + pub(super) fn mark_enclosing_snapshot_bindings_used( + &mut self, + snapshot_id: ScopedEnclosingSnapshotId, + ) { + let Some(EnclosingSnapshot::Bindings(bindings)) = self.enclosing_snapshots.get(snapshot_id) + else { + return; + }; - if matches!( - self.all_definitions[definition_id], - DefinitionState::Defined(_) - ) { - self.used_bindings[definition_id] = true; - } - } + let binding_definition_ids = bindings.iter().map(|b| b.binding).collect::>(); + self.mark_definition_ids_used(binding_definition_ids.into_iter()); } pub(super) fn record_range_reachability(&mut self, range: TextRange) { @@ -1496,6 +1500,28 @@ impl<'db> UseDefMapBuilder<'db> { } } + fn mark_definition_ids_used( + &mut self, + definition_ids: impl Iterator, + ) { + for definition_id in definition_ids { + self.mark_definition_used(definition_id); + } + } + + fn mark_definition_used(&mut self, definition_id: ScopedDefinitionId) { + if definition_id.is_unbound() { + return; + } + + if matches!( + self.all_definitions[definition_id], + DefinitionState::Defined(_) + ) { + self.used_bindings[definition_id] = true; + } + } + /// Take a snapshot of the current visible-places state. pub(super) fn snapshot(&self) -> FlowSnapshot { FlowSnapshot { diff --git a/crates/ty_python_semantic/src/types/unused_bindings.rs b/crates/ty_python_semantic/src/types/unused_bindings.rs index 2f5991f13ac4e..28a96c45b3f5c 100644 --- a/crates/ty_python_semantic/src/types/unused_bindings.rs +++ b/crates/ty_python_semantic/src/types/unused_bindings.rs @@ -11,21 +11,15 @@ use crate::{Db, semantic_index}; use ruff_db::parsed::parsed_module; use ruff_text_size::TextRange; -fn is_dunder_name(name: &str) -> bool { - name.len() > 4 && name.starts_with("__") && name.ends_with("__") -} - fn should_mark_unnecessary(scope_kind: ScopeKind, name: &str) -> bool { - if name.starts_with('_') || matches!(name, "self" | "cls") || is_dunder_name(name) { + if !matches!( + scope_kind, + ScopeKind::Function | ScopeKind::Lambda | ScopeKind::Comprehension + ) { return false; } - match scope_kind { - ScopeKind::Function | ScopeKind::Lambda | ScopeKind::Comprehension => true, - ScopeKind::Module | ScopeKind::Class | ScopeKind::TypeParams | ScopeKind::TypeAlias => { - false - } - } + !name.starts_with('_') && !matches!(name, "self" | "cls") } fn should_consider_definition(kind: &DefinitionKind<'_>) -> bool { @@ -251,4 +245,85 @@ lam = lambda x, y, z=1: x + z assert_eq!(names, vec!["b", "c", "dead", "y"]); Ok(()) } + + #[test] + fn skips_closure_captured_bindings() -> anyhow::Result<()> { + let source = r#"def outer(flag: bool): + captured = 1 + dead = 2 + + def inner(): + return captured + + if flag: + captured = 3 + + return inner +"#; + + let names = collect_unused_names(source)?; + assert_eq!(names, vec!["dead"]); + Ok(()) + } + + #[test] + fn closure_uses_nearest_shadowed_binding() -> anyhow::Result<()> { + let source = r#"def outer(): + x = 0 + + def mid(): + x = 1 + + def inner(): + return x + + return inner + + return x, mid +"#; + + let names = collect_unused_names(source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn nested_local_same_name_does_not_mark_outer_used() -> anyhow::Result<()> { + let source = r#"def outer(): + x = 1 + + def inner(): + x = 2 + return x + + return inner +"#; + + let names = collect_unused_names(source)?; + assert_eq!(names, vec!["x"]); + Ok(()) + } + + #[test] + fn comprehension_binding_captured_by_nested_lambda_is_used() -> anyhow::Result<()> { + let source = r#"def f(): + funcs = [lambda: x for x in range(3)] + return funcs +"#; + + let names = collect_unused_names(source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn skips_unused_binding_analysis_on_syntax_error() -> anyhow::Result<()> { + let source = r#"def f( + x = 1 +"#; + + let names = collect_unused_names(source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } } From 9dc375ba626a2e7a1a47c98d3c33cf32e2db2eb7 Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Fri, 20 Feb 2026 00:05:58 +0100 Subject: [PATCH 07/22] ty: introduce hint diagnostics and use them for unused bindings --- crates/ruff_db/src/diagnostic/mod.rs | 2 + crates/ruff_db/src/diagnostic/render/azure.rs | 2 +- .../ruff_db/src/diagnostic/render/concise.rs | 1 + .../ruff_db/src/diagnostic/render/github.rs | 2 +- .../ruff_db/src/diagnostic/render/gitlab.rs | 2 +- crates/ruff_server/src/lint.rs | 1 + crates/ty/docs/configuration.md | 5 +- crates/ty/src/lib.rs | 4 +- crates/ty_project/src/metadata/options.rs | 5 +- crates/ty_python_semantic/src/lint.rs | 11 ++ .../ty_server/src/server/api/diagnostics.rs | 114 ++++-------------- .../api/requests/workspace_diagnostic.rs | 22 ++-- crates/ty_wasm/src/lib.rs | 1 + ty.schema.json | 8 +- 14 files changed, 70 insertions(+), 110 deletions(-) diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index 9dcac74ecc6a4..0d0044025dc02 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -1282,6 +1282,7 @@ impl From for Span { #[cfg_attr(feature = "serde", derive(Serialize))] #[cfg_attr(feature = "serde", serde(rename_all = "lowercase"))] pub enum Severity { + Hint, Info, Warning, Error, @@ -1291,6 +1292,7 @@ pub enum Severity { impl Severity { fn to_annotate(self) -> AnnotateLevel { match self { + Severity::Hint => AnnotateLevel::Info, Severity::Info => AnnotateLevel::Info, Severity::Warning => AnnotateLevel::Warning, Severity::Error => AnnotateLevel::Error, diff --git a/crates/ruff_db/src/diagnostic/render/azure.rs b/crates/ruff_db/src/diagnostic/render/azure.rs index 99476ed7ff727..2244976ff5f6c 100644 --- a/crates/ruff_db/src/diagnostic/render/azure.rs +++ b/crates/ruff_db/src/diagnostic/render/azure.rs @@ -22,7 +22,7 @@ impl AzureRenderer<'_> { ) -> std::fmt::Result { for diag in diagnostics { let severity = match diag.severity() { - Severity::Info | Severity::Warning => "warning", + Severity::Hint | Severity::Info | Severity::Warning => "warning", Severity::Error | Severity::Fatal => "error", }; write!(f, "##vso[task.logissue type={severity};")?; diff --git a/crates/ruff_db/src/diagnostic/render/concise.rs b/crates/ruff_db/src/diagnostic/render/concise.rs index 518b38a095398..c361ece6fd5a3 100644 --- a/crates/ruff_db/src/diagnostic/render/concise.rs +++ b/crates/ruff_db/src/diagnostic/render/concise.rs @@ -93,6 +93,7 @@ impl<'a> ConciseRenderer<'a> { } } else { let (severity, severity_style) = match diag.severity() { + Severity::Hint => ("hint", stylesheet.info), Severity::Info => ("info", stylesheet.info), Severity::Warning => ("warning", stylesheet.warning), Severity::Error => ("error", stylesheet.error), diff --git a/crates/ruff_db/src/diagnostic/render/github.rs b/crates/ruff_db/src/diagnostic/render/github.rs index bba48c2ec83d5..f39343a49c455 100644 --- a/crates/ruff_db/src/diagnostic/render/github.rs +++ b/crates/ruff_db/src/diagnostic/render/github.rs @@ -21,7 +21,7 @@ impl<'a> GithubRenderer<'a> { ) -> std::fmt::Result { for diagnostic in diagnostics { let severity = match diagnostic.severity() { - Severity::Info => "notice", + Severity::Hint | Severity::Info => "notice", Severity::Warning => "warning", Severity::Error | Severity::Fatal => "error", }; diff --git a/crates/ruff_db/src/diagnostic/render/gitlab.rs b/crates/ruff_db/src/diagnostic/render/gitlab.rs index 4d5a2ec82e4a6..0c85a3cd481d9 100644 --- a/crates/ruff_db/src/diagnostic/render/gitlab.rs +++ b/crates/ruff_db/src/diagnostic/render/gitlab.rs @@ -101,7 +101,7 @@ impl Serialize for SerializedMessages<'_> { let description = diagnostic.concise_message(); let check_name = diagnostic.secondary_code_or_id(); let severity = match diagnostic.severity() { - Severity::Info => "info", + Severity::Hint | Severity::Info => "info", Severity::Warning => "minor", Severity::Error => "major", // Another option here is `blocker` diff --git a/crates/ruff_server/src/lint.rs b/crates/ruff_server/src/lint.rs index 29afd28b80c3b..263544c8d7368 100644 --- a/crates/ruff_server/src/lint.rs +++ b/crates/ruff_server/src/lint.rs @@ -302,6 +302,7 @@ fn to_lsp_diagnostic( } else { ( match diagnostic.severity() { + ruff_db::diagnostic::Severity::Hint => lsp_types::DiagnosticSeverity::HINT, ruff_db::diagnostic::Severity::Info => lsp_types::DiagnosticSeverity::INFORMATION, ruff_db::diagnostic::Severity::Warning => lsp_types::DiagnosticSeverity::WARNING, ruff_db::diagnostic::Severity::Error => lsp_types::DiagnosticSeverity::ERROR, diff --git a/crates/ty/docs/configuration.md b/crates/ty/docs/configuration.md index 974da6cf3a3cf..a36b15bb944c7 100644 --- a/crates/ty/docs/configuration.md +++ b/crates/ty/docs/configuration.md @@ -11,13 +11,14 @@ See [the rules documentation](https://ty.dev/rules) for a list of all available Valid severities are: * `ignore`: Disable the rule. +* `hint`: Enable the rule and create a hint diagnostic. * `warn`: Enable the rule and create a warning diagnostic. * `error`: Enable the rule and create an error diagnostic. ty will exit with a non-zero code if any error diagnostics are emitted. **Default value**: `{...}` -**Type**: `dict[RuleName | "all", "ignore" | "warn" | "error"]` +**Type**: `dict[RuleName | "all", "ignore" | "hint" | "warn" | "error"]` **Example usage**: @@ -503,7 +504,7 @@ severity levels or disable them entirely. **Default value**: `{...}` -**Type**: `dict[RuleName | "all", "ignore" | "warn" | "error"]` +**Type**: `dict[RuleName | "all", "ignore" | "hint" | "warn" | "error"]` **Example usage**: diff --git a/crates/ty/src/lib.rs b/crates/ty/src/lib.rs index 54f5b0afdfc07..05531e39d89d4 100644 --- a/crates/ty/src/lib.rs +++ b/crates/ty/src/lib.rs @@ -519,7 +519,7 @@ fn exit_status_from_diagnostics( return ExitStatus::Success; } - let mut max_severity = Severity::Info; + let mut max_severity = Severity::Hint; let mut io_error = false; for diagnostic in diagnostics { @@ -532,7 +532,7 @@ fn exit_status_from_diagnostics( } match max_severity { - Severity::Info => ExitStatus::Success, + Severity::Hint | Severity::Info => ExitStatus::Success, Severity::Warning => { if terminal_settings.error_on_warning { ExitStatus::Failure diff --git a/crates/ty_project/src/metadata/options.rs b/crates/ty_project/src/metadata/options.rs index c3cb7382f5ac7..47a4446b5d594 100644 --- a/crates/ty_project/src/metadata/options.rs +++ b/crates/ty_project/src/metadata/options.rs @@ -72,13 +72,14 @@ pub struct Options { /// Valid severities are: /// /// * `ignore`: Disable the rule. + /// * `hint`: Enable the rule and create a hint diagnostic. /// * `warn`: Enable the rule and create a warning diagnostic. /// * `error`: Enable the rule and create an error diagnostic. /// ty will exit with a non-zero code if any error diagnostics are emitted. #[serde(skip_serializing_if = "Option::is_none")] #[option( default = r#"{...}"#, - value_type = r#"dict[RuleName | "all", "ignore" | "warn" | "error"]"#, + value_type = r#"dict[RuleName | "all", "ignore" | "hint" | "warn" | "error"]"#, example = r#" [tool.ty.rules] possibly-unresolved-reference = "warn" @@ -1595,7 +1596,7 @@ pub struct OverrideOptions { #[serde(skip_serializing_if = "Option::is_none")] #[option( default = r#"{...}"#, - value_type = r#"dict[RuleName | "all", "ignore" | "warn" | "error"]"#, + value_type = r#"dict[RuleName | "all", "ignore" | "hint" | "warn" | "error"]"#, example = r#" [[tool.ty.overrides]] include = ["src"] diff --git a/crates/ty_python_semantic/src/lint.rs b/crates/ty_python_semantic/src/lint.rs index 8fa09835d45e7..4a64b25791c03 100644 --- a/crates/ty_python_semantic/src/lint.rs +++ b/crates/ty_python_semantic/src/lint.rs @@ -51,6 +51,11 @@ pub enum Level { /// The lint is enabled and diagnostic should have a warning severity. Warn, + /// # Hint + /// + /// The lint is enabled and diagnostic should have a hint severity. + Hint, + /// # Error /// /// The lint is enabled and diagnostics have an error severity. @@ -66,6 +71,10 @@ impl Level { matches!(self, Level::Warn) } + pub const fn is_hint(self) -> bool { + matches!(self, Level::Hint) + } + pub const fn is_ignore(self) -> bool { matches!(self, Level::Ignore) } @@ -76,6 +85,7 @@ impl fmt::Display for Level { match self { Level::Ignore => f.write_str("ignore"), Level::Warn => f.write_str("warn"), + Level::Hint => f.write_str("hint"), Level::Error => f.write_str("error"), } } @@ -88,6 +98,7 @@ impl TryFrom for Severity { match level { Level::Ignore => Err(()), Level::Warn => Ok(Severity::Warning), + Level::Hint => Ok(Severity::Hint), Level::Error => Ok(Severity::Error), } } diff --git a/crates/ty_server/src/server/api/diagnostics.rs b/crates/ty_server/src/server/api/diagnostics.rs index 516268187affa..bdc2a7aee7a6c 100644 --- a/crates/ty_server/src/server/api/diagnostics.rs +++ b/crates/ty_server/src/server/api/diagnostics.rs @@ -30,35 +30,25 @@ const UNUSED_BINDING_CODE: &str = "unused-binding"; #[derive(Debug)] pub(super) struct Diagnostics { items: Vec, - unnecessary_items: Vec, encoding: PositionEncoding, file_or_notebook: File, } -#[derive(Debug, Clone, Eq, PartialEq, Hash)] -pub(super) struct UnnecessaryBindingDiagnostic { - range: ruff_text_size::TextRange, - name: String, -} - impl Diagnostics { /// Computes the result ID for `diagnostics`. /// /// Returns `None` if there are no diagnostics. pub(super) fn result_id_from_hash( diagnostics: &[ruff_db::diagnostic::Diagnostic], - unnecessary_items: &[UnnecessaryBindingDiagnostic], ) -> Option { - if diagnostics.is_empty() && unnecessary_items.is_empty() { + if diagnostics.is_empty() { return None; } // Generate result ID based on raw diagnostic content only. let mut hasher = DefaultHasher::new(); - // Hash lengths first to ensure different numbers of diagnostics produce different hashes. diagnostics.hash(&mut hasher); - unnecessary_items.hash(&mut hasher); Some(format!("{:016x}", hasher.finish())) } @@ -67,7 +57,7 @@ impl Diagnostics { /// /// Returns `None` if there are no diagnostics. pub(super) fn result_id(&self) -> Option { - Self::result_id_from_hash(&self.items, &self.unnecessary_items) + Self::result_id_from_hash(&self.items) } pub(super) fn to_lsp_diagnostics( @@ -107,30 +97,9 @@ impl Diagnostics { .push(lsp_diagnostic); } - for unnecessary in &self.unnecessary_items { - let Some((url, lsp_diagnostic)) = to_lsp_unnecessary_diagnostic( - db, - self.file_or_notebook, - unnecessary, - self.encoding, - ) else { - continue; - }; - - let Some(url) = url else { - tracing::warn!("Unable to find notebook cell"); - continue; - }; - - cell_diagnostics - .entry(url) - .or_default() - .push(lsp_diagnostic); - } - LspDiagnostics::NotebookDocument(cell_diagnostics) } else { - let mut diagnostics: Vec = self + let diagnostics: Vec = self .items .iter() .filter_map(|diagnostic| { @@ -147,18 +116,6 @@ impl Diagnostics { }) .collect(); - diagnostics.extend(self.unnecessary_items.iter().filter_map(|unnecessary| { - Some( - to_lsp_unnecessary_diagnostic( - db, - self.file_or_notebook, - unnecessary, - self.encoding, - )? - .1, - ) - })); - LspDiagnostics::TextDocument(diagnostics) } } @@ -371,30 +328,38 @@ pub(super) fn compute_diagnostics( return None; }; - let diagnostics = db.check_file(file); - let unnecessary_items = if db.project().should_check_file(db, file) { - unnecessary_binding_diagnostics(db, file) - } else { - Vec::new() - }; + let mut diagnostics = db.check_file(file); + if db.project().should_check_file(db, file) { + diagnostics.extend(unused_binding_diagnostics(db, file)); + } Some(Diagnostics { items: diagnostics, - unnecessary_items, encoding, file_or_notebook: file, }) } -pub(super) fn unnecessary_binding_diagnostics( +pub(super) fn unused_binding_diagnostics( db: &ProjectDatabase, file: File, -) -> Vec { +) -> Vec { + use ruff_db::diagnostic::{Annotation, DiagnosticId, DiagnosticTag}; + use ruff_db::files::FileRange; + unused_bindings(db, file) .iter() - .map(|binding| UnnecessaryBindingDiagnostic { - range: binding.range, - name: binding.name.clone(), + .map(|binding| { + let mut diagnostic = ruff_db::diagnostic::Diagnostic::new( + DiagnosticId::lint(UNUSED_BINDING_CODE), + Severity::Hint, + format!("Binding `{}` is unused", binding.name), + ); + diagnostic.annotate( + Annotation::primary(FileRange::new(file, binding.range).into()) + .tag(DiagnosticTag::Unnecessary), + ); + diagnostic }) .collect() } @@ -429,6 +394,7 @@ pub(super) fn to_lsp_diagnostic( }; let severity = match diagnostic.severity() { + Severity::Hint => DiagnosticSeverity::HINT, Severity::Info => DiagnosticSeverity::INFORMATION, Severity::Warning => DiagnosticSeverity::WARNING, Severity::Error | Severity::Fatal => DiagnosticSeverity::ERROR, @@ -513,38 +479,6 @@ pub(super) fn to_lsp_diagnostic( )) } -pub(super) fn to_lsp_unnecessary_diagnostic( - db: &dyn Db, - file: File, - unnecessary: &UnnecessaryBindingDiagnostic, - encoding: PositionEncoding, -) -> Option<(Option, Diagnostic)> { - let location = unnecessary - .range - .to_lsp_range(db, file, encoding)? - .to_location(); - - let (range, url) = match location { - Some(location) => (location.range, Some(location.uri)), - None => (lsp_types::Range::default(), None), - }; - - Some(( - url, - Diagnostic { - range, - severity: Some(DiagnosticSeverity::HINT), - tags: Some(vec![DiagnosticTag::UNNECESSARY]), - code: Some(NumberOrString::String(UNUSED_BINDING_CODE.into())), - code_description: None, - source: Some(DIAGNOSTIC_NAME.into()), - message: format!("Binding `{}` is unused", unnecessary.name), - related_information: None, - data: None, - }, - )) -} - /// Converts an [`Annotation`] to a [`DiagnosticRelatedInformation`]. fn annotation_to_related_information( db: &dyn Db, diff --git a/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs b/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs index 912c103a32c28..1bf4e1cbf22c8 100644 --- a/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs +++ b/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs @@ -21,10 +21,7 @@ use ty_project::{ProgressReporter, ProjectDatabase}; use crate::PositionEncoding; use crate::capabilities::ResolvedClientCapabilities; use crate::document::DocumentKey; -use crate::server::api::diagnostics::{ - Diagnostics, UnnecessaryBindingDiagnostic, to_lsp_diagnostic, to_lsp_unnecessary_diagnostic, - unnecessary_binding_diagnostics, -}; +use crate::server::api::diagnostics::{Diagnostics, to_lsp_diagnostic, unused_binding_diagnostics}; use crate::server::api::traits::{ BackgroundRequestHandler, RequestHandler, RetriableRequestHandler, }; @@ -255,7 +252,7 @@ impl ProgressReporter for WorkspaceDiagnosticsProgressReporter<'_> { state.report_progress(&self.work_done); } - let unnecessary = unnecessary_binding_diagnostics(db, file); + let unnecessary = unused_binding_diagnostics(db, file); // Don't report empty diagnostics. We clear previous diagnostics in `into_response` // which also handles the case where a file no longer has diagnostics because @@ -376,7 +373,7 @@ impl<'a> ResponseWriter<'a> { db: &ProjectDatabase, file: File, diagnostics: &[Diagnostic], - unnecessary: &[UnnecessaryBindingDiagnostic], + unnecessary: &[Diagnostic], ) { let Some(url) = file_to_url(db, file) else { tracing::debug!("Failed to convert file path to URL at {}", file.path(db)); @@ -398,7 +395,11 @@ impl<'a> ResponseWriter<'a> { .map(|doc| i64::from(doc.version())) .ok(); - let result_id = Diagnostics::result_id_from_hash(diagnostics, unnecessary); + let combined_count = diagnostics.len() + unnecessary.len(); + let mut combined = Vec::with_capacity(combined_count); + combined.extend_from_slice(diagnostics); + combined.extend_from_slice(unnecessary); + let result_id = Diagnostics::result_id_from_hash(&combined); let previous_result_id = self.previous_result_ids.remove(&key).map(|(_url, id)| id); @@ -434,11 +435,12 @@ impl<'a> ResponseWriter<'a> { let mut lsp_diagnostics = lsp_diagnostics; lsp_diagnostics.extend(unnecessary.iter().filter_map(|diagnostic| { Some( - to_lsp_unnecessary_diagnostic( + to_lsp_diagnostic( db, - file, diagnostic, self.position_encoding, + self.client_capabilities, + self.global_settings, )? .1, ) @@ -496,7 +498,7 @@ impl<'a> ResponseWriter<'a> { .ok() .map(|doc| i64::from(doc.version())); - let new_result_id = Diagnostics::result_id_from_hash(&[], &[]); + let new_result_id = Diagnostics::result_id_from_hash(&[]); let report = match new_result_id { Some(new_id) if new_id == previous_result_id => { diff --git a/crates/ty_wasm/src/lib.rs b/crates/ty_wasm/src/lib.rs index 05ce49eb0911f..72e31c67b0fda 100644 --- a/crates/ty_wasm/src/lib.rs +++ b/crates/ty_wasm/src/lib.rs @@ -1011,6 +1011,7 @@ pub enum Severity { impl From for Severity { fn from(value: diagnostic::Severity) -> Self { match value { + diagnostic::Severity::Hint => Self::Info, diagnostic::Severity::Info => Self::Info, diagnostic::Severity::Warning => Self::Warning, diagnostic::Severity::Error => Self::Error, diff --git a/ty.schema.json b/ty.schema.json index 8a5df89aa26f5..ffc5d8a089cd5 100644 --- a/ty.schema.json +++ b/ty.schema.json @@ -36,7 +36,7 @@ ] }, "rules": { - "description": "Configures the enabled rules and their severity.\n\nThe keys are either rule names or `all` to set a default severity for all rules.\nSee [the rules documentation](https://ty.dev/rules) for a list of all available rules.\n\nValid severities are:\n\n* `ignore`: Disable the rule.\n* `warn`: Enable the rule and create a warning diagnostic.\n* `error`: Enable the rule and create an error diagnostic.\n ty will exit with a non-zero code if any error diagnostics are emitted.", + "description": "Configures the enabled rules and their severity.\n\nThe keys are either rule names or `all` to set a default severity for all rules.\nSee [the rules documentation](https://ty.dev/rules) for a list of all available rules.\n\nValid severities are:\n\n* `ignore`: Disable the rule.\n* `hint`: Enable the rule and create a hint diagnostic.\n* `warn`: Enable the rule and create a warning diagnostic.\n* `error`: Enable the rule and create an error diagnostic.\n ty will exit with a non-zero code if any error diagnostics are emitted.", "anyOf": [ { "$ref": "#/definitions/Rules" @@ -192,6 +192,12 @@ "type": "string", "const": "warn" }, + { + "title": "Hint", + "description": "The lint is enabled and diagnostic should have a hint severity.", + "type": "string", + "const": "hint" + }, { "title": "Error", "description": "The lint is enabled and diagnostics have an error severity.", From ac252e52a8676eaa1db4cfcc95c77e611bffb26a Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Wed, 25 Feb 2026 15:29:34 +0100 Subject: [PATCH 08/22] ty: fixes per feedback --- .../src/semantic_index/builder.rs | 7 +- .../src/semantic_index/definition.rs | 19 +- .../src/semantic_index/use_def.rs | 27 +- .../src/types/unused_bindings.rs | 301 +++++++++++------- .../api/requests/workspace_diagnostic.rs | 4 +- 5 files changed, 215 insertions(+), 143 deletions(-) diff --git a/crates/ty_python_semantic/src/semantic_index/builder.rs b/crates/ty_python_semantic/src/semantic_index/builder.rs index e33c8a858e353..53cf1eead255e 100644 --- a/crates/ty_python_semantic/src/semantic_index/builder.rs +++ b/crates/ty_python_semantic/src/semantic_index/builder.rs @@ -568,7 +568,10 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { let place_table = &self.place_tables[scope_id]; let symbol_id = place_table.symbol_id(name)?; let symbol = place_table.symbol(symbol_id); - (symbol.is_bound() || symbol.is_declared()).then_some(scope_id) + + // Only a true local binding in an ancestor scope can be the resolution target. + // `global`/`nonlocal` here are forwarding declarations, not owning bindings. + symbol.is_local().then_some(scope_id) }) } @@ -1714,8 +1717,8 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { // Pop the root scope self.pop_scope(); - self.sweep_nonlocal_lazy_snapshots(); self.mark_captured_bindings_used(); + self.sweep_nonlocal_lazy_snapshots(); assert!(self.scope_stack.is_empty()); assert_eq!(&self.current_assignments, &[]); diff --git a/crates/ty_python_semantic/src/semantic_index/definition.rs b/crates/ty_python_semantic/src/semantic_index/definition.rs index f713dc82b5637..4301b53506d61 100644 --- a/crates/ty_python_semantic/src/semantic_index/definition.rs +++ b/crates/ty_python_semantic/src/semantic_index/definition.rs @@ -908,7 +908,11 @@ impl DefinitionKind<'_> { DefinitionKind::MatchPattern(match_pattern) => { match_pattern.identifier.node(module).range() } - DefinitionKind::ExceptHandler(handler) => handler.node(module).range(), + DefinitionKind::ExceptHandler(handler) => handler + .node(module) + .name + .as_ref() + .map_or_else(|| handler.node(module).range(), Ranged::range), DefinitionKind::TypeVar(type_var) => type_var.node(module).name.range(), DefinitionKind::ParamSpec(param_spec) => param_spec.node(module).name.range(), DefinitionKind::TypeVarTuple(type_var_tuple) => { @@ -918,19 +922,6 @@ impl DefinitionKind<'_> { } } - /// Returns the best target range for binding-oriented diagnostics. - /// - /// Most definition kinds use [`Self::target_range`], but some kinds require - /// special handling to point at the exact bound name. - pub(crate) fn binding_name_range(&self, module: &ParsedModuleRef) -> Option { - match self { - DefinitionKind::ExceptHandler(handler) => { - handler.node(module).name.as_ref().map(Ranged::range) - } - _ => Some(self.target_range(module)), - } - } - /// Returns the [`TextRange`] of the entire definition. pub(crate) fn full_range(&self, module: &ParsedModuleRef) -> TextRange { match self { diff --git a/crates/ty_python_semantic/src/semantic_index/use_def.rs b/crates/ty_python_semantic/src/semantic_index/use_def.rs index 8d167b8861a07..f39a860d86076 100644 --- a/crates/ty_python_semantic/src/semantic_index/use_def.rs +++ b/crates/ty_python_semantic/src/semantic_index/use_def.rs @@ -983,7 +983,7 @@ impl<'db> UseDefMapBuilder<'db> { } } - fn push_definition_state(&mut self, state: DefinitionState<'db>) -> ScopedDefinitionId { + fn push_definition(&mut self, state: DefinitionState<'db>) -> ScopedDefinitionId { let def_id = self.all_definitions.push(state); let used_id = self.used_bindings.push(false); debug_assert_eq!(def_id, used_id); @@ -1057,7 +1057,7 @@ impl<'db> UseDefMapBuilder<'db> { self.bindings_by_definition .insert(binding, bindings.clone()); - let def_id = self.push_definition_state(DefinitionState::Defined(binding)); + let def_id = self.push_definition(DefinitionState::Defined(binding)); let place_state = match place { ScopedPlaceId::Symbol(symbol) => &mut self.symbol_states[symbol], ScopedPlaceId::Member(member) => &mut self.member_states[member], @@ -1305,7 +1305,7 @@ impl<'db> UseDefMapBuilder<'db> { place: ScopedPlaceId, declaration: Definition<'db>, ) { - let def_id = self.push_definition_state(DefinitionState::Defined(declaration)); + let def_id = self.push_definition(DefinitionState::Defined(declaration)); let place_state = match place { ScopedPlaceId::Symbol(symbol) => &mut self.symbol_states[symbol], @@ -1335,7 +1335,7 @@ impl<'db> UseDefMapBuilder<'db> { ) { // We don't need to store anything in self.bindings_by_declaration or // self.declarations_by_binding. - let def_id = self.push_definition_state(DefinitionState::Defined(definition)); + let def_id = self.push_definition(DefinitionState::Defined(definition)); let place_state = match place { ScopedPlaceId::Symbol(symbol) => &mut self.symbol_states[symbol], ScopedPlaceId::Member(member) => &mut self.member_states[member], @@ -1369,7 +1369,7 @@ impl<'db> UseDefMapBuilder<'db> { } pub(super) fn delete_binding(&mut self, place: ScopedPlaceId) { - let def_id = self.push_definition_state(DefinitionState::Deleted); + let def_id = self.push_definition(DefinitionState::Deleted); let place_state = match place { ScopedPlaceId::Symbol(symbol) => &mut self.symbol_states[symbol], ScopedPlaceId::Member(member) => &mut self.member_states[member], @@ -1402,12 +1402,16 @@ impl<'db> UseDefMapBuilder<'db> { let bindings = match place { ScopedPlaceId::Symbol(symbol) => self.symbol_states[symbol].bindings(), ScopedPlaceId::Member(member) => self.member_states[member].bindings(), - }; + } + .clone(); + + let binding_definition_ids = bindings.iter().map(|live_binding| live_binding.binding); + self.mark_definition_ids_used(binding_definition_ids); self.multi_bindings_by_use .entry(use_id) .or_default() - .push(bindings.clone()); + .push(bindings); } // Record a placeholder use of the parent expression to preserve the indices of `bindings_by_use`. @@ -1415,16 +1419,13 @@ impl<'db> UseDefMapBuilder<'db> { } fn record_use_bindings(&mut self, bindings: Bindings, use_id: ScopedUseId) { + let binding_definition_ids = bindings.iter().map(|live_binding| live_binding.binding); + self.mark_definition_ids_used(binding_definition_ids); + // We have a use of a place; clone the current bindings for that place, and record them // as the live bindings for this use. let new_use = self.bindings_by_use.push(bindings); debug_assert_eq!(use_id, new_use); - - let binding_definition_ids = bindings - .iter() - .map(|live_binding| live_binding.binding) - .collect::>(); - self.mark_definition_ids_used(binding_definition_ids.into_iter()); } pub(super) fn mark_enclosing_snapshot_bindings_used( diff --git a/crates/ty_python_semantic/src/types/unused_bindings.rs b/crates/ty_python_semantic/src/types/unused_bindings.rs index 28a96c45b3f5c..87930f62e2d70 100644 --- a/crates/ty_python_semantic/src/types/unused_bindings.rs +++ b/crates/ty_python_semantic/src/types/unused_bindings.rs @@ -11,14 +11,7 @@ use crate::{Db, semantic_index}; use ruff_db::parsed::parsed_module; use ruff_text_size::TextRange; -fn should_mark_unnecessary(scope_kind: ScopeKind, name: &str) -> bool { - if !matches!( - scope_kind, - ScopeKind::Function | ScopeKind::Lambda | ScopeKind::Comprehension - ) { - return false; - } - +fn should_mark_unused(name: &str) -> bool { !name.starts_with('_') && !matches!(name, "self" | "cls") } @@ -60,6 +53,9 @@ pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec Vec Vec Vec anyhow::Result> { + fn collect_unused_bindings(source: &str) -> anyhow::Result> { let db = TestDbBuilder::new() .with_file("/src/main.py", source) .build()?; let file = system_path_to_file(&db, "/src/main.py").unwrap(); - let mut names = unused_bindings(&db, file) + let mut bindings = unused_bindings(&db, file).clone(); + bindings.sort_unstable_by_key(|binding| (binding.range.start(), binding.range.end())); + Ok(bindings) + } + + fn collect_unused_names(source: &str) -> anyhow::Result> { + let mut names = collect_unused_bindings(source)? .iter() .map(|binding| binding.name.clone()) .collect::>(); @@ -139,35 +141,38 @@ mod tests { #[test] fn captures_safe_local_binding_kinds() -> anyhow::Result<()> { - let source = r#"def f(): - used_assign, dead_assign = (1, 2) - print(used_assign) - - for used_loop, dead_loop in [(1, 2)]: - print(used_loop) - - with open("x") as dead_with: - pass - - try: - 1 / 0 - except Exception as dead_exc: - pass - - if (dead_walrus := 1): - pass - - [1 for dead_comp in range(3)] - [ok_comp for ok_comp, dead_comp2 in [(1, 2)]] - - match {"x": 1, "y": 2}: - case {"x": used_match, **dead_rest}: - print(used_match) - case [used_star, *dead_star] as dead_as: - print(used_star) -"#; + let source = dedent( + " + def f(): + used_assign, dead_assign = (1, 2) + print(used_assign) + + for used_loop, dead_loop in [(1, 2)]: + print(used_loop) + + with open(\"x\") as dead_with: + pass + + try: + 1 / 0 + except Exception as dead_exc: + pass + + if (dead_walrus := 1): + pass + + [1 for dead_comp in range(3)] + [ok_comp for ok_comp, dead_comp2 in [(1, 2)]] + + match {\"x\": 1, \"y\": 2}: + case {\"x\": used_match, **dead_rest}: + print(used_match) + case [used_star, *dead_star] as dead_as: + print(used_star) + ", + ); - let names = collect_unused_names(source)?; + let names = collect_unused_names(&source)?; assert_eq!( names, vec![ @@ -187,142 +192,214 @@ mod tests { } #[test] - fn skips_module_class_placeholder_and_dunder_bindings() -> anyhow::Result<()> { - let source = r#"_module_dead = 1 + fn skips_module_and_class_scope_bindings() -> anyhow::Result<()> { + let source = dedent( + " + module_dead = 1 + + class C: + class_dead = 1 + + def method(self): + local_dead = 1 + return 0 + ", + ); -class C: - __private_dead = 1 + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["local_dead"]); + Ok(()) + } - def method(self): - local_dead = 1 - _ = 2 - __dunder__ = 3 - return 0 -"#; + #[test] + fn skips_placeholder_and_dunder_locals() -> anyhow::Result<()> { + let source = dedent( + " + def f(): + local_dead = 1 + _ = 2 + _ignored = 3 + __dunder__ = 4 + return 0 + ", + ); - let names = collect_unused_names(source)?; + let names = collect_unused_names(&source)?; assert_eq!(names, vec!["local_dead"]); Ok(()) } #[test] fn skips_global_and_nonlocal_assignments() -> anyhow::Result<()> { - let source = r#"global_value = 0 + let source = dedent( + " + global_value = 0 -def mutate_global(): - global global_value - global_value = 1 - local_dead = 1 + def mutate_global(): + global global_value + global_value = 1 + local_dead = 1 -def outer(): - captured = 0 + def outer(): + captured = 0 - def inner(): - nonlocal captured - captured = 1 + def inner(): + nonlocal captured + captured = 1 - inner() - return captured -"#; + inner() + return captured + ", + ); - let names = collect_unused_names(source)?; + let names = collect_unused_names(&source)?; assert_eq!(names, vec!["local_dead"]); Ok(()) } #[test] fn captures_unused_function_and_lambda_parameters() -> anyhow::Result<()> { - let source = r#"def fn(used, dead, _ignored, __dunder__): - return used + let source = dedent( + " + def fn(used, dead, _ignored, __dunder__): + return used -def fn_defaults(a, b=1, *, c=2, d): - return a + d + def fn_defaults(a, b=1, *, c=2, d): + return a + d -lam = lambda x, y, z=1: x + z -"#; + lam = lambda x, y, z=1: x + z + ", + ); - let names = collect_unused_names(source)?; + let names = collect_unused_names(&source)?; assert_eq!(names, vec!["b", "c", "dead", "y"]); Ok(()) } #[test] fn skips_closure_captured_bindings() -> anyhow::Result<()> { - let source = r#"def outer(flag: bool): - captured = 1 - dead = 2 + let source = dedent( + " + def outer(flag: bool): + captured = 1 + dead = 2 - def inner(): - return captured + def inner(): + return captured - if flag: - captured = 3 + if flag: + captured = 3 - return inner -"#; + return inner + ", + ); - let names = collect_unused_names(source)?; + let names = collect_unused_names(&source)?; assert_eq!(names, vec!["dead"]); Ok(()) } #[test] fn closure_uses_nearest_shadowed_binding() -> anyhow::Result<()> { - let source = r#"def outer(): - x = 0 + let source = dedent( + " + def outer(): + x = 0 + + def mid(): + x = 1 + + def inner(): + return x + + return inner + + return mid + ", + ); + + let bindings = collect_unused_bindings(&source)?; + let outer_x_start = TextSize::try_from(source.find("x = 0").unwrap()).unwrap(); + assert_eq!( + bindings, + vec![UnusedBinding { + range: TextRange::new(outer_x_start, outer_x_start + TextSize::new(1)), + name: "x".to_string(), + }] + ); + Ok(()) + } + + #[test] + fn nonlocal_proxy_scope_still_marks_outer_binding_used() -> anyhow::Result<()> { + let source = dedent( + " + def outer(): + x = 1 - def mid(): - x = 1 + def mid(): + nonlocal x + x = 2 - def inner(): - return x + def inner(): + return x - return inner + return inner - return x, mid -"#; + return mid + ", + ); - let names = collect_unused_names(source)?; + let names = collect_unused_names(&source)?; assert_eq!(names, Vec::::new()); Ok(()) } #[test] fn nested_local_same_name_does_not_mark_outer_used() -> anyhow::Result<()> { - let source = r#"def outer(): - x = 1 + let source = dedent( + " + def outer(): + x = 1 - def inner(): - x = 2 - return x + def inner(): + x = 2 + return x - return inner -"#; + return inner + ", + ); - let names = collect_unused_names(source)?; + let names = collect_unused_names(&source)?; assert_eq!(names, vec!["x"]); Ok(()) } #[test] fn comprehension_binding_captured_by_nested_lambda_is_used() -> anyhow::Result<()> { - let source = r#"def f(): - funcs = [lambda: x for x in range(3)] - return funcs -"#; + let source = dedent( + " + def f(): + funcs = [lambda: x for x in range(3)] + return funcs + ", + ); - let names = collect_unused_names(source)?; + let names = collect_unused_names(&source)?; assert_eq!(names, Vec::::new()); Ok(()) } #[test] fn skips_unused_binding_analysis_on_syntax_error() -> anyhow::Result<()> { - let source = r#"def f( - x = 1 -"#; + let source = dedent( + " + def f( + x = 1 + ", + ); - let names = collect_unused_names(source)?; + let names = collect_unused_names(&source)?; assert_eq!(names, Vec::::new()); Ok(()) } diff --git a/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs b/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs index 1bf4e1cbf22c8..056502a3fbc73 100644 --- a/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs +++ b/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs @@ -235,6 +235,8 @@ impl ProgressReporter for WorkspaceDiagnosticsProgressReporter<'_> { } fn report_checked_file(&self, db: &ProjectDatabase, file: File, diagnostics: &[Diagnostic]) { + let unnecessary = unused_binding_diagnostics(db, file); + // Another thread might have panicked at this point because of a salsa cancellation which // poisoned the result. If the response is poisoned, just don't report and wait for our thread // to unwind with a salsa cancellation next. @@ -252,8 +254,6 @@ impl ProgressReporter for WorkspaceDiagnosticsProgressReporter<'_> { state.report_progress(&self.work_done); } - let unnecessary = unused_binding_diagnostics(db, file); - // Don't report empty diagnostics. We clear previous diagnostics in `into_response` // which also handles the case where a file no longer has diagnostics because // it's no longer part of the project. From 9351aca1186318368806a27524ff90f77cc0668f Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Thu, 26 Feb 2026 22:16:28 +0100 Subject: [PATCH 09/22] ty: move unused_bindings to ide_support and cleanup --- crates/ty_python_semantic/src/types.rs | 1 - .../src/types/ide_support.rs | 408 +++++++++++++++++- .../src/types/unused_bindings.rs | 406 ----------------- .../ty_server/src/server/api/diagnostics.rs | 37 +- .../api/requests/workspace_diagnostic.rs | 15 +- 5 files changed, 423 insertions(+), 444 deletions(-) delete mode 100644 crates/ty_python_semantic/src/types/unused_bindings.rs diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index fda88ba9ddea1..0656296c085b6 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -137,7 +137,6 @@ mod type_alias; mod typed_dict; mod typevar; mod unpacker; -pub mod unused_bindings; mod variance; mod visitor; diff --git a/crates/ty_python_semantic/src/types/ide_support.rs b/crates/ty_python_semantic/src/types/ide_support.rs index 5eb5e6249789f..185646dfb5dd0 100644 --- a/crates/ty_python_semantic/src/types/ide_support.rs +++ b/crates/ty_python_semantic/src/types/ide_support.rs @@ -2,8 +2,9 @@ use std::collections::HashMap; use crate::FxIndexSet; use crate::place::builtins_module_scope; -use crate::semantic_index::definition::Definition; -use crate::semantic_index::definition::DefinitionKind; +use crate::semantic_index::definition::{Definition, DefinitionKind, DefinitionState}; +use crate::semantic_index::place::ScopedPlaceId; +use crate::semantic_index::scope::ScopeKind; use crate::semantic_index::{attribute_scopes, global_scope, semantic_index, use_def_map}; use crate::types::call::{CallArguments, CallError, MatchedArgument}; use crate::types::class::{DynamicClassAnchor, DynamicNamedTupleAnchor}; @@ -18,8 +19,7 @@ use itertools::Either; use ruff_db::files::FileRange; use ruff_db::parsed::parsed_module; use ruff_db::source::source_text; -use ruff_python_ast::name::Name; -use ruff_python_ast::{self as ast, AnyNodeRef}; +use ruff_python_ast::{self as ast, AnyNodeRef, name::Name}; use ruff_text_size::{Ranged, TextRange}; use rustc_hash::FxHashSet; @@ -1022,6 +1022,406 @@ fn find_parameter_range(parameters: &ast::Parameters, parameter_name: &str) -> O .map(|param| param.parameter.name.range()) } +/// Collects unused local bindings for IDE-facing diagnostics. +/// +/// This intentionally reports only function-, lambda-, and comprehension-scope bindings. +/// Module and class bindings can be observed indirectly (e.g., imports, attribute access), so +/// reporting them here risks false positives without cross-file/reference analysis. +fn should_mark_unused(name: &str) -> bool { + !name.starts_with('_') && !matches!(name, "self" | "cls") +} + +fn should_consider_definition(kind: &DefinitionKind<'_>) -> bool { + matches!( + kind, + DefinitionKind::NamedExpression(_) + | DefinitionKind::Assignment(_) + | DefinitionKind::AnnotatedAssignment(_) + | DefinitionKind::For(_) + | DefinitionKind::Comprehension(_) + | DefinitionKind::VariadicPositionalParameter(_) + | DefinitionKind::VariadicKeywordParameter(_) + | DefinitionKind::Parameter(_) + | DefinitionKind::WithItem(_) + | DefinitionKind::MatchPattern(_) + | DefinitionKind::ExceptHandler(_) + ) +} + +#[derive(Debug, Clone, Eq, PartialEq, Hash)] +pub struct UnusedBinding { + pub range: TextRange, + pub name: Name, +} + +#[salsa::tracked(returns(ref))] +pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec { + let parsed = parsed_module(db, file).load(db); + if !parsed.errors().is_empty() || !parsed.unsupported_syntax_errors().is_empty() { + return Vec::new(); + } + + let index = semantic_index(db, file); + let mut unused = Vec::new(); + + for scope_id in index.scope_ids() { + let file_scope_id = scope_id.file_scope_id(db); + let scope = index.scope(file_scope_id); + let scope_kind = scope.kind(); + + // Restrict to local scopes to avoid false positives for bindings that may be + // observed indirectly from module/class contexts (for example via imports or + // attribute access) without cross-file analysis. + if !matches!( + scope_kind, + ScopeKind::Function | ScopeKind::Lambda | ScopeKind::Comprehension + ) { + continue; + } + + let place_table = index.place_table(file_scope_id); + let use_def_map = index.use_def_map(file_scope_id); + + for (_, state, is_used) in use_def_map.all_definitions_with_usage() { + let DefinitionState::Defined(definition) = state else { + continue; + }; + + if is_used { + continue; + } + + let kind = definition.kind(db); + if !should_consider_definition(kind) { + continue; + } + + let ScopedPlaceId::Symbol(symbol_id) = definition.place(db) else { + continue; + }; + + let symbol = place_table.symbol(symbol_id); + let name = symbol.name().as_str(); + + if !should_mark_unused(name) { + continue; + } + + // Global and nonlocal assignments target bindings from outer scopes. + // Treat them as externally managed to avoid false positives here. + if symbol.is_global() || symbol.is_nonlocal() { + continue; + } + + let range = kind.target_range(&parsed); + + unused.push(UnusedBinding { + range, + name: symbol.name().clone(), + }); + } + } + + unused.sort_unstable_by_key(|binding| (binding.range.start(), binding.range.end())); + unused.dedup_by_key(|binding| binding.range); + + unused +} + +#[cfg(test)] +mod unused_bindings_tests { + use super::{UnusedBinding, unused_bindings}; + use crate::db::tests::TestDbBuilder; + use ruff_db::files::system_path_to_file; + use ruff_python_ast::name::Name; + use ruff_python_trivia::textwrap::dedent; + use ruff_text_size::{TextRange, TextSize}; + + fn collect_unused_bindings(source: &str) -> anyhow::Result> { + let db = TestDbBuilder::new() + .with_file("/src/main.py", source) + .build()?; + let file = system_path_to_file(&db, "/src/main.py").unwrap(); + let mut bindings = unused_bindings(&db, file).clone(); + bindings.sort_unstable_by_key(|binding| (binding.range.start(), binding.range.end())); + Ok(bindings) + } + + fn collect_unused_names(source: &str) -> anyhow::Result> { + let mut names = collect_unused_bindings(source)? + .iter() + .map(|binding| binding.name.to_string()) + .collect::>(); + names.sort(); + Ok(names) + } + + #[test] + fn captures_safe_local_binding_kinds() -> anyhow::Result<()> { + let source = dedent( + " + def f(): + used_assign, dead_assign = (1, 2) + print(used_assign) + + for used_loop, dead_loop in [(1, 2)]: + print(used_loop) + + with open(\"x\") as dead_with: + pass + + try: + 1 / 0 + except Exception as dead_exc: + pass + + if (dead_walrus := 1): + pass + + [1 for dead_comp in range(3)] + [ok_comp for ok_comp, dead_comp2 in [(1, 2)]] + + match {\"x\": 1, \"y\": 2}: + case {\"x\": used_match, **dead_rest}: + print(used_match) + case [used_star, *dead_star] as dead_as: + print(used_star) + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!( + names, + vec![ + "dead_as", + "dead_assign", + "dead_comp", + "dead_comp2", + "dead_exc", + "dead_loop", + "dead_rest", + "dead_star", + "dead_walrus", + "dead_with", + ] + ); + Ok(()) + } + + #[test] + fn skips_module_and_class_scope_bindings() -> anyhow::Result<()> { + let source = dedent( + " + module_dead = 1 + + class C: + class_dead = 1 + + def method(self): + local_dead = 1 + return 0 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["local_dead"]); + Ok(()) + } + + #[test] + fn skips_placeholder_and_dunder_locals() -> anyhow::Result<()> { + let source = dedent( + " + def f(): + local_dead = 1 + _ = 2 + _ignored = 3 + __dunder__ = 4 + return 0 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["local_dead"]); + Ok(()) + } + + #[test] + fn skips_global_and_nonlocal_assignments() -> anyhow::Result<()> { + let source = dedent( + " + global_value = 0 + + def mutate_global(): + global global_value + global_value = 1 + local_dead = 1 + + def outer(): + captured = 0 + + def inner(): + nonlocal captured + captured = 1 + + inner() + return captured + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["local_dead"]); + Ok(()) + } + + #[test] + fn captures_unused_function_and_lambda_parameters() -> anyhow::Result<()> { + let source = dedent( + " + def fn(used, dead, _ignored, __dunder__): + return used + + def fn_defaults(a, b=1, *, c=2, d): + return a + d + + lam = lambda x, y, z=1: x + z + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["b", "c", "dead", "y"]); + Ok(()) + } + + #[test] + fn skips_closure_captured_bindings() -> anyhow::Result<()> { + let source = dedent( + " + def outer(flag: bool): + captured = 1 + dead = 2 + + def inner(): + return captured + + if flag: + captured = 3 + + return inner + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["dead"]); + Ok(()) + } + + #[test] + fn closure_uses_nearest_shadowed_binding() -> anyhow::Result<()> { + let source = dedent( + " + def outer(): + x = 0 + + def mid(): + x = 1 + + def inner(): + return x + + return inner + + return mid + ", + ); + + let bindings = collect_unused_bindings(&source)?; + let outer_x_start = TextSize::try_from(source.find("x = 0").unwrap()).unwrap(); + assert_eq!( + bindings, + vec![UnusedBinding { + range: TextRange::new(outer_x_start, outer_x_start + TextSize::new(1)), + name: Name::new("x"), + }] + ); + Ok(()) + } + + #[test] + fn nonlocal_proxy_scope_still_marks_outer_binding_used() -> anyhow::Result<()> { + let source = dedent( + " + def outer(): + x = 1 + + def mid(): + nonlocal x + x = 2 + + def inner(): + return x + + return inner + + return mid + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn nested_local_same_name_does_not_mark_outer_used() -> anyhow::Result<()> { + let source = dedent( + " + def outer(): + x = 1 + + def inner(): + x = 2 + return x + + return inner + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["x"]); + Ok(()) + } + + #[test] + fn comprehension_binding_captured_by_nested_lambda_is_used() -> anyhow::Result<()> { + let source = dedent( + " + def f(): + funcs = [lambda: x for x in range(3)] + return funcs + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn skips_unused_binding_analysis_on_syntax_error() -> anyhow::Result<()> { + let source = dedent( + " + def f( + x = 1 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } +} + mod resolve_definition { //! Resolves an Import, `ImportFrom` or `StarImport` definition to one or more //! "resolved definitions". This is done recursively to find the original diff --git a/crates/ty_python_semantic/src/types/unused_bindings.rs b/crates/ty_python_semantic/src/types/unused_bindings.rs deleted file mode 100644 index 87930f62e2d70..0000000000000 --- a/crates/ty_python_semantic/src/types/unused_bindings.rs +++ /dev/null @@ -1,406 +0,0 @@ -//! Collects unused local bindings for IDE-facing diagnostics. -//! -//! This intentionally reports only function-, lambda-, and comprehension-scope bindings. -//! Module and class bindings can be observed indirectly (e.g., imports, attribute access), so -//! reporting them here risks false positives without cross-file/reference analysis. - -use crate::semantic_index::definition::{DefinitionKind, DefinitionState}; -use crate::semantic_index::place::ScopedPlaceId; -use crate::semantic_index::scope::ScopeKind; -use crate::{Db, semantic_index}; -use ruff_db::parsed::parsed_module; -use ruff_text_size::TextRange; - -fn should_mark_unused(name: &str) -> bool { - !name.starts_with('_') && !matches!(name, "self" | "cls") -} - -fn should_consider_definition(kind: &DefinitionKind<'_>) -> bool { - matches!( - kind, - DefinitionKind::NamedExpression(_) - | DefinitionKind::Assignment(_) - | DefinitionKind::AnnotatedAssignment(_) - | DefinitionKind::For(_) - | DefinitionKind::Comprehension(_) - | DefinitionKind::VariadicPositionalParameter(_) - | DefinitionKind::VariadicKeywordParameter(_) - | DefinitionKind::Parameter(_) - | DefinitionKind::WithItem(_) - | DefinitionKind::MatchPattern(_) - | DefinitionKind::ExceptHandler(_) - ) -} - -#[derive(Debug, Clone, Eq, PartialEq, Hash)] -pub struct UnusedBinding { - pub range: TextRange, - pub name: String, -} - -#[salsa::tracked(returns(ref))] -pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec { - let parsed = parsed_module(db, file).load(db); - if !parsed.errors().is_empty() || !parsed.unsupported_syntax_errors().is_empty() { - return Vec::new(); - } - - let index = semantic_index::semantic_index(db, file); - let mut unused = Vec::new(); - - for scope_id in index.scope_ids() { - let file_scope_id = scope_id.file_scope_id(db); - let scope = index.scope(file_scope_id); - let scope_kind = scope.kind(); - - // Restrict to local scopes to avoid false positives for bindings that may be - // observed indirectly from module/class contexts (for example via imports or - // attribute access) without cross-file analysis. - if !matches!( - scope_kind, - ScopeKind::Function | ScopeKind::Lambda | ScopeKind::Comprehension - ) { - continue; - } - - let place_table = index.place_table(file_scope_id); - let use_def_map = index.use_def_map(file_scope_id); - - for (_, state, is_used) in use_def_map.all_definitions_with_usage() { - let DefinitionState::Defined(definition) = state else { - continue; - }; - - if is_used { - continue; - } - - let kind = definition.kind(db); - if !should_consider_definition(kind) { - continue; - } - - let ScopedPlaceId::Symbol(symbol_id) = definition.place(db) else { - continue; - }; - - let symbol = place_table.symbol(symbol_id); - let name = symbol.name().as_str(); - - if !should_mark_unused(name) { - continue; - } - - // Global and nonlocal assignments target bindings from outer scopes. - // Treat them as externally managed to avoid false positives here. - if symbol.is_global() || symbol.is_nonlocal() { - continue; - } - - let range = kind.target_range(&parsed); - - unused.push(UnusedBinding { - range, - name: name.to_string(), - }); - } - } - - unused.sort_unstable_by_key(|binding| (binding.range.start(), binding.range.end())); - unused.dedup_by_key(|binding| binding.range); - - unused -} - -#[cfg(test)] -mod tests { - use super::{UnusedBinding, unused_bindings}; - use crate::db::tests::TestDbBuilder; - use ruff_db::files::system_path_to_file; - use ruff_python_trivia::textwrap::dedent; - use ruff_text_size::{TextRange, TextSize}; - - fn collect_unused_bindings(source: &str) -> anyhow::Result> { - let db = TestDbBuilder::new() - .with_file("/src/main.py", source) - .build()?; - let file = system_path_to_file(&db, "/src/main.py").unwrap(); - let mut bindings = unused_bindings(&db, file).clone(); - bindings.sort_unstable_by_key(|binding| (binding.range.start(), binding.range.end())); - Ok(bindings) - } - - fn collect_unused_names(source: &str) -> anyhow::Result> { - let mut names = collect_unused_bindings(source)? - .iter() - .map(|binding| binding.name.clone()) - .collect::>(); - names.sort(); - Ok(names) - } - - #[test] - fn captures_safe_local_binding_kinds() -> anyhow::Result<()> { - let source = dedent( - " - def f(): - used_assign, dead_assign = (1, 2) - print(used_assign) - - for used_loop, dead_loop in [(1, 2)]: - print(used_loop) - - with open(\"x\") as dead_with: - pass - - try: - 1 / 0 - except Exception as dead_exc: - pass - - if (dead_walrus := 1): - pass - - [1 for dead_comp in range(3)] - [ok_comp for ok_comp, dead_comp2 in [(1, 2)]] - - match {\"x\": 1, \"y\": 2}: - case {\"x\": used_match, **dead_rest}: - print(used_match) - case [used_star, *dead_star] as dead_as: - print(used_star) - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!( - names, - vec![ - "dead_as", - "dead_assign", - "dead_comp", - "dead_comp2", - "dead_exc", - "dead_loop", - "dead_rest", - "dead_star", - "dead_walrus", - "dead_with", - ] - ); - Ok(()) - } - - #[test] - fn skips_module_and_class_scope_bindings() -> anyhow::Result<()> { - let source = dedent( - " - module_dead = 1 - - class C: - class_dead = 1 - - def method(self): - local_dead = 1 - return 0 - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["local_dead"]); - Ok(()) - } - - #[test] - fn skips_placeholder_and_dunder_locals() -> anyhow::Result<()> { - let source = dedent( - " - def f(): - local_dead = 1 - _ = 2 - _ignored = 3 - __dunder__ = 4 - return 0 - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["local_dead"]); - Ok(()) - } - - #[test] - fn skips_global_and_nonlocal_assignments() -> anyhow::Result<()> { - let source = dedent( - " - global_value = 0 - - def mutate_global(): - global global_value - global_value = 1 - local_dead = 1 - - def outer(): - captured = 0 - - def inner(): - nonlocal captured - captured = 1 - - inner() - return captured - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["local_dead"]); - Ok(()) - } - - #[test] - fn captures_unused_function_and_lambda_parameters() -> anyhow::Result<()> { - let source = dedent( - " - def fn(used, dead, _ignored, __dunder__): - return used - - def fn_defaults(a, b=1, *, c=2, d): - return a + d - - lam = lambda x, y, z=1: x + z - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["b", "c", "dead", "y"]); - Ok(()) - } - - #[test] - fn skips_closure_captured_bindings() -> anyhow::Result<()> { - let source = dedent( - " - def outer(flag: bool): - captured = 1 - dead = 2 - - def inner(): - return captured - - if flag: - captured = 3 - - return inner - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["dead"]); - Ok(()) - } - - #[test] - fn closure_uses_nearest_shadowed_binding() -> anyhow::Result<()> { - let source = dedent( - " - def outer(): - x = 0 - - def mid(): - x = 1 - - def inner(): - return x - - return inner - - return mid - ", - ); - - let bindings = collect_unused_bindings(&source)?; - let outer_x_start = TextSize::try_from(source.find("x = 0").unwrap()).unwrap(); - assert_eq!( - bindings, - vec![UnusedBinding { - range: TextRange::new(outer_x_start, outer_x_start + TextSize::new(1)), - name: "x".to_string(), - }] - ); - Ok(()) - } - - #[test] - fn nonlocal_proxy_scope_still_marks_outer_binding_used() -> anyhow::Result<()> { - let source = dedent( - " - def outer(): - x = 1 - - def mid(): - nonlocal x - x = 2 - - def inner(): - return x - - return inner - - return mid - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, Vec::::new()); - Ok(()) - } - - #[test] - fn nested_local_same_name_does_not_mark_outer_used() -> anyhow::Result<()> { - let source = dedent( - " - def outer(): - x = 1 - - def inner(): - x = 2 - return x - - return inner - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["x"]); - Ok(()) - } - - #[test] - fn comprehension_binding_captured_by_nested_lambda_is_used() -> anyhow::Result<()> { - let source = dedent( - " - def f(): - funcs = [lambda: x for x in range(3)] - return funcs - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, Vec::::new()); - Ok(()) - } - - #[test] - fn skips_unused_binding_analysis_on_syntax_error() -> anyhow::Result<()> { - let source = dedent( - " - def f( - x = 1 - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, Vec::::new()); - Ok(()) - } -} diff --git a/crates/ty_server/src/server/api/diagnostics.rs b/crates/ty_server/src/server/api/diagnostics.rs index bdc2a7aee7a6c..7de97458fd3fd 100644 --- a/crates/ty_server/src/server/api/diagnostics.rs +++ b/crates/ty_server/src/server/api/diagnostics.rs @@ -9,7 +9,7 @@ use lsp_types::{ use ruff_diagnostics::Applicability; use ruff_text_size::Ranged; use rustc_hash::FxHashMap; -use ty_python_semantic::types::unused_bindings::unused_bindings; +use ty_python_semantic::types::ide_support::unused_bindings; use ruff_db::diagnostic::{Annotation, Severity, SubDiagnostic}; use ruff_db::files::{File, FileRange}; @@ -99,24 +99,23 @@ impl Diagnostics { LspDiagnostics::NotebookDocument(cell_diagnostics) } else { - let diagnostics: Vec = self - .items - .iter() - .filter_map(|diagnostic| { - Some( - to_lsp_diagnostic( - db, - diagnostic, - self.encoding, - client_capabilities, - global_settings, - )? - .1, - ) - }) - .collect(); - - LspDiagnostics::TextDocument(diagnostics) + LspDiagnostics::TextDocument( + self.items + .iter() + .filter_map(|diagnostic| { + Some( + to_lsp_diagnostic( + db, + diagnostic, + self.encoding, + client_capabilities, + global_settings, + )? + .1, + ) + }) + .collect(), + ) } } } diff --git a/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs b/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs index 056502a3fbc73..daed372209d8a 100644 --- a/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs +++ b/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs @@ -418,6 +418,7 @@ impl<'a> ResponseWriter<'a> { new_id => { let lsp_diagnostics = diagnostics .iter() + .chain(unnecessary.iter()) .filter_map(|diagnostic| { Some( to_lsp_diagnostic( @@ -432,20 +433,6 @@ impl<'a> ResponseWriter<'a> { }) .collect::>(); - let mut lsp_diagnostics = lsp_diagnostics; - lsp_diagnostics.extend(unnecessary.iter().filter_map(|diagnostic| { - Some( - to_lsp_diagnostic( - db, - diagnostic, - self.position_encoding, - self.client_capabilities, - self.global_settings, - )? - .1, - ) - })); - WorkspaceDocumentDiagnosticReport::Full(WorkspaceFullDocumentDiagnosticReport { uri: url, version, From 04ad9fd82f9438bdfc69048835a89ba3654cfcfe Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Tue, 10 Mar 2026 19:44:44 +0100 Subject: [PATCH 10/22] ty: get rid of Hint diagnostic --- crates/ruff_db/src/diagnostic/mod.rs | 2 - crates/ruff_db/src/diagnostic/render/azure.rs | 2 +- .../ruff_db/src/diagnostic/render/concise.rs | 1 - .../ruff_db/src/diagnostic/render/github.rs | 2 +- .../ruff_db/src/diagnostic/render/gitlab.rs | 2 +- crates/ruff_server/src/lint.rs | 1 - crates/ty/docs/configuration.md | 5 +- crates/ty/src/lib.rs | 4 +- crates/ty_project/src/metadata/options.rs | 5 +- crates/ty_python_semantic/src/lint.rs | 11 -- .../ty_server/src/server/api/diagnostics.rs | 101 ++++++++++-------- .../api/requests/workspace_diagnostic.rs | 30 +++--- crates/ty_wasm/src/lib.rs | 1 - ty.schema.json | 10 +- 14 files changed, 87 insertions(+), 90 deletions(-) diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index 0d0044025dc02..9dcac74ecc6a4 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -1282,7 +1282,6 @@ impl From for Span { #[cfg_attr(feature = "serde", derive(Serialize))] #[cfg_attr(feature = "serde", serde(rename_all = "lowercase"))] pub enum Severity { - Hint, Info, Warning, Error, @@ -1292,7 +1291,6 @@ pub enum Severity { impl Severity { fn to_annotate(self) -> AnnotateLevel { match self { - Severity::Hint => AnnotateLevel::Info, Severity::Info => AnnotateLevel::Info, Severity::Warning => AnnotateLevel::Warning, Severity::Error => AnnotateLevel::Error, diff --git a/crates/ruff_db/src/diagnostic/render/azure.rs b/crates/ruff_db/src/diagnostic/render/azure.rs index 2244976ff5f6c..99476ed7ff727 100644 --- a/crates/ruff_db/src/diagnostic/render/azure.rs +++ b/crates/ruff_db/src/diagnostic/render/azure.rs @@ -22,7 +22,7 @@ impl AzureRenderer<'_> { ) -> std::fmt::Result { for diag in diagnostics { let severity = match diag.severity() { - Severity::Hint | Severity::Info | Severity::Warning => "warning", + Severity::Info | Severity::Warning => "warning", Severity::Error | Severity::Fatal => "error", }; write!(f, "##vso[task.logissue type={severity};")?; diff --git a/crates/ruff_db/src/diagnostic/render/concise.rs b/crates/ruff_db/src/diagnostic/render/concise.rs index c361ece6fd5a3..518b38a095398 100644 --- a/crates/ruff_db/src/diagnostic/render/concise.rs +++ b/crates/ruff_db/src/diagnostic/render/concise.rs @@ -93,7 +93,6 @@ impl<'a> ConciseRenderer<'a> { } } else { let (severity, severity_style) = match diag.severity() { - Severity::Hint => ("hint", stylesheet.info), Severity::Info => ("info", stylesheet.info), Severity::Warning => ("warning", stylesheet.warning), Severity::Error => ("error", stylesheet.error), diff --git a/crates/ruff_db/src/diagnostic/render/github.rs b/crates/ruff_db/src/diagnostic/render/github.rs index f39343a49c455..bba48c2ec83d5 100644 --- a/crates/ruff_db/src/diagnostic/render/github.rs +++ b/crates/ruff_db/src/diagnostic/render/github.rs @@ -21,7 +21,7 @@ impl<'a> GithubRenderer<'a> { ) -> std::fmt::Result { for diagnostic in diagnostics { let severity = match diagnostic.severity() { - Severity::Hint | Severity::Info => "notice", + Severity::Info => "notice", Severity::Warning => "warning", Severity::Error | Severity::Fatal => "error", }; diff --git a/crates/ruff_db/src/diagnostic/render/gitlab.rs b/crates/ruff_db/src/diagnostic/render/gitlab.rs index 0c85a3cd481d9..4d5a2ec82e4a6 100644 --- a/crates/ruff_db/src/diagnostic/render/gitlab.rs +++ b/crates/ruff_db/src/diagnostic/render/gitlab.rs @@ -101,7 +101,7 @@ impl Serialize for SerializedMessages<'_> { let description = diagnostic.concise_message(); let check_name = diagnostic.secondary_code_or_id(); let severity = match diagnostic.severity() { - Severity::Hint | Severity::Info => "info", + Severity::Info => "info", Severity::Warning => "minor", Severity::Error => "major", // Another option here is `blocker` diff --git a/crates/ruff_server/src/lint.rs b/crates/ruff_server/src/lint.rs index 263544c8d7368..29afd28b80c3b 100644 --- a/crates/ruff_server/src/lint.rs +++ b/crates/ruff_server/src/lint.rs @@ -302,7 +302,6 @@ fn to_lsp_diagnostic( } else { ( match diagnostic.severity() { - ruff_db::diagnostic::Severity::Hint => lsp_types::DiagnosticSeverity::HINT, ruff_db::diagnostic::Severity::Info => lsp_types::DiagnosticSeverity::INFORMATION, ruff_db::diagnostic::Severity::Warning => lsp_types::DiagnosticSeverity::WARNING, ruff_db::diagnostic::Severity::Error => lsp_types::DiagnosticSeverity::ERROR, diff --git a/crates/ty/docs/configuration.md b/crates/ty/docs/configuration.md index a36b15bb944c7..974da6cf3a3cf 100644 --- a/crates/ty/docs/configuration.md +++ b/crates/ty/docs/configuration.md @@ -11,14 +11,13 @@ See [the rules documentation](https://ty.dev/rules) for a list of all available Valid severities are: * `ignore`: Disable the rule. -* `hint`: Enable the rule and create a hint diagnostic. * `warn`: Enable the rule and create a warning diagnostic. * `error`: Enable the rule and create an error diagnostic. ty will exit with a non-zero code if any error diagnostics are emitted. **Default value**: `{...}` -**Type**: `dict[RuleName | "all", "ignore" | "hint" | "warn" | "error"]` +**Type**: `dict[RuleName | "all", "ignore" | "warn" | "error"]` **Example usage**: @@ -504,7 +503,7 @@ severity levels or disable them entirely. **Default value**: `{...}` -**Type**: `dict[RuleName | "all", "ignore" | "hint" | "warn" | "error"]` +**Type**: `dict[RuleName | "all", "ignore" | "warn" | "error"]` **Example usage**: diff --git a/crates/ty/src/lib.rs b/crates/ty/src/lib.rs index 05531e39d89d4..54f5b0afdfc07 100644 --- a/crates/ty/src/lib.rs +++ b/crates/ty/src/lib.rs @@ -519,7 +519,7 @@ fn exit_status_from_diagnostics( return ExitStatus::Success; } - let mut max_severity = Severity::Hint; + let mut max_severity = Severity::Info; let mut io_error = false; for diagnostic in diagnostics { @@ -532,7 +532,7 @@ fn exit_status_from_diagnostics( } match max_severity { - Severity::Hint | Severity::Info => ExitStatus::Success, + Severity::Info => ExitStatus::Success, Severity::Warning => { if terminal_settings.error_on_warning { ExitStatus::Failure diff --git a/crates/ty_project/src/metadata/options.rs b/crates/ty_project/src/metadata/options.rs index 47a4446b5d594..c3cb7382f5ac7 100644 --- a/crates/ty_project/src/metadata/options.rs +++ b/crates/ty_project/src/metadata/options.rs @@ -72,14 +72,13 @@ pub struct Options { /// Valid severities are: /// /// * `ignore`: Disable the rule. - /// * `hint`: Enable the rule and create a hint diagnostic. /// * `warn`: Enable the rule and create a warning diagnostic. /// * `error`: Enable the rule and create an error diagnostic. /// ty will exit with a non-zero code if any error diagnostics are emitted. #[serde(skip_serializing_if = "Option::is_none")] #[option( default = r#"{...}"#, - value_type = r#"dict[RuleName | "all", "ignore" | "hint" | "warn" | "error"]"#, + value_type = r#"dict[RuleName | "all", "ignore" | "warn" | "error"]"#, example = r#" [tool.ty.rules] possibly-unresolved-reference = "warn" @@ -1596,7 +1595,7 @@ pub struct OverrideOptions { #[serde(skip_serializing_if = "Option::is_none")] #[option( default = r#"{...}"#, - value_type = r#"dict[RuleName | "all", "ignore" | "hint" | "warn" | "error"]"#, + value_type = r#"dict[RuleName | "all", "ignore" | "warn" | "error"]"#, example = r#" [[tool.ty.overrides]] include = ["src"] diff --git a/crates/ty_python_semantic/src/lint.rs b/crates/ty_python_semantic/src/lint.rs index 4a64b25791c03..8fa09835d45e7 100644 --- a/crates/ty_python_semantic/src/lint.rs +++ b/crates/ty_python_semantic/src/lint.rs @@ -51,11 +51,6 @@ pub enum Level { /// The lint is enabled and diagnostic should have a warning severity. Warn, - /// # Hint - /// - /// The lint is enabled and diagnostic should have a hint severity. - Hint, - /// # Error /// /// The lint is enabled and diagnostics have an error severity. @@ -71,10 +66,6 @@ impl Level { matches!(self, Level::Warn) } - pub const fn is_hint(self) -> bool { - matches!(self, Level::Hint) - } - pub const fn is_ignore(self) -> bool { matches!(self, Level::Ignore) } @@ -85,7 +76,6 @@ impl fmt::Display for Level { match self { Level::Ignore => f.write_str("ignore"), Level::Warn => f.write_str("warn"), - Level::Hint => f.write_str("hint"), Level::Error => f.write_str("error"), } } @@ -98,7 +88,6 @@ impl TryFrom for Severity { match level { Level::Ignore => Err(()), Level::Warn => Ok(Severity::Warning), - Level::Hint => Ok(Severity::Hint), Level::Error => Ok(Severity::Error), } } diff --git a/crates/ty_server/src/server/api/diagnostics.rs b/crates/ty_server/src/server/api/diagnostics.rs index 7de97458fd3fd..52ff4feb70c68 100644 --- a/crates/ty_server/src/server/api/diagnostics.rs +++ b/crates/ty_server/src/server/api/diagnostics.rs @@ -9,7 +9,7 @@ use lsp_types::{ use ruff_diagnostics::Applicability; use ruff_text_size::Ranged; use rustc_hash::FxHashMap; -use ty_python_semantic::types::ide_support::unused_bindings; +use ty_python_semantic::types::ide_support::{UnusedBinding, unused_bindings}; use ruff_db::diagnostic::{Annotation, Severity, SubDiagnostic}; use ruff_db::files::{File, FileRange}; @@ -25,11 +25,13 @@ use crate::system::{AnySystemPath, file_to_url}; use crate::{DIAGNOSTIC_NAME, Db, DiagnosticMode}; use crate::{PositionEncoding, Session}; +/// Server-only LSP code for IDE dimming (not a configurable lint rule). const UNUSED_BINDING_CODE: &str = "unused-binding"; #[derive(Debug)] pub(super) struct Diagnostics { items: Vec, + unused_bindings: Vec, encoding: PositionEncoding, file_or_notebook: File, } @@ -40,8 +42,9 @@ impl Diagnostics { /// Returns `None` if there are no diagnostics. pub(super) fn result_id_from_hash( diagnostics: &[ruff_db::diagnostic::Diagnostic], + unused_bindings: &[UnusedBinding], ) -> Option { - if diagnostics.is_empty() { + if diagnostics.is_empty() && unused_bindings.is_empty() { return None; } @@ -49,6 +52,7 @@ impl Diagnostics { let mut hasher = DefaultHasher::new(); diagnostics.hash(&mut hasher); + unused_bindings.hash(&mut hasher); Some(format!("{:016x}", hasher.finish())) } @@ -57,7 +61,7 @@ impl Diagnostics { /// /// Returns `None` if there are no diagnostics. pub(super) fn result_id(&self) -> Option { - Self::result_id_from_hash(&self.items) + Self::result_id_from_hash(&self.items, &self.unused_bindings) } pub(super) fn to_lsp_diagnostics( @@ -99,23 +103,29 @@ impl Diagnostics { LspDiagnostics::NotebookDocument(cell_diagnostics) } else { - LspDiagnostics::TextDocument( - self.items - .iter() - .filter_map(|diagnostic| { - Some( - to_lsp_diagnostic( - db, - diagnostic, - self.encoding, - client_capabilities, - global_settings, - )? - .1, - ) - }) - .collect(), - ) + let mut diagnostics = self + .items + .iter() + .filter_map(|diagnostic| { + Some( + to_lsp_diagnostic( + db, + diagnostic, + self.encoding, + client_capabilities, + global_settings, + )? + .1, + ) + }) + .collect::>(); + diagnostics.extend(unused_bindings_to_lsp_diagnostics( + db, + self.file_or_notebook, + self.encoding, + &self.unused_bindings, + )); + LspDiagnostics::TextDocument(diagnostics) } } } @@ -327,38 +337,46 @@ pub(super) fn compute_diagnostics( return None; }; - let mut diagnostics = db.check_file(file); - if db.project().should_check_file(db, file) { - diagnostics.extend(unused_binding_diagnostics(db, file)); - } + let diagnostics = db.check_file(file); + let unused_bindings = collect_unused_bindings(db, file); Some(Diagnostics { items: diagnostics, + unused_bindings, encoding, file_or_notebook: file, }) } -pub(super) fn unused_binding_diagnostics( +pub(super) fn collect_unused_bindings(db: &ProjectDatabase, file: File) -> Vec { + if db.notebook_document(file).is_some() || !db.project().should_check_file(db, file) { + return Vec::new(); + } + unused_bindings(db, file).clone() +} + +pub(super) fn unused_bindings_to_lsp_diagnostics( db: &ProjectDatabase, file: File, -) -> Vec { - use ruff_db::diagnostic::{Annotation, DiagnosticId, DiagnosticTag}; - use ruff_db::files::FileRange; - - unused_bindings(db, file) + encoding: PositionEncoding, + unused_bindings: &[UnusedBinding], +) -> Vec { + unused_bindings .iter() - .map(|binding| { - let mut diagnostic = ruff_db::diagnostic::Diagnostic::new( - DiagnosticId::lint(UNUSED_BINDING_CODE), - Severity::Hint, - format!("Binding `{}` is unused", binding.name), - ); - diagnostic.annotate( - Annotation::primary(FileRange::new(file, binding.range).into()) - .tag(DiagnosticTag::Unnecessary), - ); - diagnostic + .map(|binding| Diagnostic { + range: binding + .range + .to_lsp_range(db, file, encoding) + .map(|range| range.local_range()) + .unwrap_or_default(), + severity: Some(DiagnosticSeverity::HINT), + code: Some(NumberOrString::String(UNUSED_BINDING_CODE.to_owned())), + code_description: None, + source: Some(DIAGNOSTIC_NAME.into()), + message: format!("Binding `{}` is unused", binding.name), + related_information: None, + tags: Some(vec![DiagnosticTag::UNNECESSARY]), + data: None, }) .collect() } @@ -393,7 +411,6 @@ pub(super) fn to_lsp_diagnostic( }; let severity = match diagnostic.severity() { - Severity::Hint => DiagnosticSeverity::HINT, Severity::Info => DiagnosticSeverity::INFORMATION, Severity::Warning => DiagnosticSeverity::WARNING, Severity::Error | Severity::Fatal => DiagnosticSeverity::ERROR, diff --git a/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs b/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs index daed372209d8a..78271b335b2cd 100644 --- a/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs +++ b/crates/ty_server/src/server/api/requests/workspace_diagnostic.rs @@ -17,11 +17,14 @@ use ruff_db::source::source_text; use rustc_hash::FxHashMap; use serde::{Deserialize, Serialize}; use ty_project::{ProgressReporter, ProjectDatabase}; +use ty_python_semantic::types::ide_support::UnusedBinding; use crate::PositionEncoding; use crate::capabilities::ResolvedClientCapabilities; use crate::document::DocumentKey; -use crate::server::api::diagnostics::{Diagnostics, to_lsp_diagnostic, unused_binding_diagnostics}; +use crate::server::api::diagnostics::{ + Diagnostics, collect_unused_bindings, to_lsp_diagnostic, unused_bindings_to_lsp_diagnostics, +}; use crate::server::api::traits::{ BackgroundRequestHandler, RequestHandler, RetriableRequestHandler, }; @@ -235,7 +238,7 @@ impl ProgressReporter for WorkspaceDiagnosticsProgressReporter<'_> { } fn report_checked_file(&self, db: &ProjectDatabase, file: File, diagnostics: &[Diagnostic]) { - let unnecessary = unused_binding_diagnostics(db, file); + let unused_bindings = collect_unused_bindings(db, file); // Another thread might have panicked at this point because of a salsa cancellation which // poisoned the result. If the response is poisoned, just don't report and wait for our thread @@ -257,10 +260,10 @@ impl ProgressReporter for WorkspaceDiagnosticsProgressReporter<'_> { // Don't report empty diagnostics. We clear previous diagnostics in `into_response` // which also handles the case where a file no longer has diagnostics because // it's no longer part of the project. - if !diagnostics.is_empty() || !unnecessary.is_empty() { + if !diagnostics.is_empty() || !unused_bindings.is_empty() { state .response - .write_diagnostics_for_file(db, file, diagnostics, &unnecessary); + .write_diagnostics_for_file(db, file, diagnostics, &unused_bindings); } state.response.maybe_flush(); @@ -373,7 +376,7 @@ impl<'a> ResponseWriter<'a> { db: &ProjectDatabase, file: File, diagnostics: &[Diagnostic], - unnecessary: &[Diagnostic], + unused_bindings: &[UnusedBinding], ) { let Some(url) = file_to_url(db, file) else { tracing::debug!("Failed to convert file path to URL at {}", file.path(db)); @@ -395,11 +398,7 @@ impl<'a> ResponseWriter<'a> { .map(|doc| i64::from(doc.version())) .ok(); - let combined_count = diagnostics.len() + unnecessary.len(); - let mut combined = Vec::with_capacity(combined_count); - combined.extend_from_slice(diagnostics); - combined.extend_from_slice(unnecessary); - let result_id = Diagnostics::result_id_from_hash(&combined); + let result_id = Diagnostics::result_id_from_hash(diagnostics, unused_bindings); let previous_result_id = self.previous_result_ids.remove(&key).map(|(_url, id)| id); @@ -416,9 +415,8 @@ impl<'a> ResponseWriter<'a> { ) } new_id => { - let lsp_diagnostics = diagnostics + let mut lsp_diagnostics = diagnostics .iter() - .chain(unnecessary.iter()) .filter_map(|diagnostic| { Some( to_lsp_diagnostic( @@ -432,6 +430,12 @@ impl<'a> ResponseWriter<'a> { ) }) .collect::>(); + lsp_diagnostics.extend(unused_bindings_to_lsp_diagnostics( + db, + file, + self.position_encoding, + unused_bindings, + )); WorkspaceDocumentDiagnosticReport::Full(WorkspaceFullDocumentDiagnosticReport { uri: url, @@ -485,7 +489,7 @@ impl<'a> ResponseWriter<'a> { .ok() .map(|doc| i64::from(doc.version())); - let new_result_id = Diagnostics::result_id_from_hash(&[]); + let new_result_id = Diagnostics::result_id_from_hash(&[], &[]); let report = match new_result_id { Some(new_id) if new_id == previous_result_id => { diff --git a/crates/ty_wasm/src/lib.rs b/crates/ty_wasm/src/lib.rs index 72e31c67b0fda..05ce49eb0911f 100644 --- a/crates/ty_wasm/src/lib.rs +++ b/crates/ty_wasm/src/lib.rs @@ -1011,7 +1011,6 @@ pub enum Severity { impl From for Severity { fn from(value: diagnostic::Severity) -> Self { match value { - diagnostic::Severity::Hint => Self::Info, diagnostic::Severity::Info => Self::Info, diagnostic::Severity::Warning => Self::Warning, diagnostic::Severity::Error => Self::Error, diff --git a/ty.schema.json b/ty.schema.json index ffc5d8a089cd5..ccd6d5da137b6 100644 --- a/ty.schema.json +++ b/ty.schema.json @@ -36,7 +36,7 @@ ] }, "rules": { - "description": "Configures the enabled rules and their severity.\n\nThe keys are either rule names or `all` to set a default severity for all rules.\nSee [the rules documentation](https://ty.dev/rules) for a list of all available rules.\n\nValid severities are:\n\n* `ignore`: Disable the rule.\n* `hint`: Enable the rule and create a hint diagnostic.\n* `warn`: Enable the rule and create a warning diagnostic.\n* `error`: Enable the rule and create an error diagnostic.\n ty will exit with a non-zero code if any error diagnostics are emitted.", + "description": "Configures the enabled rules and their severity.\n\nThe keys are either rule names or `all` to set a default severity for all rules.\nSee [the rules documentation](https://ty.dev/rules) for a list of all available rules.\n\nValid severities are:\n\n* `ignore`: Disable the rule.\n* `warn`: Enable the rule and create a warning diagnostic.\n* `error`: Enable the rule and create an error diagnostic.\n ty will exit with a non-zero code if any error diagnostics are emitted.", "anyOf": [ { "$ref": "#/definitions/Rules" @@ -192,12 +192,6 @@ "type": "string", "const": "warn" }, - { - "title": "Hint", - "description": "The lint is enabled and diagnostic should have a hint severity.", - "type": "string", - "const": "hint" - }, { "title": "Error", "description": "The lint is enabled and diagnostics have an error severity.", @@ -1614,4 +1608,4 @@ "type": "string" } } -} \ No newline at end of file +} From c34ebad1f7767369f483e3810e428427bcc3b2af Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Tue, 10 Mar 2026 20:09:02 +0100 Subject: [PATCH 11/22] ty: small cleanup --- ty.schema.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ty.schema.json b/ty.schema.json index ccd6d5da137b6..8a5df89aa26f5 100644 --- a/ty.schema.json +++ b/ty.schema.json @@ -1608,4 +1608,4 @@ "type": "string" } } -} +} \ No newline at end of file From 1d31823cb9c4ff684131d4e8839c1a343c8660b5 Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Thu, 19 Mar 2026 16:25:39 +0100 Subject: [PATCH 12/22] [ty] fix several false positives and some minor fixes --- crates/ruff_python_ast/src/helpers.rs | 8 + .../src/semantic_index/builder.rs | 25 +- .../src/semantic_index/definition.rs | 9 + .../ty_python_semantic/src/types/function.rs | 8 +- .../src/types/ide_support.rs | 245 ++++++++++++++++-- .../ty_server/src/server/api/diagnostics.rs | 51 +++- crates/ty_server/tests/e2e/notebook.rs | 24 ++ ...blish_unused_binding_diagnostics_open.snap | 27 ++ 8 files changed, 362 insertions(+), 35 deletions(-) create mode 100644 crates/ty_server/tests/e2e/snapshots/e2e__notebook__publish_unused_binding_diagnostics_open.snap diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 37d8c5ac48b5e..c1a89d7adbb55 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -1119,6 +1119,14 @@ pub fn is_stub_body(body: &[Stmt]) -> bool { }) } +/// Returns `body` without its leading docstring statement, if present. +pub fn body_without_leading_docstring(body: &[Stmt]) -> &[Stmt] { + match body.split_first() { + Some((first, rest)) if is_docstring_stmt(first) => rest, + _ => body, + } +} + /// Check if a node is part of a conditional branch. pub fn on_conditional_branch<'a>(parents: &mut impl Iterator) -> bool { parents.any(|parent| { diff --git a/crates/ty_python_semantic/src/semantic_index/builder.rs b/crates/ty_python_semantic/src/semantic_index/builder.rs index 53cf1eead255e..020a92fcc3648 100644 --- a/crates/ty_python_semantic/src/semantic_index/builder.rs +++ b/crates/ty_python_semantic/src/semantic_index/builder.rs @@ -204,6 +204,24 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { self.current_scope_info().file_scope_id } + /// Returns an iterator over ancestors of `scope` that are visible for name resolution, + /// starting with `scope` itself. This follows Python's lexical scoping rules where + /// class scopes are skipped during name resolution (except for the starting scope + /// if it happens to be a class scope). + /// + /// For example, in this code: + /// ```python + /// x = 1 + /// class A: + /// x = 2 + /// def method(self): + /// print(x) # Refers to global x=1, not class x=2 + /// ``` + /// The `method` function can see the global scope but not the class scope. + fn visible_ancestor_scopes(&self, scope: FileScopeId) -> VisibleAncestorsIter<'_> { + VisibleAncestorsIter::new(&self.scopes, scope) + } + /// Returns the scope ID of the current scope if the current scope /// is a method inside a class body or an eagerly executed scope inside a method. /// Returns `None` otherwise, e.g. if the current scope is a function body outside of a class, or if the current scope is not a @@ -499,9 +517,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { .symbol(enclosing_symbol) .name(); let is_reassignment_of_snapshotted_symbol = || { - for (ancestor, _) in - VisibleAncestorsIter::new(&self.scopes, key.enclosing_scope) - { + for (ancestor, _) in self.visible_ancestor_scopes(key.enclosing_scope) { if ancestor == current_scope { return true; } @@ -557,12 +573,13 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { }); } + /// Finds the nearest visible ancestor scope that actually owns a local binding for `name`. fn resolve_nested_reference_scope( &self, nested_scope: FileScopeId, name: &str, ) -> Option { - VisibleAncestorsIter::new(&self.scopes, nested_scope) + self.visible_ancestor_scopes(nested_scope) .skip(1) .find_map(|(scope_id, _)| { let place_table = &self.place_tables[scope_id]; diff --git a/crates/ty_python_semantic/src/semantic_index/definition.rs b/crates/ty_python_semantic/src/semantic_index/definition.rs index 4301b53506d61..0e7399950869c 100644 --- a/crates/ty_python_semantic/src/semantic_index/definition.rs +++ b/crates/ty_python_semantic/src/semantic_index/definition.rs @@ -863,6 +863,15 @@ impl DefinitionKind<'_> { matches!(self, DefinitionKind::Function(_)) } + pub(crate) const fn is_parameter_definition(&self) -> bool { + matches!( + self, + DefinitionKind::VariadicPositionalParameter(_) + | DefinitionKind::VariadicKeywordParameter(_) + | DefinitionKind::Parameter(_) + ) + } + pub(crate) const fn is_loop_header(&self) -> bool { matches!(self, DefinitionKind::LoopHeader(_)) } diff --git a/crates/ty_python_semantic/src/types/function.rs b/crates/ty_python_semantic/src/types/function.rs index 80ff811c8a3d6..ed5a8b3c7beed 100644 --- a/crates/ty_python_semantic/src/types/function.rs +++ b/crates/ty_python_semantic/src/types/function.rs @@ -1605,13 +1605,7 @@ pub(super) fn function_body_kind<'db>( infer_type: impl Fn(&ast::Expr) -> Type<'db>, ) -> FunctionBodyKind { // Allow docstrings, but only as the first statement. - let suite = if let Some(ast::Stmt::Expr(ast::StmtExpr { value, .. })) = node.body.first() - && value.is_string_literal_expr() - { - &node.body[1..] - } else { - &node.body[..] - }; + let suite = ast::helpers::body_without_leading_docstring(&node.body); if suite.iter().all(|stmt| match stmt { ast::Stmt::Pass(_) => true, diff --git a/crates/ty_python_semantic/src/types/ide_support.rs b/crates/ty_python_semantic/src/types/ide_support.rs index 185646dfb5dd0..58e2c7eab59fa 100644 --- a/crates/ty_python_semantic/src/types/ide_support.rs +++ b/crates/ty_python_semantic/src/types/ide_support.rs @@ -4,7 +4,7 @@ use crate::FxIndexSet; use crate::place::builtins_module_scope; use crate::semantic_index::definition::{Definition, DefinitionKind, DefinitionState}; use crate::semantic_index::place::ScopedPlaceId; -use crate::semantic_index::scope::ScopeKind; +use crate::semantic_index::scope::{FileScopeId, ScopeKind}; use crate::semantic_index::{attribute_scopes, global_scope, semantic_index, use_def_map}; use crate::types::call::{CallArguments, CallError, MatchedArgument}; use crate::types::class::{DynamicClassAnchor, DynamicNamedTupleAnchor}; @@ -1022,15 +1022,8 @@ fn find_parameter_range(parameters: &ast::Parameters, parameter_name: &str) -> O .map(|param| param.parameter.name.range()) } -/// Collects unused local bindings for IDE-facing diagnostics. -/// -/// This intentionally reports only function-, lambda-, and comprehension-scope bindings. -/// Module and class bindings can be observed indirectly (e.g., imports, attribute access), so -/// reporting them here risks false positives without cross-file/reference analysis. -fn should_mark_unused(name: &str) -> bool { - !name.starts_with('_') && !matches!(name, "self" | "cls") -} - +/// Returns `true` for definition kinds that create user-facing bindings we consider for +/// unused-binding diagnostics. fn should_consider_definition(kind: &DefinitionKind<'_>) -> bool { matches!( kind, @@ -1048,16 +1041,84 @@ fn should_consider_definition(kind: &DefinitionKind<'_>) -> bool { ) } +fn function_has_stub_body(function: &ast::StmtFunctionDef) -> bool { + let suite = ruff_python_ast::helpers::body_without_leading_docstring(&function.body); + + suite.iter().all(|stmt| match stmt { + ast::Stmt::Pass(_) => true, + ast::Stmt::Expr(ast::StmtExpr { value, .. }) => value.is_ellipsis_literal_expr(), + _ => false, + }) +} + +fn class_defines_member_named(db: &dyn Db, class: ClassType<'_>, member_name: &str) -> bool { + let Some((class_literal, specialization)) = class.static_class_literal(db) else { + // If we cannot inspect class members precisely, be conservative and avoid false positives. + return true; + }; + + let class_scope = class_literal.body_scope(db); + let class_place_table = crate::semantic_index::place_table(db, class_scope); + + class_place_table + .symbol_id(member_name) + .is_some_and(|symbol_id| { + let symbol = class_place_table.symbol(symbol_id); + symbol.is_bound() || symbol.is_declared() + }) + || class_literal + .own_synthesized_member(db, specialization, None, member_name) + .is_some() +} + +// Returns true if a superclass in the method's MRO defines the same method name. +// Used to suppress unused-parameter diagnostics for likely overrides. +fn method_name_exists_in_superclass( + db: &dyn Db, + index: &crate::semantic_index::SemanticIndex<'_>, + parsed: &ruff_db::parsed::ParsedModuleRef, + file_scope_id: FileScopeId, +) -> bool { + let scope = index.scope(file_scope_id); + let Some(function) = scope.node().as_function() else { + return false; + }; + + let Some(class_definition) = index.class_definition_of_method(file_scope_id) else { + return false; + }; + + let method_name = function.node(parsed).name.as_str(); + let Some(class_type) = crate::types::binding_type(db, class_definition).to_class_type(db) + else { + return false; + }; + + class_type + .iter_mro(db) + .skip(1) + .any(|class_base| match class_base { + ClassBase::Protocol | ClassBase::Generic | ClassBase::TypedDict => false, + ClassBase::Dynamic(_) => true, + ClassBase::Class(superclass) => class_defines_member_named(db, superclass, method_name), + }) +} + #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub struct UnusedBinding { pub range: TextRange, pub name: Name, } +/// Collects unused local bindings for IDE-facing diagnostics. +/// +/// This intentionally reports only function-, lambda-, and comprehension-scope bindings. +/// Module and class bindings can be observed indirectly (e.g., imports, attribute access), so +/// reporting them here risks false positives without cross-file/reference analysis. #[salsa::tracked(returns(ref))] pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec { let parsed = parsed_module(db, file).load(db); - if !parsed.errors().is_empty() || !parsed.unsupported_syntax_errors().is_empty() { + if !parsed.errors().is_empty() { return Vec::new(); } @@ -1069,9 +1130,6 @@ pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec Vec Vec Vec anyhow::Result<()> { + let source = dedent( + " + class Test: + def a(self, bar): + print(bar) + + class Test2(Test): + def a(self, bar): + ... + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn skips_unused_parameter_for_indirect_override() -> anyhow::Result<()> { + let source = dedent( + " + class A: + def a(self, bar): + print(bar) + + class B(A): + pass + + class C(B): + def a(self, bar): + ... + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn reports_unused_parameter_for_non_overriding_method() -> anyhow::Result<()> { + let source = dedent( + " + class Base: + def keep(self): + return 0 + + class Child(Base): + def new_method(self, dead): + return 1 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["dead"]); + Ok(()) + } + + #[test] + fn overriding_method_reports_unused_local_bindings() -> anyhow::Result<()> { + let source = dedent( + " + class Base: + def a(self, bar): + print(bar) + + class Child(Base): + def a(self, bar): + local_dead = 1 + return 0 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["local_dead"]); + Ok(()) + } + + #[test] + fn skips_unused_parameter_for_method_with_stub_body() -> anyhow::Result<()> { + let source = dedent( + " + class Test: + def a(self, bar): + ... + + def b(self, baz): + pass + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn skips_unused_parameter_for_overload_stub_declarations() -> anyhow::Result<()> { + let source = dedent( + " + import typing + + class Test: + @typing.overload + def a(self, bar: str): ... + + @typing.overload + def a(self, bar: int) -> None: + ... + + def a(self, bar: str | int) -> None: + print(bar) + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + #[test] fn captures_unused_function_and_lambda_parameters() -> anyhow::Result<()> { let source = dedent( @@ -1293,6 +1492,22 @@ mod unused_bindings_tests { Ok(()) } + #[test] + fn reports_non_parameter_self_and_cls_bindings() -> anyhow::Result<()> { + let source = dedent( + " + def f(xs): + self = 1 + [0 for cls in xs] + return 0 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["cls", "self"]); + Ok(()) + } + #[test] fn skips_closure_captured_bindings() -> anyhow::Result<()> { let source = dedent( diff --git a/crates/ty_server/src/server/api/diagnostics.rs b/crates/ty_server/src/server/api/diagnostics.rs index 52ff4feb70c68..893e2939c7b46 100644 --- a/crates/ty_server/src/server/api/diagnostics.rs +++ b/crates/ty_server/src/server/api/diagnostics.rs @@ -101,6 +101,27 @@ impl Diagnostics { .push(lsp_diagnostic); } + for binding in &self.unused_bindings { + let Some((url, lsp_diagnostic)) = unused_binding_to_lsp_diagnostic( + db, + self.file_or_notebook, + self.encoding, + binding, + ) else { + continue; + }; + + let Some(url) = url else { + tracing::warn!("Unable to find notebook cell"); + continue; + }; + + cell_diagnostics + .entry(url) + .or_default() + .push(lsp_diagnostic); + } + LspDiagnostics::NotebookDocument(cell_diagnostics) } else { let mut diagnostics = self @@ -349,7 +370,7 @@ pub(super) fn compute_diagnostics( } pub(super) fn collect_unused_bindings(db: &ProjectDatabase, file: File) -> Vec { - if db.notebook_document(file).is_some() || !db.project().should_check_file(db, file) { + if !db.project().should_check_file(db, file) { return Vec::new(); } unused_bindings(db, file).clone() @@ -363,12 +384,24 @@ pub(super) fn unused_bindings_to_lsp_diagnostics( ) -> Vec { unused_bindings .iter() - .map(|binding| Diagnostic { - range: binding - .range - .to_lsp_range(db, file, encoding) - .map(|range| range.local_range()) - .unwrap_or_default(), + .filter_map(|binding| unused_binding_to_lsp_diagnostic(db, file, encoding, binding)) + .map(|(_, diagnostic)| diagnostic) + .collect() +} + +fn unused_binding_to_lsp_diagnostic( + db: &ProjectDatabase, + file: File, + encoding: PositionEncoding, + binding: &UnusedBinding, +) -> Option<(Option, Diagnostic)> { + let range = binding.range.to_lsp_range(db, file, encoding)?; + let url = range.to_location().map(|location| location.uri); + + Some(( + url, + Diagnostic { + range: range.local_range(), severity: Some(DiagnosticSeverity::HINT), code: Some(NumberOrString::String(UNUSED_BINDING_CODE.to_owned())), code_description: None, @@ -377,8 +410,8 @@ pub(super) fn unused_bindings_to_lsp_diagnostics( related_information: None, tags: Some(vec![DiagnosticTag::UNNECESSARY]), data: None, - }) - .collect() + }, + )) } /// Converts the tool specific [`Diagnostic`][ruff_db::diagnostic::Diagnostic] to an LSP diff --git a/crates/ty_server/tests/e2e/notebook.rs b/crates/ty_server/tests/e2e/notebook.rs index 2247be813d88e..5192341358834 100644 --- a/crates/ty_server/tests/e2e/notebook.rs +++ b/crates/ty_server/tests/e2e/notebook.rs @@ -56,6 +56,30 @@ type Style = Literal["italic", "bold", "underline"]"#, Ok(()) } +#[test] +fn publish_unused_binding_diagnostics_open() -> anyhow::Result<()> { + let mut server = TestServerBuilder::new()? + .build() + .wait_until_workspaces_are_initialized(); + + server.initialization_result().unwrap(); + + let mut builder = NotebookBuilder::virtual_file("test.ipynb"); + builder.add_python_cell( + r#"def f(): + x = 1 + return 0 +"#, + ); + + builder.open(&mut server); + + let diagnostics = server.collect_publish_diagnostic_notifications(1); + assert_json_snapshot!(diagnostics); + + Ok(()) +} + #[test] fn diagnostic_end_of_file() -> anyhow::Result<()> { let mut server = TestServerBuilder::new()? diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__notebook__publish_unused_binding_diagnostics_open.snap b/crates/ty_server/tests/e2e/snapshots/e2e__notebook__publish_unused_binding_diagnostics_open.snap new file mode 100644 index 0000000000000..0ecea44095ffc --- /dev/null +++ b/crates/ty_server/tests/e2e/snapshots/e2e__notebook__publish_unused_binding_diagnostics_open.snap @@ -0,0 +1,27 @@ +--- +source: crates/ty_server/tests/e2e/notebook.rs +expression: diagnostics +--- +{ + "vscode-notebook-cell://test.ipynb#0": [ + { + "range": { + "start": { + "line": 1, + "character": 4 + }, + "end": { + "line": 1, + "character": 5 + } + }, + "severity": 4, + "code": "unused-binding", + "source": "ty", + "message": "Binding `x` is unused", + "tags": [ + 1 + ] + } + ] +} From 4a8fd05de38f23e4f678d3a0f70b282dee4c427d Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Thu, 19 Mar 2026 17:00:01 +0100 Subject: [PATCH 13/22] [ty] func name change --- crates/ty_python_semantic/src/semantic_index/definition.rs | 2 +- crates/ty_python_semantic/src/types/ide_support.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ty_python_semantic/src/semantic_index/definition.rs b/crates/ty_python_semantic/src/semantic_index/definition.rs index 0e7399950869c..83bbec5c1992c 100644 --- a/crates/ty_python_semantic/src/semantic_index/definition.rs +++ b/crates/ty_python_semantic/src/semantic_index/definition.rs @@ -863,7 +863,7 @@ impl DefinitionKind<'_> { matches!(self, DefinitionKind::Function(_)) } - pub(crate) const fn is_parameter_definition(&self) -> bool { + pub(crate) const fn is_parameter_def(&self) -> bool { matches!( self, DefinitionKind::VariadicPositionalParameter(_) diff --git a/crates/ty_python_semantic/src/types/ide_support.rs b/crates/ty_python_semantic/src/types/ide_support.rs index 58e2c7eab59fa..fcd6e68ad45d1 100644 --- a/crates/ty_python_semantic/src/types/ide_support.rs +++ b/crates/ty_python_semantic/src/types/ide_support.rs @@ -1163,7 +1163,7 @@ pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec Date: Mon, 23 Mar 2026 17:51:14 +0100 Subject: [PATCH 14/22] [ty] move unused_bindings to it's separate module, plus few more fixes --- .../src/types/ide_support.rs | 623 +--------------- .../src/types/ide_support/unused_bindings.rs | 666 ++++++++++++++++++ 2 files changed, 671 insertions(+), 618 deletions(-) create mode 100644 crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs diff --git a/crates/ty_python_semantic/src/types/ide_support.rs b/crates/ty_python_semantic/src/types/ide_support.rs index fcd6e68ad45d1..a315034544b0b 100644 --- a/crates/ty_python_semantic/src/types/ide_support.rs +++ b/crates/ty_python_semantic/src/types/ide_support.rs @@ -2,9 +2,7 @@ use std::collections::HashMap; use crate::FxIndexSet; use crate::place::builtins_module_scope; -use crate::semantic_index::definition::{Definition, DefinitionKind, DefinitionState}; -use crate::semantic_index::place::ScopedPlaceId; -use crate::semantic_index::scope::{FileScopeId, ScopeKind}; +use crate::semantic_index::definition::{Definition, DefinitionKind}; use crate::semantic_index::{attribute_scopes, global_scope, semantic_index, use_def_map}; use crate::types::call::{CallArguments, CallError, MatchedArgument}; use crate::types::class::{DynamicClassAnchor, DynamicNamedTupleAnchor}; @@ -23,8 +21,12 @@ use ruff_python_ast::{self as ast, AnyNodeRef, name::Name}; use ruff_text_size::{Ranged, TextRange}; use rustc_hash::FxHashSet; +#[path = "ide_support/unused_bindings.rs"] +mod unused_binding_support; + pub use resolve_definition::{ImportAliasResolution, ResolvedDefinition, map_stub_definition}; use resolve_definition::{find_symbol_in_scope, resolve_definition}; +pub use unused_binding_support::{UnusedBinding, unused_bindings}; /// Get the primary definition kind for a name expression within a specific file. /// Returns the first definition kind that is reachable for this name in its scope. @@ -1022,621 +1024,6 @@ fn find_parameter_range(parameters: &ast::Parameters, parameter_name: &str) -> O .map(|param| param.parameter.name.range()) } -/// Returns `true` for definition kinds that create user-facing bindings we consider for -/// unused-binding diagnostics. -fn should_consider_definition(kind: &DefinitionKind<'_>) -> bool { - matches!( - kind, - DefinitionKind::NamedExpression(_) - | DefinitionKind::Assignment(_) - | DefinitionKind::AnnotatedAssignment(_) - | DefinitionKind::For(_) - | DefinitionKind::Comprehension(_) - | DefinitionKind::VariadicPositionalParameter(_) - | DefinitionKind::VariadicKeywordParameter(_) - | DefinitionKind::Parameter(_) - | DefinitionKind::WithItem(_) - | DefinitionKind::MatchPattern(_) - | DefinitionKind::ExceptHandler(_) - ) -} - -fn function_has_stub_body(function: &ast::StmtFunctionDef) -> bool { - let suite = ruff_python_ast::helpers::body_without_leading_docstring(&function.body); - - suite.iter().all(|stmt| match stmt { - ast::Stmt::Pass(_) => true, - ast::Stmt::Expr(ast::StmtExpr { value, .. }) => value.is_ellipsis_literal_expr(), - _ => false, - }) -} - -fn class_defines_member_named(db: &dyn Db, class: ClassType<'_>, member_name: &str) -> bool { - let Some((class_literal, specialization)) = class.static_class_literal(db) else { - // If we cannot inspect class members precisely, be conservative and avoid false positives. - return true; - }; - - let class_scope = class_literal.body_scope(db); - let class_place_table = crate::semantic_index::place_table(db, class_scope); - - class_place_table - .symbol_id(member_name) - .is_some_and(|symbol_id| { - let symbol = class_place_table.symbol(symbol_id); - symbol.is_bound() || symbol.is_declared() - }) - || class_literal - .own_synthesized_member(db, specialization, None, member_name) - .is_some() -} - -// Returns true if a superclass in the method's MRO defines the same method name. -// Used to suppress unused-parameter diagnostics for likely overrides. -fn method_name_exists_in_superclass( - db: &dyn Db, - index: &crate::semantic_index::SemanticIndex<'_>, - parsed: &ruff_db::parsed::ParsedModuleRef, - file_scope_id: FileScopeId, -) -> bool { - let scope = index.scope(file_scope_id); - let Some(function) = scope.node().as_function() else { - return false; - }; - - let Some(class_definition) = index.class_definition_of_method(file_scope_id) else { - return false; - }; - - let method_name = function.node(parsed).name.as_str(); - let Some(class_type) = crate::types::binding_type(db, class_definition).to_class_type(db) - else { - return false; - }; - - class_type - .iter_mro(db) - .skip(1) - .any(|class_base| match class_base { - ClassBase::Protocol | ClassBase::Generic | ClassBase::TypedDict => false, - ClassBase::Dynamic(_) => true, - ClassBase::Class(superclass) => class_defines_member_named(db, superclass, method_name), - }) -} - -#[derive(Debug, Clone, Eq, PartialEq, Hash)] -pub struct UnusedBinding { - pub range: TextRange, - pub name: Name, -} - -/// Collects unused local bindings for IDE-facing diagnostics. -/// -/// This intentionally reports only function-, lambda-, and comprehension-scope bindings. -/// Module and class bindings can be observed indirectly (e.g., imports, attribute access), so -/// reporting them here risks false positives without cross-file/reference analysis. -#[salsa::tracked(returns(ref))] -pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec { - let parsed = parsed_module(db, file).load(db); - if !parsed.errors().is_empty() { - return Vec::new(); - } - - let index = semantic_index(db, file); - let mut unused = Vec::new(); - - for scope_id in index.scope_ids() { - let file_scope_id = scope_id.file_scope_id(db); - let scope = index.scope(file_scope_id); - let scope_kind = scope.kind(); - - if !matches!( - scope_kind, - ScopeKind::Function | ScopeKind::Lambda | ScopeKind::Comprehension - ) { - continue; - } - - let is_method_scope = index.class_definition_of_method(file_scope_id).is_some(); - let method_has_stub_body = is_method_scope - && scope - .node() - .as_function() - .is_some_and(|function| function_has_stub_body(function.node(&parsed))); - let skip_unused_parameters_for_override = - method_name_exists_in_superclass(db, index, &parsed, file_scope_id); - - let place_table = index.place_table(file_scope_id); - let use_def_map = index.use_def_map(file_scope_id); - - for (_, state, is_used) in use_def_map.all_definitions_with_usage() { - let DefinitionState::Defined(definition) = state else { - continue; - }; - - if is_used { - continue; - } - - let kind = definition.kind(db); - if !should_consider_definition(kind) { - continue; - } - - let is_parameter = kind.is_parameter_def(); - if is_parameter && (skip_unused_parameters_for_override || method_has_stub_body) { - continue; - } - - let ScopedPlaceId::Symbol(symbol_id) = definition.place(db) else { - continue; - }; - - let symbol = place_table.symbol(symbol_id); - let name = symbol.name().as_str(); - - // Skip conventional method receiver parameters - if is_parameter && is_method_scope && matches!(name, "self" | "cls") { - continue; - } - - if name.starts_with('_') { - continue; - } - - // Global and nonlocal assignments target bindings from outer scopes. - // Treat them as externally managed to avoid false positives here. - if symbol.is_global() || symbol.is_nonlocal() { - continue; - } - - let range = kind.target_range(&parsed); - - unused.push(UnusedBinding { - range, - name: symbol.name().clone(), - }); - } - } - - unused.sort_unstable_by_key(|binding| (binding.range.start(), binding.range.end())); - unused.dedup_by_key(|binding| binding.range); - - unused -} - -#[cfg(test)] -mod unused_bindings_tests { - use super::{UnusedBinding, unused_bindings}; - use crate::db::tests::TestDbBuilder; - use ruff_db::files::system_path_to_file; - use ruff_python_ast::name::Name; - use ruff_python_trivia::textwrap::dedent; - use ruff_text_size::{TextRange, TextSize}; - - fn collect_unused_bindings(source: &str) -> anyhow::Result> { - let db = TestDbBuilder::new() - .with_file("/src/main.py", source) - .build()?; - let file = system_path_to_file(&db, "/src/main.py").unwrap(); - let mut bindings = unused_bindings(&db, file).clone(); - bindings.sort_unstable_by_key(|binding| (binding.range.start(), binding.range.end())); - Ok(bindings) - } - - fn collect_unused_names(source: &str) -> anyhow::Result> { - let mut names = collect_unused_bindings(source)? - .iter() - .map(|binding| binding.name.to_string()) - .collect::>(); - names.sort(); - Ok(names) - } - - #[test] - fn captures_safe_local_binding_kinds() -> anyhow::Result<()> { - let source = dedent( - " - def f(): - used_assign, dead_assign = (1, 2) - print(used_assign) - - for used_loop, dead_loop in [(1, 2)]: - print(used_loop) - - with open(\"x\") as dead_with: - pass - - try: - 1 / 0 - except Exception as dead_exc: - pass - - if (dead_walrus := 1): - pass - - [1 for dead_comp in range(3)] - [ok_comp for ok_comp, dead_comp2 in [(1, 2)]] - - match {\"x\": 1, \"y\": 2}: - case {\"x\": used_match, **dead_rest}: - print(used_match) - case [used_star, *dead_star] as dead_as: - print(used_star) - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!( - names, - vec![ - "dead_as", - "dead_assign", - "dead_comp", - "dead_comp2", - "dead_exc", - "dead_loop", - "dead_rest", - "dead_star", - "dead_walrus", - "dead_with", - ] - ); - Ok(()) - } - - #[test] - fn skips_module_and_class_scope_bindings() -> anyhow::Result<()> { - let source = dedent( - " - module_dead = 1 - - class C: - class_dead = 1 - - def method(self): - local_dead = 1 - return 0 - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["local_dead"]); - Ok(()) - } - - #[test] - fn skips_placeholder_and_dunder_locals() -> anyhow::Result<()> { - let source = dedent( - " - def f(): - local_dead = 1 - _ = 2 - _ignored = 3 - __dunder__ = 4 - return 0 - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["local_dead"]); - Ok(()) - } - - #[test] - fn skips_global_and_nonlocal_assignments() -> anyhow::Result<()> { - let source = dedent( - " - global_value = 0 - - def mutate_global(): - global global_value - global_value = 1 - local_dead = 1 - - def outer(): - captured = 0 - - def inner(): - nonlocal captured - captured = 1 - - inner() - return captured - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["local_dead"]); - Ok(()) - } - - #[test] - fn skips_unused_parameter_for_overriding_method() -> anyhow::Result<()> { - let source = dedent( - " - class Test: - def a(self, bar): - print(bar) - - class Test2(Test): - def a(self, bar): - ... - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, Vec::::new()); - Ok(()) - } - - #[test] - fn skips_unused_parameter_for_indirect_override() -> anyhow::Result<()> { - let source = dedent( - " - class A: - def a(self, bar): - print(bar) - - class B(A): - pass - - class C(B): - def a(self, bar): - ... - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, Vec::::new()); - Ok(()) - } - - #[test] - fn reports_unused_parameter_for_non_overriding_method() -> anyhow::Result<()> { - let source = dedent( - " - class Base: - def keep(self): - return 0 - - class Child(Base): - def new_method(self, dead): - return 1 - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["dead"]); - Ok(()) - } - - #[test] - fn overriding_method_reports_unused_local_bindings() -> anyhow::Result<()> { - let source = dedent( - " - class Base: - def a(self, bar): - print(bar) - - class Child(Base): - def a(self, bar): - local_dead = 1 - return 0 - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["local_dead"]); - Ok(()) - } - - #[test] - fn skips_unused_parameter_for_method_with_stub_body() -> anyhow::Result<()> { - let source = dedent( - " - class Test: - def a(self, bar): - ... - - def b(self, baz): - pass - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, Vec::::new()); - Ok(()) - } - - #[test] - fn skips_unused_parameter_for_overload_stub_declarations() -> anyhow::Result<()> { - let source = dedent( - " - import typing - - class Test: - @typing.overload - def a(self, bar: str): ... - - @typing.overload - def a(self, bar: int) -> None: - ... - - def a(self, bar: str | int) -> None: - print(bar) - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, Vec::::new()); - Ok(()) - } - - #[test] - fn captures_unused_function_and_lambda_parameters() -> anyhow::Result<()> { - let source = dedent( - " - def fn(used, dead, _ignored, __dunder__): - return used - - def fn_defaults(a, b=1, *, c=2, d): - return a + d - - lam = lambda x, y, z=1: x + z - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["b", "c", "dead", "y"]); - Ok(()) - } - - #[test] - fn reports_non_parameter_self_and_cls_bindings() -> anyhow::Result<()> { - let source = dedent( - " - def f(xs): - self = 1 - [0 for cls in xs] - return 0 - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["cls", "self"]); - Ok(()) - } - - #[test] - fn skips_closure_captured_bindings() -> anyhow::Result<()> { - let source = dedent( - " - def outer(flag: bool): - captured = 1 - dead = 2 - - def inner(): - return captured - - if flag: - captured = 3 - - return inner - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["dead"]); - Ok(()) - } - - #[test] - fn closure_uses_nearest_shadowed_binding() -> anyhow::Result<()> { - let source = dedent( - " - def outer(): - x = 0 - - def mid(): - x = 1 - - def inner(): - return x - - return inner - - return mid - ", - ); - - let bindings = collect_unused_bindings(&source)?; - let outer_x_start = TextSize::try_from(source.find("x = 0").unwrap()).unwrap(); - assert_eq!( - bindings, - vec![UnusedBinding { - range: TextRange::new(outer_x_start, outer_x_start + TextSize::new(1)), - name: Name::new("x"), - }] - ); - Ok(()) - } - - #[test] - fn nonlocal_proxy_scope_still_marks_outer_binding_used() -> anyhow::Result<()> { - let source = dedent( - " - def outer(): - x = 1 - - def mid(): - nonlocal x - x = 2 - - def inner(): - return x - - return inner - - return mid - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, Vec::::new()); - Ok(()) - } - - #[test] - fn nested_local_same_name_does_not_mark_outer_used() -> anyhow::Result<()> { - let source = dedent( - " - def outer(): - x = 1 - - def inner(): - x = 2 - return x - - return inner - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["x"]); - Ok(()) - } - - #[test] - fn comprehension_binding_captured_by_nested_lambda_is_used() -> anyhow::Result<()> { - let source = dedent( - " - def f(): - funcs = [lambda: x for x in range(3)] - return funcs - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, Vec::::new()); - Ok(()) - } - - #[test] - fn skips_unused_binding_analysis_on_syntax_error() -> anyhow::Result<()> { - let source = dedent( - " - def f( - x = 1 - ", - ); - - let names = collect_unused_names(&source)?; - assert_eq!(names, Vec::::new()); - Ok(()) - } -} - mod resolve_definition { //! Resolves an Import, `ImportFrom` or `StarImport` definition to one or more //! "resolved definitions". This is done recursively to find the original diff --git a/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs new file mode 100644 index 0000000000000..47c7066d46567 --- /dev/null +++ b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs @@ -0,0 +1,666 @@ +use crate::Db; +use crate::semantic_index::definition::{DefinitionKind, DefinitionState}; +use crate::semantic_index::place::ScopedPlaceId; +use crate::semantic_index::scope::{FileScopeId, ScopeKind}; +use crate::semantic_index::semantic_index; +use crate::types::{ClassBase, ClassType}; +use ruff_db::parsed::{ParsedModuleRef, parsed_module}; +use ruff_python_ast::{self as ast, name::Name}; +use ruff_text_size::TextRange; + +/// Returns `true` for definition kinds that create user-facing bindings we consider for +/// unused-binding diagnostics. +fn should_consider_definition(kind: &DefinitionKind<'_>) -> bool { + match kind { + DefinitionKind::NamedExpression(_) + | DefinitionKind::Assignment(_) + | DefinitionKind::AnnotatedAssignment(_) + | DefinitionKind::For(_) + | DefinitionKind::Comprehension(_) + | DefinitionKind::VariadicPositionalParameter(_) + | DefinitionKind::VariadicKeywordParameter(_) + | DefinitionKind::Parameter(_) + | DefinitionKind::WithItem(_) + | DefinitionKind::MatchPattern(_) + | DefinitionKind::ExceptHandler(_) => true, + + DefinitionKind::Import(_) + | DefinitionKind::ImportFrom(_) + | DefinitionKind::ImportFromSubmodule(_) + | DefinitionKind::StarImport(_) + | DefinitionKind::Function(_) + | DefinitionKind::Class(_) + | DefinitionKind::TypeAlias(_) + | DefinitionKind::AugmentedAssignment(_) + | DefinitionKind::DictKeyAssignment(_) + | DefinitionKind::TypeVar(_) + | DefinitionKind::ParamSpec(_) + | DefinitionKind::TypeVarTuple(_) + | DefinitionKind::LoopHeader(_) => false, + } +} + +fn function_has_stub_body(function: &ast::StmtFunctionDef) -> bool { + let suite = ruff_python_ast::helpers::body_without_leading_docstring(&function.body); + + suite.iter().all(|stmt| match stmt { + ast::Stmt::Pass(_) => true, + ast::Stmt::Expr(ast::StmtExpr { value, .. }) => value.is_ellipsis_literal_expr(), + _ => false, + }) +} + +fn class_defines_member_named(db: &dyn Db, class: ClassType<'_>, member_name: &str) -> bool { + let Some((class_literal, specialization)) = class.static_class_literal(db) else { + // If we cannot inspect class members precisely, be conservative and avoid false positives. + return true; + }; + + let class_scope = class_literal.body_scope(db); + let class_place_table = crate::semantic_index::place_table(db, class_scope); + + class_place_table + .symbol_id(member_name) + .is_some_and(|symbol_id| { + let symbol = class_place_table.symbol(symbol_id); + symbol.is_bound() || symbol.is_declared() + }) + || class_literal + .own_synthesized_member(db, specialization, None, member_name) + .is_some() +} + +// Returns true if a superclass in the method's MRO defines the same method name. +// Used to suppress unused-parameter diagnostics for likely overrides. +fn method_name_exists_in_superclass( + db: &dyn Db, + index: &crate::semantic_index::SemanticIndex<'_>, + parsed: &ParsedModuleRef, + file_scope_id: FileScopeId, +) -> bool { + let scope = index.scope(file_scope_id); + let Some(function) = scope.node().as_function() else { + return false; + }; + + let Some(class_definition) = index.class_definition_of_method(file_scope_id) else { + return false; + }; + + let method_name = function.node(parsed).name.as_str(); + let Some(class_type) = crate::types::binding_type(db, class_definition).to_class_type(db) + else { + return false; + }; + + class_type + .iter_mro(db) + .skip(1) + .any(|class_base| match class_base { + ClassBase::Protocol | ClassBase::Generic | ClassBase::TypedDict => false, + ClassBase::Dynamic(_) => true, + ClassBase::Class(superclass) => class_defines_member_named(db, superclass, method_name), + }) +} + +#[derive(Debug, Clone, Eq, PartialEq, Hash)] +pub struct UnusedBinding { + pub range: TextRange, + pub name: Name, +} + +/// Collects unused local bindings for IDE-facing diagnostics. +/// +/// This intentionally reports only function-, lambda-, and comprehension-scope bindings. +/// Even with local checks such as override detection, module- and class-scope bindings +/// can still be observed indirectly (for example via imports or attribute access), so +/// reporting them here would risk false positives without broader reference analysis. +#[salsa::tracked(returns(ref))] +pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec { + let parsed = parsed_module(db, file).load(db); + if !parsed.errors().is_empty() { + return Vec::new(); + } + + let is_stub_file = file.is_stub(db); + let index = semantic_index(db, file); + let mut unused = Vec::new(); + + for scope_id in index.scope_ids() { + let file_scope_id = scope_id.file_scope_id(db); + let scope = index.scope(file_scope_id); + let scope_kind = scope.kind(); + + if !matches!( + scope_kind, + ScopeKind::Function | ScopeKind::Lambda | ScopeKind::Comprehension + ) { + continue; + } + + let is_method_scope = index.class_definition_of_method(file_scope_id).is_some(); + let method_has_stub_body = is_method_scope + && scope + .node() + .as_function() + .is_some_and(|function| function_has_stub_body(function.node(&parsed))); + let skip_unused_parameters_for_override = + method_name_exists_in_superclass(db, index, &parsed, file_scope_id); + + let place_table = index.place_table(file_scope_id); + let use_def_map = index.use_def_map(file_scope_id); + + for (_, state, is_used) in use_def_map.all_definitions_with_usage() { + let DefinitionState::Defined(definition) = state else { + continue; + }; + + if is_used { + continue; + } + + let kind = definition.kind(db); + if !should_consider_definition(kind) { + continue; + } + + let is_parameter = kind.is_parameter_def(); + if is_parameter + && (is_stub_file || skip_unused_parameters_for_override || method_has_stub_body) + { + continue; + } + + let ScopedPlaceId::Symbol(symbol_id) = definition.place(db) else { + continue; + }; + + let symbol = place_table.symbol(symbol_id); + let name = symbol.name().as_str(); + + // Skip conventional method receiver parameters. + if is_parameter && is_method_scope && matches!(name, "self" | "cls") { + continue; + } + + if name.starts_with('_') { + continue; + } + + // Global and nonlocal assignments target bindings from outer scopes. + // Treat them as externally managed to avoid false positives here. + if symbol.is_global() || symbol.is_nonlocal() { + continue; + } + + let range = kind.target_range(&parsed); + + unused.push(UnusedBinding { + range, + name: symbol.name().clone(), + }); + } + } + + unused.sort_unstable_by_key(|binding| (binding.range.start(), binding.range.end())); + unused.dedup_by_key(|binding| binding.range); + + unused +} + +#[cfg(test)] +mod tests { + use super::{UnusedBinding, unused_bindings}; + use crate::db::tests::TestDbBuilder; + use ruff_db::files::system_path_to_file; + use ruff_python_ast::name::Name; + use ruff_python_trivia::textwrap::dedent; + use ruff_text_size::{TextRange, TextSize}; + + fn collect_unused_bindings_in_file( + path: &str, + source: &str, + ) -> anyhow::Result> { + let db = TestDbBuilder::new().with_file(path, source).build()?; + let file = system_path_to_file(&db, path).unwrap(); + let mut bindings = unused_bindings(&db, file).clone(); + bindings.sort_unstable_by_key(|binding| (binding.range.start(), binding.range.end())); + Ok(bindings) + } + + fn collect_unused_bindings(source: &str) -> anyhow::Result> { + collect_unused_bindings_in_file("/src/main.py", source) + } + + fn collect_unused_names_in_file(path: &str, source: &str) -> anyhow::Result> { + let mut names = collect_unused_bindings_in_file(path, source)? + .iter() + .map(|binding| binding.name.to_string()) + .collect::>(); + names.sort(); + Ok(names) + } + + fn collect_unused_names(source: &str) -> anyhow::Result> { + collect_unused_names_in_file("/src/main.py", source) + } + + #[test] + fn captures_safe_local_binding_kinds() -> anyhow::Result<()> { + let source = dedent( + " + def f(): + used_assign, dead_assign = (1, 2) + print(used_assign) + + for used_loop, dead_loop in [(1, 2)]: + print(used_loop) + + with open(\"x\") as dead_with: + pass + + try: + 1 / 0 + except Exception as dead_exc: + pass + + if (dead_walrus := 1): + pass + + [1 for dead_comp in range(3)] + [ok_comp for ok_comp, dead_comp2 in [(1, 2)]] + + match {\"x\": 1, \"y\": 2}: + case {\"x\": used_match, **dead_rest}: + print(used_match) + case [used_star, *dead_star] as dead_as: + print(used_star) + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!( + names, + vec![ + "dead_as", + "dead_assign", + "dead_comp", + "dead_comp2", + "dead_exc", + "dead_loop", + "dead_rest", + "dead_star", + "dead_walrus", + "dead_with", + ] + ); + Ok(()) + } + + #[test] + fn skips_module_and_class_scope_bindings() -> anyhow::Result<()> { + let source = dedent( + " + module_dead = 1 + + class C: + class_dead = 1 + + def method(self): + local_dead = 1 + return 0 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["local_dead"]); + Ok(()) + } + + #[test] + fn skips_placeholder_and_dunder_locals() -> anyhow::Result<()> { + let source = dedent( + " + def f(): + local_dead = 1 + _ = 2 + _ignored = 3 + __dunder__ = 4 + return 0 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["local_dead"]); + Ok(()) + } + + #[test] + fn skips_global_and_nonlocal_assignments() -> anyhow::Result<()> { + let source = dedent( + " + global_value = 0 + + def mutate_global(): + global global_value + global_value = 1 + local_dead = 1 + + def outer(): + captured = 0 + + def inner(): + nonlocal captured + captured = 1 + + inner() + return captured + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["local_dead"]); + Ok(()) + } + + #[test] + fn skips_unused_parameter_for_overriding_method() -> anyhow::Result<()> { + let source = dedent( + " + class Test: + def a(self, bar): + print(bar) + + class Test2(Test): + def a(self, bar): + ... + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn skips_unused_parameter_for_indirect_override() -> anyhow::Result<()> { + let source = dedent( + " + class A: + def a(self, bar): + print(bar) + + class B(A): + pass + + class C(B): + def a(self, bar): + ... + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn reports_unused_parameter_for_non_overriding_method() -> anyhow::Result<()> { + let source = dedent( + " + class Base: + def keep(self): + return 0 + + class Child(Base): + def new_method(self, dead): + return 1 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["dead"]); + Ok(()) + } + + #[test] + fn overriding_method_reports_unused_local_bindings() -> anyhow::Result<()> { + let source = dedent( + " + class Base: + def a(self, bar): + print(bar) + + class Child(Base): + def a(self, bar): + local_dead = 1 + return 0 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["local_dead"]); + Ok(()) + } + + #[test] + fn skips_unused_parameter_for_method_with_stub_body() -> anyhow::Result<()> { + let source = dedent( + " + class Test: + def a(self, bar): + ... + + def b(self, baz): + pass + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn skips_unused_parameter_for_overload_stub_declarations() -> anyhow::Result<()> { + let source = dedent( + " + import typing + + class Test: + @typing.overload + def a(self, bar: str): ... + + @typing.overload + def a(self, bar: int) -> None: + ... + + def a(self, bar: str | int) -> None: + print(bar) + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn skips_unused_parameters_in_stub_files() -> anyhow::Result<()> { + let source = dedent( + " + def f(x, y) -> None: ... + + class C: + def m(self, z) -> None: ... + ", + ); + + let names = collect_unused_names_in_file("/src/main.pyi", &source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn captures_unused_function_and_lambda_parameters() -> anyhow::Result<()> { + let source = dedent( + " + def fn(used, dead, _ignored, __dunder__): + return used + + def fn_defaults(a, b=1, *, c=2, d): + return a + d + + lam = lambda x, y, z=1: x + z + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["b", "c", "dead", "y"]); + Ok(()) + } + + #[test] + fn reports_non_parameter_self_and_cls_bindings() -> anyhow::Result<()> { + let source = dedent( + " + def f(xs): + self = 1 + [0 for cls in xs] + return 0 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["cls", "self"]); + Ok(()) + } + + #[test] + fn skips_closure_captured_bindings() -> anyhow::Result<()> { + let source = dedent( + " + def outer(flag: bool): + captured = 1 + dead = 2 + + def inner(): + return captured + + if flag: + captured = 3 + + return inner + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["dead"]); + Ok(()) + } + + #[test] + fn closure_uses_nearest_shadowed_binding() -> anyhow::Result<()> { + let source = dedent( + " + def outer(): + x = 0 + + def mid(): + x = 1 + + def inner(): + return x + + return inner + + return mid + ", + ); + + let bindings = collect_unused_bindings(&source)?; + let outer_x_start = TextSize::try_from(source.find("x = 0").unwrap()).unwrap(); + assert_eq!( + bindings, + vec![UnusedBinding { + range: TextRange::new(outer_x_start, outer_x_start + TextSize::new(1)), + name: Name::new("x"), + }] + ); + Ok(()) + } + + #[test] + fn nonlocal_proxy_scope_still_marks_outer_binding_used() -> anyhow::Result<()> { + let source = dedent( + " + def outer(): + x = 1 + + def mid(): + nonlocal x + x = 2 + + def inner(): + return x + + return inner + + return mid + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn nested_local_same_name_does_not_mark_outer_used() -> anyhow::Result<()> { + let source = dedent( + " + def outer(): + x = 1 + + def inner(): + x = 2 + return x + + return inner + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["x"]); + Ok(()) + } + + #[test] + fn comprehension_binding_captured_by_nested_lambda_is_used() -> anyhow::Result<()> { + let source = dedent( + " + def f(): + funcs = [lambda: x for x in range(3)] + return funcs + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + + #[test] + fn skips_unused_binding_analysis_on_syntax_error() -> anyhow::Result<()> { + let source = dedent( + " + def f( + x = 1 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } +} From e0f63004c6be7ef421081fca055b587de5a65e40 Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Mon, 23 Mar 2026 18:15:38 +0100 Subject: [PATCH 15/22] [ty] move override suppression to loop and compute lazily --- .../src/types/ide_support/unused_bindings.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs index 47c7066d46567..d6c1826e8687a 100644 --- a/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs +++ b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs @@ -144,11 +144,9 @@ pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec Vec Date: Wed, 25 Mar 2026 15:20:09 +0100 Subject: [PATCH 16/22] [ty] cover overload for module level, omit lsp code reporting --- .../src/types/ide_support/unused_bindings.rs | 49 +++++++++++++++++++ .../ty_server/src/server/api/diagnostics.rs | 7 +-- ...blish_unused_binding_diagnostics_open.snap | 3 +- ...used_binding_has_unnecessary_hint_tag.snap | 3 +- ...space_reports_unused_binding_hint_tag.snap | 3 +- 5 files changed, 54 insertions(+), 11 deletions(-) diff --git a/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs index d6c1826e8687a..b85c22ee37758 100644 --- a/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs +++ b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs @@ -50,6 +50,29 @@ fn function_has_stub_body(function: &ast::StmtFunctionDef) -> bool { }) } +fn function_scope_is_overload_declaration( + db: &dyn Db, + index: &crate::semantic_index::SemanticIndex<'_>, + file_scope_id: FileScopeId, +) -> bool { + let scope = index.scope(file_scope_id); + let Some(function) = scope.node().as_function() else { + return false; + }; + + let definition = index.expect_single_definition(function); + let Some(function_type) = crate::types::binding_type(db, definition).as_function_literal() + else { + return false; + }; + + function_type + .iter_overloads_and_implementation(db) + .any(|overload| { + overload.body_scope(db).file_scope_id(db) == file_scope_id && overload.is_overload(db) + }) +} + fn class_defines_member_named(db: &dyn Db, class: ClassType<'_>, member_name: &str) -> bool { let Some((class_literal, specialization)) = class.static_class_literal(db) else { // If we cannot inspect class members precisely, be conservative and avoid false positives. @@ -144,6 +167,8 @@ pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec Vec anyhow::Result<()> { + let source = dedent( + " + import typing + + @typing.overload + def f(x: str) -> str: ... + + @typing.overload + def f(x: int) -> int: + ... + + def f(x: str | int) -> str | int: + return x + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, Vec::::new()); + Ok(()) + } + #[test] fn skips_unused_parameters_in_stub_files() -> anyhow::Result<()> { let source = dedent( diff --git a/crates/ty_server/src/server/api/diagnostics.rs b/crates/ty_server/src/server/api/diagnostics.rs index 893e2939c7b46..f942b91ae8502 100644 --- a/crates/ty_server/src/server/api/diagnostics.rs +++ b/crates/ty_server/src/server/api/diagnostics.rs @@ -25,9 +25,6 @@ use crate::system::{AnySystemPath, file_to_url}; use crate::{DIAGNOSTIC_NAME, Db, DiagnosticMode}; use crate::{PositionEncoding, Session}; -/// Server-only LSP code for IDE dimming (not a configurable lint rule). -const UNUSED_BINDING_CODE: &str = "unused-binding"; - #[derive(Debug)] pub(super) struct Diagnostics { items: Vec, @@ -403,10 +400,10 @@ fn unused_binding_to_lsp_diagnostic( Diagnostic { range: range.local_range(), severity: Some(DiagnosticSeverity::HINT), - code: Some(NumberOrString::String(UNUSED_BINDING_CODE.to_owned())), + code: None, code_description: None, source: Some(DIAGNOSTIC_NAME.into()), - message: format!("Binding `{}` is unused", binding.name), + message: format!("`{}` is unused", binding.name), related_information: None, tags: Some(vec![DiagnosticTag::UNNECESSARY]), data: None, diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__notebook__publish_unused_binding_diagnostics_open.snap b/crates/ty_server/tests/e2e/snapshots/e2e__notebook__publish_unused_binding_diagnostics_open.snap index 0ecea44095ffc..f82afce67e97b 100644 --- a/crates/ty_server/tests/e2e/snapshots/e2e__notebook__publish_unused_binding_diagnostics_open.snap +++ b/crates/ty_server/tests/e2e/snapshots/e2e__notebook__publish_unused_binding_diagnostics_open.snap @@ -16,9 +16,8 @@ expression: diagnostics } }, "severity": 4, - "code": "unused-binding", "source": "ty", - "message": "Binding `x` is unused", + "message": "`x` is unused", "tags": [ 1 ] diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__unused_binding_has_unnecessary_hint_tag.snap b/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__unused_binding_has_unnecessary_hint_tag.snap index dc6fe5610a8d2..33e093b0a6a8b 100644 --- a/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__unused_binding_has_unnecessary_hint_tag.snap +++ b/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__unused_binding_has_unnecessary_hint_tag.snap @@ -18,9 +18,8 @@ expression: diagnostics } }, "severity": 4, - "code": "unused-binding", "source": "ty", - "message": "Binding `x` is unused", + "message": "`x` is unused", "tags": [ 1 ] diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__workspace_reports_unused_binding_hint_tag.snap b/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__workspace_reports_unused_binding_hint_tag.snap index 2e056552f399f..ab01655f55c8b 100644 --- a/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__workspace_reports_unused_binding_hint_tag.snap +++ b/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__workspace_reports_unused_binding_hint_tag.snap @@ -22,9 +22,8 @@ expression: diagnostics } }, "severity": 4, - "code": "unused-binding", "source": "ty", - "message": "Binding `x` is unused", + "message": "`x` is unused", "tags": [ 1 ] From f02d9b62b6d90f9759ea97d5e6cd4fc59ebfbed7 Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Wed, 25 Mar 2026 18:36:15 +0200 Subject: [PATCH 17/22] Update crates/ty_python_semantic/src/types/ide_support.rs Co-authored-by: Micha Reiser --- crates/ty_python_semantic/src/types/ide_support.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/ty_python_semantic/src/types/ide_support.rs b/crates/ty_python_semantic/src/types/ide_support.rs index a315034544b0b..de05248c44d32 100644 --- a/crates/ty_python_semantic/src/types/ide_support.rs +++ b/crates/ty_python_semantic/src/types/ide_support.rs @@ -21,8 +21,7 @@ use ruff_python_ast::{self as ast, AnyNodeRef, name::Name}; use ruff_text_size::{Ranged, TextRange}; use rustc_hash::FxHashSet; -#[path = "ide_support/unused_bindings.rs"] -mod unused_binding_support; +mod unused_bindings; pub use resolve_definition::{ImportAliasResolution, ResolvedDefinition, map_stub_definition}; use resolve_definition::{find_symbol_in_scope, resolve_definition}; From 183cff04efd9c828f3ed5108e7981093bac67219 Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Wed, 25 Mar 2026 17:45:19 +0100 Subject: [PATCH 18/22] [ty] get back to module alias --- crates/ty_python_semantic/src/types/ide_support.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ty_python_semantic/src/types/ide_support.rs b/crates/ty_python_semantic/src/types/ide_support.rs index de05248c44d32..a315034544b0b 100644 --- a/crates/ty_python_semantic/src/types/ide_support.rs +++ b/crates/ty_python_semantic/src/types/ide_support.rs @@ -21,7 +21,8 @@ use ruff_python_ast::{self as ast, AnyNodeRef, name::Name}; use ruff_text_size::{Ranged, TextRange}; use rustc_hash::FxHashSet; -mod unused_bindings; +#[path = "ide_support/unused_bindings.rs"] +mod unused_binding_support; pub use resolve_definition::{ImportAliasResolution, ResolvedDefinition, map_stub_definition}; use resolve_definition::{find_symbol_in_scope, resolve_definition}; From 7a8c1b1fc1022e88f2f5f7fbeadd41ce76e6f843 Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Thu, 26 Mar 2026 17:30:21 +0100 Subject: [PATCH 19/22] [ty] don't quit early on parsing errors --- .../src/types/ide_support/unused_bindings.rs | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs index b85c22ee37758..a6fa784a3d452 100644 --- a/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs +++ b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs @@ -141,10 +141,6 @@ pub struct UnusedBinding { #[salsa::tracked(returns(ref))] pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec { let parsed = parsed_module(db, file).load(db); - if !parsed.errors().is_empty() { - return Vec::new(); - } - let is_stub_file = file.is_stub(db); let index = semantic_index(db, file); let mut unused = Vec::new(); @@ -703,7 +699,7 @@ mod tests { } #[test] - fn skips_unused_binding_analysis_on_syntax_error() -> anyhow::Result<()> { + fn reports_unused_binding_on_syntax_error() -> anyhow::Result<()> { let source = dedent( " def f( @@ -711,6 +707,20 @@ mod tests { ", ); + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["x"]); + Ok(()) + } + + #[test] + fn does_not_report_used_parameter_on_syntax_error() -> anyhow::Result<()> { + let source = dedent( + " + def f(x + return x + ", + ); + let names = collect_unused_names(&source)?; assert_eq!(names, Vec::::new()); Ok(()) From 51a2da1a459384d532196fc8fd778572ec79599b Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Thu, 26 Mar 2026 18:06:29 +0100 Subject: [PATCH 20/22] [ty] we publish unused-bindings for notebooks even when syntax errors disabled --- crates/ty_server/tests/e2e/notebook.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/ty_server/tests/e2e/notebook.rs b/crates/ty_server/tests/e2e/notebook.rs index 5192341358834..0211ee070e972 100644 --- a/crates/ty_server/tests/e2e/notebook.rs +++ b/crates/ty_server/tests/e2e/notebook.rs @@ -484,12 +484,14 @@ fn invalid_syntax_with_syntax_errors_disabled() -> anyhow::Result<()> { let diagnostics = server.collect_publish_diagnostic_notifications(2); - assert_json_snapshot!(diagnostics, @r#" - { - "vscode-notebook-cell://src/test.ipynb#0": [], - "vscode-notebook-cell://src/test.ipynb#1": [] - } - "#); + // We still publish unused-bindings for both cells, even when syntax-error reporting is disabled. + assert_eq!(diagnostics.len(), 2); + assert!( + diagnostics + .values() + .flatten() + .all(|diagnostic| diagnostic.message != "unexpected EOF while parsing") + ); Ok(()) } From c7a1d99f537134931d2a179438dc4a2bfa445360 Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Fri, 27 Mar 2026 01:08:35 +0100 Subject: [PATCH 21/22] [ty] refactor for shared api --- .../ty_python_semantic/src/types/function.rs | 17 ++++-- .../src/types/ide_support/unused_bindings.rs | 57 ++++--------------- 2 files changed, 22 insertions(+), 52 deletions(-) diff --git a/crates/ty_python_semantic/src/types/function.rs b/crates/ty_python_semantic/src/types/function.rs index ed5a8b3c7beed..60095b4bce77c 100644 --- a/crates/ty_python_semantic/src/types/function.rs +++ b/crates/ty_python_semantic/src/types/function.rs @@ -1591,6 +1591,17 @@ fn last_definition_signature_cycle_initial<'db>( Signature::bottom() } +/// Returns `true` if the function body is stub-like, ignoring a leading docstring. +pub(crate) fn function_has_stub_body(node: &ast::StmtFunctionDef) -> bool { + let suite = ast::helpers::body_without_leading_docstring(&node.body); + + suite.iter().all(|stmt| match stmt { + ast::Stmt::Pass(_) => true, + ast::Stmt::Expr(ast::StmtExpr { value, .. }) => value.is_ellipsis_literal_expr(), + _ => false, + }) +} + /// Classify the body of this function: /// - [`FunctionBodyKind::Stub`] if it is a stub function (i.e., only contains `pass` or `...` /// - [`FunctionBodyKind::AlwaysRaisesNotImplementedError`] if it consists of a single @@ -1607,11 +1618,7 @@ pub(super) fn function_body_kind<'db>( // Allow docstrings, but only as the first statement. let suite = ast::helpers::body_without_leading_docstring(&node.body); - if suite.iter().all(|stmt| match stmt { - ast::Stmt::Pass(_) => true, - ast::Stmt::Expr(ast::StmtExpr { value, .. }) => value.is_ellipsis_literal_expr(), - _ => false, - }) { + if function_has_stub_body(node) { return FunctionBodyKind::Stub; } diff --git a/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs index a6fa784a3d452..3516e66c5e500 100644 --- a/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs +++ b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs @@ -3,9 +3,9 @@ use crate::semantic_index::definition::{DefinitionKind, DefinitionState}; use crate::semantic_index::place::ScopedPlaceId; use crate::semantic_index::scope::{FileScopeId, ScopeKind}; use crate::semantic_index::semantic_index; -use crate::types::{ClassBase, ClassType}; +use crate::types::ClassBase; use ruff_db::parsed::{ParsedModuleRef, parsed_module}; -use ruff_python_ast::{self as ast, name::Name}; +use ruff_python_ast::name::Name; use ruff_text_size::TextRange; /// Returns `true` for definition kinds that create user-facing bindings we consider for @@ -40,16 +40,6 @@ fn should_consider_definition(kind: &DefinitionKind<'_>) -> bool { } } -fn function_has_stub_body(function: &ast::StmtFunctionDef) -> bool { - let suite = ruff_python_ast::helpers::body_without_leading_docstring(&function.body); - - suite.iter().all(|stmt| match stmt { - ast::Stmt::Pass(_) => true, - ast::Stmt::Expr(ast::StmtExpr { value, .. }) => value.is_ellipsis_literal_expr(), - _ => false, - }) -} - fn function_scope_is_overload_declaration( db: &dyn Db, index: &crate::semantic_index::SemanticIndex<'_>, @@ -61,36 +51,8 @@ fn function_scope_is_overload_declaration( }; let definition = index.expect_single_definition(function); - let Some(function_type) = crate::types::binding_type(db, definition).as_function_literal() - else { - return false; - }; - - function_type - .iter_overloads_and_implementation(db) - .any(|overload| { - overload.body_scope(db).file_scope_id(db) == file_scope_id && overload.is_overload(db) - }) -} - -fn class_defines_member_named(db: &dyn Db, class: ClassType<'_>, member_name: &str) -> bool { - let Some((class_literal, specialization)) = class.static_class_literal(db) else { - // If we cannot inspect class members precisely, be conservative and avoid false positives. - return true; - }; - - let class_scope = class_literal.body_scope(db); - let class_place_table = crate::semantic_index::place_table(db, class_scope); - - class_place_table - .symbol_id(member_name) - .is_some_and(|symbol_id| { - let symbol = class_place_table.symbol(symbol_id); - symbol.is_bound() || symbol.is_declared() - }) - || class_literal - .own_synthesized_member(db, specialization, None, member_name) - .is_some() + crate::types::infer::function_known_decorator_flags(db, definition) + .contains(crate::types::FunctionDecorators::OVERLOAD) } // Returns true if a superclass in the method's MRO defines the same method name. @@ -122,7 +84,9 @@ fn method_name_exists_in_superclass( .any(|class_base| match class_base { ClassBase::Protocol | ClassBase::Generic | ClassBase::TypedDict => false, ClassBase::Dynamic(_) => true, - ClassBase::Class(superclass) => class_defines_member_named(db, superclass, method_name), + ClassBase::Class(superclass) => !superclass + .own_class_member(db, None, method_name) + .is_undefined(), }) } @@ -159,10 +123,9 @@ pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec Date: Fri, 27 Mar 2026 10:59:24 +0100 Subject: [PATCH 22/22] [ty] don't apply override supression --- .../src/types/ide_support/unused_bindings.rs | 66 ++++--------------- 1 file changed, 12 insertions(+), 54 deletions(-) diff --git a/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs index 3516e66c5e500..1fa3cfc9c15e1 100644 --- a/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs +++ b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs @@ -3,8 +3,7 @@ use crate::semantic_index::definition::{DefinitionKind, DefinitionState}; use crate::semantic_index::place::ScopedPlaceId; use crate::semantic_index::scope::{FileScopeId, ScopeKind}; use crate::semantic_index::semantic_index; -use crate::types::ClassBase; -use ruff_db::parsed::{ParsedModuleRef, parsed_module}; +use ruff_db::parsed::parsed_module; use ruff_python_ast::name::Name; use ruff_text_size::TextRange; @@ -55,41 +54,6 @@ fn function_scope_is_overload_declaration( .contains(crate::types::FunctionDecorators::OVERLOAD) } -// Returns true if a superclass in the method's MRO defines the same method name. -// Used to suppress unused-parameter diagnostics for likely overrides. -fn method_name_exists_in_superclass( - db: &dyn Db, - index: &crate::semantic_index::SemanticIndex<'_>, - parsed: &ParsedModuleRef, - file_scope_id: FileScopeId, -) -> bool { - let scope = index.scope(file_scope_id); - let Some(function) = scope.node().as_function() else { - return false; - }; - - let Some(class_definition) = index.class_definition_of_method(file_scope_id) else { - return false; - }; - - let method_name = function.node(parsed).name.as_str(); - let Some(class_type) = crate::types::binding_type(db, class_definition).to_class_type(db) - else { - return false; - }; - - class_type - .iter_mro(db) - .skip(1) - .any(|class_base| match class_base { - ClassBase::Protocol | ClassBase::Generic | ClassBase::TypedDict => false, - ClassBase::Dynamic(_) => true, - ClassBase::Class(superclass) => !superclass - .own_class_member(db, None, method_name) - .is_undefined(), - }) -} - #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub struct UnusedBinding { pub range: TextRange, @@ -99,9 +63,9 @@ pub struct UnusedBinding { /// Collects unused local bindings for IDE-facing diagnostics. /// /// This intentionally reports only function-, lambda-, and comprehension-scope bindings. -/// Even with local checks such as override detection, module- and class-scope bindings -/// can still be observed indirectly (for example via imports or attribute access), so -/// reporting them here would risk false positives without broader reference analysis. +/// Module- and class-scope bindings can still be observed indirectly (for example via +/// imports or attribute access), so reporting them here would risk false positives +/// without broader reference analysis. #[salsa::tracked(returns(ref))] pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec { let parsed = parsed_module(db, file).load(db); @@ -130,7 +94,6 @@ pub fn unused_bindings(db: &dyn Db, file: ruff_db::files::File) -> Vec Vec anyhow::Result<()> { + fn reports_unused_parameter_for_overriding_method() -> anyhow::Result<()> { let source = dedent( " class Test: @@ -361,17 +319,17 @@ mod tests { class Test2(Test): def a(self, bar): - ... + return 0 ", ); let names = collect_unused_names(&source)?; - assert_eq!(names, Vec::::new()); + assert_eq!(names, vec!["bar"]); Ok(()) } #[test] - fn skips_unused_parameter_for_indirect_override() -> anyhow::Result<()> { + fn reports_unused_parameter_for_indirect_override() -> anyhow::Result<()> { let source = dedent( " class A: @@ -383,12 +341,12 @@ mod tests { class C(B): def a(self, bar): - ... + return 0 ", ); let names = collect_unused_names(&source)?; - assert_eq!(names, Vec::::new()); + assert_eq!(names, vec!["bar"]); Ok(()) } @@ -427,7 +385,7 @@ mod tests { ); let names = collect_unused_names(&source)?; - assert_eq!(names, vec!["local_dead"]); + assert_eq!(names, vec!["bar", "local_dead"]); Ok(()) }