From 616f65f193348860602ce29810f5285d237db6e1 Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Mon, 5 Oct 2026 18:59:06 -0400 Subject: [PATCH] linter: cleanup some allocations --- .../src/rules/ban_drop_column.rs | 10 +++--- .../src/rules/ban_drop_constraint.rs | 10 +++--- .../rules/ban_duplicate_column_assignments.rs | 16 ++++----- .../src/rules/changing_column_type.rs | 10 +++--- .../src/rules/prefer_robust_stmts.rs | 27 ++++++++++----- .../src/rules/require_timeout_settings.rs | 34 ++++++++----------- 6 files changed, 54 insertions(+), 53 deletions(-) diff --git a/crates/squawk_linter/src/rules/ban_drop_column.rs b/crates/squawk_linter/src/rules/ban_drop_column.rs index 1c23a97d..a59e8fd6 100644 --- a/crates/squawk_linter/src/rules/ban_drop_column.rs +++ b/crates/squawk_linter/src/rules/ban_drop_column.rs @@ -8,9 +8,9 @@ use crate::{Linter, Rule, Violation}; pub(crate) fn ban_drop_column(ctx: &mut Linter, parse: &Parse) { let file = parse.tree(); for stmt in file.stmts() { - let actions: Vec<_> = match stmt { - ast::Stmt::AlterTable(table) => table.actions().collect(), - ast::Stmt::AlterForeignTable(table) => table.actions().collect(), + let actions = match stmt { + ast::Stmt::AlterTable(table) => table.actions(), + ast::Stmt::AlterForeignTable(table) => table.actions(), ast::Stmt::AlterType(ty) => { if let Some(ast::AlterTypeAction::AlterTypeAttributeActionList(list)) = ty.action() { @@ -24,9 +24,9 @@ pub(crate) fn ban_drop_column(ctx: &mut Linter, parse: &Parse) { } } } - Vec::new() + continue; } - _ => Vec::new(), + _ => continue, }; for action in actions { if let ast::AlterTableAction::DropColumn(drop_column) = action { diff --git a/crates/squawk_linter/src/rules/ban_drop_constraint.rs b/crates/squawk_linter/src/rules/ban_drop_constraint.rs index 42fb19cc..40a1f475 100644 --- a/crates/squawk_linter/src/rules/ban_drop_constraint.rs +++ b/crates/squawk_linter/src/rules/ban_drop_constraint.rs @@ -6,12 +6,12 @@ use squawk_syntax::{ pub(crate) fn ban_drop_constraint(ctx: &mut Linter, parse: &Parse) { for stmt in parse.tree().stmts() { - let actions: Vec<_> = match &stmt { - ast::Stmt::AlterTable(table) => table.actions().collect(), - ast::Stmt::AlterForeignTable(table) => table.actions().collect(), - _ => Vec::new(), + let actions = match &stmt { + ast::Stmt::AlterTable(table) => Some(table.actions()), + ast::Stmt::AlterForeignTable(table) => Some(table.actions()), + _ => None, }; - for action in actions { + for action in actions.into_iter().flatten() { match action { ast::AlterTableAction::DropConstraint(node) => ctx.report(Violation::for_node( Rule::BanDropConstraint, diff --git a/crates/squawk_linter/src/rules/ban_duplicate_column_assignments.rs b/crates/squawk_linter/src/rules/ban_duplicate_column_assignments.rs index 98158a4f..258a44b0 100644 --- a/crates/squawk_linter/src/rules/ban_duplicate_column_assignments.rs +++ b/crates/squawk_linter/src/rules/ban_duplicate_column_assignments.rs @@ -167,11 +167,10 @@ fn check_set_clause(ctx: &mut Linter, set_clause: &ast::SetClause) { let Some(set_column_list) = set_clause.set_column_list() else { return; }; - let set_columns = set_column_list.set_columns().collect::>(); let mut assigned_columns: FxHashMap> = FxHashMap::default(); - for set_column in &set_columns { - match set_column { + for set_column in set_column_list.set_columns() { + match &set_column { ast::SetColumn::SetMultipleColumns(set_multiple_columns) => { let Some(column_target_list) = set_multiple_columns.column_target_list() else { continue; @@ -192,7 +191,7 @@ fn check_set_clause(ctx: &mut Linter, set_clause: &ast::SetClause) { if assignments.len() < 2 || assignments.iter().all(|assignment| assignment.is_partial) { continue; } - let mut fix = create_fix(&name, &assignments, &set_columns); + let mut fix = create_fix(&name, &assignments, &set_column_list); let last_index = assignments.len() - 1; for (index, assignment) in assignments.iter().enumerate() { @@ -232,7 +231,7 @@ fn add_assignment( fn create_fix( name: &Name, assignments: &[Assignment], - set_columns: &[ast::SetColumn], + set_column_list: &ast::SetColumnList, ) -> Option { let mut edits = Vec::with_capacity(assignments.len() - 1); @@ -240,10 +239,9 @@ fn create_fix( if !matches!(assignment.set_column, ast::SetColumn::SetSingleColumn(_)) { return None; } - let index = set_columns - .iter() - .position(|set_column| set_column.syntax() == assignment.set_column.syntax())?; - let next_set_column = set_columns.get(index + 1)?; + let mut set_columns = set_column_list.set_columns(); + set_columns.find(|set_column| set_column.syntax() == assignment.set_column.syntax())?; + let next_set_column = set_columns.next()?; let next_start = next_set_column.syntax().text_range().start(); let mut end = next_start; let mut seen_comma = false; diff --git a/crates/squawk_linter/src/rules/changing_column_type.rs b/crates/squawk_linter/src/rules/changing_column_type.rs index ab8827f0..25417adc 100644 --- a/crates/squawk_linter/src/rules/changing_column_type.rs +++ b/crates/squawk_linter/src/rules/changing_column_type.rs @@ -8,9 +8,9 @@ use crate::{Linter, Rule, Violation}; pub(crate) fn changing_column_type(ctx: &mut Linter, parse: &Parse) { let file = parse.tree(); for stmt in file.stmts() { - let actions: Vec<_> = match stmt { - ast::Stmt::AlterTable(table) => table.actions().collect(), - ast::Stmt::AlterForeignTable(table) => table.actions().collect(), + let actions = match stmt { + ast::Stmt::AlterTable(table) => table.actions(), + ast::Stmt::AlterForeignTable(table) => table.actions(), ast::Stmt::AlterType(ty) => { if let Some(ast::AlterTypeAction::AlterTypeAttributeActionList(list)) = ty.action() { @@ -24,9 +24,9 @@ pub(crate) fn changing_column_type(ctx: &mut Linter, parse: &Parse) } } } - Vec::new() + continue; } - _ => Vec::new(), + _ => continue, }; for action in actions { if let ast::AlterTableAction::AlterColumn(alter_column) = action { diff --git a/crates/squawk_linter/src/rules/prefer_robust_stmts.rs b/crates/squawk_linter/src/rules/prefer_robust_stmts.rs index 8efe4ff4..4d8e8eb1 100644 --- a/crates/squawk_linter/src/rules/prefer_robust_stmts.rs +++ b/crates/squawk_linter/src/rules/prefer_robust_stmts.rs @@ -22,18 +22,28 @@ fn is_alembic_version_update(stmt: &ast::Stmt) -> bool { .is_some_and(|name| name.text().eq_ignore_ascii_case("alembic_version"))) } -fn alembic_version_commit_tx_indices(stmts: &[ast::Stmt]) -> Option> { +fn alembic_version_commit_tx_indices( + stmts: impl Iterator, +) -> Option> { let mut indices = FxHashSet::default(); let mut tx_start = None; - for (i, stmt) in stmts.iter().enumerate() { - match stmt { - ast::Stmt::Begin(_) => tx_start = Some(i), + let mut has_alembic_version_update = false; + for (i, stmt) in stmts.enumerate() { + match &stmt { + ast::Stmt::Begin(_) => { + tx_start = Some(i); + has_alembic_version_update = false; + } ast::Stmt::Commit(_) | ast::Stmt::Rollback(_) => { if let Some(start) = tx_start.take() - && stmts[start..=i].iter().any(is_alembic_version_update) + && has_alembic_version_update { indices.extend(start..=i); } + has_alembic_version_update = false; + } + _ if tx_start.is_some() && is_alembic_version_update(&stmt) => { + has_alembic_version_update = true; } _ => {} } @@ -43,11 +53,10 @@ fn alembic_version_commit_tx_indices(stmts: &[ast::Stmt]) -> Option) { let file = parse.tree(); - let stmts: Vec = file.stmts().collect(); let mut inside_transaction = ctx.settings.assume_in_transaction; let mut constraint_names: FxHashMap = FxHashMap::default(); - let version_tx_indices = alembic_version_commit_tx_indices(&stmts); + let version_tx_indices = alembic_version_commit_tx_indices(file.stmts()); enum ActionErrorMessage { IfExists, @@ -55,13 +64,13 @@ pub(crate) fn prefer_robust_stmts(ctx: &mut Linter, parse: &Parse) { None, } - for (i, stmt) in stmts.iter().enumerate() { + for (i, stmt) in file.stmts().enumerate() { let in_robust_tx = match &version_tx_indices { Some(set) => ctx.settings.assume_in_transaction || set.contains(&i), None => inside_transaction, }; - match stmt { + match &stmt { ast::Stmt::Begin(_) => { inside_transaction = true; } diff --git a/crates/squawk_linter/src/rules/require_timeout_settings.rs b/crates/squawk_linter/src/rules/require_timeout_settings.rs index 96ff4ac3..c892b0c7 100644 --- a/crates/squawk_linter/src/rules/require_timeout_settings.rs +++ b/crates/squawk_linter/src/rules/require_timeout_settings.rs @@ -62,18 +62,17 @@ impl LockImpact { Self(blocks) } - fn blocked(self) -> Vec<&'static str> { - let mut items = vec![]; - for (bit, label) in [ - (Self::READS, "reads"), - (Self::WRITES, "writes"), - (Self::SCHEMA_CHANGES, "schema changes"), - ] { - if self.0 & bit != 0 { - items.push(label); - } + fn blocked(self) -> &'static str { + match self.0 { + 0b001 => "reads", + 0b010 => "writes", + 0b011 => "reads, writes", + 0b100 => "schema changes", + 0b101 => "reads, schema changes", + 0b110 => "writes, schema changes", + 0b111 => "reads, writes, schema changes", + _ => "", } - items } } @@ -274,11 +273,10 @@ impl LockKind { } fn violation_message(self) -> String { - let name = self.to_string(); - if name.is_empty() { + if self == LockKind::Unknown { "Missing `set lock_timeout` before potentially slow operations".to_string() } else { - format!("Missing `set lock_timeout` before potentially slow {name} lock operations") + format!("Missing `set lock_timeout` before potentially slow {self} lock operations") } } @@ -300,19 +298,15 @@ impl LockKind { fn help(self) -> String { let help = "Configure a `lock_timeout` before this statement."; - let name = self.to_string(); let Some(impact) = self.impact() else { return help.to_string(); }; let blocked = impact.blocked(); if blocked.is_empty() { - format!("{help} Statement requires: {name} lock.") + format!("{help} Statement requires: {self} lock.") } else { - format!( - "{help} Statement requires: {name} lock; blocking: {}.", - blocked.join(", ") - ) + format!("{help} Statement requires: {self} lock; blocking: {blocked}.") } } }