From 2dcd9e2e9c7e0eadd8c063d00449b6961361eead Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 21 Jul 2023 22:08:44 -0400 Subject: [PATCH] Remove unnecessary `check_deferred_assignments` (#5963) ## Summary These rules can just be included in the `check_deferred_scopes`. --- crates/ruff/src/checkers/ast/deferred.rs | 1 - crates/ruff/src/checkers/ast/mod.rs | 154 ++++++++-------- .../rules/unused_arguments.rs | 168 ++++++++++-------- crates/ruff/src/rules/pyflakes/mod.rs | 2 +- .../rules/pyflakes/rules/unused_annotation.rs | 41 ++--- .../rules/pyflakes/rules/unused_variable.rs | 13 +- ...tests__augmented_assignment_after_del.snap | 10 +- 7 files changed, 194 insertions(+), 195 deletions(-) diff --git a/crates/ruff/src/checkers/ast/deferred.rs b/crates/ruff/src/checkers/ast/deferred.rs index fdf375f30d770..d380da3041aaa 100644 --- a/crates/ruff/src/checkers/ast/deferred.rs +++ b/crates/ruff/src/checkers/ast/deferred.rs @@ -14,5 +14,4 @@ pub(crate) struct Deferred<'a> { pub(crate) functions: Vec, pub(crate) lambdas: Vec<(&'a Expr, Snapshot)>, pub(crate) for_loops: Vec, - pub(crate) assignments: Vec, } diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index ea3b270fbba68..9b8f4894cba5d 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -4623,8 +4623,6 @@ impl<'a> Checker<'a> { unreachable!("Expected Stmt::FunctionDef | Stmt::AsyncFunctionDef") } } - - self.deferred.assignments.push(snapshot); } } } @@ -4646,40 +4644,6 @@ impl<'a> Checker<'a> { } else { unreachable!("Expected Expr::Lambda"); } - - self.deferred.assignments.push(snapshot); - } - } - } - - fn check_deferred_assignments(&mut self) { - while !self.deferred.assignments.is_empty() { - let assignments = std::mem::take(&mut self.deferred.assignments); - for snapshot in assignments { - self.semantic.restore(snapshot); - - if self.enabled(Rule::UnusedVariable) { - pyflakes::rules::unused_variable(self, self.semantic.scope_id); - } - if self.enabled(Rule::UnusedAnnotation) { - pyflakes::rules::unused_annotation(self, self.semantic.scope_id); - } - if !self.is_stub { - if self.any_enabled(&[ - Rule::UnusedFunctionArgument, - Rule::UnusedMethodArgument, - Rule::UnusedClassMethodArgument, - Rule::UnusedStaticMethodArgument, - Rule::UnusedLambdaArgument, - ]) { - let scope = &self.semantic.scopes[self.semantic.scope_id]; - let parent = &self.semantic.scopes[scope.parent.unwrap()]; - self.diagnostics - .extend(flake8_unused_arguments::rules::unused_arguments( - self, parent, scope, - )); - } - } } } } @@ -4687,7 +4651,6 @@ impl<'a> Checker<'a> { fn check_deferred_for_loops(&mut self) { while !self.deferred.for_loops.is_empty() { let for_loops = std::mem::take(&mut self.deferred.for_loops); - for snapshot in for_loops { self.semantic.restore(snapshot); @@ -4765,7 +4728,6 @@ impl<'a> Checker<'a> { } for binding in self.semantic.bindings.iter() { - // F841 if self.enabled(Rule::UnusedVariable) { if binding.kind.is_bound_exception() && !binding.is_used() { let mut diagnostic = Diagnostic::new( @@ -4842,7 +4804,6 @@ impl<'a> Checker<'a> { .add_global_reference(binding_id, range, ExecutionContext::Runtime); } else { if self.semantic.global_scope().uses_star_imports() { - // F405 if self.enabled(Rule::UndefinedLocalWithImportStarUsage) { self.diagnostics.push(Diagnostic::new( pyflakes::rules::UndefinedLocalWithImportStarUsage { @@ -4852,7 +4813,6 @@ impl<'a> Checker<'a> { )); } } else { - // F822 if self.enabled(Rule::UndefinedExport) { if !self.path.ends_with("__init__.py") { self.diagnostics.push(Diagnostic::new( @@ -4877,8 +4837,15 @@ impl<'a> Checker<'a> { Rule::TypingOnlyFirstPartyImport, Rule::TypingOnlyStandardLibraryImport, Rule::TypingOnlyThirdPartyImport, - Rule::UnusedImport, Rule::UndefinedLocal, + Rule::UnusedAnnotation, + Rule::UnusedClassMethodArgument, + Rule::UnusedFunctionArgument, + Rule::UnusedImport, + Rule::UnusedLambdaArgument, + Rule::UnusedMethodArgument, + Rule::UnusedStaticMethodArgument, + Rule::UnusedVariable, ]) { return; } @@ -4925,12 +4892,10 @@ impl<'a> Checker<'a> { for scope_id in self.deferred.scopes.iter().rev().copied() { let scope = &self.semantic.scopes[scope_id]; - // F823 if self.enabled(Rule::UndefinedLocal) { pyflakes::rules::undefined_local(self, scope_id, scope, &mut diagnostics); } - // PLW0602 if self.enabled(Rule::GlobalVariableNotAssigned) { for (name, binding_id) in scope.bindings() { let binding = self.semantic.binding(binding_id); @@ -4945,7 +4910,6 @@ impl<'a> Checker<'a> { } } - // F402 if self.enabled(Rule::ImportShadowedByLoopVar) { for (name, binding_id) in scope.bindings() { for shadow in self.semantic.shadowed_bindings(scope_id, binding_id) { @@ -4990,7 +4954,6 @@ impl<'a> Checker<'a> { } } - // F811 if self.enabled(Rule::RedefinedWhileUnused) { for (name, binding_id) in scope.bindings() { for shadow in self.semantic.shadowed_bindings(scope_id, binding_id) { @@ -5077,47 +5040,77 @@ impl<'a> Checker<'a> { } } - // Imports in classes are public members. - if scope.kind.is_class() { - continue; - } - - if enforce_typing_imports { - let runtime_imports: Vec<&Binding> = if self.settings.flake8_type_checking.strict { - vec![] - } else { - self.semantic - .scopes - .ancestor_ids(scope_id) - .flat_map(|scope_id| runtime_imports[scope_id.as_usize()].iter()) - .copied() - .collect() - }; + if matches!( + scope.kind, + ScopeKind::Function(_) | ScopeKind::AsyncFunction(_) | ScopeKind::Lambda(_) + ) { + if self.enabled(Rule::UnusedVariable) { + pyflakes::rules::unused_variable(self, scope, &mut diagnostics); + } - if self.enabled(Rule::RuntimeImportInTypeCheckingBlock) { - flake8_type_checking::rules::runtime_import_in_type_checking_block( - self, - scope, - &mut diagnostics, - ); + if self.enabled(Rule::UnusedAnnotation) { + pyflakes::rules::unused_annotation(self, scope, &mut diagnostics); } - if self.any_enabled(&[ - Rule::TypingOnlyFirstPartyImport, - Rule::TypingOnlyThirdPartyImport, - Rule::TypingOnlyStandardLibraryImport, - ]) { - flake8_type_checking::rules::typing_only_runtime_import( - self, - scope, - &runtime_imports, - &mut diagnostics, - ); + if !self.is_stub { + if self.any_enabled(&[ + Rule::UnusedFunctionArgument, + Rule::UnusedMethodArgument, + Rule::UnusedClassMethodArgument, + Rule::UnusedStaticMethodArgument, + Rule::UnusedLambdaArgument, + ]) { + flake8_unused_arguments::rules::unused_arguments( + self, + scope, + &mut diagnostics, + ); + } } } - if self.enabled(Rule::UnusedImport) { - pyflakes::rules::unused_import(self, scope, &mut diagnostics); + if matches!( + scope.kind, + ScopeKind::Function(_) | ScopeKind::AsyncFunction(_) | ScopeKind::Module + ) { + if enforce_typing_imports { + let runtime_imports: Vec<&Binding> = + if self.settings.flake8_type_checking.strict { + vec![] + } else { + self.semantic + .scopes + .ancestor_ids(scope_id) + .flat_map(|scope_id| runtime_imports[scope_id.as_usize()].iter()) + .copied() + .collect() + }; + + if self.enabled(Rule::RuntimeImportInTypeCheckingBlock) { + flake8_type_checking::rules::runtime_import_in_type_checking_block( + self, + scope, + &mut diagnostics, + ); + } + + if self.any_enabled(&[ + Rule::TypingOnlyFirstPartyImport, + Rule::TypingOnlyThirdPartyImport, + Rule::TypingOnlyStandardLibraryImport, + ]) { + flake8_type_checking::rules::typing_only_runtime_import( + self, + scope, + &runtime_imports, + &mut diagnostics, + ); + } + } + + if self.enabled(Rule::UnusedImport) { + pyflakes::rules::unused_import(self, scope, &mut diagnostics); + } } } self.diagnostics.extend(diagnostics); @@ -5460,7 +5453,6 @@ pub(crate) fn check_ast( checker.check_deferred_future_type_definitions(); let allocator = typed_arena::Arena::new(); checker.check_deferred_string_type_definitions(&allocator); - checker.check_deferred_assignments(); checker.check_deferred_for_loops(); // Check docstrings, exports, bindings, and unresolved references. diff --git a/crates/ruff/src/rules/flake8_unused_arguments/rules/unused_arguments.rs b/crates/ruff/src/rules/flake8_unused_arguments/rules/unused_arguments.rs index 9c4d406474808..7df519c887587 100644 --- a/crates/ruff/src/rules/flake8_unused_arguments/rules/unused_arguments.rs +++ b/crates/ruff/src/rules/flake8_unused_arguments/rules/unused_arguments.rs @@ -12,40 +12,7 @@ use ruff_python_semantic::{Scope, ScopeKind, SemanticModel}; use crate::checkers::ast::Checker; use crate::registry::Rule; - -use super::super::helpers; - -/// An AST node that can contain arguments. -#[derive(Debug, Copy, Clone)] -enum Argumentable { - Function, - Method, - ClassMethod, - StaticMethod, - Lambda, -} - -impl Argumentable { - fn check_for(self, name: String) -> DiagnosticKind { - match self { - Self::Function => UnusedFunctionArgument { name }.into(), - Self::Method => UnusedMethodArgument { name }.into(), - Self::ClassMethod => UnusedClassMethodArgument { name }.into(), - Self::StaticMethod => UnusedStaticMethodArgument { name }.into(), - Self::Lambda => UnusedLambdaArgument { name }.into(), - } - } - - const fn rule_code(self) -> Rule { - match self { - Self::Function => Rule::UnusedFunctionArgument, - Self::Method => Rule::UnusedMethodArgument, - Self::ClassMethod => Rule::UnusedClassMethodArgument, - Self::StaticMethod => Rule::UnusedStaticMethodArgument, - Self::Lambda => Rule::UnusedLambdaArgument, - } - } -} +use crate::rules::flake8_unused_arguments::helpers; /// ## What it does /// Checks for the presence of unused arguments in function definitions. @@ -67,7 +34,7 @@ impl Argumentable { /// ``` #[violation] pub struct UnusedFunctionArgument { - pub(super) name: String, + name: String, } impl Violation for UnusedFunctionArgument { @@ -100,7 +67,7 @@ impl Violation for UnusedFunctionArgument { /// ``` #[violation] pub struct UnusedMethodArgument { - pub(super) name: String, + name: String, } impl Violation for UnusedMethodArgument { @@ -135,7 +102,7 @@ impl Violation for UnusedMethodArgument { /// ``` #[violation] pub struct UnusedClassMethodArgument { - pub(super) name: String, + name: String, } impl Violation for UnusedClassMethodArgument { @@ -170,7 +137,7 @@ impl Violation for UnusedClassMethodArgument { /// ``` #[violation] pub struct UnusedStaticMethodArgument { - pub(super) name: String, + name: String, } impl Violation for UnusedStaticMethodArgument { @@ -202,7 +169,7 @@ impl Violation for UnusedStaticMethodArgument { /// ``` #[violation] pub struct UnusedLambdaArgument { - pub(super) name: String, + name: String, } impl Violation for UnusedLambdaArgument { @@ -213,6 +180,38 @@ impl Violation for UnusedLambdaArgument { } } +/// An AST node that can contain arguments. +#[derive(Debug, Copy, Clone)] +enum Argumentable { + Function, + Method, + ClassMethod, + StaticMethod, + Lambda, +} + +impl Argumentable { + fn check_for(self, name: String) -> DiagnosticKind { + match self { + Self::Function => UnusedFunctionArgument { name }.into(), + Self::Method => UnusedMethodArgument { name }.into(), + Self::ClassMethod => UnusedClassMethodArgument { name }.into(), + Self::StaticMethod => UnusedStaticMethodArgument { name }.into(), + Self::Lambda => UnusedLambdaArgument { name }.into(), + } + } + + const fn rule_code(self) -> Rule { + match self { + Self::Function => Rule::UnusedFunctionArgument, + Self::Method => Rule::UnusedMethodArgument, + Self::ClassMethod => Rule::UnusedClassMethodArgument, + Self::StaticMethod => Rule::UnusedStaticMethodArgument, + Self::Lambda => Rule::UnusedLambdaArgument, + } + } +} + /// Check a plain function for unused arguments. fn function( argumentable: Argumentable, @@ -221,7 +220,8 @@ fn function( semantic: &SemanticModel, dummy_variable_rgx: &Regex, ignore_variadic_names: bool, -) -> Vec { + diagnostics: &mut Vec, +) { let args = args .posonlyargs .iter() @@ -238,7 +238,14 @@ fn function( .flatten() .skip(usize::from(ignore_variadic_names)), ); - call(argumentable, args, values, semantic, dummy_variable_rgx) + call( + argumentable, + args, + values, + semantic, + dummy_variable_rgx, + diagnostics, + ); } /// Check a method for unused arguments. @@ -249,7 +256,8 @@ fn method( semantic: &SemanticModel, dummy_variable_rgx: &Regex, ignore_variadic_names: bool, -) -> Vec { + diagnostics: &mut Vec, +) { let args = args .posonlyargs .iter() @@ -267,7 +275,14 @@ fn method( .flatten() .skip(usize::from(ignore_variadic_names)), ); - call(argumentable, args, values, semantic, dummy_variable_rgx) + call( + argumentable, + args, + values, + semantic, + dummy_variable_rgx, + diagnostics, + ); } fn call<'a>( @@ -276,33 +291,39 @@ fn call<'a>( values: &Scope, semantic: &SemanticModel, dummy_variable_rgx: &Regex, -) -> Vec { - let mut diagnostics: Vec = vec![]; - for arg in args { - if let Some(binding) = values + diagnostics: &mut Vec, +) { + diagnostics.extend(args.filter_map(|arg| { + let binding = values .get(arg.arg.as_str()) - .map(|binding_id| semantic.binding(binding_id)) + .map(|binding_id| semantic.binding(binding_id))?; + if binding.kind.is_argument() + && !binding.is_used() + && !dummy_variable_rgx.is_match(arg.arg.as_str()) { - if binding.kind.is_argument() - && !binding.is_used() - && !dummy_variable_rgx.is_match(arg.arg.as_str()) - { - diagnostics.push(Diagnostic::new( - argumentable.check_for(arg.arg.to_string()), - binding.range, - )); - } + Some(Diagnostic::new( + argumentable.check_for(arg.arg.to_string()), + binding.range, + )) + } else { + None } - } - diagnostics + })); } /// ARG001, ARG002, ARG003, ARG004, ARG005 pub(crate) fn unused_arguments( checker: &Checker, - parent: &Scope, scope: &Scope, -) -> Vec { + diagnostics: &mut Vec, +) { + let Some(parent) = scope + .parent + .map(|scope_id| &checker.semantic().scopes[scope_id]) + else { + return; + }; + match &scope.kind { ScopeKind::Function(ast::StmtFunctionDef { name, @@ -340,9 +361,8 @@ pub(crate) fn unused_arguments( .settings .flake8_unused_arguments .ignore_variadic_names, - ) - } else { - vec![] + diagnostics, + ); } } function_type::FunctionType::Method => { @@ -366,9 +386,8 @@ pub(crate) fn unused_arguments( .settings .flake8_unused_arguments .ignore_variadic_names, - ) - } else { - vec![] + diagnostics, + ); } } function_type::FunctionType::ClassMethod => { @@ -392,9 +411,8 @@ pub(crate) fn unused_arguments( .settings .flake8_unused_arguments .ignore_variadic_names, - ) - } else { - vec![] + diagnostics, + ); } } function_type::FunctionType::StaticMethod => { @@ -418,9 +436,8 @@ pub(crate) fn unused_arguments( .settings .flake8_unused_arguments .ignore_variadic_names, - ) - } else { - vec![] + diagnostics, + ); } } } @@ -437,9 +454,8 @@ pub(crate) fn unused_arguments( .settings .flake8_unused_arguments .ignore_variadic_names, - ) - } else { - vec![] + diagnostics, + ); } } _ => panic!("Expected ScopeKind::Function | ScopeKind::Lambda"), diff --git a/crates/ruff/src/rules/pyflakes/mod.rs b/crates/ruff/src/rules/pyflakes/mod.rs index 441d0d9bb480f..930447f4185c3 100644 --- a/crates/ruff/src/rules/pyflakes/mod.rs +++ b/crates/ruff/src/rules/pyflakes/mod.rs @@ -1027,8 +1027,8 @@ mod tests { &[ Rule::UndefinedName, Rule::UndefinedName, - Rule::UnusedVariable, Rule::UndefinedName, + Rule::UnusedVariable, Rule::UndefinedName, ], ); diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_annotation.rs b/crates/ruff/src/rules/pyflakes/rules/unused_annotation.rs index 1f2e92f25d7f6..b71d048d20d3e 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_annotation.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_annotation.rs @@ -1,6 +1,6 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_semantic::ScopeId; +use ruff_python_semantic::Scope; use crate::checkers::ast::Checker; @@ -33,27 +33,22 @@ impl Violation for UnusedAnnotation { } /// F842 -pub(crate) fn unused_annotation(checker: &mut Checker, scope: ScopeId) { - let scope = &checker.semantic().scopes[scope]; - - let bindings: Vec<_> = scope - .bindings() - .filter_map(|(name, binding_id)| { - let binding = checker.semantic().binding(binding_id); - if binding.kind.is_annotation() - && !binding.is_used() - && !checker.settings.dummy_variable_rgx.is_match(name) - { - Some((name.to_string(), binding.range)) - } else { - None - } - }) - .collect(); - - for (name, range) in bindings { - checker - .diagnostics - .push(Diagnostic::new(UnusedAnnotation { name }, range)); +pub(crate) fn unused_annotation( + checker: &Checker, + scope: &Scope, + diagnostics: &mut Vec, +) { + for (name, range) in scope.bindings().filter_map(|(name, binding_id)| { + let binding = checker.semantic().binding(binding_id); + if binding.kind.is_annotation() + && !binding.is_used() + && !checker.settings.dummy_variable_rgx.is_match(name) + { + Some((name.to_string(), binding.range)) + } else { + None + } + }) { + diagnostics.push(Diagnostic::new(UnusedAnnotation { name }, range)); } } diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs index a611eb8375a0e..f7bb26d326859 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs @@ -7,7 +7,7 @@ use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::contains_effect; use ruff_python_ast::source_code::Locator; -use ruff_python_semantic::ScopeId; +use ruff_python_semantic::Scope; use crate::autofix::edits::delete_stmt; use crate::checkers::ast::Checker; @@ -272,13 +272,12 @@ fn remove_unused_variable( } /// F841 -pub(crate) fn unused_variable(checker: &mut Checker, scope: ScopeId) { - let scope = &checker.semantic().scopes[scope]; +pub(crate) fn unused_variable(checker: &Checker, scope: &Scope, diagnostics: &mut Vec) { if scope.uses_locals() && scope.kind.is_any_function() { return; } - let bindings: Vec<_> = scope + for (name, range, source) in scope .bindings() .map(|(name, binding_id)| (name, checker.semantic().binding(binding_id))) .filter_map(|(name, binding)| { @@ -297,9 +296,7 @@ pub(crate) fn unused_variable(checker: &mut Checker, scope: ScopeId) { None }) - .collect(); - - for (name, range, source) in bindings { + { let mut diagnostic = Diagnostic::new(UnusedVariable { name }, range); if checker.patch(diagnostic.kind.rule()) { if let Some(source) = source { @@ -310,6 +307,6 @@ pub(crate) fn unused_variable(checker: &mut Checker, scope: ScopeId) { } } } - checker.diagnostics.push(diagnostic); + diagnostics.push(diagnostic); } } diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__augmented_assignment_after_del.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__augmented_assignment_after_del.snap index bae62655e8128..57a88f04c40c0 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__augmented_assignment_after_del.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__augmented_assignment_after_del.snap @@ -1,21 +1,21 @@ --- source: crates/ruff/src/rules/pyflakes/mod.rs --- -:10:5: F841 Local variable `x` is assigned to but never used +:10:5: F821 Undefined name `x` | 8 | # entirely after the `del` statement. However, it should be an F821 9 | # error, because the name is defined in the scope, but unbound. 10 | x += 1 - | ^ F841 + | ^ F821 | - = help: Remove assignment to unused variable `x` -:10:5: F821 Undefined name `x` +:10:5: F841 Local variable `x` is assigned to but never used | 8 | # entirely after the `del` statement. However, it should be an F821 9 | # error, because the name is defined in the scope, but unbound. 10 | x += 1 - | ^ F821 + | ^ F841 | + = help: Remove assignment to unused variable `x`