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`
This commit is contained in:
Charlie Marsh 2023-07-27 22:16:09 -04:00 committed by GitHub
parent 0bc3edf6c9
commit d15436458f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 90 additions and 75 deletions

View File

@ -1,7 +1,8 @@
use ruff_diagnostics::{Diagnostic, Fix};
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
use crate::codes::Rule; use crate::codes::Rule;
use crate::rules::{flake8_import_conventions, flake8_pyi, pyflakes, pylint}; use crate::rules::{flake8_import_conventions, flake8_pyi, pyflakes, pylint};
use ruff_diagnostics::{Diagnostic, Fix};
/// Run lint rules over the [`Binding`]s. /// Run lint rules over the [`Binding`]s.
pub(crate) fn bindings(checker: &mut Checker) { pub(crate) fn bindings(checker: &mut Checker) {
@ -10,9 +11,7 @@ pub(crate) fn bindings(checker: &mut Checker) {
Rule::InvalidAllObject, Rule::InvalidAllObject,
Rule::UnaliasedCollectionsAbcSetImport, Rule::UnaliasedCollectionsAbcSetImport,
Rule::UnconventionalImportAlias, Rule::UnconventionalImportAlias,
Rule::UnusedPrivateTypeVar,
Rule::UnusedVariable, Rule::UnusedVariable,
Rule::UnusedPrivateProtocol,
]) { ]) {
return; return;
} }
@ -65,20 +64,6 @@ pub(crate) fn bindings(checker: &mut Checker) {
checker.diagnostics.push(diagnostic); 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);
}
}
} }
} }
} }

View File

@ -5,7 +5,7 @@ use ruff_python_semantic::{Binding, BindingKind, ScopeKind};
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
use crate::codes::Rule; 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`]. /// Run lint rules over all deferred scopes in the [`SemanticModel`].
pub(crate) fn deferred_scopes(checker: &mut Checker) { pub(crate) fn deferred_scopes(checker: &mut Checker) {
@ -24,6 +24,8 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
Rule::UnusedImport, Rule::UnusedImport,
Rule::UnusedLambdaArgument, Rule::UnusedLambdaArgument,
Rule::UnusedMethodArgument, Rule::UnusedMethodArgument,
Rule::UnusedPrivateProtocol,
Rule::UnusedPrivateTypeVar,
Rule::UnusedStaticMethodArgument, Rule::UnusedStaticMethodArgument,
Rule::UnusedVariable, 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!( if matches!(
scope.kind, scope.kind,
ScopeKind::Function(_) | ScopeKind::AsyncFunction(_) | ScopeKind::Lambda(_) ScopeKind::Function(_) | ScopeKind::AsyncFunction(_) | ScopeKind::Lambda(_)

View File

@ -1,7 +1,7 @@
use ruff_diagnostics::{Diagnostic, Violation}; use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr, Stmt}; use ruff_python_ast::{self as ast, Expr, Stmt};
use ruff_python_semantic::Binding; use ruff_python_semantic::Scope;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
@ -74,67 +74,86 @@ impl Violation for UnusedPrivateProtocol {
} }
/// PYI018 /// PYI018
pub(crate) fn unused_private_type_var(checker: &Checker, binding: &Binding) -> Option<Diagnostic> { pub(crate) fn unused_private_type_var(
if !(binding.kind.is_assignment() && binding.is_private_declaration()) { checker: &Checker,
return None; scope: &Scope,
} diagnostics: &mut Vec<Diagnostic>,
if binding.is_used() { ) {
return None; 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 { let Some(source) = binding.source else {
return None; continue;
}; };
let Stmt::Assign(ast::StmtAssign { targets, value, .. }) = checker.semantic().stmts[source] let Stmt::Assign(ast::StmtAssign { targets, value, .. }) = checker.semantic().stmts[source]
else { else {
return None; continue;
}; };
let [Expr::Name(ast::ExprName { id, .. })] = &targets[..] else { let [Expr::Name(ast::ExprName { id, .. })] = &targets[..] else {
return None; continue;
}; };
let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() else { let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() else {
return None; continue;
}; };
if !checker.semantic().match_typing_expr(func, "TypeVar") { if !checker.semantic().match_typing_expr(func, "TypeVar") {
return None; continue;
} }
Some(Diagnostic::new( diagnostics.push(Diagnostic::new(
UnusedPrivateTypeVar { UnusedPrivateTypeVar {
name: id.to_string(), name: id.to_string(),
}, },
binding.range, binding.range,
)) ));
}
} }
/// PYI046 /// PYI046
pub(crate) fn unused_private_protocol(checker: &Checker, binding: &Binding) -> Option<Diagnostic> { pub(crate) fn unused_private_protocol(
if !(binding.kind.is_class_definition() && binding.is_private_declaration()) { checker: &Checker,
return None; scope: &Scope,
} diagnostics: &mut Vec<Diagnostic>,
if binding.is_used() { ) {
return None; for binding in scope
} .binding_ids()
.map(|binding_id| checker.semantic().binding(binding_id))
let Some(source) = binding.source else {
return None;
};
let Stmt::ClassDef(ast::StmtClassDef { name, bases, .. }) = checker.semantic().stmts[source]
else {
return None;
};
if !bases
.iter()
.any(|base| checker.semantic().match_typing_expr(base, "Protocol"))
{ {
return None; if !(binding.kind.is_class_definition() && binding.is_private_declaration()) {
} continue;
}
if binding.is_used() {
continue;
}
Some(Diagnostic::new( let Some(source) = binding.source else {
UnusedPrivateProtocol { continue;
name: name.to_string(), };
}, let Stmt::ClassDef(ast::StmtClassDef { name, bases, .. }) =
binding.range, checker.semantic().stmts[source]
)) else {
continue;
};
if !bases
.iter()
.any(|base| checker.semantic().match_typing_expr(base, "Protocol"))
{
continue;
}
diagnostics.push(Diagnostic::new(
UnusedPrivateProtocol {
name: name.to_string(),
},
binding.range,
));
}
} }