diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 37d8c5ac48b5ef..c1a89d7adbb552 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 c91afbed4b1ef6..020a92fcc36483 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,6 +573,74 @@ 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 { + self.visible_ancestor_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); + + // 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) + }) + } + + /// 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(); @@ -1650,6 +1734,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { // Pop the root scope self.pop_scope(); + self.mark_captured_bindings_used(); self.sweep_nonlocal_lazy_snapshots(); assert!(self.scope_stack.is_empty()); diff --git a/crates/ty_python_semantic/src/semantic_index/definition.rs b/crates/ty_python_semantic/src/semantic_index/definition.rs index 26c570e6458c86..83bbec5c1992c6 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_def(&self) -> bool { + matches!( + self, + DefinitionKind::VariadicPositionalParameter(_) + | DefinitionKind::VariadicKeywordParameter(_) + | DefinitionKind::Parameter(_) + ) + } + pub(crate) const fn is_loop_header(&self) -> bool { matches!(self, DefinitionKind::LoopHeader(_)) } @@ -908,7 +917,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) => { 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 7e0f60c38396aa..f39a860d860767 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(&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(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(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(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(DefinitionState::Deleted); let place_state = match place { ScopedPlaceId::Symbol(symbol) => &mut self.symbol_states[symbol], ScopedPlaceId::Member(member) => &mut self.member_states[member], @@ -1380,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`. @@ -1393,12 +1419,28 @@ 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); } + 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; + }; + + 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) { // If the last entry has the same reachability constraint, extend it // to cover this range too, collapsing consecutive statements in the @@ -1459,6 +1501,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 { @@ -1565,6 +1629,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 +1722,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/function.rs b/crates/ty_python_semantic/src/types/function.rs index 80ff811c8a3d6c..60095b4bce77c6 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 @@ -1605,19 +1616,9 @@ 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, - 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.rs b/crates/ty_python_semantic/src/types/ide_support.rs index 5eb5e6249789fb..a315034544b0bd 100644 --- a/crates/ty_python_semantic/src/types/ide_support.rs +++ b/crates/ty_python_semantic/src/types/ide_support.rs @@ -2,8 +2,7 @@ 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}; 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,13 +17,16 @@ 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; +#[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. 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 00000000000000..1fa3cfc9c15e14 --- /dev/null +++ b/crates/ty_python_semantic/src/types/ide_support/unused_bindings.rs @@ -0,0 +1,649 @@ +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 ruff_db::parsed::parsed_module; +use ruff_python_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_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); + crate::types::infer::function_known_decorator_flags(db, definition) + .contains(crate::types::FunctionDecorators::OVERLOAD) +} + +#[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-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); + 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| { + crate::types::function::function_has_stub_body(function.node(&parsed)) + }); + let function_is_overload_declaration = + function_scope_is_overload_declaration(db, index, 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 || function_is_overload_declaration || 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 reports_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): + return 0 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["bar"]); + Ok(()) + } + + #[test] + fn reports_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): + return 0 + ", + ); + + let names = collect_unused_names(&source)?; + assert_eq!(names, vec!["bar"]); + 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!["bar", "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_parameter_for_module_level_overload_stub_declarations() -> 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( + " + 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 reports_unused_binding_on_syntax_error() -> anyhow::Result<()> { + let source = dedent( + " + def f( + x = 1 + ", + ); + + 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(()) + } +} diff --git a/crates/ty_server/src/server/api/diagnostics.rs b/crates/ty_server/src/server/api/diagnostics.rs index c687b65fd8e2fb..f942b91ae85028 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::ide_support::{UnusedBinding, unused_bindings}; use ruff_db::diagnostic::{Annotation, Severity, SubDiagnostic}; use ruff_db::files::{File, FileRange}; @@ -27,6 +28,7 @@ use crate::{PositionEncoding, Session}; #[derive(Debug)] pub(super) struct Diagnostics { items: Vec, + unused_bindings: Vec, encoding: PositionEncoding, file_or_notebook: File, } @@ -37,16 +39,17 @@ 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; } - // 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 diagnostics.hash(&mut hasher); + unused_bindings.hash(&mut hasher); Some(format!("{:016x}", hasher.finish())) } @@ -55,7 +58,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( @@ -95,25 +98,52 @@ 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 { - 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) } } } @@ -326,14 +356,61 @@ pub(super) fn compute_diagnostics( }; 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 collect_unused_bindings(db: &ProjectDatabase, file: File) -> Vec { + if !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, + encoding: PositionEncoding, + unused_bindings: &[UnusedBinding], +) -> Vec { + unused_bindings + .iter() + .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: None, + code_description: None, + source: Some(DIAGNOSTIC_NAME.into()), + message: format!("`{}` is unused", binding.name), + related_information: None, + tags: Some(vec![DiagnosticTag::UNNECESSARY]), + data: None, + }, + )) +} + /// Converts the tool specific [`Diagnostic`][ruff_db::diagnostic::Diagnostic] to an LSP /// [`Diagnostic`]. pub(super) fn to_lsp_diagnostic( 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 54d323a3bd9a28..78271b335b2cd0 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}; +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,6 +238,8 @@ impl ProgressReporter for WorkspaceDiagnosticsProgressReporter<'_> { } fn report_checked_file(&self, db: &ProjectDatabase, file: File, diagnostics: &[Diagnostic]) { + 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 // to unwind with a salsa cancellation next. @@ -255,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() { + if !diagnostics.is_empty() || !unused_bindings.is_empty() { state .response - .write_diagnostics_for_file(db, file, diagnostics); + .write_diagnostics_for_file(db, file, diagnostics, &unused_bindings); } 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], + 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)); @@ -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, unused_bindings); let previous_result_id = self.previous_result_ids.remove(&key).map(|(_url, id)| id); @@ -409,7 +415,7 @@ impl<'a> ResponseWriter<'a> { ) } new_id => { - let lsp_diagnostics = diagnostics + let mut lsp_diagnostics = diagnostics .iter() .filter_map(|diagnostic| { Some( @@ -424,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, @@ -477,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_server/tests/e2e/notebook.rs b/crates/ty_server/tests/e2e/notebook.rs index 2247be813d88e8..0211ee070e972f 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()? @@ -460,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(()) } diff --git a/crates/ty_server/tests/e2e/pull_diagnostics.rs b/crates/ty_server/tests/e2e/pull_diagnostics.rs index dc749a046d6849..54c34dbaaa9de6 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__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 00000000000000..f82afce67e97bc --- /dev/null +++ b/crates/ty_server/tests/e2e/snapshots/e2e__notebook__publish_unused_binding_diagnostics_open.snap @@ -0,0 +1,26 @@ +--- +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, + "source": "ty", + "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 new file mode 100644 index 00000000000000..33e093b0a6a8ba --- /dev/null +++ b/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__unused_binding_has_unnecessary_hint_tag.snap @@ -0,0 +1,28 @@ +--- +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, + "source": "ty", + "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 new file mode 100644 index 00000000000000..ab01655f55c8bf --- /dev/null +++ b/crates/ty_server/tests/e2e/snapshots/e2e__pull_diagnostics__workspace_reports_unused_binding_hint_tag.snap @@ -0,0 +1,34 @@ +--- +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, + "source": "ty", + "message": "`x` is unused", + "tags": [ + 1 + ] + } + ] + } + ] +}