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`.
This commit is contained in:
Charlie Marsh 2023-07-18 20:59:02 -04:00 committed by GitHub
parent 626d8dc2cc
commit 1181d25e5a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 86 additions and 81 deletions

View file

@ -4776,7 +4776,11 @@ impl<'a> Checker<'a> {
/// Run any lint rules that operate over a single [`Binding`]. /// Run any lint rules that operate over a single [`Binding`].
fn check_bindings(&mut self) { fn check_bindings(&mut self) {
if !self.any_enabled(&[Rule::UnusedVariable]) { if !self.any_enabled(&[
Rule::UnusedVariable,
Rule::UnconventionalImportAlias,
Rule::UnaliasedCollectionsAbcSetImport,
]) {
return; return;
} }
@ -4802,6 +4806,27 @@ impl<'a> Checker<'a> {
self.diagnostics.push(diagnostic); 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::TypingOnlyFirstPartyImport,
Rule::TypingOnlyStandardLibraryImport, Rule::TypingOnlyStandardLibraryImport,
Rule::TypingOnlyThirdPartyImport, Rule::TypingOnlyThirdPartyImport,
Rule::UnaliasedCollectionsAbcSetImport,
Rule::UnconventionalImportAlias,
Rule::UndefinedExport, Rule::UndefinedExport,
Rule::UndefinedLocalWithImportStarUsage, Rule::UndefinedLocalWithImportStarUsage,
Rule::UndefinedLocalWithImportStarUsage, Rule::UndefinedLocalWithImportStarUsage,
@ -5115,23 +5138,6 @@ impl<'a> Checker<'a> {
if self.enabled(Rule::UnusedImport) { if self.enabled(Rule::UnusedImport) {
pyflakes::rules::unused_import(self, scope, &mut diagnostics); 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); self.diagnostics.extend(diagnostics);
} }

View file

@ -2,7 +2,7 @@ use rustc_hash::FxHashMap;
use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation}; use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation};
use ruff_macros::{derive_message_formats, 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::checkers::ast::Checker;
use crate::registry::AsRule; use crate::registry::AsRule;
@ -53,42 +53,38 @@ impl Violation for UnconventionalImportAlias {
/// ICN001 /// ICN001
pub(crate) fn unconventional_import_alias( pub(crate) fn unconventional_import_alias(
checker: &Checker, checker: &Checker,
scope: &Scope, binding: &Binding,
diagnostics: &mut Vec<Diagnostic>,
conventions: &FxHashMap<String, String>, conventions: &FxHashMap<String, String>,
) -> Option<Diagnostic> { ) -> Option<Diagnostic> {
for (name, binding_id) in scope.all_bindings() { let Some(qualified_name) = binding.qualified_name() else {
let binding = checker.semantic().binding(binding_id); return None;
};
let Some(qualified_name) = binding.qualified_name() else { let Some(expected_alias) = conventions.get(qualified_name) else {
continue; return None;
}; };
let Some(expected_alias) = conventions.get(qualified_name) else { let name = binding.name(checker.locator);
continue; if binding.is_alias() && name == expected_alias {
}; return None;
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);
} }
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)
} }

View file

@ -1,6 +1,6 @@
use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation}; use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation};
use ruff_macros::{derive_message_formats, 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::checkers::ast::Checker;
use crate::registry::AsRule; use crate::registry::AsRule;
@ -48,31 +48,29 @@ impl Violation for UnaliasedCollectionsAbcSetImport {
/// PYI025 /// PYI025
pub(crate) fn unaliased_collections_abc_set_import( pub(crate) fn unaliased_collections_abc_set_import(
checker: &Checker, checker: &Checker,
scope: &Scope, binding: &Binding,
diagnostics: &mut Vec<Diagnostic>, ) -> Option<Diagnostic> {
) { let BindingKind::FromImport(FromImport { qualified_name }) = &binding.kind else {
for (name, binding_id) in scope.all_bindings() { return None;
let binding = checker.semantic().binding(binding_id); };
let BindingKind::FromImport(FromImport { qualified_name }) = &binding.kind else { if qualified_name.as_str() != "collections.abc.Set" {
continue; return None;
};
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);
} }
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)
} }

View file

@ -17,6 +17,8 @@ use crate::ScopeId;
pub struct Binding<'a> { pub struct Binding<'a> {
pub kind: BindingKind<'a>, pub kind: BindingKind<'a>,
pub range: TextRange, 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. /// The context in which the [`Binding`] was created.
pub context: ExecutionContext, pub context: ExecutionContext,
/// The statement in which the [`Binding`] was defined. /// The statement in which the [`Binding`] was defined.

View file

@ -189,6 +189,7 @@ impl<'a> SemanticModel<'a> {
self.bindings.push(Binding { self.bindings.push(Binding {
range: TextRange::default(), range: TextRange::default(),
kind: BindingKind::Builtin, kind: BindingKind::Builtin,
scope: ScopeId::global(),
references: Vec::new(), references: Vec::new(),
flags: BindingFlags::empty(), flags: BindingFlags::empty(),
source: None, source: None,
@ -209,6 +210,7 @@ impl<'a> SemanticModel<'a> {
kind, kind,
flags, flags,
references: Vec::new(), references: Vec::new(),
scope: self.scope_id,
source: self.stmt_id, source: self.stmt_id,
context: self.execution_context(), context: self.execution_context(),
exceptions: self.exceptions(), exceptions: self.exceptions(),
@ -795,6 +797,7 @@ impl<'a> SemanticModel<'a> {
kind: BindingKind::Assignment, kind: BindingKind::Assignment,
range: *range, range: *range,
references: Vec::new(), references: Vec::new(),
scope: self.scope_id,
source: self.stmt_id, source: self.stmt_id,
context: self.execution_context(), context: self.execution_context(),
exceptions: self.exceptions(), exceptions: self.exceptions(),