Skip to content

Commit

Permalink
[flake8-pyi] Implement PYI018 (#6018)
Browse files Browse the repository at this point in the history
## Summary

Check for unused private `TypeVar`. See [original
implementation](https://github.com/PyCQA/flake8-pyi/blob/2a86db8271dc500247a8a69419536240334731cf/pyi.py#L1958).

```
$ flake8 --select Y018 crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi

crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi:4:1: Y018 TypeVar "_T" is not used
crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi:5:1: Y018 TypeVar "_P" is not used
```

```
$ ./target/debug/ruff --select PYI018 crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi --no-cache

crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi:4:1: PYI018 TypeVar `_T` is never used
crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi:5:1: PYI018 TypeVar `_P` is never used
Found 2 errors.
```
In the file `unused_private_type_declaration.rs`, I'm planning to add
other rules that are similar to `PYI018` like the `PYI046`, `PYI047` and
`PYI049`.

ref #848

## Test Plan

Snapshots and manual runs of flake8.
  • Loading branch information
LaBatata101 authored Jul 26, 2023
1 parent 788643f commit c0dbcb3
Show file tree
Hide file tree
Showing 12 changed files with 149 additions and 0 deletions.
12 changes: 12 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import typing
from typing import TypeVar

_T = typing.TypeVar("_T")
_P = TypeVar("_P")

# OK
_UsedTypeVar = TypeVar("_UsedTypeVar")
def func(arg: _UsedTypeVar) -> _UsedTypeVar: ...

_A, _B = TypeVar("_A"), TypeVar("_B")
_C = _D = TypeVar("_C")
12 changes: 12 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import typing
from typing import TypeVar

_T = typing.TypeVar("_T")
_P = TypeVar("_P")

# OK
_UsedTypeVar = TypeVar("_UsedTypeVar")
def func(arg: _UsedTypeVar) -> _UsedTypeVar: ...

_A, _B = TypeVar("_A"), TypeVar("_B")
_C = _D = TypeVar("_C")
8 changes: 8 additions & 0 deletions crates/ruff/src/checkers/ast/analyze/bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pub(crate) fn bindings(checker: &mut Checker) {
Rule::InvalidAllObject,
Rule::UnaliasedCollectionsAbcSetImport,
Rule::UnconventionalImportAlias,
Rule::UnusedPrivateTypeVar,
Rule::UnusedVariable,
]) {
return;
Expand Down Expand Up @@ -63,6 +64,13 @@ 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);
}
}
}
}
}
5 changes: 5 additions & 0 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1474,6 +1474,11 @@ impl<'a> Checker<'a> {
// Create the `Binding`.
let binding_id = self.semantic.push_binding(range, kind, flags);

// If the name is private, mark is as such.
if name.starts_with('_') {
self.semantic.bindings[binding_id].flags |= BindingFlags::PRIVATE_DECLARATION;
}

// If there's an existing binding in this scope, copy its references.
if let Some(shadowed_id) = self.semantic.scopes[scope_id].get(name) {
// If this is an annotation, and we already have an existing value in the same scope,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Pyi, "015") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::AssignmentDefaultInStub),
(Flake8Pyi, "016") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::DuplicateUnionMember),
(Flake8Pyi, "017") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::ComplexAssignmentInStub),
(Flake8Pyi, "018") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::UnusedPrivateTypeVar),
(Flake8Pyi, "020") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::QuotedAnnotationInStub),
(Flake8Pyi, "021") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::DocstringInStub),
(Flake8Pyi, "024") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::CollectionsNamedTuple),
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/flake8_pyi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ mod tests {
#[test_case(Rule::TypeAliasWithoutAnnotation, Path::new("PYI026.pyi"))]
#[test_case(Rule::UnsupportedMethodCallOnAll, Path::new("PYI056.py"))]
#[test_case(Rule::UnsupportedMethodCallOnAll, Path::new("PYI056.pyi"))]
#[test_case(Rule::UnusedPrivateTypeVar, Path::new("PYI018.py"))]
#[test_case(Rule::UnusedPrivateTypeVar, Path::new("PYI018.pyi"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/flake8_pyi/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub(crate) use unnecessary_literal_union::*;
pub(crate) use unrecognized_platform::*;
pub(crate) use unrecognized_version_info::*;
pub(crate) use unsupported_method_call_on_all::*;
pub(crate) use unused_private_type_definition::*;

mod any_eq_ne_annotation;
mod bad_version_info_comparison;
Expand Down Expand Up @@ -61,3 +62,4 @@ mod unnecessary_literal_union;
mod unrecognized_platform;
mod unrecognized_version_info;
mod unsupported_method_call_on_all;
mod unused_private_type_definition;
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_semantic::Binding;
use rustpython_parser::ast::{self, Expr, Stmt};

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for the presence of unused private `TypeVar` declarations.
///
/// ## Why is this bad?
/// A private `TypeVar` that is defined but not used is likely a mistake, and
/// should be removed to avoid confusion.
///
/// ## Example
/// ```python
/// import typing
///
/// _T = typing.TypeVar("_T")
/// ```
#[violation]
pub struct UnusedPrivateTypeVar {
name: String,
}

impl Violation for UnusedPrivateTypeVar {
#[derive_message_formats]
fn message(&self) -> String {
let UnusedPrivateTypeVar { name } = self;
format!("Private TypeVar `{name}` is never used")
}
}

/// PYI018
pub(crate) fn unused_private_type_var(checker: &Checker, binding: &Binding) -> Option<Diagnostic> {
if !(binding.kind.is_assignment() && binding.is_private_variable()) {
return None;
}
if binding.is_used() {
return None;
}

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;
}

Some(Diagnostic::new(
UnusedPrivateTypeVar {
name: id.to_string(),
},
binding.range,
))
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
source: crates/ruff/src/rules/flake8_pyi/mod.rs
---

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
source: crates/ruff/src/rules/flake8_pyi/mod.rs
---
PYI018.pyi:4:1: PYI018 Private TypeVar `_T` is never used
|
2 | from typing import TypeVar
3 |
4 | _T = typing.TypeVar("_T")
| ^^ PYI018
5 | _P = TypeVar("_P")
|

PYI018.pyi:5:1: PYI018 Private TypeVar `_P` is never used
|
4 | _T = typing.TypeVar("_T")
5 | _P = TypeVar("_P")
| ^^ PYI018
6 |
7 | # OK
|


14 changes: 14 additions & 0 deletions crates/ruff_python_semantic/src/binding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,12 @@ impl<'a> Binding<'a> {
)
}

/// Return `true` if this [`Binding`] represents an private variable
/// (e.g., `_x` in `_x = "private variable"`)
pub const fn is_private_variable(&self) -> bool {
self.flags.contains(BindingFlags::PRIVATE_DECLARATION)
}

/// Return `true` if this binding redefines the given binding.
pub fn redefines(&self, existing: &'a Binding) -> bool {
match &self.kind {
Expand Down Expand Up @@ -264,6 +270,14 @@ bitflags! {
/// __all__ = [1]
/// ```
const INVALID_ALL_OBJECT = 1 << 6;

/// The binding represents a private declaration.
///
/// For example, the binding could be `_T` in:
/// ```python
/// _T = "This is a private variable"
/// ```
const PRIVATE_DECLARATION = 1 << 7;
}
}

Expand Down
1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit c0dbcb3

Please sign in to comment.