diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 694ff08a87e61..1967f0a79ce1b 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -1426,6 +1426,16 @@ impl<'a> Visitor<'a> for Checker<'a> { } } } + Expr::Attribute(ast::ExprAttribute { + value: _, + range: _, + ctx, + attr: _, + }) => { + if ctx == &ExprContext::Load { + self.handle_attribute_load(expr); + } + } Expr::Name(ast::ExprName { id, ctx, range: _ }) => match ctx { ExprContext::Load => self.handle_node_load(expr), ExprContext::Store => self.handle_node_store(id, expr), @@ -2414,6 +2424,13 @@ impl<'a> Checker<'a> { self.semantic.resolve_load(expr); } + fn handle_attribute_load(&mut self, expr: &Expr) { + let Expr::Attribute(expr) = expr else { + return; + }; + self.semantic.resolve_attribute_load(expr); + } + fn handle_node_store(&mut self, id: &'a str, expr: &Expr) { let parent = self.semantic.current_statement(); diff --git a/crates/ruff_linter/src/rules/pyflakes/mod.rs b/crates/ruff_linter/src/rules/pyflakes/mod.rs index ce2162e73d7fd..ffd50bf540f3f 100644 --- a/crates/ruff_linter/src/rules/pyflakes/mod.rs +++ b/crates/ruff_linter/src/rules/pyflakes/mod.rs @@ -2694,7 +2694,7 @@ lambda: fu import foo.baz as foo foo ", - &[Rule::RedefinedWhileUnused], + &[Rule::UnusedImport, Rule::RedefinedWhileUnused], ); } @@ -2744,13 +2744,13 @@ lambda: fu #[test] fn unused_package_with_submodule_import() { - // When a package and its submodule are imported, only report once. + // When a package and its submodule are imported, report both. flakes( r" import fu import fu.bar ", - &[Rule::UnusedImport], + &[Rule::UnusedImport, Rule::UnusedImport], ); } diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/undefined_local.rs b/crates/ruff_linter/src/rules/pyflakes/rules/undefined_local.rs index 7a01716e90dea..25093d9ea0f37 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/undefined_local.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/undefined_local.rs @@ -2,6 +2,7 @@ use std::string::ToString; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_python_semantic::BindingKind::SubmoduleImport; use ruff_python_semantic::{Scope, ScopeId}; use ruff_text_size::Ranged; @@ -56,7 +57,12 @@ pub(crate) fn undefined_local(checker: &Checker, scope_id: ScopeId, scope: &Scop if let Some(range) = shadowed.references().find_map(|reference_id| { let reference = checker.semantic().reference(reference_id); if reference.scope_id() == scope_id { - Some(reference.range()) + // FIXME: ignore submodules + if let SubmoduleImport(..) = shadowed.kind { + None + } else { + Some(reference.range()) + } } else { None } diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs index d996fdedc6ed0..1f97bac5f9efa 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs @@ -1,8 +1,9 @@ use std::borrow::Cow; use std::iter; +use unicode_normalization::UnicodeNormalization; use anyhow::{Result, anyhow, bail}; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashSet}; use ruff_diagnostics::{Applicability, Diagnostic, Fix, FixAvailability, Violation}; use ruff_macros::{ViolationMetadata, derive_message_formats}; @@ -292,76 +293,96 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope) { // Collect all unused imports by statement. let mut unused: BTreeMap<(NodeId, Exceptions), Vec> = BTreeMap::default(); let mut ignored: BTreeMap<(NodeId, Exceptions), Vec> = BTreeMap::default(); + let mut already_checked_imports: HashSet<&str> = HashSet::new(); for binding_id in scope.binding_ids() { - let binding = checker.semantic().binding(binding_id); + let top_binding = checker.semantic().binding(binding_id); - if binding.is_used() - || binding.is_explicit_export() - || binding.is_nonlocal() - || binding.is_global() - { - continue; - } - - let Some(import) = binding.as_any_import() else { + let Some(_) = top_binding.as_any_import() else { continue; }; - let Some(node_id) = binding.source else { - continue; - }; - - let name = binding.name(checker.source()); + let binding_name = top_binding + .name(checker.source()) + .split('.') + .next() + .unwrap_or(""); - // If an import is marked as required, avoid treating it as unused, regardless of whether - // it was _actually_ used. - if checker - .settings - .isort - .required_imports - .iter() - .any(|required_import| required_import.matches(name, &import)) + for binding in scope + .get_all(&binding_name.nfkc().collect::()) + .map(|binding_id| checker.semantic().binding(binding_id)) { - continue; - } + let name = binding.name(checker.source()); - // If an import was marked as allowed, avoid treating it as unused. - if checker - .settings - .pyflakes - .allowed_unused_imports - .iter() - .any(|allowed_unused_import| { - let allowed_unused_import = QualifiedName::from_dotted_name(allowed_unused_import); - import.qualified_name().starts_with(&allowed_unused_import) - }) - { - continue; - } + if already_checked_imports.contains(&name) { + continue; + } + { + already_checked_imports.insert(name); + } - let import = ImportBinding { - name, - import, - range: binding.range(), - parent_range: binding.parent_range(checker.semantic()), - scope: binding.scope, - }; + if binding.is_used() + || binding.is_explicit_export() + || binding.is_nonlocal() + || binding.is_global() + { + continue; + } - if checker.rule_is_ignored(Rule::UnusedImport, import.start()) - || import.parent_range.is_some_and(|parent_range| { - checker.rule_is_ignored(Rule::UnusedImport, parent_range.start()) - }) - { - ignored - .entry((node_id, binding.exceptions)) - .or_default() - .push(import); - } else { - unused - .entry((node_id, binding.exceptions)) - .or_default() - .push(import); + let Some(import) = binding.as_any_import() else { + continue; + }; + + let Some(stmt_id) = binding.source else { + continue; + }; + + // If an import is marked as required, avoid treating it as unused, regardless of whether + // it was _actually_ used. + if checker + .settings + .isort + .required_imports + .iter() + .any(|required_import| required_import.matches(name, &import)) + { + continue; + } + + // If an import was marked as allowed, avoid treating it as unused. + if checker.settings.pyflakes.allowed_unused_imports.iter().any( + |allowed_unused_import| { + let allowed_unused_import = + QualifiedName::from_dotted_name(allowed_unused_import); + import.qualified_name().starts_with(&allowed_unused_import) + }, + ) { + continue; + } + + let import = ImportBinding { + name, + import, + range: binding.range(), + parent_range: binding.parent_range(checker.semantic()), + scope: binding.scope, + }; + + if checker.rule_is_ignored(Rule::UnusedImport, import.start()) + || import.parent_range.is_some_and(|parent_range| { + checker.rule_is_ignored(Rule::UnusedImport, parent_range.start()) + }) + { + ignored + .entry((stmt_id, binding.exceptions)) + .or_default() + .push(import); + } else { + unused + .entry((stmt_id, binding.exceptions)) + .or_default() + .push(import); + } } } diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_0.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_0.py.snap index 386a6e2892afb..879fe8013b676 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_0.py.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_0.py.snap @@ -39,6 +39,46 @@ F401_0.py:6:5: F401 [*] `collections.OrderedDict` imported but unused 8 7 | ) 9 8 | import multiprocessing.pool +F401_0.py:10:8: F401 [*] `multiprocessing.process` imported but unused + | + 8 | ) + 9 | import multiprocessing.pool +10 | import multiprocessing.process + | ^^^^^^^^^^^^^^^^^^^^^^^ F401 +11 | import logging.config +12 | import logging.handlers + | + = help: Remove unused import: `multiprocessing.process` + +ℹ Safe fix +7 7 | namedtuple, +8 8 | ) +9 9 | import multiprocessing.pool +10 |-import multiprocessing.process +11 10 | import logging.config +12 11 | import logging.handlers +13 12 | from typing import ( + +F401_0.py:11:8: F401 [*] `logging.config` imported but unused + | + 9 | import multiprocessing.pool +10 | import multiprocessing.process +11 | import logging.config + | ^^^^^^^^^^^^^^ F401 +12 | import logging.handlers +13 | from typing import ( + | + = help: Remove unused import: `logging.config` + +ℹ Safe fix +8 8 | ) +9 9 | import multiprocessing.pool +10 10 | import multiprocessing.process +11 |-import logging.config +12 11 | import logging.handlers +13 12 | from typing import ( +14 13 | TYPE_CHECKING, + F401_0.py:12:8: F401 [*] `logging.handlers` imported but unused | 10 | import multiprocessing.process diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 76ef4a8617ce0..73be405167492 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::path::Path; use bitflags::bitflags; @@ -5,7 +6,7 @@ use rustc_hash::FxHashMap; use ruff_python_ast::helpers::from_relative_import; use ruff_python_ast::name::{QualifiedName, UnqualifiedName}; -use ruff_python_ast::{self as ast, Expr, ExprContext, PySourceType, Stmt}; +use ruff_python_ast::{self as ast, Expr, ExprContext, ExprName, PySourceType, Stmt}; use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::Imported; @@ -367,10 +368,252 @@ impl<'a> SemanticModel<'a> { } } + fn resolve_binding( + &mut self, + binding_id: BindingId, + name_expr: &ExprName, + scope_id: ScopeId, + ) -> Option { + let reference_id = self.resolved_references.push( + self.scope_id, + self.node_id, + ExprContext::Load, + self.flags, + name_expr.range, + ); + + self.bindings[binding_id].references.push(reference_id); + + if let Some(binding_id) = + self.resolve_submodule(name_expr.id.as_str(), scope_id, binding_id) + { + let reference_id = self.resolved_references.push( + self.scope_id, + self.node_id, + ExprContext::Load, + self.flags, + name_expr.range, + ); + self.bindings[binding_id].references.push(reference_id); + } + + match self.bindings[binding_id].kind { + // If it's a type annotation, don't treat it as resolved. For example, given: + // + // ```python + // name: str + // print(name) + // ``` + // + // The `name` in `print(name)` should be treated as unresolved, but the `name` in + // `name: str` should be treated as used. + // + // Stub files are an exception. In a stub file, it _is_ considered valid to + // resolve to a type annotation. + BindingKind::Annotation if !self.in_stub_file() => None, + + // If it's a deletion, don't treat it as resolved, since the name is now + // unbound. For example, given: + // + // ```python + // x = 1 + // del x + // print(x) + // ``` + // + // The `x` in `print(x)` should be treated as unresolved. + // + // Similarly, given: + // + // ```python + // try: + // pass + // except ValueError as x: + // pass + // + // print(x) + // + // The `x` in `print(x)` should be treated as unresolved. + BindingKind::Deletion | BindingKind::UnboundException(None) => { + self.unresolved_references.push( + name_expr.range, + self.exceptions(), + UnresolvedReferenceFlags::empty(), + ); + Some(ReadResult::UnboundLocal(binding_id)) + } + + BindingKind::ConditionalDeletion(binding_id) => { + self.unresolved_references.push( + name_expr.range, + self.exceptions(), + UnresolvedReferenceFlags::empty(), + ); + Some(ReadResult::UnboundLocal(binding_id)) + } + + // If we hit an unbound exception that shadowed a bound name, resole to the + // bound name. For example, given: + // + // ```python + // x = 1 + // + // try: + // pass + // except ValueError as x: + // pass + // + // print(x) + // ``` + // + // The `x` in `print(x)` should resolve to the `x` in `x = 1`. + BindingKind::UnboundException(Some(binding_id)) => { + // Mark the binding as used. + let reference_id = self.resolved_references.push( + self.scope_id, + self.node_id, + ExprContext::Load, + self.flags, + name_expr.range, + ); + self.bindings[binding_id].references.push(reference_id); + + // Mark any submodule aliases as used. + if let Some(binding_id) = + self.resolve_submodule(name_expr.id.as_str(), scope_id, binding_id) + { + let reference_id = self.resolved_references.push( + self.scope_id, + self.node_id, + ExprContext::Load, + self.flags, + name_expr.range, + ); + self.bindings[binding_id].references.push(reference_id); + } + + self.resolved_names.insert(name_expr.into(), binding_id); + Some(ReadResult::Resolved(binding_id)) + } + + BindingKind::Global(Some(binding_id)) | BindingKind::Nonlocal(binding_id, _) => { + // Mark the shadowed binding as used. + let reference_id = self.resolved_references.push( + self.scope_id, + self.node_id, + ExprContext::Load, + self.flags, + name_expr.range, + ); + self.bindings[binding_id].references.push(reference_id); + + // Treat it as resolved. + self.resolved_names.insert(name_expr.into(), binding_id); + Some(ReadResult::Resolved(binding_id)) + } + + _ => { + // Otherwise, treat it as resolved. + self.resolved_names.insert(name_expr.into(), binding_id); + Some(ReadResult::Resolved(binding_id)) + } + } + } + + /// Resolve a `load` reference to an [`ast::ExprAttribute`]. + pub fn resolve_attribute_load(&mut self, attribute: &ast::ExprAttribute) -> ReadResult { + let mut name_segments = vec![attribute.attr.id.as_str()]; + let mut current_expr = &*attribute.value; + let mut result = None; + let mut is_name_exist = false; + let mut already_checked_imports: HashSet = HashSet::new(); + + while let Expr::Attribute(expr_attr) = current_expr { + name_segments.push(expr_attr.attr.id.as_str()); + current_expr = &expr_attr.value; + } + + let name_expr = if let Expr::Name(ref expr_name) = current_expr { + name_segments.push(expr_name.id.as_str()); + Some(expr_name) + } else { + return ReadResult::NotFound; + }; + + name_segments.reverse(); + let full_name = name_segments.join("."); + + let binding_ids: Vec<_> = self + .scopes + .ancestor_ids(self.scope_id) + .flat_map(|scope_id| { + self.scopes[scope_id] + .get_all(name_expr.unwrap().id.as_str()) + .into_iter() + .map(move |binding_id| (binding_id, scope_id)) + }) + .collect(); + + for (binding_id, scope_id) in &binding_ids { + if let BindingKind::SubmoduleImport(binding_kind) = &self.binding(*binding_id).kind { + if binding_kind.qualified_name.to_string() == full_name { + if let Some(result) = + self.resolve_binding(*binding_id, name_expr.unwrap(), *scope_id) + { + return result; + } + } + } + if let BindingKind::Import(_) = &self.binding(*binding_id).kind { + is_name_exist = true; + } + } + + // TODO: need to move the block implementation to resolve_load, but carefully + // start check module import + for (binding_id, scope_id) in &binding_ids { + let Some(import) = self.binding(*binding_id).as_any_import() else { + continue; + }; + let name = &import + .qualified_name() + .to_string() + .split('.') + .next() + .unwrap_or("") + .to_owned(); + + match self.bindings[*binding_id].kind { + BindingKind::SubmoduleImport(_) if !is_name_exist => continue, + BindingKind::WithItemVar => continue, + BindingKind::SubmoduleImport(_) => { + result = self.resolve_binding(*binding_id, name_expr.unwrap(), *scope_id); + } + BindingKind::Import(_) => { + if already_checked_imports.contains(&name.to_string()) { + continue; + } + already_checked_imports.insert(name.to_string()); + + result = self.resolve_binding(*binding_id, name_expr.unwrap(), *scope_id); + } + _ => {} + } + } + // end check module import + + if let Some(result) = result { + result + } else { + ReadResult::NotFound + } + } + /// Resolve a `load` reference to an [`ast::ExprName`]. pub fn resolve_load(&mut self, name: &ast::ExprName) -> ReadResult { // PEP 563 indicates that if a forward reference can be resolved in the module scope, we // should prefer it over local resolutions. + if self.in_forward_reference() { if let Some(binding_id) = self.scopes.global().get(name.id.as_str()) { if !self.bindings[binding_id].is_unbound() { @@ -407,9 +650,17 @@ impl<'a> SemanticModel<'a> { let mut seen_function = false; let mut import_starred = false; let mut class_variables_visible = true; - for (index, scope_id) in self.scopes.ancestor_ids(self.scope_id).enumerate() { + + let ancestor_scope_ids: Vec<_> = self.scopes.ancestor_ids(self.scope_id).collect(); + + for (index, scope_id) in ancestor_scope_ids.into_iter().enumerate() { let scope = &self.scopes[scope_id]; - if scope.kind.is_class() { + let is_class = scope.kind.is_class(); + let is_function = scope.kind.is_function(); + let uses_star_imports = scope.uses_star_imports(); + let mut is_name = false; + + if is_class { // Allow usages of `__class__` within methods, e.g.: // // ```python @@ -445,154 +696,35 @@ impl<'a> SemanticModel<'a> { class_variables_visible = scope.kind.is_type() && index == 0; if let Some(binding_id) = scope.get(name.id.as_str()) { - // Mark the binding as used. - let reference_id = self.resolved_references.push( - self.scope_id, - self.node_id, - ExprContext::Load, - self.flags, - name.range, - ); - self.bindings[binding_id].references.push(reference_id); - - // Mark any submodule aliases as used. - if let Some(binding_id) = - self.resolve_submodule(name.id.as_str(), scope_id, binding_id) - { - let reference_id = self.resolved_references.push( - self.scope_id, - self.node_id, - ExprContext::Load, - self.flags, - name.range, - ); - self.bindings[binding_id].references.push(reference_id); - } - - match self.bindings[binding_id].kind { - // If it's a type annotation, don't treat it as resolved. For example, given: - // - // ```python - // name: str - // print(name) - // ``` - // - // The `name` in `print(name)` should be treated as unresolved, but the `name` in - // `name: str` should be treated as used. - // - // Stub files are an exception. In a stub file, it _is_ considered valid to - // resolve to a type annotation. - BindingKind::Annotation if !self.in_stub_file() => continue, - - // If it's a deletion, don't treat it as resolved, since the name is now - // unbound. For example, given: - // - // ```python - // x = 1 - // del x - // print(x) - // ``` - // - // The `x` in `print(x)` should be treated as unresolved. - // - // Similarly, given: - // - // ```python - // try: - // pass - // except ValueError as x: - // pass - // - // print(x) - // - // The `x` in `print(x)` should be treated as unresolved. - BindingKind::Deletion | BindingKind::UnboundException(None) => { - self.unresolved_references.push( - name.range, - self.exceptions(), - UnresolvedReferenceFlags::empty(), - ); - return ReadResult::UnboundLocal(binding_id); - } - - BindingKind::ConditionalDeletion(binding_id) => { - self.unresolved_references.push( - name.range, - self.exceptions(), - UnresolvedReferenceFlags::empty(), - ); - return ReadResult::UnboundLocal(binding_id); + // Return solved if there is at least one import with a submodule + for temp_binding_id in scope.get_all(name.id.as_str()) { + if let BindingKind::Import(_) = &self.bindings[temp_binding_id].kind { + is_name = true; } + } - // If we hit an unbound exception that shadowed a bound name, resole to the - // bound name. For example, given: - // - // ```python - // x = 1 - // - // try: - // pass - // except ValueError as x: - // pass - // - // print(x) - // ``` - // - // The `x` in `print(x)` should resolve to the `x` in `x = 1`. - BindingKind::UnboundException(Some(binding_id)) => { - // Mark the binding as used. - let reference_id = self.resolved_references.push( - self.scope_id, - self.node_id, - ExprContext::Load, - self.flags, - name.range, - ); - self.bindings[binding_id].references.push(reference_id); - - // Mark any submodule aliases as used. - if let Some(binding_id) = - self.resolve_submodule(name.id.as_str(), scope_id, binding_id) - { - let reference_id = self.resolved_references.push( - self.scope_id, - self.node_id, - ExprContext::Load, - self.flags, - name.range, - ); - self.bindings[binding_id].references.push(reference_id); + // Todo: Move the implementation here + for temp_binding_id in scope.get_all(name.id.as_str()) { + if let BindingKind::SubmoduleImport(_) = &self.bindings[temp_binding_id].kind { + if !is_name { + return ReadResult::NotFound; } - self.resolved_names.insert(name.into(), binding_id); - return ReadResult::Resolved(binding_id); - } - - BindingKind::Global(Some(binding_id)) - | BindingKind::Nonlocal(binding_id, _) => { - // Mark the shadowed binding as used. - let reference_id = self.resolved_references.push( - self.scope_id, - self.node_id, - ExprContext::Load, - self.flags, - name.range, - ); - self.bindings[binding_id].references.push(reference_id); - - // Treat it as resolved. - self.resolved_names.insert(name.into(), binding_id); - return ReadResult::Resolved(binding_id); + for reference_id in self.bindings[temp_binding_id].references() { + if self.resolved_references[reference_id] + .range() + .contains_range(self.bindings[temp_binding_id].range) + { + return ReadResult::Resolved(temp_binding_id); + } + } } + } - _ => { - // Otherwise, treat it as resolved. - self.resolved_names.insert(name.into(), binding_id); - return ReadResult::Resolved(binding_id); - } + if let Some(res) = self.resolve_binding(binding_id, name, scope_id) { + return res; } } - // Allow usages of `__module__` and `__qualname__` within class scopes, e.g.: // // ```python @@ -608,14 +740,14 @@ impl<'a> SemanticModel<'a> { // __qualname__ = "Bar" // print(__qualname__) // ``` - if index == 0 && scope.kind.is_class() { + if index == 0 && is_class { if matches!(name.id.as_str(), "__module__" | "__qualname__") { return ReadResult::ImplicitGlobal; } } - seen_function |= scope.kind.is_function(); - import_starred = import_starred || scope.uses_star_imports(); + seen_function |= is_function; + import_starred = import_starred || uses_star_imports; } if import_starred { @@ -928,6 +1060,13 @@ impl<'a> SemanticModel<'a> { return None; } + if !submodule + .qualified_name() + .starts_with(import.qualified_name()) + { + return None; + } + Some(binding_id) }