From 61d3977f95498f90f7ffe96b661adc9de495f7c7 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 7 Aug 2023 11:02:14 -0400 Subject: [PATCH] Make the `statement` vector private on `SemanticModel` (#6348) ## Summary Instead, expose these as methods, now that we can use a reasonable nomenclature on the API. --- .../checkers/ast/analyze/deferred_scopes.rs | 12 +++++---- crates/ruff/src/checkers/ast/mod.rs | 2 +- .../rules/unused_private_type_definition.rs | 8 +++--- .../runtime_import_in_type_checking_block.rs | 8 +++--- .../rules/typing_only_runtime_import.rs | 8 +++--- .../perflint/rules/unnecessary_list_cast.rs | 2 +- .../src/rules/pyflakes/rules/unused_import.rs | 7 +++--- .../rules/pyflakes/rules/unused_variable.rs | 6 ++--- crates/ruff_python_semantic/src/binding.rs | 2 +- crates/ruff_python_semantic/src/model.rs | 25 ++++++++++++++++++- 10 files changed, 55 insertions(+), 25 deletions(-) diff --git a/crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs index b9c6e56601..bcb655a16b 100644 --- a/crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs @@ -115,7 +115,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { branch_detection::different_forks( left, right, - &checker.semantic.statements, + checker.semantic.statements(), ) }) }) { @@ -172,12 +172,14 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { continue; } + let Some(source) = shadowed.source else { + continue; + }; + // If this is an overloaded function, abort. if shadowed.kind.is_function_definition() && visibility::is_overload( - cast::decorator_list( - checker.semantic.statements[shadowed.source.unwrap()], - ), + cast::decorator_list(checker.semantic.statement(source)), &checker.semantic, ) { @@ -202,7 +204,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { branch_detection::different_forks( left, right, - &checker.semantic.statements, + checker.semantic.statements(), ) }) }) { diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index e604047ccf..4b6972ead1 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -202,7 +202,7 @@ impl<'a> Checker<'a> { /// thus be applied whenever we delete a statement, but can otherwise be omitted. pub(crate) fn isolation(&self, parent: Option<&Stmt>) -> IsolationLevel { parent - .and_then(|stmt| self.semantic.statements.node_id(stmt)) + .and_then(|stmt| self.semantic.statement_id(stmt)) .map_or(IsolationLevel::default(), |node_id| { IsolationLevel::Group(node_id.into()) }) diff --git a/crates/ruff/src/rules/flake8_pyi/rules/unused_private_type_definition.rs b/crates/ruff/src/rules/flake8_pyi/rules/unused_private_type_definition.rs index 4d90d77371..481aa88671 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/unused_private_type_definition.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/unused_private_type_definition.rs @@ -174,7 +174,7 @@ pub(crate) fn unused_private_type_var( continue; }; let Stmt::Assign(ast::StmtAssign { targets, value, .. }) = - checker.semantic().statements[source] + checker.semantic().statement(source) else { continue; }; @@ -218,7 +218,7 @@ pub(crate) fn unused_private_protocol( continue; }; - let Stmt::ClassDef(class_def) = checker.semantic().statements[source] else { + let Stmt::ClassDef(class_def) = checker.semantic().statement(source) else { continue; }; @@ -261,7 +261,7 @@ pub(crate) fn unused_private_type_alias( }; let Stmt::AnnAssign(ast::StmtAnnAssign { target, annotation, .. - }) = checker.semantic().statements[source] + }) = checker.semantic().statement(source) else { continue; }; @@ -305,7 +305,7 @@ pub(crate) fn unused_private_typed_dict( let Some(source) = binding.source else { continue; }; - let Stmt::ClassDef(class_def) = checker.semantic().statements[source] else { + let Stmt::ClassDef(class_def) = checker.semantic().statement(source) else { continue; }; 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 5fc6722cf0..e351b3b5fb 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 @@ -193,11 +193,13 @@ struct ImportBinding<'a> { /// Generate a [`Fix`] to remove runtime imports from a type-checking block. fn fix_imports(checker: &Checker, statement_id: NodeId, imports: &[ImportBinding]) -> Result { - let statement = checker.semantic().statements[statement_id]; - let parent = checker.semantic().statements.parent(statement); + let statement = checker.semantic().statement(statement_id); + let parent = checker.semantic().parent_statement(statement); + let member_names: Vec> = imports .iter() - .map(|ImportBinding { import, .. }| import.member_name()) + .map(|ImportBinding { import, .. }| import) + .map(Imported::member_name) .collect(); // Find the first reference across all imports. 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 15a7f658c2..821bfa9ed4 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 @@ -403,11 +403,13 @@ fn is_exempt(name: &str, exempt_modules: &[&str]) -> bool { /// Generate a [`Fix`] to remove typing-only imports from a runtime context. fn fix_imports(checker: &Checker, statement_id: NodeId, imports: &[ImportBinding]) -> Result { - let statement = checker.semantic().statements[statement_id]; - let parent = checker.semantic().statements.parent(statement); + let statement = checker.semantic().statement(statement_id); + let parent = checker.semantic().parent_statement(statement); + let member_names: Vec> = imports .iter() - .map(|ImportBinding { import, .. }| import.member_name()) + .map(|ImportBinding { import, .. }| import) + .map(Imported::member_name) .collect(); // Find the first reference across all imports. diff --git a/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs b/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs index 67551e0988..94385fca3b 100644 --- a/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs +++ b/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs @@ -107,7 +107,7 @@ pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr) { let binding = checker.semantic().binding(binding_id); if binding.kind.is_assignment() || binding.kind.is_named_expr_assignment() { if let Some(parent_id) = binding.source { - let parent = checker.semantic().statements[parent_id]; + let parent = checker.semantic().statement(parent_id); if let Stmt::Assign(ast::StmtAssign { value, .. }) | Stmt::AnnAssign(ast::StmtAnnAssign { value: Some(value), .. diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs index 22d39f3e23..580167bde3 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs @@ -226,12 +226,13 @@ struct ImportBinding<'a> { /// Generate a [`Fix`] to remove unused imports from a statement. fn fix_imports(checker: &Checker, statement_id: NodeId, imports: &[ImportBinding]) -> Result { - let statement = checker.semantic().statements[statement_id]; - let parent = checker.semantic().statements.parent(statement); + let statement = checker.semantic().statement(statement_id); + let parent = checker.semantic().parent_statement(statement); let member_names: Vec> = imports .iter() - .map(|ImportBinding { import, .. }| import.member_name()) + .map(|ImportBinding { import, .. }| import) + .map(Imported::member_name) .collect(); let edit = autofix::edits::remove_unused_imports( diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs index 911582c29e..69c44529af 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs @@ -326,9 +326,9 @@ pub(crate) fn unused_variable(checker: &Checker, scope: &Scope, diagnostics: &mu let mut diagnostic = Diagnostic::new(UnusedVariable { name }, range); if checker.patch(diagnostic.kind.rule()) { if let Some(source) = source { - let stmt = checker.semantic().statements[source]; - let parent = checker.semantic().statements.parent(stmt); - if let Some(fix) = remove_unused_variable(stmt, parent, range, checker) { + let statement = checker.semantic().statement(source); + let parent = checker.semantic().parent_statement(statement); + if let Some(fix) = remove_unused_variable(statement, parent, range, checker) { diagnostic.set_fix(fix); } } diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index ef06cce239..ecbba9c3d7 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -185,7 +185,7 @@ impl<'a> Binding<'a> { /// Returns the range of the binding's parent. pub fn parent_range(&self, semantic: &SemanticModel) -> Option { self.source - .map(|node_id| semantic.statements[node_id]) + .map(|statement_id| semantic.statement(statement_id)) .and_then(|parent| { if parent.is_import_from_stmt() { Some(parent.range()) diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 5238269da3..e1c1ebdfaf 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -31,7 +31,7 @@ pub struct SemanticModel<'a> { module_path: Option<&'a [String]>, /// Stack of all visited statements. - pub statements: Nodes<'a, Stmt>, + statements: Nodes<'a, Stmt>, /// The identifier of the current statement. statement_id: Option, @@ -919,6 +919,29 @@ impl<'a> SemanticModel<'a> { None } + /// Return the [`Nodes`] vector of all statements. + pub const fn statements(&self) -> &Nodes<'a, Stmt> { + &self.statements + } + + /// Return the [`NodeId`] corresponding to the given [`Stmt`]. + #[inline] + pub fn statement_id(&self, statement: &Stmt) -> Option { + self.statements.node_id(statement) + } + + /// Return the [`Stmt]` corresponding to the given [`NodeId`]. + #[inline] + pub fn statement(&self, statement_id: NodeId) -> &'a Stmt { + self.statements[statement_id] + } + + /// Given a [`Stmt`], return its parent, if any. + #[inline] + pub fn parent_statement(&self, statement: &'a Stmt) -> Option<&'a Stmt> { + self.statements.parent(statement) + } + /// Set the [`Globals`] for the current [`Scope`]. pub fn set_globals(&mut self, globals: Globals<'a>) { // If any global bindings don't already exist in the global scope, add them.