From 26b1dd0ca24dcbbc27b5cdcd7b673ce9c379d7e8 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 2 Jun 2023 23:02:47 -0400 Subject: [PATCH] Remove `name` field from import binding kinds (#4817) --- crates/ruff/src/checkers/ast/mod.rs | 8 +- .../runtime_import_in_type_checking_block.rs | 6 +- .../rules/typing_only_runtime_import.rs | 39 +++----- crates/ruff/src/rules/pandas_vet/helpers.rs | 8 +- .../pandas_vet/rules/inplace_argument.rs | 3 +- .../src/rules/pyflakes/rules/unused_import.rs | 6 +- crates/ruff_python_semantic/src/binding.rs | 46 ++++------ crates/ruff_python_semantic/src/model.rs | 92 +++++++++---------- 8 files changed, 88 insertions(+), 120 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 8b3746d307..c5e547eb93 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -872,7 +872,6 @@ where name, Binding { kind: BindingKind::SubmoduleImportation(SubmoduleImportation { - name, full_name, }), range: alias.range(), @@ -889,7 +888,7 @@ where self.add_binding( name, Binding { - kind: BindingKind::Importation(Importation { name, full_name }), + kind: BindingKind::Importation(Importation { full_name }), range: alias.range(), references: Vec::new(), source: self.semantic_model.stmt_id, @@ -1196,10 +1195,7 @@ where self.add_binding( name, Binding { - kind: BindingKind::FromImportation(FromImportation { - name, - full_name, - }), + kind: BindingKind::FromImportation(FromImportation { full_name }), range: alias.range(), references: Vec::new(), source: self.semantic_model.stmt_id, diff --git a/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs b/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs index 82f839664b..61586fdf75 100644 --- a/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs +++ b/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs @@ -67,9 +67,9 @@ pub(crate) fn runtime_import_in_type_checking_block( diagnostics: &mut Vec, ) { let full_name = match &binding.kind { - BindingKind::Importation(Importation { full_name, .. }) => full_name, - BindingKind::FromImportation(FromImportation { full_name, .. }) => full_name.as_str(), - BindingKind::SubmoduleImportation(SubmoduleImportation { full_name, .. }) => full_name, + BindingKind::Importation(Importation { full_name }) => full_name, + BindingKind::FromImportation(FromImportation { full_name }) => full_name.as_str(), + BindingKind::SubmoduleImportation(SubmoduleImportation { full_name }) => full_name, _ => return, }; diff --git a/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index 3b26aadd0b..8abcb73943 100644 --- a/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs @@ -183,61 +183,50 @@ fn is_implicit_import(this: &Binding, that: &Binding) -> bool { match &this.kind { BindingKind::Importation(Importation { full_name: this_name, - .. }) | BindingKind::SubmoduleImportation(SubmoduleImportation { - name: this_name, .. + full_name: this_name, }) => match &that.kind { BindingKind::FromImportation(FromImportation { full_name: that_name, - .. }) => { // Ex) `pkg.A` vs. `pkg` - this_name + let this_name = this_name.split('.').next().unwrap_or(this_name); + that_name .rfind('.') - .map_or(false, |i| this_name[..i] == *that_name) + .map_or(false, |i| that_name[..i] == *this_name) } BindingKind::Importation(Importation { full_name: that_name, - .. }) | BindingKind::SubmoduleImportation(SubmoduleImportation { - name: that_name, .. + full_name: that_name, }) => { // Submodule importation with an alias (`import pkg.A as B`) // are represented as `Importation`. - match (this_name.find('.'), that_name.find('.')) { - // Ex) `pkg.A` vs. `pkg.B` - (Some(i), Some(j)) => this_name[..i] == that_name[..j], - // Ex) `pkg.A` vs. `pkg` - (Some(i), None) => this_name[..i] == **that_name, - // Ex) `pkg` vs. `pkg.B` - (None, Some(j)) => **this_name == that_name[..j], - // Ex) `pkg` vs. `pkg` - (None, None) => this_name == that_name, - } + let this_name = this_name.split('.').next().unwrap_or(this_name); + let that_name = that_name.split('.').next().unwrap_or(that_name); + this_name == that_name } _ => false, }, BindingKind::FromImportation(FromImportation { full_name: this_name, - .. }) => match &that.kind { BindingKind::Importation(Importation { full_name: that_name, - .. }) | BindingKind::SubmoduleImportation(SubmoduleImportation { - name: that_name, .. + full_name: that_name, }) => { // Ex) `pkg.A` vs. `pkg` + let that_name = that_name.split('.').next().unwrap_or(that_name); this_name .rfind('.') - .map_or(false, |i| &this_name[..i] == *that_name) + .map_or(false, |i| &this_name[..i] == that_name) } BindingKind::FromImportation(FromImportation { full_name: that_name, - .. }) => { // Ex) `pkg.A` vs. `pkg.B` this_name.rfind('.').map_or(false, |i| { @@ -286,9 +275,9 @@ pub(crate) fn typing_only_runtime_import( } let full_name = match &binding.kind { - BindingKind::Importation(Importation { full_name, .. }) => full_name, - BindingKind::FromImportation(FromImportation { full_name, .. }) => full_name.as_str(), - BindingKind::SubmoduleImportation(SubmoduleImportation { full_name, .. }) => full_name, + BindingKind::Importation(Importation { full_name }) => full_name, + BindingKind::FromImportation(FromImportation { full_name }) => full_name.as_str(), + BindingKind::SubmoduleImportation(SubmoduleImportation { full_name }) => full_name, _ => return, }; diff --git a/crates/ruff/src/rules/pandas_vet/helpers.rs b/crates/ruff/src/rules/pandas_vet/helpers.rs index 6783a960dd..1ec93a6d80 100644 --- a/crates/ruff/src/rules/pandas_vet/helpers.rs +++ b/crates/ruff/src/rules/pandas_vet/helpers.rs @@ -40,9 +40,11 @@ pub(crate) fn test_expression(expr: &Expr, model: &SemanticModel) -> Resolution | BindingKind::LoopVar | BindingKind::Global | BindingKind::Nonlocal => Resolution::RelevantLocal, - BindingKind::Importation(Importation { - full_name: module, .. - }) if module == "pandas" => Resolution::PandasModule, + BindingKind::Importation(Importation { full_name: module }) + if module == "pandas" => + { + Resolution::PandasModule + } _ => Resolution::IrrelevantBinding, } }) diff --git a/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs b/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs index 4785b5866c..9295bd6b62 100644 --- a/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs +++ b/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs @@ -71,8 +71,7 @@ pub(crate) fn inplace_argument( matches!( binding.kind, BindingKind::Importation(Importation { - full_name: "pandas", - .. + full_name: "pandas" }) ) }); diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs index 0e1fd4a4e4..acb5e2c6d9 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs @@ -118,9 +118,9 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut } let full_name = match &binding.kind { - BindingKind::Importation(Importation { full_name, .. }) => full_name, - BindingKind::FromImportation(FromImportation { full_name, .. }) => full_name.as_str(), - BindingKind::SubmoduleImportation(SubmoduleImportation { full_name, .. }) => full_name, + BindingKind::Importation(Importation { full_name }) => full_name, + BindingKind::FromImportation(FromImportation { full_name }) => full_name.as_str(), + BindingKind::SubmoduleImportation(SubmoduleImportation { full_name }) => full_name, _ => continue, }; diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 740b898eea..a384c54bc4 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -48,39 +48,34 @@ impl<'a> Binding<'a> { /// Return `true` if this binding redefines the given binding. pub fn redefines(&self, existing: &'a Binding) -> bool { match &self.kind { - BindingKind::Importation(Importation { full_name, .. }) => { + BindingKind::Importation(Importation { full_name }) => { if let BindingKind::SubmoduleImportation(SubmoduleImportation { full_name: existing, - .. }) = &existing.kind { return full_name == existing; } } - BindingKind::FromImportation(FromImportation { full_name, .. }) => { + BindingKind::FromImportation(FromImportation { full_name }) => { if let BindingKind::SubmoduleImportation(SubmoduleImportation { full_name: existing, - .. }) = &existing.kind { return full_name == existing; } } - BindingKind::SubmoduleImportation(SubmoduleImportation { full_name, .. }) => { + BindingKind::SubmoduleImportation(SubmoduleImportation { full_name }) => { match &existing.kind { BindingKind::Importation(Importation { full_name: existing, - .. }) | BindingKind::SubmoduleImportation(SubmoduleImportation { full_name: existing, - .. }) => { return full_name == existing; } BindingKind::FromImportation(FromImportation { full_name: existing, - .. }) => { return full_name == existing; } @@ -211,37 +206,34 @@ pub struct Export<'a> { pub names: Vec<&'a str>, } +/// A binding for an `import`, keyed on the name to which the import is bound. +/// Ex) `import foo` would be keyed on "foo". +/// Ex) `import foo as bar` would be keyed on "bar". #[derive(Clone, Debug)] pub struct Importation<'a> { - /// The name to which the import is bound. - /// Given `import foo`, `name` would be "foo". - /// Given `import foo as bar`, `name` would be "bar". - pub name: &'a str, /// The full name of the module being imported. - /// Given `import foo`, `full_name` would be "foo". - /// Given `import foo as bar`, `full_name` would be "foo". + /// Ex) Given `import foo`, `full_name` would be "foo". + /// Ex) Given `import foo as bar`, `full_name` would be "foo". pub full_name: &'a str, } +/// A binding for a member imported from a module, keyed on the name to which the member is bound. +/// Ex) `from foo import bar` would be keyed on "bar". +/// Ex) `from foo import bar as baz` would be keyed on "baz". #[derive(Clone, Debug)] -pub struct FromImportation<'a> { - /// The name to which the import is bound. - /// Given `from foo import bar`, `name` would be "bar". - /// Given `from foo import bar as baz`, `name` would be "baz". - pub name: &'a str, - /// The full name of the module being imported. - /// Given `from foo import bar`, `full_name` would be "foo.bar". - /// Given `from foo import bar as baz`, `full_name` would be "foo.bar". +pub struct FromImportation { + /// The full name of the member being imported. + /// Ex) Given `from foo import bar`, `full_name` would be "foo.bar". + /// Ex) Given `from foo import bar as baz`, `full_name` would be "foo.bar". pub full_name: String, } +/// A binding for a submodule imported from a module, keyed on the name of the parent module. +/// Ex) `import foo.bar` would be keyed on "foo". #[derive(Clone, Debug)] pub struct SubmoduleImportation<'a> { - /// The parent module imported by the submodule import. - /// Given `import foo.bar`, `module` would be "foo". - pub name: &'a str, /// The full name of the submodule being imported. - /// Given `import foo.bar`, `full_name` would be "foo.bar". + /// Ex) Given `import foo.bar`, `full_name` would be "foo.bar". pub full_name: &'a str, } @@ -261,7 +253,7 @@ pub enum BindingKind<'a> { Export(Export<'a>), FutureImportation, Importation(Importation<'a>), - FromImportation(FromImportation<'a>), + FromImportation(FromImportation), SubmoduleImportation(SubmoduleImportation<'a>), } diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 363e8053ed..ccb7a9ea8e 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -145,7 +145,9 @@ impl<'a> SemanticModel<'a> { self.bindings[binding_id].references.push(reference_id); // Mark any submodule aliases as used. - if let Some(binding_id) = self.resolve_submodule(ScopeId::global(), binding_id) { + if let Some(binding_id) = + self.resolve_submodule(symbol, ScopeId::global(), binding_id) + { let reference_id = self.references.push(ScopeId::global(), range, context); self.bindings[binding_id].references.push(reference_id); } @@ -181,7 +183,7 @@ impl<'a> SemanticModel<'a> { self.bindings[binding_id].references.push(reference_id); // Mark any submodule aliases as used. - if let Some(binding_id) = self.resolve_submodule(scope_id, binding_id) { + if let Some(binding_id) = self.resolve_submodule(symbol, scope_id, binding_id) { let reference_id = self.references.push(self.scope_id, range, context); self.bindings[binding_id].references.push(reference_id); } @@ -238,7 +240,12 @@ impl<'a> SemanticModel<'a> { } /// Given a `BindingId`, return the `BindingId` of the submodule import that it aliases. - fn resolve_submodule(&self, scope_id: ScopeId, binding_id: BindingId) -> Option { + fn resolve_submodule( + &self, + symbol: &str, + scope_id: ScopeId, + binding_id: BindingId, + ) -> Option { // If the name of a submodule import is the same as an alias of another import, and the // alias is used, then the submodule import should be marked as used too. // @@ -249,21 +256,17 @@ impl<'a> SemanticModel<'a> { // import pyarrow.csv // print(pa.csv.read_csv("test.csv")) // ``` - let (name, full_name) = match &self.bindings[binding_id].kind { - BindingKind::Importation(Importation { name, full_name }) => (*name, *full_name), - BindingKind::SubmoduleImportation(SubmoduleImportation { name, full_name }) => { - (*name, *full_name) - } - BindingKind::FromImportation(FromImportation { name, full_name }) => { - (*name, full_name.as_str()) - } + let full_name = match &self.bindings[binding_id].kind { + BindingKind::Importation(Importation { full_name }) => *full_name, + BindingKind::SubmoduleImportation(SubmoduleImportation { full_name }) => *full_name, + BindingKind::FromImportation(FromImportation { full_name }) => full_name.as_str(), _ => return None, }; let has_alias = full_name .split('.') .last() - .map(|segment| segment != name) + .map(|segment| segment != symbol) .unwrap_or_default(); if !has_alias { return None; @@ -285,31 +288,18 @@ impl<'a> SemanticModel<'a> { /// /// ...then `resolve_call_path(${python_version})` will resolve to `sys.version_info`. pub fn resolve_call_path(&'a self, value: &'a Expr) -> Option> { - let Some(call_path) = collect_call_path(value) else { - return None; - }; - let Some(head) = call_path.first() else { - return None; - }; - let Some(binding) = self.find_binding(head) else { - return None; - }; + let call_path = collect_call_path(value)?; + let head = call_path.first()?; + let binding = self.find_binding(head)?; match &binding.kind { - BindingKind::Importation(Importation { - full_name: name, .. - }) - | BindingKind::SubmoduleImportation(SubmoduleImportation { name, .. }) => { + BindingKind::Importation(Importation { full_name: name }) => { if name.starts_with('.') { - if let Some(module) = &self.module_path { - let mut source_path = from_relative_import(module, name); - if source_path.is_empty() { - None - } else { - source_path.extend(call_path.into_iter().skip(1)); - Some(source_path) - } - } else { + let mut source_path = from_relative_import(self.module_path?, name); + if source_path.is_empty() { None + } else { + source_path.extend(call_path.into_iter().skip(1)); + Some(source_path) } } else { let mut source_path: CallPath = from_unqualified_name(name); @@ -317,20 +307,20 @@ impl<'a> SemanticModel<'a> { Some(source_path) } } - BindingKind::FromImportation(FromImportation { - full_name: name, .. - }) => { + BindingKind::SubmoduleImportation(SubmoduleImportation { full_name: name }) => { + let name = name.split('.').next().unwrap_or(name); + let mut source_path: CallPath = from_unqualified_name(name); + source_path.extend(call_path.into_iter().skip(1)); + Some(source_path) + } + BindingKind::FromImportation(FromImportation { full_name: name }) => { if name.starts_with('.') { - if let Some(module) = &self.module_path { - let mut source_path = from_relative_import(module, name); - if source_path.is_empty() { - None - } else { - source_path.extend(call_path.into_iter().skip(1)); - Some(source_path) - } - } else { + let mut source_path = from_relative_import(self.module_path?, name); + if source_path.is_empty() { None + } else { + source_path.extend(call_path.into_iter().skip(1)); + Some(source_path) } } else { let mut source_path: CallPath = from_unqualified_name(name); @@ -366,13 +356,13 @@ impl<'a> SemanticModel<'a> { member: &str, ) -> Option { self.scopes().enumerate().find_map(|(scope_index, scope)| { - scope.binding_ids().find_map(|binding_id| { + scope.bindings().find_map(|(name, binding_id)| { let binding = &self.bindings[binding_id]; match &binding.kind { // Ex) Given `module="sys"` and `object="exit"`: // `import sys` -> `sys.exit` // `import sys as sys2` -> `sys2.exit` - BindingKind::Importation(Importation { name, full_name }) => { + BindingKind::Importation(Importation { full_name }) => { if full_name == &module { if let Some(source) = binding.source { // Verify that `sys` isn't bound in an inner scope. @@ -393,7 +383,7 @@ impl<'a> SemanticModel<'a> { // Ex) Given `module="os.path"` and `object="join"`: // `from os.path import join` -> `join` // `from os.path import join as join2` -> `join2` - BindingKind::FromImportation(FromImportation { name, full_name }) => { + BindingKind::FromImportation(FromImportation { full_name }) => { if let Some((target_module, target_member)) = full_name.split_once('.') { if target_module == module && target_member == member { if let Some(source) = binding.source { @@ -415,8 +405,8 @@ impl<'a> SemanticModel<'a> { } // Ex) Given `module="os"` and `object="name"`: // `import os.path ` -> `os.name` - BindingKind::SubmoduleImportation(SubmoduleImportation { name, .. }) => { - if name == &module { + BindingKind::SubmoduleImportation(SubmoduleImportation { .. }) => { + if name == module { if let Some(source) = binding.source { // Verify that `os` isn't bound in an inner scope. if self