diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index 4221fd81e355ce..44b39d351fb27e 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -1,12 +1,7 @@ -use ruff_python_semantic::analyze::visibility; -use ruff_python_semantic::{Binding, BindingKind, Imported, ResolvedReference, ScopeKind}; -use ruff_text_size::Ranged; -use rustc_hash::FxHashMap; +use ruff_python_semantic::{Binding, ScopeKind}; -use crate::Fix; use crate::checkers::ast::Checker; use crate::codes::Rule; -use crate::fix; use crate::rules::{ flake8_builtins, flake8_pyi, flake8_type_checking, flake8_unused_arguments, pep8_naming, pyflakes, pylint, ruff, @@ -93,265 +88,20 @@ pub(crate) fn deferred_scopes(checker: &Checker) { pyflakes::rules::undefined_local(checker, scope_id, scope); } - // PLW0602 if checker.is_rule_enabled(Rule::GlobalVariableNotAssigned) { - for (name, binding_id) in scope.bindings() { - let binding = checker.semantic.binding(binding_id); - // If the binding is a `global`, then it's a top-level `global` that was never - // assigned in the current scope. If it were assigned, the `global` would be - // shadowed by the assignment. - if binding.kind.is_global() { - // If the binding was conditionally deleted, it will include a reference within - // a `Del` context, but won't be shadowed by a `BindingKind::Deletion`, as in: - // ```python - // if condition: - // del var - // ``` - if binding - .references - .iter() - .map(|id| checker.semantic.reference(*id)) - .all(ResolvedReference::is_load) - { - checker.report_diagnostic( - pylint::rules::GlobalVariableNotAssigned { - name: (*name).to_string(), - }, - binding.range(), - ); - } - } - } + pylint::rules::global_variable_not_assigned(checker, scope); } - // PLR1704 if checker.is_rule_enabled(Rule::RedefinedArgumentFromLocal) { - for (name, binding_id) in scope.bindings() { - for shadow in checker.semantic.shadowed_bindings(scope_id, binding_id) { - let binding = &checker.semantic.bindings[shadow.binding_id()]; - if !matches!( - binding.kind, - BindingKind::LoopVar - | BindingKind::BoundException - | BindingKind::WithItemVar - ) { - continue; - } - let shadowed = &checker.semantic.bindings[shadow.shadowed_id()]; - if !shadowed.kind.is_argument() { - continue; - } - if checker.settings().dummy_variable_rgx.is_match(name) { - continue; - } - let scope = &checker.semantic.scopes[binding.scope]; - if scope.kind.is_generator() { - continue; - } - checker.report_diagnostic( - pylint::rules::RedefinedArgumentFromLocal { - name: name.to_string(), - }, - binding.range(), - ); - } - } + pylint::rules::redefined_argument_from_local(checker, scope_id, scope); } - // F402 if checker.is_rule_enabled(Rule::ImportShadowedByLoopVar) { - for (name, binding_id) in scope.bindings() { - for shadow in checker.semantic.shadowed_bindings(scope_id, binding_id) { - // If the shadowing binding isn't a loop variable, abort. - let binding = &checker.semantic.bindings[shadow.binding_id()]; - if !binding.kind.is_loop_var() { - continue; - } - - // If the shadowed binding isn't an import, abort. - let shadowed = &checker.semantic.bindings[shadow.shadowed_id()]; - if !matches!( - shadowed.kind, - BindingKind::Import(..) - | BindingKind::FromImport(..) - | BindingKind::SubmoduleImport(..) - | BindingKind::FutureImport - ) { - continue; - } - - // If the bindings are in different forks, abort. - if shadowed.source.is_none_or(|left| { - binding - .source - .is_none_or(|right| !checker.semantic.same_branch(left, right)) - }) { - continue; - } - - checker.report_diagnostic( - pyflakes::rules::ImportShadowedByLoopVar { - name: name.to_string(), - row: checker.compute_source_row(shadowed.start()), - }, - binding.range(), - ); - } - } + pyflakes::rules::import_shadowed_by_loop_var(checker, scope_id, scope); } - // F811 if checker.is_rule_enabled(Rule::RedefinedWhileUnused) { - // Index the redefined bindings by statement. - let mut redefinitions = FxHashMap::default(); - - for (name, binding_id) in scope.bindings() { - for shadow in checker.semantic.shadowed_bindings(scope_id, binding_id) { - // If the shadowing binding is a loop variable, abort, to avoid overlap - // with F402. - let binding = &checker.semantic.bindings[shadow.binding_id()]; - if binding.kind.is_loop_var() { - continue; - } - - // If the shadowed binding is used, abort. - let shadowed = &checker.semantic.bindings[shadow.shadowed_id()]; - if shadowed.is_used() { - continue; - } - - // If the shadowing binding isn't considered a "redefinition" of the - // shadowed binding, abort. - if !binding.redefines(shadowed) { - continue; - } - - if shadow.same_scope() { - // If the symbol is a dummy variable, abort, unless the shadowed - // binding is an import. - if !matches!( - shadowed.kind, - BindingKind::Import(..) - | BindingKind::FromImport(..) - | BindingKind::SubmoduleImport(..) - | BindingKind::FutureImport - ) && checker.settings().dummy_variable_rgx.is_match(name) - { - continue; - } - - let Some(node_id) = shadowed.source else { - continue; - }; - - // If this is an overloaded function, abort. - if shadowed.kind.is_function_definition() { - if checker - .semantic - .statement(node_id) - .as_function_def_stmt() - .is_some_and(|function| { - visibility::is_overload( - &function.decorator_list, - &checker.semantic, - ) - }) - { - continue; - } - } - } else { - // Only enforce cross-scope shadowing for imports. - if !matches!( - shadowed.kind, - BindingKind::Import(..) - | BindingKind::FromImport(..) - | BindingKind::SubmoduleImport(..) - | BindingKind::FutureImport - ) { - continue; - } - } - - // If the bindings are in different forks, abort. - if shadowed.source.is_none_or(|left| { - binding - .source - .is_none_or(|right| !checker.semantic.same_branch(left, right)) - }) { - continue; - } - - redefinitions - .entry(binding.source) - .or_insert_with(Vec::new) - .push((shadowed, binding)); - } - } - - // Create a fix for each source statement. - let mut fixes = FxHashMap::default(); - for (source, entries) in &redefinitions { - let Some(source) = source else { - continue; - }; - - let member_names = entries - .iter() - .filter_map(|(shadowed, binding)| { - if let Some(shadowed_import) = shadowed.as_any_import() { - if let Some(import) = binding.as_any_import() { - if shadowed_import.qualified_name() == import.qualified_name() { - return Some(import.member_name()); - } - } - } - None - }) - .collect::>(); - - if !member_names.is_empty() { - let statement = checker.semantic.statement(*source); - let parent = checker.semantic.parent_statement(*source); - let Ok(edit) = fix::edits::remove_unused_imports( - member_names.iter().map(std::convert::AsRef::as_ref), - statement, - parent, - checker.locator(), - checker.stylist(), - checker.indexer(), - ) else { - continue; - }; - fixes.insert( - *source, - Fix::safe_edit(edit).isolate(Checker::isolation( - checker.semantic().parent_statement_id(*source), - )), - ); - } - } - - // Create diagnostics for each statement. - for (source, entries) in &redefinitions { - for (shadowed, binding) in entries { - let mut diagnostic = checker.report_diagnostic( - pyflakes::rules::RedefinedWhileUnused { - name: binding.name(checker.source()).to_string(), - row: checker.compute_source_row(shadowed.start()), - }, - binding.range(), - ); - - if let Some(range) = binding.parent_range(&checker.semantic) { - diagnostic.set_parent(range.start()); - } - - if let Some(fix) = source.as_ref().and_then(|source| fixes.get(source)) { - diagnostic.set_fix(fix.clone()); - } - } - } + pyflakes::rules::redefined_while_unused(checker, scope_id, scope); } if checker.source_type.is_stub() diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 2768bcd17ca484..0876cf5ec55df0 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -45,18 +45,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } if checker.is_rule_enabled(Rule::NonlocalWithoutBinding) { - if !checker.semantic.scope_id.is_global() { - for name in names { - if checker.semantic.nonlocal(name).is_none() { - checker.report_diagnostic( - pylint::rules::NonlocalWithoutBinding { - name: name.to_string(), - }, - name.range(), - ); - } - } - } + pylint::rules::nonlocal_without_binding(checker, nonlocal); } if checker.is_rule_enabled(Rule::NonlocalAndGlobal) { pylint::rules::nonlocal_and_global(checker, nonlocal); diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/imports.rs b/crates/ruff_linter/src/rules/pyflakes/rules/imports.rs index 170e4b17143e62..3b3c33d0fe669c 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/imports.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/imports.rs @@ -1,7 +1,10 @@ use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_python_semantic::{BindingKind, Scope, ScopeId}; use ruff_source_file::SourceRow; +use ruff_text_size::Ranged; use crate::Violation; +use crate::checkers::ast::Checker; /// ## What it does /// Checks for import bindings that are shadowed by loop variables. @@ -44,6 +47,48 @@ impl Violation for ImportShadowedByLoopVar { } } +/// F402 +pub(crate) fn import_shadowed_by_loop_var(checker: &Checker, scope_id: ScopeId, scope: &Scope) { + for (name, binding_id) in scope.bindings() { + for shadow in checker.semantic().shadowed_bindings(scope_id, binding_id) { + // If the shadowing binding isn't a loop variable, abort. + let binding = &checker.semantic().bindings[shadow.binding_id()]; + if !binding.kind.is_loop_var() { + continue; + } + + // If the shadowed binding isn't an import, abort. + let shadowed = &checker.semantic().bindings[shadow.shadowed_id()]; + if !matches!( + shadowed.kind, + BindingKind::Import(..) + | BindingKind::FromImport(..) + | BindingKind::SubmoduleImport(..) + | BindingKind::FutureImport + ) { + continue; + } + + // If the bindings are in different forks, abort. + if shadowed.source.is_none_or(|left| { + binding + .source + .is_none_or(|right| !checker.semantic().same_branch(left, right)) + }) { + continue; + } + + checker.report_diagnostic( + ImportShadowedByLoopVar { + name: name.to_string(), + row: checker.compute_source_row(shadowed.start()), + }, + binding.range(), + ); + } + } +} + /// ## What it does /// Checks for the use of wildcard imports. /// diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs b/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs index cec4d60495298d..a60b67f82d0649 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs @@ -1,7 +1,14 @@ use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_python_semantic::analyze::visibility; +use ruff_python_semantic::{BindingKind, Imported, Scope, ScopeId}; use ruff_source_file::SourceRow; +use ruff_text_size::Ranged; -use crate::{FixAvailability, Violation}; +use crate::checkers::ast::Checker; +use crate::fix::edits; +use crate::{Fix, FixAvailability, Violation}; + +use rustc_hash::FxHashMap; /// ## What it does /// Checks for variable definitions that redefine (or "shadow") unused @@ -43,3 +50,154 @@ impl Violation for RedefinedWhileUnused { Some(format!("Remove definition: `{name}`")) } } + +/// F811 +pub(crate) fn redefined_while_unused(checker: &Checker, scope_id: ScopeId, scope: &Scope) { + // Index the redefined bindings by statement. + let mut redefinitions = FxHashMap::default(); + + for (name, binding_id) in scope.bindings() { + for shadow in checker.semantic().shadowed_bindings(scope_id, binding_id) { + // If the shadowing binding is a loop variable, abort, to avoid overlap + // with F402. + let binding = &checker.semantic().bindings[shadow.binding_id()]; + if binding.kind.is_loop_var() { + continue; + } + + // If the shadowed binding is used, abort. + let shadowed = &checker.semantic().bindings[shadow.shadowed_id()]; + if shadowed.is_used() { + continue; + } + + // If the shadowing binding isn't considered a "redefinition" of the + // shadowed binding, abort. + if !binding.redefines(shadowed) { + continue; + } + + if shadow.same_scope() { + // If the symbol is a dummy variable, abort, unless the shadowed + // binding is an import. + if !matches!( + shadowed.kind, + BindingKind::Import(..) + | BindingKind::FromImport(..) + | BindingKind::SubmoduleImport(..) + | BindingKind::FutureImport + ) && checker.settings().dummy_variable_rgx.is_match(name) + { + continue; + } + + let Some(node_id) = shadowed.source else { + continue; + }; + + // If this is an overloaded function, abort. + if shadowed.kind.is_function_definition() { + if checker + .semantic() + .statement(node_id) + .as_function_def_stmt() + .is_some_and(|function| { + visibility::is_overload(&function.decorator_list, checker.semantic()) + }) + { + continue; + } + } + } else { + // Only enforce cross-scope shadowing for imports. + if !matches!( + shadowed.kind, + BindingKind::Import(..) + | BindingKind::FromImport(..) + | BindingKind::SubmoduleImport(..) + | BindingKind::FutureImport + ) { + continue; + } + } + + // If the bindings are in different forks, abort. + if shadowed.source.is_none_or(|left| { + binding + .source + .is_none_or(|right| !checker.semantic().same_branch(left, right)) + }) { + continue; + } + + redefinitions + .entry(binding.source) + .or_insert_with(Vec::new) + .push((shadowed, binding)); + } + } + + // Create a fix for each source statement. + let mut fixes = FxHashMap::default(); + for (source, entries) in &redefinitions { + let Some(source) = source else { + continue; + }; + + let member_names = entries + .iter() + .filter_map(|(shadowed, binding)| { + if let Some(shadowed_import) = shadowed.as_any_import() { + if let Some(import) = binding.as_any_import() { + if shadowed_import.qualified_name() == import.qualified_name() { + return Some(import.member_name()); + } + } + } + None + }) + .collect::>(); + + if !member_names.is_empty() { + let statement = checker.semantic().statement(*source); + let parent = checker.semantic().parent_statement(*source); + let Ok(edit) = edits::remove_unused_imports( + member_names.iter().map(std::convert::AsRef::as_ref), + statement, + parent, + checker.locator(), + checker.stylist(), + checker.indexer(), + ) else { + continue; + }; + fixes.insert( + *source, + Fix::safe_edit(edit).isolate(Checker::isolation( + checker.semantic().parent_statement_id(*source), + )), + ); + } + } + + // Create diagnostics for each statement. + for (source, entries) in &redefinitions { + for (shadowed, binding) in entries { + let mut diagnostic = checker.report_diagnostic( + RedefinedWhileUnused { + name: binding.name(checker.source()).to_string(), + row: checker.compute_source_row(shadowed.start()), + }, + binding.range(), + ); + + if let Some(range) = binding.parent_range(checker.semantic()) { + diagnostic.set_parent(range.start()); + } + + if let Some(fix) = source.as_ref().and_then(|source| fixes.get(source)) { + diagnostic.set_fix(fix.clone()); + } + } + } +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/global_variable_not_assigned.rs b/crates/ruff_linter/src/rules/pylint/rules/global_variable_not_assigned.rs index d6f3c6fa7d4fa0..a5421541f3df93 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/global_variable_not_assigned.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/global_variable_not_assigned.rs @@ -1,6 +1,9 @@ use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_python_semantic::{ResolvedReference, Scope}; +use ruff_text_size::Ranged; use crate::Violation; +use crate::checkers::ast::Checker; /// ## What it does /// Checks for `global` variables that are not assigned a value in the current @@ -38,7 +41,7 @@ use crate::Violation; /// - [Python documentation: The `global` statement](https://docs.python.org/3/reference/simple_stmts.html#the-global-statement) #[derive(ViolationMetadata)] pub(crate) struct GlobalVariableNotAssigned { - pub name: String, + name: String, } impl Violation for GlobalVariableNotAssigned { @@ -48,3 +51,34 @@ impl Violation for GlobalVariableNotAssigned { format!("Using global for `{name}` but no assignment is done") } } + +/// PLW0602 +pub(crate) fn global_variable_not_assigned(checker: &Checker, scope: &Scope) { + for (name, binding_id) in scope.bindings() { + let binding = checker.semantic().binding(binding_id); + // If the binding is a `global`, then it's a top-level `global` that was never + // assigned in the current scope. If it were assigned, the `global` would be + // shadowed by the assignment. + if binding.kind.is_global() { + // If the binding was conditionally deleted, it will include a reference within + // a `Del` context, but won't be shadowed by a `BindingKind::Deletion`, as in: + // ```python + // if condition: + // del var + // ``` + if binding + .references + .iter() + .map(|id| checker.semantic().reference(*id)) + .all(ResolvedReference::is_load) + { + checker.report_diagnostic( + GlobalVariableNotAssigned { + name: (*name).to_string(), + }, + binding.range(), + ); + } + } + } +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/nonlocal_without_binding.rs b/crates/ruff_linter/src/rules/pylint/rules/nonlocal_without_binding.rs index c6e82e3958c259..ff2bd1a2ab0de5 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/nonlocal_without_binding.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/nonlocal_without_binding.rs @@ -1,6 +1,9 @@ use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_python_ast as ast; +use ruff_text_size::Ranged; use crate::Violation; +use crate::checkers::ast::Checker; /// ## What it does /// Checks for `nonlocal` names without bindings. @@ -42,3 +45,19 @@ impl Violation for NonlocalWithoutBinding { format!("Nonlocal name `{name}` found without binding") } } + +/// PLE0117 +pub(crate) fn nonlocal_without_binding(checker: &Checker, nonlocal: &ast::StmtNonlocal) { + if !checker.semantic().scope_id.is_global() { + for name in &nonlocal.names { + if checker.semantic().nonlocal(name).is_none() { + checker.report_diagnostic( + NonlocalWithoutBinding { + name: name.to_string(), + }, + name.range(), + ); + } + } + } +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/redefined_argument_from_local.rs b/crates/ruff_linter/src/rules/pylint/rules/redefined_argument_from_local.rs index c8cc0f8e6df638..856447d810545e 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/redefined_argument_from_local.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/redefined_argument_from_local.rs @@ -1,6 +1,9 @@ use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_python_semantic::{BindingKind, Scope, ScopeId}; +use ruff_text_size::Ranged; use crate::Violation; +use crate::checkers::ast::Checker; /// ## What it does /// Checks for variables defined in `for`, `try`, `with` statements @@ -41,3 +44,35 @@ impl Violation for RedefinedArgumentFromLocal { format!("Redefining argument with the local name `{name}`") } } + +/// PLR1704 +pub(crate) fn redefined_argument_from_local(checker: &Checker, scope_id: ScopeId, scope: &Scope) { + for (name, binding_id) in scope.bindings() { + for shadow in checker.semantic().shadowed_bindings(scope_id, binding_id) { + let binding = &checker.semantic().bindings[shadow.binding_id()]; + if !matches!( + binding.kind, + BindingKind::LoopVar | BindingKind::BoundException | BindingKind::WithItemVar + ) { + continue; + } + let shadowed = &checker.semantic().bindings[shadow.shadowed_id()]; + if !shadowed.kind.is_argument() { + continue; + } + if checker.settings().dummy_variable_rgx.is_match(name) { + continue; + } + let scope = &checker.semantic().scopes[binding.scope]; + if scope.kind.is_generator() { + continue; + } + checker.report_diagnostic( + RedefinedArgumentFromLocal { + name: name.to_string(), + }, + binding.range(), + ); + } + } +}