diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index ca5afee26ceb7..21fd277794cc5 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -1130,6 +1130,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), @@ -2046,6 +2056,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/flake8_type_checking/mod.rs b/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs index ab95fe3ac56fd..b09455737e2fe 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs @@ -471,6 +471,42 @@ mod tests { ", "tc010_precedence_over_tc008" )] + #[test_case( + r" + from __future__ import annotations + import importlib.abc + from typing import TYPE_CHECKING + if TYPE_CHECKING: + import importlib.machinery + class Foo(importlib.abc.MetaPathFinder): + def bar(self) -> importlib.machinery.ModuleSpec: ... + ", + "github_issue_15723_regression_test" + )] + #[test_case( + r" + from __future__ import annotations + from typing import TYPE_CHECKING + if TYPE_CHECKING: + import importlib.abc + import importlib.machinery + class Foo(importlib.abc.MetaPathFinder): + def bar(self) -> importlib.machinery.ModuleSpec: ... + ", + "github_issue_15723_false_negative" + )] + #[test_case( + r" + from __future__ import annotations + from typing import TYPE_CHECKING + if TYPE_CHECKING: + import importlib.machinery + import importlib.abc + class Foo(importlib.abc.MetaPathFinder): + def bar(self) -> importlib.machinery.ModuleSpec: ... + ", + "github_issue_15723_ideal_import_order" + )] fn contents(contents: &str, snapshot: &str) { let diagnostics = test_snippet( contents, diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs index f5f9114b71d6a..a0407c9dabba1 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs @@ -2,7 +2,7 @@ use std::borrow::Cow; use anyhow::Result; use rustc_hash::FxHashMap; - +use unicode_normalization::UnicodeNormalization; use ruff_diagnostics::{Diagnostic, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_semantic::{Imported, NodeId, Scope}; @@ -106,74 +106,91 @@ pub(crate) fn runtime_import_in_type_checking_block( let mut actions: FxHashMap<(NodeId, Action), Vec> = FxHashMap::default(); for binding_id in scope.binding_ids() { - let binding = checker.semantic().binding(binding_id); + let top_binding = checker.semantic().binding(binding_id); - let Some(import) = binding.as_any_import() else { + let Some(_) = top_binding.as_any_import() else { continue; }; - let Some(reference_id) = binding.references.first().copied() else { + let Some(reference_id) = top_binding.references.first().copied() else { continue; }; - if binding.context.is_typing() - && binding.references().any(|reference_id| { + let binding_name = top_binding + .name(checker.source()) + .split('.') + .next() + .unwrap_or(""); + + // NOTE: It’s necessary to go through all bindings, including shadowed ones, + // using the module name. + for binding in scope + .get_all(&binding_name.nfkc().collect::()) + .map(|binding_id| checker.semantic().binding(binding_id)) + { + if binding.context.is_typing() + && binding.references().any(|reference_id| { checker .semantic() .reference(reference_id) .in_runtime_context() }) - { - let Some(node_id) = binding.source else { - continue; - }; + { + let Some(import) = binding.as_any_import() else { + continue; + }; - let import = ImportBinding { - import, - reference_id, - binding, - range: binding.range(), - parent_range: binding.parent_range(checker.semantic()), - }; + let Some(node_id) = binding.source else { + continue; + }; - if checker.rule_is_ignored(Rule::RuntimeImportInTypeCheckingBlock, import.start()) - || import.parent_range.is_some_and(|parent_range| { + let import = ImportBinding { + import, + reference_id, + binding, + range: binding.range(), + parent_range: binding.parent_range(checker.semantic()), + }; + + if checker.rule_is_ignored(Rule::RuntimeImportInTypeCheckingBlock, import.start()) + || import.parent_range.is_some_and(|parent_range| { checker.rule_is_ignored( Rule::RuntimeImportInTypeCheckingBlock, parent_range.start(), ) }) - { - actions - .entry((node_id, Action::Ignore)) - .or_default() - .push(import); - } else { - // Determine whether the member should be fixed by moving the import out of the - // type-checking block, or by quoting its references. - // TODO: We should check `reference.in_annotated_type_alias()` - // as well to match the behavior of the flake8 plugin - // although maybe the best way forward is to add an - // additional setting to configure whether quoting - // or moving the import is preferred for type aliases - // since some people will consistently use their - // type aliases at runtimes, while others won't, so - // the best solution is unclear. - if checker.settings.flake8_type_checking.quote_annotations - && binding.references().all(|reference_id| { - let reference = checker.semantic().reference(reference_id); - reference.in_typing_context() || reference.in_runtime_evaluated_annotation() - }) { actions - .entry((node_id, Action::Quote)) + .entry((node_id, Action::Ignore)) .or_default() .push(import); } else { - actions - .entry((node_id, Action::Move)) - .or_default() - .push(import); + // Determine whether the member should be fixed by moving the import out of the + // type-checking block, or by quoting its references. + // TODO: We should check `reference.in_annotated_type_alias()` + // as well to match the behavior of the flake8 plugin + // although maybe the best way forward is to add an + // additional setting to configure whether quoting + // or moving the import is preferred for type aliases + // since some people will consistently use their + // type aliases at runtimes, while others won't, so + // the best solution is unclear. + if checker.settings.flake8_type_checking.quote_annotations + && binding.references().all(|reference_id| { + let reference = checker.semantic().reference(reference_id); + reference.in_typing_context() || reference.in_runtime_evaluated_annotation() + }) + { + actions + .entry((node_id, Action::Quote)) + .or_default() + .push(import); + } else { + actions + .entry((node_id, Action::Move)) + .or_default() + .push(import); + } } } } diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__github_issue_15723_false_negative.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__github_issue_15723_false_negative.snap new file mode 100644 index 0000000000000..ae13107db3a1f --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__github_issue_15723_false_negative.snap @@ -0,0 +1,24 @@ +--- +source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +--- +:5:12: TC004 [*] Move import `importlib.abc` out of type-checking block. Import is used for more than type hinting. + | +3 | from typing import TYPE_CHECKING +4 | if TYPE_CHECKING: +5 | import importlib.abc + | ^^^^^^^^^^^^^ TC004 +6 | import importlib.machinery +7 | class Foo(importlib.abc.MetaPathFinder): + | + = help: Move out of type-checking block + +ℹ Unsafe fix +1 1 | +2 2 | from __future__ import annotations +3 3 | from typing import TYPE_CHECKING + 4 |+import importlib.abc +4 5 | if TYPE_CHECKING: +5 |- import importlib.abc +6 6 | import importlib.machinery +7 7 | class Foo(importlib.abc.MetaPathFinder): +8 8 | def bar(self) -> importlib.machinery.ModuleSpec: ... diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__github_issue_15723_ideal_import_order.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__github_issue_15723_ideal_import_order.snap new file mode 100644 index 0000000000000..595576e5fd53c --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__github_issue_15723_ideal_import_order.snap @@ -0,0 +1,24 @@ +--- +source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +--- +:6:12: TC004 [*] Move import `importlib.abc` out of type-checking block. Import is used for more than type hinting. + | +4 | if TYPE_CHECKING: +5 | import importlib.machinery +6 | import importlib.abc + | ^^^^^^^^^^^^^ TC004 +7 | class Foo(importlib.abc.MetaPathFinder): +8 | def bar(self) -> importlib.machinery.ModuleSpec: ... + | + = help: Move out of type-checking block + +ℹ Unsafe fix +1 1 | +2 2 | from __future__ import annotations +3 3 | from typing import TYPE_CHECKING + 4 |+import importlib.abc +4 5 | if TYPE_CHECKING: +5 6 | import importlib.machinery +6 |- import importlib.abc +7 7 | class Foo(importlib.abc.MetaPathFinder): +8 8 | def bar(self) -> importlib.machinery.ModuleSpec: ... diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__github_issue_15723_regression_test.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__github_issue_15723_regression_test.snap new file mode 100644 index 0000000000000..6c5ead27428ce --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__github_issue_15723_regression_test.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +--- + diff --git a/crates/ruff_linter/src/rules/pyflakes/mod.rs b/crates/ruff_linter/src/rules/pyflakes/mod.rs index 7fb0f32673a8d..0c962ffef8702 100644 --- a/crates/ruff_linter/src/rules/pyflakes/mod.rs +++ b/crates/ruff_linter/src/rules/pyflakes/mod.rs @@ -2654,7 +2654,7 @@ lambda: fu import foo.baz as foo foo ", - &[Rule::RedefinedWhileUnused], + &[Rule::UnusedImport, Rule::RedefinedWhileUnused], ); } @@ -2704,13 +2704,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 d13aa29107864..8cea6984088f1 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::{derive_message_formats, ViolationMetadata}; +use ruff_python_semantic::BindingKind::SubmoduleImport; use ruff_python_semantic::{Scope, ScopeId}; use ruff_text_size::Ranged; @@ -61,7 +62,12 @@ pub(crate) fn undefined_local( 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 09e4149b32c98..db2c3d24274a6 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::{anyhow, bail, Result}; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashSet}; use ruff_diagnostics::{Applicability, Diagnostic, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; @@ -276,75 +277,95 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut // 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()), - }; + 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()), + }; + + 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 b77ae8d19f8e3..adf6b6942cb5f 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 @@ -40,6 +40,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 21d8c41f87140..d0d9833b14b00 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::binding::{ @@ -352,10 +353,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() { @@ -392,9 +635,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 @@ -430,154 +681,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 @@ -593,14 +725,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 { @@ -856,6 +988,13 @@ impl<'a> SemanticModel<'a> { return None; } + if !submodule + .qualified_name() + .starts_with(import.qualified_name()) + { + return None; + } + Some(binding_id) }