Skip to content

Commit

Permalink
Only run unused private type rules over finalized bindings (#6142)
Browse files Browse the repository at this point in the history
## 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`
  • Loading branch information
charliermarsh committed Jul 28, 2023
1 parent 0bc3edf commit d154364
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 74 deletions.
19 changes: 2 additions & 17 deletions crates/ruff/src/checkers/ast/analyze/bindings.rs
Original file line number Diff line number Diff line change
@@ -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) {
Expand All @@ -10,9 +11,7 @@ pub(crate) fn bindings(checker: &mut Checker) {
Rule::InvalidAllObject,
Rule::UnaliasedCollectionsAbcSetImport,
Rule::UnconventionalImportAlias,
Rule::UnusedPrivateTypeVar,
Rule::UnusedVariable,
Rule::UnusedPrivateProtocol,
]) {
return;
}
Expand Down Expand Up @@ -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);
}
}
}
}
}
13 changes: 12 additions & 1 deletion crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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,
]) {
Expand Down Expand Up @@ -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(_)
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -74,67 +74,86 @@ impl Violation for UnusedPrivateProtocol {
}

/// PYI018
pub(crate) fn unused_private_type_var(checker: &Checker, binding: &Binding) -> Option<Diagnostic> {
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<Diagnostic>,
) {
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<Diagnostic> {
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<Diagnostic>,
) {
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,
));
}
}

0 comments on commit d154364

Please sign in to comment.