From d15436458f4a93e4d15d8ca46266c28180a15c66 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 27 Jul 2023 22:16:09 -0400 Subject: [PATCH] Only run unused private type rules over finalized bindings (#6142) ## Summary In #6134 and #6136, we see some false positives for "shadowed" class definitions. For example, here, the first definition is flagged as unused, since from the perspective of the semantic model (which doesn't understand branching), it appears to be immediately shadowed in the `else`, and thus never used: ```python if sys.version_info >= (3, 11): class _RootLoggerConfiguration(TypedDict, total=False): level: _Level filters: Sequence[str | _FilterType] handlers: Sequence[str] else: class _RootLoggerConfiguration(TypedDict, total=False): level: _Level filters: Sequence[str] handlers: Sequence[str] ``` Instead of looking at _all_ bindings, we should instead look at the "live" bindings, which is similar to how other rules (like unused variables detection) is structured. We thus move the rule from `bindings.rs` (which iterates over _all_ bindings, regardless of whether they're shadowed) to `deferred_scopes.rs`, which iterates over all "live" bindings once a scope has been fully analyzed. ## Test Plan `cargo test` --- .../ruff/src/checkers/ast/analyze/bindings.rs | 19 +-- .../checkers/ast/analyze/deferred_scopes.rs | 13 +- .../rules/unused_private_type_definition.rs | 131 ++++++++++-------- 3 files changed, 89 insertions(+), 74 deletions(-) diff --git a/crates/ruff/src/checkers/ast/analyze/bindings.rs b/crates/ruff/src/checkers/ast/analyze/bindings.rs index 1c8804272ebef..80c632308fbea 100644 --- a/crates/ruff/src/checkers/ast/analyze/bindings.rs +++ b/crates/ruff/src/checkers/ast/analyze/bindings.rs @@ -1,7 +1,8 @@ +use ruff_diagnostics::{Diagnostic, Fix}; + use crate::checkers::ast::Checker; use crate::codes::Rule; use crate::rules::{flake8_import_conventions, flake8_pyi, pyflakes, pylint}; -use ruff_diagnostics::{Diagnostic, Fix}; /// Run lint rules over the [`Binding`]s. pub(crate) fn bindings(checker: &mut Checker) { @@ -10,9 +11,7 @@ pub(crate) fn bindings(checker: &mut Checker) { Rule::InvalidAllObject, Rule::UnaliasedCollectionsAbcSetImport, Rule::UnconventionalImportAlias, - Rule::UnusedPrivateTypeVar, Rule::UnusedVariable, - Rule::UnusedPrivateProtocol, ]) { return; } @@ -65,20 +64,6 @@ pub(crate) fn bindings(checker: &mut Checker) { checker.diagnostics.push(diagnostic); } } - if checker.enabled(Rule::UnusedPrivateTypeVar) { - if let Some(diagnostic) = - flake8_pyi::rules::unused_private_type_var(checker, binding) - { - checker.diagnostics.push(diagnostic); - } - } - if checker.enabled(Rule::UnusedPrivateProtocol) { - if let Some(diagnostic) = - flake8_pyi::rules::unused_private_protocol(checker, binding) - { - checker.diagnostics.push(diagnostic); - } - } } } } diff --git a/crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs index 1d0f2a5ba0bb9..8fb6363c19bcd 100644 --- a/crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs @@ -5,7 +5,7 @@ use ruff_python_semantic::{Binding, BindingKind, ScopeKind}; use crate::checkers::ast::Checker; use crate::codes::Rule; -use crate::rules::{flake8_type_checking, flake8_unused_arguments, pyflakes, pylint}; +use crate::rules::{flake8_pyi, flake8_type_checking, flake8_unused_arguments, pyflakes, pylint}; /// Run lint rules over all deferred scopes in the [`SemanticModel`]. pub(crate) fn deferred_scopes(checker: &mut Checker) { @@ -24,6 +24,8 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { Rule::UnusedImport, Rule::UnusedLambdaArgument, Rule::UnusedMethodArgument, + Rule::UnusedPrivateProtocol, + Rule::UnusedPrivateTypeVar, Rule::UnusedStaticMethodArgument, Rule::UnusedVariable, ]) { @@ -214,6 +216,15 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { } } + if checker.is_stub { + if checker.enabled(Rule::UnusedPrivateTypeVar) { + flake8_pyi::rules::unused_private_type_var(checker, scope, &mut diagnostics); + } + if checker.enabled(Rule::UnusedPrivateProtocol) { + flake8_pyi::rules::unused_private_protocol(checker, scope, &mut diagnostics); + } + } + if matches!( scope.kind, ScopeKind::Function(_) | ScopeKind::AsyncFunction(_) | ScopeKind::Lambda(_) diff --git a/crates/ruff/src/rules/flake8_pyi/rules/unused_private_type_definition.rs b/crates/ruff/src/rules/flake8_pyi/rules/unused_private_type_definition.rs index db0f35a83e62a..2d5e0d3082b37 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/unused_private_type_definition.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/unused_private_type_definition.rs @@ -1,7 +1,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::{self as ast, Expr, Stmt}; -use ruff_python_semantic::Binding; +use ruff_python_semantic::Scope; use crate::checkers::ast::Checker; @@ -74,67 +74,86 @@ impl Violation for UnusedPrivateProtocol { } /// PYI018 -pub(crate) fn unused_private_type_var(checker: &Checker, binding: &Binding) -> Option { - if !(binding.kind.is_assignment() && binding.is_private_declaration()) { - return None; - } - if binding.is_used() { - return None; - } +pub(crate) fn unused_private_type_var( + checker: &Checker, + scope: &Scope, + diagnostics: &mut Vec, +) { + for binding in scope + .binding_ids() + .map(|binding_id| checker.semantic().binding(binding_id)) + { + if !(binding.kind.is_assignment() && binding.is_private_declaration()) { + continue; + } + if binding.is_used() { + continue; + } - let Some(source) = binding.source else { - return None; - }; - let Stmt::Assign(ast::StmtAssign { targets, value, .. }) = checker.semantic().stmts[source] - else { - return None; - }; - let [Expr::Name(ast::ExprName { id, .. })] = &targets[..] else { - return None; - }; - let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() else { - return None; - }; - if !checker.semantic().match_typing_expr(func, "TypeVar") { - return None; - } + let Some(source) = binding.source else { + continue; + }; + let Stmt::Assign(ast::StmtAssign { targets, value, .. }) = checker.semantic().stmts[source] + else { + continue; + }; + let [Expr::Name(ast::ExprName { id, .. })] = &targets[..] else { + continue; + }; + let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() else { + continue; + }; + if !checker.semantic().match_typing_expr(func, "TypeVar") { + continue; + } - Some(Diagnostic::new( - UnusedPrivateTypeVar { - name: id.to_string(), - }, - binding.range, - )) + diagnostics.push(Diagnostic::new( + UnusedPrivateTypeVar { + name: id.to_string(), + }, + binding.range, + )); + } } /// PYI046 -pub(crate) fn unused_private_protocol(checker: &Checker, binding: &Binding) -> Option { - if !(binding.kind.is_class_definition() && binding.is_private_declaration()) { - return None; - } - if binding.is_used() { - return None; - } +pub(crate) fn unused_private_protocol( + checker: &Checker, + scope: &Scope, + diagnostics: &mut Vec, +) { + for binding in scope + .binding_ids() + .map(|binding_id| checker.semantic().binding(binding_id)) + { + if !(binding.kind.is_class_definition() && binding.is_private_declaration()) { + continue; + } + if binding.is_used() { + continue; + } - let Some(source) = binding.source else { - return None; - }; - let Stmt::ClassDef(ast::StmtClassDef { name, bases, .. }) = checker.semantic().stmts[source] - else { - return None; - }; + let Some(source) = binding.source else { + continue; + }; + let Stmt::ClassDef(ast::StmtClassDef { name, bases, .. }) = + checker.semantic().stmts[source] + else { + continue; + }; - if !bases - .iter() - .any(|base| checker.semantic().match_typing_expr(base, "Protocol")) - { - return None; - } + if !bases + .iter() + .any(|base| checker.semantic().match_typing_expr(base, "Protocol")) + { + continue; + } - Some(Diagnostic::new( - UnusedPrivateProtocol { - name: name.to_string(), - }, - binding.range, - )) + diagnostics.push(Diagnostic::new( + UnusedPrivateProtocol { + name: name.to_string(), + }, + binding.range, + )); + } }