From ee8ef4bc82e751eb5ab42a8ac50cd8a97546d7bb Mon Sep 17 00:00:00 2001 From: LaBatata101 Date: Sun, 23 Jul 2023 18:06:01 -0300 Subject: [PATCH 1/6] [WIP] Implement PYI018 --- .../resources/test/fixtures/flake8_pyi/PYI018.py | 0 .../resources/test/fixtures/flake8_pyi/PYI018.pyi | 9 +++++++++ crates/ruff/src/checkers/ast/mod.rs | 9 +++++++++ crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/flake8_pyi/mod.rs | 2 ++ crates/ruff/src/rules/flake8_pyi/rules/mod.rs | 2 ++ .../rules/unused_private_type_definition.rs | 15 +++++++++++++++ 7 files changed, 38 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi create mode 100644 crates/ruff/src/rules/flake8_pyi/rules/unused_private_type_definition.rs diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.py b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.py new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi new file mode 100644 index 0000000000000..b4cc4768ae79a --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi @@ -0,0 +1,9 @@ +import typing +from typing import TypeVar + +_T = typing.TypeVar("_T") +_P = TypeVar("_P") + +# OK +_UsedTypeVar = TypeVar("_UsedTypeVar") +def func(arg: _UsedTypeVar) -> _UsedTypeVar: ... diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 9dd96ce866426..f6e6ae2e8419d 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1724,6 +1724,15 @@ where }) => { self.handle_node_load(target); } + Stmt::Assign(ast::StmtAssign {targets, range: _, value, type_comment: _ }) => { + if let [Expr::Name(ast::ExprName {id, ..})] = &targets[..] { + if id.starts_with('_') { + if self.semantic.match_typing_expr(value, "TypeVar") { + // never gets executed + } + } + } + } Stmt::Import(ast::StmtImport { names, range: _ }) => { for alias in names { if alias.name.contains('.') && alias.asname.is_none() { diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 745489e464413..c7668d309e67e 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -630,6 +630,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), diff --git a/crates/ruff/src/rules/flake8_pyi/mod.rs b/crates/ruff/src/rules/flake8_pyi/mod.rs index c43a023ae0591..b39ae31186a3d 100644 --- a/crates/ruff/src/rules/flake8_pyi/mod.rs +++ b/crates/ruff/src/rules/flake8_pyi/mod.rs @@ -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( diff --git a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs index 4b8d229df9ecd..6ee3f89e8a74e 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs @@ -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; @@ -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; 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 new file mode 100644 index 0000000000000..a1a8e83abdb1b --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/rules/unused_private_type_definition.rs @@ -0,0 +1,15 @@ +use ruff_diagnostics::Violation; +use ruff_macros::{derive_message_formats, violation}; + +#[violation] +pub struct UnusedPrivateTypeVar; + +impl Violation for UnusedPrivateTypeVar { + #[derive_message_formats] + fn message(&self) -> String { + format!("TODO") + } +} + +/// PYI018 +pub(crate) fn unused_private_type_var() {} From 3178529d0f7c23f6e99402d85ef4f5c1c7166b20 Mon Sep 17 00:00:00 2001 From: LaBatata101 Date: Sun, 23 Jul 2023 19:58:40 -0300 Subject: [PATCH 2/6] update --- crates/ruff/src/checkers/ast/mod.rs | 16 +++-- .../rules/unused_private_type_definition.rs | 31 +++++++-- crates/ruff_python_semantic/src/binding.rs | 63 +++++++++---------- 3 files changed, 69 insertions(+), 41 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index f6e6ae2e8419d..de597d0055b43 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1724,11 +1724,13 @@ where }) => { self.handle_node_load(target); } - Stmt::Assign(ast::StmtAssign {targets, range: _, value, type_comment: _ }) => { - if let [Expr::Name(ast::ExprName {id, ..})] = &targets[..] { + Stmt::Assign(ast::StmtAssign {targets, value, range: _, type_comment: _ }) => { + if let [Expr::Name(ast::ExprName {id, range, ..})] = &targets[..] { if id.starts_with('_') { - if self.semantic.match_typing_expr(value, "TypeVar") { - // never gets executed + if let Expr::Call(ast::ExprCall {func, ..}) = value.as_ref() { + if self.semantic.match_typing_expr(func, "TypeVar") { + self.add_binding(id, *range, BindingKind::Assignment, BindingFlags::PRIVATE_TYPE_VAR); + } } } } @@ -4756,6 +4758,7 @@ impl<'a> Checker<'a> { Rule::UnaliasedCollectionsAbcSetImport, Rule::UnconventionalImportAlias, Rule::UnusedVariable, + Rule::UnusedPrivateTypeVar ]) { return; } @@ -4810,6 +4813,11 @@ impl<'a> Checker<'a> { self.diagnostics.push(diagnostic); } } + if self.enabled(Rule::UnusedPrivateTypeVar) { + if let Some(diagnostic) = flake8_pyi::rules::unused_private_type_var(binding, self.locator) { + self.diagnostics.push(diagnostic); + } + } } } } 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 a1a8e83abdb1b..4bccece1e8bf6 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,15 +1,38 @@ -use ruff_diagnostics::Violation; +use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::source_code::Locator; +use ruff_python_semantic::Binding; #[violation] -pub struct UnusedPrivateTypeVar; +pub struct UnusedPrivateTypeVar { + name: String, +} impl Violation for UnusedPrivateTypeVar { #[derive_message_formats] fn message(&self) -> String { - format!("TODO") + let UnusedPrivateTypeVar { name } = self; + format!("TypeVar `{name}` is never used") } } /// PYI018 -pub(crate) fn unused_private_type_var() {} +pub(crate) fn unused_private_type_var(binding: &Binding, locator: &Locator) -> Option { + if !binding.kind.is_assignment() { + return None; + } + if !binding.is_private_type_var() { + return None; + } + if dbg!(binding.is_used()) { + return None; + } + + dbg!(binding.name(locator), binding.range, &binding.references); + Some(Diagnostic::new( + UnusedPrivateTypeVar { + name: binding.name(locator).to_string(), + }, + binding.range, + )) +} diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 1f47db394f2ad..1e82dabc3fd82 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -94,6 +94,10 @@ impl<'a> Binding<'a> { ) } + pub const fn is_private_type_var(&self) -> bool { + self.flags.contains(BindingFlags::PRIVATE_TYPE_VAR) + } + /// Return `true` if this binding redefines the given binding. pub fn redefines(&self, existing: &'a Binding) -> bool { match &self.kind { @@ -113,28 +117,23 @@ impl<'a> Binding<'a> { return qualified_name == existing; } } - BindingKind::SubmoduleImport(SubmoduleImport { qualified_name }) => { - match &existing.kind { - BindingKind::Import(Import { - qualified_name: existing, - }) - | BindingKind::SubmoduleImport(SubmoduleImport { - qualified_name: existing, - }) => { - return qualified_name == existing; - } - BindingKind::FromImport(FromImport { - qualified_name: existing, - }) => { - return qualified_name == existing; - } - _ => {} + BindingKind::SubmoduleImport(SubmoduleImport { qualified_name }) => match &existing.kind { + BindingKind::Import(Import { + qualified_name: existing, + }) + | BindingKind::SubmoduleImport(SubmoduleImport { + qualified_name: existing, + }) => { + return qualified_name == existing; } - } - BindingKind::Deletion - | BindingKind::Annotation - | BindingKind::FutureImport - | BindingKind::Builtin => { + BindingKind::FromImport(FromImport { + qualified_name: existing, + }) => { + return qualified_name == existing; + } + _ => {} + }, + BindingKind::Deletion | BindingKind::Annotation | BindingKind::FutureImport | BindingKind::Builtin => { return false; } _ => {} @@ -154,9 +153,7 @@ impl<'a> Binding<'a> { match &self.kind { BindingKind::Import(Import { qualified_name }) => Some(qualified_name), BindingKind::FromImport(FromImport { qualified_name }) => Some(qualified_name), - BindingKind::SubmoduleImport(SubmoduleImport { qualified_name }) => { - Some(qualified_name) - } + BindingKind::SubmoduleImport(SubmoduleImport { qualified_name }) => Some(qualified_name), _ => None, } } @@ -185,15 +182,13 @@ impl<'a> Binding<'a> { /// Returns the range of the binding's parent. pub fn parent_range(&self, semantic: &SemanticModel) -> Option { - self.source - .map(|node_id| semantic.stmts[node_id]) - .and_then(|parent| { - if parent.is_import_from_stmt() { - Some(parent.range()) - } else { - None - } - }) + self.source.map(|node_id| semantic.stmts[node_id]).and_then(|parent| { + if parent.is_import_from_stmt() { + Some(parent.range()) + } else { + None + } + }) } } @@ -264,6 +259,8 @@ bitflags! { /// __all__ = [1] /// ``` const INVALID_ALL_OBJECT = 1 << 6; + + const PRIVATE_TYPE_VAR = 1 << 7; } } From a8377770b21706ad82bee7f1230ef5e9d2ddaa12 Mon Sep 17 00:00:00 2001 From: LaBatata101 Date: Mon, 24 Jul 2023 19:45:33 -0300 Subject: [PATCH 3/6] Implement PYI018 ## 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") ``` Use instead: ```python import typing _T = typing.TypeVar("_T") def func(arg: _T) -> _T: ... ``` --- .../test/fixtures/flake8_pyi/PYI018.py | 12 ++++ .../test/fixtures/flake8_pyi/PYI018.pyi | 3 + crates/ruff/src/checkers/ast/mod.rs | 35 ++++++---- .../rules/unused_private_type_definition.rs | 23 ++++++- ...__flake8_pyi__tests__PYI018_PYI018.py.snap | 4 ++ ..._flake8_pyi__tests__PYI018_PYI018.pyi.snap | 22 +++++++ crates/ruff_python_semantic/src/binding.rs | 65 ++++++++++++------- ruff.schema.json | 1 + 8 files changed, 126 insertions(+), 39 deletions(-) create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI018_PYI018.py.snap create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI018_PYI018.pyi.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.py b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.py index e69de29bb2d1d..75dbf08dd698b 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.py +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.py @@ -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") diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi index b4cc4768ae79a..75dbf08dd698b 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI018.pyi @@ -7,3 +7,6 @@ _P = TypeVar("_P") # OK _UsedTypeVar = TypeVar("_UsedTypeVar") def func(arg: _UsedTypeVar) -> _UsedTypeVar: ... + +_A, _B = TypeVar("_A"), TypeVar("_B") +_C = _D = TypeVar("_C") diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index de597d0055b43..8fd43c74f231f 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1724,17 +1724,6 @@ where }) => { self.handle_node_load(target); } - Stmt::Assign(ast::StmtAssign {targets, value, range: _, type_comment: _ }) => { - if let [Expr::Name(ast::ExprName {id, range, ..})] = &targets[..] { - if id.starts_with('_') { - if let Expr::Call(ast::ExprCall {func, ..}) = value.as_ref() { - if self.semantic.match_typing_expr(func, "TypeVar") { - self.add_binding(id, *range, BindingKind::Assignment, BindingFlags::PRIVATE_TYPE_VAR); - } - } - } - } - } Stmt::Import(ast::StmtImport { names, range: _ }) => { for alias in names { if alias.name.contains('.') && alias.asname.is_none() { @@ -4494,6 +4483,24 @@ impl<'a> Checker<'a> { return; } + if let Stmt::Assign(ast::StmtAssign { targets, value, .. }) = parent { + if let [Expr::Name(ast::ExprName { id, range, .. })] = &targets[..] { + if id.starts_with('_') { + if let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() { + if self.semantic.match_typing_expr(func, "TypeVar") { + self.add_binding( + id, + *range, + BindingKind::Assignment, + BindingFlags::PRIVATE_TYPE_VAR, + ); + return; + } + } + } + } + } + let scope = self.semantic.scope(); if scope.kind.is_module() @@ -4758,7 +4765,7 @@ impl<'a> Checker<'a> { Rule::UnaliasedCollectionsAbcSetImport, Rule::UnconventionalImportAlias, Rule::UnusedVariable, - Rule::UnusedPrivateTypeVar + Rule::UnusedPrivateTypeVar, ]) { return; } @@ -4814,7 +4821,9 @@ impl<'a> Checker<'a> { } } if self.enabled(Rule::UnusedPrivateTypeVar) { - if let Some(diagnostic) = flake8_pyi::rules::unused_private_type_var(binding, self.locator) { + if let Some(diagnostic) = + flake8_pyi::rules::unused_private_type_var(binding, self.locator) + { self.diagnostics.push(diagnostic); } } 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 4bccece1e8bf6..ca1d6f8a37577 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 @@ -3,6 +3,26 @@ use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::source_code::Locator; use ruff_python_semantic::Binding; +/// ## 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") +/// ``` +/// +/// Use instead: +/// ```python +/// import typing +/// _T = typing.TypeVar("_T") +/// +/// def func(arg: _T) -> _T: ... +/// ``` #[violation] pub struct UnusedPrivateTypeVar { name: String, @@ -24,11 +44,10 @@ pub(crate) fn unused_private_type_var(binding: &Binding, locator: &Locator) -> O if !binding.is_private_type_var() { return None; } - if dbg!(binding.is_used()) { + if binding.is_used() { return None; } - dbg!(binding.name(locator), binding.range, &binding.references); Some(Diagnostic::new( UnusedPrivateTypeVar { name: binding.name(locator).to_string(), diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI018_PYI018.py.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI018_PYI018.py.snap new file mode 100644 index 0000000000000..d1aa2e9116558 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI018_PYI018.py.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff/src/rules/flake8_pyi/mod.rs +--- + diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI018_PYI018.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI018_PYI018.pyi.snap new file mode 100644 index 0000000000000..2eedd594357a6 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI018_PYI018.pyi.snap @@ -0,0 +1,22 @@ +--- +source: crates/ruff/src/rules/flake8_pyi/mod.rs +--- +PYI018.pyi:4:1: PYI018 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 TypeVar `_P` is never used + | +4 | _T = typing.TypeVar("_T") +5 | _P = TypeVar("_P") + | ^^ PYI018 +6 | +7 | # OK + | + + diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 1e82dabc3fd82..9a615bf6bd6a9 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -94,6 +94,7 @@ impl<'a> Binding<'a> { ) } + /// Return `true` if this [`Binding`] is a private `typing.TypeVar`. pub const fn is_private_type_var(&self) -> bool { self.flags.contains(BindingFlags::PRIVATE_TYPE_VAR) } @@ -117,23 +118,28 @@ impl<'a> Binding<'a> { return qualified_name == existing; } } - BindingKind::SubmoduleImport(SubmoduleImport { qualified_name }) => match &existing.kind { - BindingKind::Import(Import { - qualified_name: existing, - }) - | BindingKind::SubmoduleImport(SubmoduleImport { - qualified_name: existing, - }) => { - return qualified_name == existing; + BindingKind::SubmoduleImport(SubmoduleImport { qualified_name }) => { + match &existing.kind { + BindingKind::Import(Import { + qualified_name: existing, + }) + | BindingKind::SubmoduleImport(SubmoduleImport { + qualified_name: existing, + }) => { + return qualified_name == existing; + } + BindingKind::FromImport(FromImport { + qualified_name: existing, + }) => { + return qualified_name == existing; + } + _ => {} } - BindingKind::FromImport(FromImport { - qualified_name: existing, - }) => { - return qualified_name == existing; - } - _ => {} - }, - BindingKind::Deletion | BindingKind::Annotation | BindingKind::FutureImport | BindingKind::Builtin => { + } + BindingKind::Deletion + | BindingKind::Annotation + | BindingKind::FutureImport + | BindingKind::Builtin => { return false; } _ => {} @@ -153,7 +159,9 @@ impl<'a> Binding<'a> { match &self.kind { BindingKind::Import(Import { qualified_name }) => Some(qualified_name), BindingKind::FromImport(FromImport { qualified_name }) => Some(qualified_name), - BindingKind::SubmoduleImport(SubmoduleImport { qualified_name }) => Some(qualified_name), + BindingKind::SubmoduleImport(SubmoduleImport { qualified_name }) => { + Some(qualified_name) + } _ => None, } } @@ -182,13 +190,15 @@ impl<'a> Binding<'a> { /// Returns the range of the binding's parent. pub fn parent_range(&self, semantic: &SemanticModel) -> Option { - self.source.map(|node_id| semantic.stmts[node_id]).and_then(|parent| { - if parent.is_import_from_stmt() { - Some(parent.range()) - } else { - None - } - }) + self.source + .map(|node_id| semantic.stmts[node_id]) + .and_then(|parent| { + if parent.is_import_from_stmt() { + Some(parent.range()) + } else { + None + } + }) } } @@ -260,6 +270,13 @@ bitflags! { /// ``` const INVALID_ALL_OBJECT = 1 << 6; + /// The binding represents and private `TypeVar` declaration. + /// + /// For example, the binding could be `_T` in: + /// ```python + /// import typing + /// _T = typing.TypeVar("_T") + /// ``` const PRIVATE_TYPE_VAR = 1 << 7; } } diff --git a/ruff.schema.json b/ruff.schema.json index 20530484bf28e..03c883e2b017f 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2354,6 +2354,7 @@ "PYI015", "PYI016", "PYI017", + "PYI018", "PYI02", "PYI020", "PYI021", From b1f73e3f6d49457963c94c4b37662e83b3b9b296 Mon Sep 17 00:00:00 2001 From: LaBatata101 Date: Wed, 26 Jul 2023 12:16:10 -0300 Subject: [PATCH 4/6] Tweaks --- crates/ruff/src/checkers/ast/mod.rs | 2 +- .../flake8_pyi/rules/unused_private_type_definition.rs | 6 ++++-- crates/ruff_python_semantic/src/binding.rs | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 8fd43c74f231f..dea9010c063fa 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -4822,7 +4822,7 @@ impl<'a> Checker<'a> { } if self.enabled(Rule::UnusedPrivateTypeVar) { if let Some(diagnostic) = - flake8_pyi::rules::unused_private_type_var(binding, self.locator) + flake8_pyi::rules::unused_private_type_var(self, binding) { self.diagnostics.push(diagnostic); } 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 ca1d6f8a37577..e90d30838790b 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 @@ -3,6 +3,8 @@ use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::source_code::Locator; use ruff_python_semantic::Binding; +use crate::checkers::ast::Checker; + /// ## What it does /// Checks for the presence of unused private `TypeVar` declarations. /// @@ -37,7 +39,7 @@ impl Violation for UnusedPrivateTypeVar { } /// PYI018 -pub(crate) fn unused_private_type_var(binding: &Binding, locator: &Locator) -> Option { +pub(crate) fn unused_private_type_var(checker: &Checker, binding: &Binding) -> Option { if !binding.kind.is_assignment() { return None; } @@ -50,7 +52,7 @@ pub(crate) fn unused_private_type_var(binding: &Binding, locator: &Locator) -> O Some(Diagnostic::new( UnusedPrivateTypeVar { - name: binding.name(locator).to_string(), + name: binding.name(checker.locator()).to_string(), }, binding.range, )) diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 9a615bf6bd6a9..069aadee2a542 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -270,7 +270,7 @@ bitflags! { /// ``` const INVALID_ALL_OBJECT = 1 << 6; - /// The binding represents and private `TypeVar` declaration. + /// The binding represents a private `typing.TypeVar` declaration. /// /// For example, the binding could be `_T` in: /// ```python From 91620e20e4b32d1f5cb80c1286717ecf044a7506 Mon Sep 17 00:00:00 2001 From: LaBatata101 Date: Wed, 26 Jul 2023 17:55:11 -0300 Subject: [PATCH 5/6] review update --- crates/ruff/src/checkers/ast/mod.rs | 22 +++------------- .../rules/unused_private_type_definition.rs | 25 ++++++++++++++----- ..._flake8_pyi__tests__PYI018_PYI018.pyi.snap | 4 +-- crates/ruff_python_semantic/src/binding.rs | 17 +++++++------ 4 files changed, 34 insertions(+), 34 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index dea9010c063fa..ebf94d75188e1 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -4419,6 +4419,10 @@ impl<'a> Checker<'a> { .insert(binding_id, shadowed_id); } + if name.starts_with('_') { + self.semantic.bindings[binding_id].flags |= BindingFlags::PRIVATE_VARIABLE; + } + // Add the binding to the scope. let scope = &mut self.semantic.scopes[scope_id]; scope.add(name, binding_id); @@ -4483,24 +4487,6 @@ impl<'a> Checker<'a> { return; } - if let Stmt::Assign(ast::StmtAssign { targets, value, .. }) = parent { - if let [Expr::Name(ast::ExprName { id, range, .. })] = &targets[..] { - if id.starts_with('_') { - if let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() { - if self.semantic.match_typing_expr(func, "TypeVar") { - self.add_binding( - id, - *range, - BindingKind::Assignment, - BindingFlags::PRIVATE_TYPE_VAR, - ); - return; - } - } - } - } - } - let scope = self.semantic.scope(); if scope.kind.is_module() 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 e90d30838790b..2e6de6800bb11 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::source_code::Locator; use ruff_python_semantic::Binding; +use rustpython_parser::ast::{self, Expr, Stmt}; use crate::checkers::ast::Checker; @@ -34,25 +34,38 @@ impl Violation for UnusedPrivateTypeVar { #[derive_message_formats] fn message(&self) -> String { let UnusedPrivateTypeVar { name } = self; - format!("TypeVar `{name}` is never used") + format!("Private TypeVar `{name}` is never used") } } /// PYI018 pub(crate) fn unused_private_type_var(checker: &Checker, binding: &Binding) -> Option { - if !binding.kind.is_assignment() { + if !binding.kind.is_assignment() && !binding.is_private_variable() { return None; } - if !binding.is_private_type_var() { + if binding.is_used() { return None; } - if binding.is_used() { + + 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: binding.name(checker.locator()).to_string(), + name: id.to_string(), }, binding.range, )) diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI018_PYI018.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI018_PYI018.pyi.snap index 2eedd594357a6..b06ccc51bd19a 100644 --- a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI018_PYI018.pyi.snap +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI018_PYI018.pyi.snap @@ -1,7 +1,7 @@ --- source: crates/ruff/src/rules/flake8_pyi/mod.rs --- -PYI018.pyi:4:1: PYI018 TypeVar `_T` is never used +PYI018.pyi:4:1: PYI018 Private TypeVar `_T` is never used | 2 | from typing import TypeVar 3 | @@ -10,7 +10,7 @@ PYI018.pyi:4:1: PYI018 TypeVar `_T` is never used 5 | _P = TypeVar("_P") | -PYI018.pyi:5:1: PYI018 TypeVar `_P` is never used +PYI018.pyi:5:1: PYI018 Private TypeVar `_P` is never used | 4 | _T = typing.TypeVar("_T") 5 | _P = TypeVar("_P") diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 069aadee2a542..89bf36fe6fa2e 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -94,9 +94,10 @@ impl<'a> Binding<'a> { ) } - /// Return `true` if this [`Binding`] is a private `typing.TypeVar`. - pub const fn is_private_type_var(&self) -> bool { - self.flags.contains(BindingFlags::PRIVATE_TYPE_VAR) + /// 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_VARIABLE) } /// Return `true` if this binding redefines the given binding. @@ -270,14 +271,14 @@ bitflags! { /// ``` const INVALID_ALL_OBJECT = 1 << 6; - /// The binding represents a private `typing.TypeVar` declaration. + /// The binding represents a private variable declaration. /// - /// For example, the binding could be `_T` in: + /// For example, the binding could be `_x` in: /// ```python - /// import typing - /// _T = typing.TypeVar("_T") + /// _x = "This is a private variable" /// ``` - const PRIVATE_TYPE_VAR = 1 << 7; + // const PRIVATE_TYPE_VAR = 1 << 7; + const PRIVATE_VARIABLE = 1 << 7; } } From 69f7bcaf0d4f28e90e3bedd0540ae2825b71afc3 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 26 Jul 2023 18:39:58 -0400 Subject: [PATCH 6/6] Tweaks --- crates/ruff/src/checkers/ast/mod.rs | 9 +++++---- .../rules/unused_private_type_definition.rs | 20 +++++++------------ crates/ruff_python_semantic/src/binding.rs | 5 ++--- 3 files changed, 14 insertions(+), 20 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index ebf94d75188e1..af63705c69480 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -4373,6 +4373,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_VARIABLE; + } + // 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, @@ -4419,10 +4424,6 @@ impl<'a> Checker<'a> { .insert(binding_id, shadowed_id); } - if name.starts_with('_') { - self.semantic.bindings[binding_id].flags |= BindingFlags::PRIVATE_VARIABLE; - } - // Add the binding to the scope. let scope = &mut self.semantic.scopes[scope_id]; scope.add(name, binding_id); 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 2e6de6800bb11..5f0fff3923a7d 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 @@ -9,21 +9,14 @@ use crate::checkers::ast::Checker; /// 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. +/// 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") -/// ``` /// -/// Use instead: -/// ```python -/// import typing /// _T = typing.TypeVar("_T") -/// -/// def func(arg: _T) -> _T: ... /// ``` #[violation] pub struct UnusedPrivateTypeVar { @@ -40,7 +33,7 @@ impl Violation for UnusedPrivateTypeVar { /// PYI018 pub(crate) fn unused_private_type_var(checker: &Checker, binding: &Binding) -> Option { - if !binding.kind.is_assignment() && !binding.is_private_variable() { + if !(binding.kind.is_assignment() && binding.is_private_variable()) { return None; } if binding.is_used() { @@ -50,13 +43,14 @@ pub(crate) fn unused_private_type_var(checker: &Checker, binding: &Binding) -> O let Some(source) = binding.source else { return None; }; - let Stmt::Assign(ast::StmtAssign {targets, value, ..}) = checker.semantic().stmts[source] else { + let Stmt::Assign(ast::StmtAssign { targets, value, .. }) = checker.semantic().stmts[source] + else { return None; }; - let [Expr::Name(ast::ExprName {id, ..})] = &targets[..] else { + let [Expr::Name(ast::ExprName { id, .. })] = &targets[..] else { return None; }; - let Expr::Call(ast::ExprCall {func, ..}) = value.as_ref() else { + let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() else { return None; }; if !checker.semantic().match_typing_expr(func, "TypeVar") { diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 89bf36fe6fa2e..9aa999fcc2a36 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -273,11 +273,10 @@ bitflags! { /// The binding represents a private variable declaration. /// - /// For example, the binding could be `_x` in: + /// For example, the binding could be `_T` in: /// ```python - /// _x = "This is a private variable" + /// _T = "This is a private variable" /// ``` - // const PRIVATE_TYPE_VAR = 1 << 7; const PRIVATE_VARIABLE = 1 << 7; } }