From 1181d25e5a5192ef2374b272f4fb8e24aa5e568f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 18 Jul 2023 20:59:02 -0400 Subject: [PATCH] Move a few more candidate rules to the deferred `Binding`-only pass (#5853) ## Summary No behavior change, but this is in theory more efficient, since we can just iterate over the flat `Binding` vector rather than having to iterate over binding chains via the `Scope`. --- crates/ruff/src/checkers/ast/mod.rs | 46 +++++++------ .../rules/unconventional_import_alias.rs | 64 +++++++++---------- .../unaliased_collections_abc_set_import.rs | 52 ++++++++------- crates/ruff_python_semantic/src/binding.rs | 2 + crates/ruff_python_semantic/src/model.rs | 3 + 5 files changed, 86 insertions(+), 81 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index c8077d7ab9..a628befeb7 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -4776,7 +4776,11 @@ impl<'a> Checker<'a> { /// Run any lint rules that operate over a single [`Binding`]. fn check_bindings(&mut self) { - if !self.any_enabled(&[Rule::UnusedVariable]) { + if !self.any_enabled(&[ + Rule::UnusedVariable, + Rule::UnconventionalImportAlias, + Rule::UnaliasedCollectionsAbcSetImport, + ]) { return; } @@ -4802,6 +4806,27 @@ impl<'a> Checker<'a> { self.diagnostics.push(diagnostic); } } + + if self.enabled(Rule::UnconventionalImportAlias) { + if let Some(diagnostic) = + flake8_import_conventions::rules::unconventional_import_alias( + self, + binding, + &self.settings.flake8_import_conventions.aliases, + ) + { + self.diagnostics.push(diagnostic); + } + } + if self.is_stub { + if self.enabled(Rule::UnaliasedCollectionsAbcSetImport) { + if let Some(diagnostic) = + flake8_pyi::rules::unaliased_collections_abc_set_import(self, binding) + { + self.diagnostics.push(diagnostic); + } + } + } } } @@ -4814,8 +4839,6 @@ impl<'a> Checker<'a> { Rule::TypingOnlyFirstPartyImport, Rule::TypingOnlyStandardLibraryImport, Rule::TypingOnlyThirdPartyImport, - Rule::UnaliasedCollectionsAbcSetImport, - Rule::UnconventionalImportAlias, Rule::UndefinedExport, Rule::UndefinedLocalWithImportStarUsage, Rule::UndefinedLocalWithImportStarUsage, @@ -5115,23 +5138,6 @@ impl<'a> Checker<'a> { if self.enabled(Rule::UnusedImport) { pyflakes::rules::unused_import(self, scope, &mut diagnostics); } - if self.enabled(Rule::UnconventionalImportAlias) { - flake8_import_conventions::rules::unconventional_import_alias( - self, - scope, - &mut diagnostics, - &self.settings.flake8_import_conventions.aliases, - ); - } - if self.is_stub { - if self.enabled(Rule::UnaliasedCollectionsAbcSetImport) { - flake8_pyi::rules::unaliased_collections_abc_set_import( - self, - scope, - &mut diagnostics, - ); - } - } } self.diagnostics.extend(diagnostics); } diff --git a/crates/ruff/src/rules/flake8_import_conventions/rules/unconventional_import_alias.rs b/crates/ruff/src/rules/flake8_import_conventions/rules/unconventional_import_alias.rs index dc5876dac3..4ee7dcbb0d 100644 --- a/crates/ruff/src/rules/flake8_import_conventions/rules/unconventional_import_alias.rs +++ b/crates/ruff/src/rules/flake8_import_conventions/rules/unconventional_import_alias.rs @@ -2,7 +2,7 @@ use rustc_hash::FxHashMap; use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_semantic::Scope; +use ruff_python_semantic::Binding; use crate::checkers::ast::Checker; use crate::registry::AsRule; @@ -53,42 +53,38 @@ impl Violation for UnconventionalImportAlias { /// ICN001 pub(crate) fn unconventional_import_alias( checker: &Checker, - scope: &Scope, - diagnostics: &mut Vec, + binding: &Binding, conventions: &FxHashMap, ) -> Option { - for (name, binding_id) in scope.all_bindings() { - let binding = checker.semantic().binding(binding_id); + let Some(qualified_name) = binding.qualified_name() else { + return None; + }; - let Some(qualified_name) = binding.qualified_name() else { - continue; - }; + let Some(expected_alias) = conventions.get(qualified_name) else { + return None; + }; - let Some(expected_alias) = conventions.get(qualified_name) else { - continue; - }; - - if binding.is_alias() && name == expected_alias { - continue; - } - - let mut diagnostic = Diagnostic::new( - UnconventionalImportAlias { - name: qualified_name.to_string(), - asname: expected_alias.to_string(), - }, - binding.range, - ); - if checker.patch(diagnostic.kind.rule()) { - if checker.semantic().is_available(expected_alias) { - diagnostic.try_set_fix(|| { - let (edit, rest) = - Renamer::rename(name, expected_alias, scope, checker.semantic())?; - Ok(Fix::suggested_edits(edit, rest)) - }); - } - } - diagnostics.push(diagnostic); + let name = binding.name(checker.locator); + if binding.is_alias() && name == expected_alias { + return None; } - None + + let mut diagnostic = Diagnostic::new( + UnconventionalImportAlias { + name: qualified_name.to_string(), + asname: expected_alias.to_string(), + }, + binding.range, + ); + if checker.patch(diagnostic.kind.rule()) { + if checker.semantic().is_available(expected_alias) { + diagnostic.try_set_fix(|| { + let scope = &checker.semantic().scopes[binding.scope]; + let (edit, rest) = + Renamer::rename(name, expected_alias, scope, checker.semantic())?; + Ok(Fix::suggested_edits(edit, rest)) + }); + } + } + Some(diagnostic) } diff --git a/crates/ruff/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs b/crates/ruff/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs index 5fb0a3f5f6..6ebd42e3a9 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs @@ -1,6 +1,6 @@ use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_semantic::{BindingKind, FromImport, Scope}; +use ruff_python_semantic::{Binding, BindingKind, FromImport}; use crate::checkers::ast::Checker; use crate::registry::AsRule; @@ -48,31 +48,29 @@ impl Violation for UnaliasedCollectionsAbcSetImport { /// PYI025 pub(crate) fn unaliased_collections_abc_set_import( checker: &Checker, - scope: &Scope, - diagnostics: &mut Vec, -) { - for (name, binding_id) in scope.all_bindings() { - let binding = checker.semantic().binding(binding_id); - let BindingKind::FromImport(FromImport { qualified_name }) = &binding.kind else { - continue; - }; - if qualified_name.as_str() != "collections.abc.Set" { - continue; - } - if name == "AbstractSet" { - continue; - } - - let mut diagnostic = Diagnostic::new(UnaliasedCollectionsAbcSetImport, binding.range); - if checker.patch(diagnostic.kind.rule()) { - if checker.semantic().is_available("AbstractSet") { - diagnostic.try_set_fix(|| { - let (edit, rest) = - Renamer::rename(name, "AbstractSet", scope, checker.semantic())?; - Ok(Fix::suggested_edits(edit, rest)) - }); - } - } - diagnostics.push(diagnostic); + binding: &Binding, +) -> Option { + let BindingKind::FromImport(FromImport { qualified_name }) = &binding.kind else { + return None; + }; + if qualified_name.as_str() != "collections.abc.Set" { + return None; } + + let name = binding.name(checker.locator); + if name == "AbstractSet" { + return None; + } + + let mut diagnostic = Diagnostic::new(UnaliasedCollectionsAbcSetImport, binding.range); + if checker.patch(diagnostic.kind.rule()) { + if checker.semantic().is_available("AbstractSet") { + diagnostic.try_set_fix(|| { + let scope = &checker.semantic().scopes[binding.scope]; + let (edit, rest) = Renamer::rename(name, "AbstractSet", scope, checker.semantic())?; + Ok(Fix::suggested_edits(edit, rest)) + }); + } + } + Some(diagnostic) } diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index f66da2a80a..45002b8700 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -17,6 +17,8 @@ use crate::ScopeId; pub struct Binding<'a> { pub kind: BindingKind<'a>, pub range: TextRange, + /// The [`ScopeId`] of the scope in which the [`Binding`] was defined. + pub scope: ScopeId, /// The context in which the [`Binding`] was created. pub context: ExecutionContext, /// The statement in which the [`Binding`] was defined. diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 0106dfcfc0..d1aba8460d 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -189,6 +189,7 @@ impl<'a> SemanticModel<'a> { self.bindings.push(Binding { range: TextRange::default(), kind: BindingKind::Builtin, + scope: ScopeId::global(), references: Vec::new(), flags: BindingFlags::empty(), source: None, @@ -209,6 +210,7 @@ impl<'a> SemanticModel<'a> { kind, flags, references: Vec::new(), + scope: self.scope_id, source: self.stmt_id, context: self.execution_context(), exceptions: self.exceptions(), @@ -795,6 +797,7 @@ impl<'a> SemanticModel<'a> { kind: BindingKind::Assignment, range: *range, references: Vec::new(), + scope: self.scope_id, source: self.stmt_id, context: self.execution_context(), exceptions: self.exceptions(),