diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index b61ba83627..6aa74c7f00 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1194,22 +1194,10 @@ where )); } } else if alias.node.name == "*" { - self.add_binding( - "*", - Binding { - kind: BindingKind::StarImportation(StarImportation { - level: *level, - module: module.clone(), - }), - runtime_usage: None, - synthetic_usage: None, - typing_usage: None, - range: Range::from(stmt), - source: Some(*self.ctx.current_stmt()), - context: self.ctx.execution_context(), - exceptions: self.ctx.exceptions(), - }, - ); + self.ctx.scope_mut().add_star_import(StarImportation { + module: module.as_ref().map(String::as_str), + level: *level, + }); if self .settings @@ -1242,9 +1230,6 @@ where Range::from(stmt), )); } - - let scope = self.ctx.scope_mut(); - scope.import_starred = true; } else { if let Some(asname) = &alias.node.asname { self.check_builtin_shadowing(asname, stmt, false); @@ -4045,7 +4030,6 @@ impl<'a> Checker<'a> { BindingKind::Importation(..) | BindingKind::FromImportation(..) | BindingKind::SubmoduleImportation(..) - | BindingKind::StarImportation(..) | BindingKind::FutureImportation ); if binding.kind.is_loop_var() && existing_is_import { @@ -4252,32 +4236,31 @@ impl<'a> Checker<'a> { first_iter = false; in_generator = matches!(scope.kind, ScopeKind::Generator); - import_starred = import_starred || scope.import_starred; + import_starred = import_starred || scope.uses_star_imports(); } if import_starred { + // F405 if self .settings .rules .enabled(Rule::UndefinedLocalWithImportStarUsage) { - let mut from_list = vec![]; - for scope_index in self.ctx.scope_stack.iter() { - let scope = &self.ctx.scopes[*scope_index]; - for binding in scope.binding_ids().map(|index| &self.ctx.bindings[*index]) { - if let BindingKind::StarImportation(StarImportation { level, module }) = - &binding.kind - { - from_list.push(helpers::format_import_from(*level, module.as_deref())); - } - } - } - from_list.sort(); - + let sources: Vec = self + .ctx + .scopes + .iter() + .flat_map(Scope::star_imports) + .map(|StarImportation { level, module }| { + helpers::format_import_from(*level, *module) + }) + .sorted() + .dedup() + .collect(); self.diagnostics.push(Diagnostic::new( pyflakes::rules::UndefinedLocalWithImportStarUsage { name: id.to_string(), - sources: from_list, + sources, }, Range::from(expr), )); @@ -4755,7 +4738,6 @@ impl<'a> Checker<'a> { } // Mark anything referenced in `__all__` as used. - let all_bindings: Option<(Vec, Range)> = { let global_scope = self.ctx.global_scope(); let all_names: Option<(&Vec, Range)> = global_scope @@ -4829,13 +4811,45 @@ impl<'a> Checker<'a> { for (index, stack) in self.ctx.dead_scopes.iter().rev() { let scope = &self.ctx.scopes[*index]; - // F822 if index.is_global() { + // F822 if self.settings.rules.enabled(Rule::UndefinedExport) { + if !self.path.ends_with("__init__.py") { + if let Some((names, range)) = &all_names { + diagnostics + .extend(pyflakes::rules::undefined_export(names, range, scope)); + } + } + } + + // F405 + if self + .settings + .rules + .enabled(Rule::UndefinedLocalWithImportStarUsage) + { if let Some((names, range)) = &all_names { - diagnostics.extend(pyflakes::rules::undefined_export( - names, range, self.path, scope, - )); + let sources: Vec = scope + .star_imports() + .map(|StarImportation { level, module }| { + helpers::format_import_from(*level, *module) + }) + .sorted() + .dedup() + .collect(); + if !sources.is_empty() { + for &name in names { + if !scope.defines(name) { + diagnostics.push(Diagnostic::new( + pyflakes::rules::UndefinedLocalWithImportStarUsage { + name: name.to_string(), + sources: sources.clone(), + }, + *range, + )); + } + } + } } } } @@ -4876,7 +4890,6 @@ impl<'a> Checker<'a> { BindingKind::Importation(..) | BindingKind::FromImportation(..) | BindingKind::SubmoduleImportation(..) - | BindingKind::StarImportation(..) | BindingKind::FutureImportation ) { if binding.used() { @@ -4907,39 +4920,6 @@ impl<'a> Checker<'a> { } } - if self - .settings - .rules - .enabled(Rule::UndefinedLocalWithImportStarUsage) - { - if scope.import_starred { - if let Some((names, range)) = &all_names { - let mut from_list = vec![]; - for binding in scope.binding_ids().map(|index| &self.ctx.bindings[*index]) { - if let BindingKind::StarImportation(StarImportation { level, module }) = - &binding.kind - { - from_list - .push(helpers::format_import_from(*level, module.as_deref())); - } - } - from_list.sort(); - - for &name in names { - if !scope.defines(name) { - diagnostics.push(Diagnostic::new( - pyflakes::rules::UndefinedLocalWithImportStarUsage { - name: name.to_string(), - sources: from_list.clone(), - }, - *range, - )); - } - } - } - } - } - if enforce_typing_imports { let runtime_imports: Vec<&Binding> = if self.settings.flake8_type_checking.strict { vec![] diff --git a/crates/ruff/src/rules/pandas_vet/rules/check_attr.rs b/crates/ruff/src/rules/pandas_vet/rules/check_attr.rs index 25bd01a719..aee0237b9b 100644 --- a/crates/ruff/src/rules/pandas_vet/rules/check_attr.rs +++ b/crates/ruff/src/rules/pandas_vet/rules/check_attr.rs @@ -82,7 +82,6 @@ pub fn check_attr(checker: &mut Checker, attr: &str, value: &Expr, attr_expr: &E | BindingKind::FunctionDefinition | BindingKind::Export(..) | BindingKind::FutureImportation - | BindingKind::StarImportation(..) | BindingKind::Importation(..) | BindingKind::FromImportation(..) | BindingKind::SubmoduleImportation(..) diff --git a/crates/ruff/src/rules/pandas_vet/rules/check_call.rs b/crates/ruff/src/rules/pandas_vet/rules/check_call.rs index abc3ffd54f..4c90fc6cc9 100644 --- a/crates/ruff/src/rules/pandas_vet/rules/check_call.rs +++ b/crates/ruff/src/rules/pandas_vet/rules/check_call.rs @@ -99,7 +99,6 @@ pub fn check_call(checker: &mut Checker, func: &Expr) { | BindingKind::FunctionDefinition | BindingKind::Export(..) | BindingKind::FutureImportation - | BindingKind::StarImportation(..) | BindingKind::Importation(..) | BindingKind::FromImportation(..) | BindingKind::SubmoduleImportation(..) diff --git a/crates/ruff/src/rules/pyflakes/rules/undefined_export.rs b/crates/ruff/src/rules/pyflakes/rules/undefined_export.rs index 1cfa2709d9..a4ad0520f4 100644 --- a/crates/ruff/src/rules/pyflakes/rules/undefined_export.rs +++ b/crates/ruff/src/rules/pyflakes/rules/undefined_export.rs @@ -1,5 +1,3 @@ -use std::path::Path; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::scope::Scope; @@ -19,14 +17,9 @@ impl Violation for UndefinedExport { } /// F822 -pub fn undefined_export( - names: &[&str], - range: &Range, - path: &Path, - scope: &Scope, -) -> Vec { +pub fn undefined_export(names: &[&str], range: &Range, scope: &Scope) -> Vec { let mut diagnostics = Vec::new(); - if !scope.import_starred && !path.ends_with("__init__.py") { + if !scope.uses_star_imports() { for name in names { if !scope.defines(name) { diagnostics.push(Diagnostic::new( diff --git a/crates/ruff_python_ast/src/scope.rs b/crates/ruff_python_ast/src/scope.rs index b5b1fc1496..4d5893f8ff 100644 --- a/crates/ruff_python_ast/src/scope.rs +++ b/crates/ruff_python_ast/src/scope.rs @@ -9,8 +9,10 @@ use std::ops::{Deref, Index, IndexMut}; pub struct Scope<'a> { pub id: ScopeId, pub kind: ScopeKind<'a>, - pub import_starred: bool, pub uses_locals: bool, + /// A list of star imports in this scope. These represent _module_ imports (e.g., `sys` in + /// `from sys import *`), rather than individual bindings (e.g., individual members in `sys`). + star_imports: Vec>, /// A map from bound name to binding index, for live bindings. bindings: FxHashMap<&'a str, BindingId>, /// A map from bound name to binding index, for bindings that were created @@ -28,8 +30,8 @@ impl<'a> Scope<'a> { Scope { id, kind, - import_starred: false, uses_locals: false, + star_imports: Vec::default(), bindings: FxHashMap::default(), rebounds: FxHashMap::default(), } @@ -60,9 +62,25 @@ impl<'a> Scope<'a> { self.bindings.values() } + /// Returns a tuple of the name and id of all bindings defined in this scope. pub fn bindings(&self) -> std::collections::hash_map::Iter<&'a str, BindingId> { self.bindings.iter() } + + /// Adds a reference to a star import (e.g., `from sys import *`) to this scope. + pub fn add_star_import(&mut self, import: StarImportation<'a>) { + self.star_imports.push(import); + } + + /// Returns `true` if this scope contains a star import (e.g., `from sys import *`). + pub fn uses_star_imports(&self) -> bool { + !self.star_imports.is_empty() + } + + /// Returns an iterator over all star imports (e.g., `from sys import *`) in this scope. + pub fn star_imports(&self) -> impl Iterator> { + self.star_imports.iter() + } } /// Id uniquely identifying a scope in a program. @@ -286,7 +304,6 @@ impl<'a> Binding<'a> { | BindingKind::FunctionDefinition | BindingKind::Builtin | BindingKind::FutureImportation - | BindingKind::StarImportation(..) | BindingKind::Importation(..) | BindingKind::FromImportation(..) | BindingKind::SubmoduleImportation(..) @@ -340,9 +357,6 @@ impl<'a> Binding<'a> { BindingKind::FutureImportation => { return false; } - BindingKind::StarImportation(..) => { - return false; - } _ => {} } existing.is_definition() @@ -379,6 +393,14 @@ impl TryFrom for BindingId { impl nohash_hasher::IsEnabled for BindingId {} +#[derive(Debug, Clone)] +pub struct StarImportation<'a> { + /// The level of the import. `None` or `Some(0)` indicate an absolute import. + pub level: Option, + /// The module being imported. `None` indicates a wildcard import. + pub module: Option<&'a str>, +} + // Pyflakes defines the following binding hierarchy (via inheritance): // Binding // ExportBinding @@ -393,7 +415,6 @@ impl nohash_hasher::IsEnabled for BindingId {} // Importation // SubmoduleImportation // ImportationFrom -// StarImportation // FutureImportation #[derive(Clone, Debug)] @@ -402,14 +423,6 @@ pub struct Export { pub names: Vec, } -#[derive(Clone, Debug)] -pub struct StarImportation { - /// The level of the import. `None` or `Some(0)` indicate an absolute import. - pub level: Option, - /// The module being imported. `None` indicates a wildcard import. - pub module: Option, -} - #[derive(Clone, Debug)] pub struct Importation<'a> { /// The name to which the import is bound. @@ -458,7 +471,6 @@ pub enum BindingKind<'a> { FunctionDefinition, Export(Export), FutureImportation, - StarImportation(StarImportation), Importation(Importation<'a>), FromImportation(FromImportation<'a>), SubmoduleImportation(SubmoduleImportation<'a>),