From b67d4e7adaaa2e2959e1da7679db17f00d349ae1 Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Wed, 20 Aug 2025 20:06:26 -0400 Subject: [PATCH 1/2] linter: fix violations to not include leading trivia previously we'd mark leading comments and whitespace as being part of the violation instead of just the actual SQL nodes --- crates/squawk/src/reporter.rs | 2 +- crates/squawk_linter/src/ignore.rs | 45 ++++++++++++++++--- crates/squawk_linter/src/lib.rs | 31 ++++++++++++- .../src/rules/adding_field_with_default.rs | 8 ++-- .../rules/adding_foreign_key_constraint.rs | 8 ++-- .../src/rules/adding_not_null_field.rs | 4 +- .../rules/adding_primary_key_constraint.rs | 8 ++-- .../src/rules/adding_required_field.rs | 4 +- .../ban_alter_domain_with_add_constraint.rs | 4 +- .../squawk_linter/src/rules/ban_char_field.rs | 8 ++-- ...oncurrent_index_creation_in_transaction.rs | 2 +- .../ban_create_domain_with_constraint.rs | 2 +- .../src/rules/ban_drop_column.rs | 4 +- .../src/rules/ban_drop_database.rs | 4 +- .../src/rules/ban_drop_not_null.rs | 4 +- .../squawk_linter/src/rules/ban_drop_table.rs | 4 +- .../src/rules/ban_truncate_cascade.rs | 2 +- .../src/rules/changing_column_type.rs | 4 +- .../src/rules/constraint_missing_not_valid.rs | 8 ++-- .../src/rules/disallow_unique_constraint.rs | 8 ++-- .../src/rules/prefer_bigint_over_int.rs | 4 +- .../src/rules/prefer_bigint_over_smallint.rs | 4 +- .../src/rules/prefer_identity.rs | 4 +- .../src/rules/prefer_robust_stmts.rs | 28 ++++++------ .../src/rules/prefer_text_field.rs | 4 +- .../src/rules/prefer_timestamptz.rs | 4 +- .../src/rules/renaming_column.rs | 4 +- .../squawk_linter/src/rules/renaming_table.rs | 4 +- .../require_concurrent_index_creation.rs | 4 +- .../require_concurrent_index_deletion.rs | 4 +- ..._robust_stmts__test__create_table_err.snap | 2 +- .../src/rules/transaction_nesting.rs | 32 ++++++------- crates/squawk_linter/src/test_utils.rs | 4 +- crates/squawk_server/src/lib.rs | 2 +- crates/squawk_wasm/src/lib.rs | 2 +- crates/xtask/src/new_rule.rs | 2 +- 36 files changed, 165 insertions(+), 107 deletions(-) diff --git a/crates/squawk/src/reporter.rs b/crates/squawk/src/reporter.rs index 4359894c..bd278fd6 100644 --- a/crates/squawk/src/reporter.rs +++ b/crates/squawk/src/reporter.rs @@ -34,7 +34,7 @@ fn check_sql( linter.settings.assume_in_transaction = assume_in_transaction; let parse = SourceFile::parse(sql); let parse_errors = parse.errors(); - let errors = linter.lint(parse, sql); + let errors = linter.lint(&parse, sql); let line_index = LineIndex::new(sql); let mut violations = Vec::with_capacity(parse_errors.len() + errors.len()); diff --git a/crates/squawk_linter/src/ignore.rs b/crates/squawk_linter/src/ignore.rs index d4c05b03..d8837da1 100644 --- a/crates/squawk_linter/src/ignore.rs +++ b/crates/squawk_linter/src/ignore.rs @@ -98,7 +98,7 @@ pub(crate) fn find_ignores(ctx: &mut Linter, file: &SyntaxNode) { let end = start + TextSize::new(trimmed.len() as u32); let range = TextRange::new(start, end); - ctx.report(Violation::new( + ctx.report(Violation::for_range( Rule::UnusedIgnore, format!("unknown name {trimmed}"), range, @@ -144,6 +144,37 @@ alter table t drop column c cascade; assert!(ignore.violation_names.contains(&Rule::BanDropColumn)); } + #[test] + fn multiple_sql_comments_with_ignore_is_ok() { + let sql = " +-- fooo bar +-- buzz +-- squawk-ignore prefer-robust-stmts +create table x(); + +select 1; +"; + + let parse = squawk_syntax::SourceFile::parse(sql); + let mut linter = Linter::with_all_rules(); + find_ignores(&mut linter, &parse.syntax_node()); + + assert_eq!(linter.ignores.len(), 1); + let ignore = &linter.ignores[0]; + assert!( + ignore.violation_names.contains(&Rule::PreferRobustStmts), + "Make sure we picked up the ignore" + ); + + let errors = linter.lint(&parse, sql); + + assert_eq!( + errors, + vec![], + "We shouldn't have any errors because we have the ignore setup" + ); + } + #[test] fn single_ignore_c_style_comment() { let sql = r#" @@ -217,7 +248,7 @@ create table users ( "#; let parse = squawk_syntax::SourceFile::parse(sql); - let errors = linter.lint(parse, sql); + let errors = linter.lint(&parse, sql); assert_eq!(errors.len(), 0); } @@ -227,7 +258,7 @@ create table users ( let sql = r#"alter table t add column c char;"#; let parse = squawk_syntax::SourceFile::parse(sql); - let errors = linter.lint(parse, sql); + let errors = linter.lint(&parse, sql); assert_eq!(errors.len(), 1); } @@ -244,7 +275,7 @@ create table test_table ( "#; let parse = squawk_syntax::SourceFile::parse(sql); - let errors = linter.lint(parse, sql); + let errors = linter.lint(&parse, sql); assert_eq!(errors.len(), 0); } @@ -282,7 +313,7 @@ alter table t drop column c cascade; assert!(ignore.violation_names.is_empty()); let errors: Vec<_> = linter - .lint(parse, sql) + .lint(&parse, sql) .into_iter() .map(|x| x.code) .collect(); @@ -353,7 +384,7 @@ alter table t2 drop column c2 cascade; let parse = squawk_syntax::SourceFile::parse(sql); let errors: Vec<_> = linter - .lint(parse, sql) + .lint(&parse, sql) .into_iter() .map(|x| x.code) .collect(); @@ -377,7 +408,7 @@ alter table t2 drop column c2 cascade; let parse = squawk_syntax::SourceFile::parse(sql); let errors: Vec<_> = linter - .lint(parse, sql) + .lint(&parse, sql) .into_iter() .map(|x| x.code) .collect(); diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index ba51fce9..29316798 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -10,6 +10,7 @@ use rowan::TextRange; use rowan::TextSize; use serde::{Deserialize, Serialize}; +use squawk_syntax::SyntaxNode; use squawk_syntax::{Parse, SourceFile}; pub use version::Version; @@ -259,7 +260,33 @@ impl Edit { impl Violation { #[must_use] - pub fn new( + pub fn for_node( + code: Rule, + message: String, + node: &SyntaxNode, + help: impl Into>, + ) -> Self { + let range = node.text_range(); + + let start = node + .children_with_tokens() + .filter(|x| !x.kind().is_trivia()) + .next() + .map(|x| x.text_range().start()) + // Not sure we actually hit this, but just being safe + .unwrap_or_else(|| range.start()); + + Self { + code, + text_range: TextRange::new(start, range.end()), + message, + help: help.into(), + fix: None, + } + } + + #[must_use] + pub fn for_range( code: Rule, message: String, text_range: TextRange, @@ -303,7 +330,7 @@ impl Linter { } #[must_use] - pub fn lint(&mut self, file: Parse, text: &str) -> Vec { + pub fn lint(&mut self, file: &Parse, text: &str) -> Vec { if self.rules.contains(&Rule::AddingFieldWithDefault) { adding_field_with_default(self, &file); } diff --git a/crates/squawk_linter/src/rules/adding_field_with_default.rs b/crates/squawk_linter/src/rules/adding_field_with_default.rs index beb1f549..90ab5454 100644 --- a/crates/squawk_linter/src/rules/adding_field_with_default.rs +++ b/crates/squawk_linter/src/rules/adding_field_with_default.rs @@ -73,18 +73,18 @@ pub(crate) fn adding_field_with_default(ctx: &mut Linter, parse: &Parse { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::AddingFieldWithDefault, message.into(), - generated.syntax().text_range(), + generated.syntax(), help.to_string(), )); } diff --git a/crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs b/crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs index 07324113..8a2f36fe 100644 --- a/crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs +++ b/crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs @@ -25,10 +25,10 @@ pub(crate) fn adding_foreign_key_constraint(ctx: &mut Linter, parse: &Parse) }; if matches!(option, ast::AlterColumnOption::SetNotNull(_)) { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::AddingNotNullableField, "Setting a column `NOT NULL` blocks reads while the table is scanned." .into(), - option.syntax().text_range(), + option.syntax(), "Make the field nullable and use a `CHECK` constraint instead." .to_string(), )); diff --git a/crates/squawk_linter/src/rules/adding_primary_key_constraint.rs b/crates/squawk_linter/src/rules/adding_primary_key_constraint.rs index bbd58bc3..37da152f 100644 --- a/crates/squawk_linter/src/rules/adding_primary_key_constraint.rs +++ b/crates/squawk_linter/src/rules/adding_primary_key_constraint.rs @@ -18,10 +18,10 @@ pub(crate) fn adding_primary_key_constraint(ctx: &mut Linter, parse: &Parse) continue; } if has_not_null_and_no_default_constraint(add_column.constraints()) { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::AddingRequiredField, "Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.".into(), - add_column.syntax().text_range(), + add_column.syntax(), "Make the field nullable or add a non-VOLATILE DEFAULT".to_string(), )); } diff --git a/crates/squawk_linter/src/rules/ban_alter_domain_with_add_constraint.rs b/crates/squawk_linter/src/rules/ban_alter_domain_with_add_constraint.rs index 8d17040e..6238576e 100644 --- a/crates/squawk_linter/src/rules/ban_alter_domain_with_add_constraint.rs +++ b/crates/squawk_linter/src/rules/ban_alter_domain_with_add_constraint.rs @@ -12,10 +12,10 @@ pub(crate) fn ban_alter_domain_with_add_constraint(ctx: &mut Linter, parse: &Par if let Some(ast::AlterDomainAction::AddConstraint(add_constraint)) = alter_domain.action() { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::BanAlterDomainWithAddConstraint, "Domains with constraints have poor support for online migrations. Use table and column constraints instead.".into(), - add_constraint.syntax().text_range(), + add_constraint.syntax(), None, )) } diff --git a/crates/squawk_linter/src/rules/ban_char_field.rs b/crates/squawk_linter/src/rules/ban_char_field.rs index 6677274a..69cb7425 100644 --- a/crates/squawk_linter/src/rules/ban_char_field.rs +++ b/crates/squawk_linter/src/rules/ban_char_field.rs @@ -20,10 +20,10 @@ fn check_path_type(ctx: &mut Linter, path_type: ast::PathType) { .and_then(|x| x.name_ref()) { if is_char_type(name_ref.text()) { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::BanCharField, "Using `character` is likely a mistake and should almost always be replaced by `text` or `varchar`.".into(), - path_type.syntax().text_range(), + path_type.syntax(), None, )); } @@ -32,10 +32,10 @@ fn check_path_type(ctx: &mut Linter, path_type: ast::PathType) { fn check_char_type(ctx: &mut Linter, char_type: ast::CharType) { if is_char_type(char_type.text()) { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::BanCharField, "Using `character` is likey a mistake and should almost always be replaced by `text` or `varchar`.".into(), - char_type.syntax().text_range(), + char_type.syntax(), None, )); } diff --git a/crates/squawk_linter/src/rules/ban_concurrent_index_creation_in_transaction.rs b/crates/squawk_linter/src/rules/ban_concurrent_index_creation_in_transaction.rs index 420b71fd..909a0d14 100644 --- a/crates/squawk_linter/src/rules/ban_concurrent_index_creation_in_transaction.rs +++ b/crates/squawk_linter/src/rules/ban_concurrent_index_creation_in_transaction.rs @@ -22,7 +22,7 @@ pub(crate) fn ban_concurrent_index_creation_in_transaction( ast::Stmt::CreateIndex(create_index) => { if in_transaction { if let Some(concurrently) = create_index.concurrently_token() { - errors.push(Violation::new( + errors.push(Violation::for_range( Rule::BanConcurrentIndexCreationInTransaction, "While regular index creation can happen inside a transaction, this is not allowed when the `CONCURRENTLY` option is used.".into(), concurrently.text_range(), diff --git a/crates/squawk_linter/src/rules/ban_create_domain_with_constraint.rs b/crates/squawk_linter/src/rules/ban_create_domain_with_constraint.rs index 0657fe88..2eac4dc9 100644 --- a/crates/squawk_linter/src/rules/ban_create_domain_with_constraint.rs +++ b/crates/squawk_linter/src/rules/ban_create_domain_with_constraint.rs @@ -23,7 +23,7 @@ pub(crate) fn ban_create_domain_with_constraint(ctx: &mut Linter, parse: &Parse< } }); if let Some(range) = range { - ctx.report(Violation::new( + ctx.report(Violation::for_range( Rule::BanCreateDomainWithConstraint, "Domains with constraints have poor support for online migrations. Use table and column constraints instead.".into(), range, diff --git a/crates/squawk_linter/src/rules/ban_drop_column.rs b/crates/squawk_linter/src/rules/ban_drop_column.rs index a09a66f0..e4f8c27f 100644 --- a/crates/squawk_linter/src/rules/ban_drop_column.rs +++ b/crates/squawk_linter/src/rules/ban_drop_column.rs @@ -11,10 +11,10 @@ pub(crate) fn ban_drop_column(ctx: &mut Linter, parse: &Parse) { if let ast::Stmt::AlterTable(alter_table) = stmt { for action in alter_table.actions() { if let ast::AlterTableAction::DropColumn(drop_column) = action { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::BanDropColumn, "Dropping a column may break existing clients.".into(), - drop_column.syntax().text_range(), + drop_column.syntax(), None, )); } diff --git a/crates/squawk_linter/src/rules/ban_drop_database.rs b/crates/squawk_linter/src/rules/ban_drop_database.rs index 249e7b10..3d94b386 100644 --- a/crates/squawk_linter/src/rules/ban_drop_database.rs +++ b/crates/squawk_linter/src/rules/ban_drop_database.rs @@ -10,10 +10,10 @@ pub(crate) fn ban_drop_database(ctx: &mut Linter, parse: &Parse) { let file = parse.tree(); for stmt in file.stmts() { if let ast::Stmt::DropDatabase(drop_database) = stmt { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::BanDropDatabase, "Dropping a database may break existing clients.".into(), - drop_database.syntax().text_range(), + drop_database.syntax(), None, )); } diff --git a/crates/squawk_linter/src/rules/ban_drop_not_null.rs b/crates/squawk_linter/src/rules/ban_drop_not_null.rs index c146663a..98b9f6d6 100644 --- a/crates/squawk_linter/src/rules/ban_drop_not_null.rs +++ b/crates/squawk_linter/src/rules/ban_drop_not_null.rs @@ -14,10 +14,10 @@ pub(crate) fn ban_drop_not_null(ctx: &mut Linter, parse: &Parse) { if let Some(ast::AlterColumnOption::DropNotNull(drop_not_null)) = alter_column.option() { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::BanDropNotNull, "Dropping a `NOT NULL` constraint may break existing clients.".into(), - drop_not_null.syntax().text_range(), + drop_not_null.syntax(), None, )); } diff --git a/crates/squawk_linter/src/rules/ban_drop_table.rs b/crates/squawk_linter/src/rules/ban_drop_table.rs index 53cfa14c..4a70a39a 100644 --- a/crates/squawk_linter/src/rules/ban_drop_table.rs +++ b/crates/squawk_linter/src/rules/ban_drop_table.rs @@ -9,10 +9,10 @@ pub(crate) fn ban_drop_table(ctx: &mut Linter, parse: &Parse) { let file = parse.tree(); for stmt in file.stmts() { if let ast::Stmt::DropTable(drop_table) = stmt { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::BanDropTable, "Dropping a table may break existing clients.".into(), - drop_table.syntax().text_range(), + drop_table.syntax(), None, )); } diff --git a/crates/squawk_linter/src/rules/ban_truncate_cascade.rs b/crates/squawk_linter/src/rules/ban_truncate_cascade.rs index 5b3abcdf..a242bce5 100644 --- a/crates/squawk_linter/src/rules/ban_truncate_cascade.rs +++ b/crates/squawk_linter/src/rules/ban_truncate_cascade.rs @@ -10,7 +10,7 @@ pub(crate) fn ban_truncate_cascade(ctx: &mut Linter, parse: &Parse) // TODO: if we had knowledge about the entire schema, we // could be more precise here and actually navigate the // foreign keys. - ctx.report(Violation::new( + ctx.report(Violation::for_range( Rule::BanTruncateCascade, "Using `CASCADE` will recursively truncate any tables that foreign key to the referenced tables! So if you had foreign keys setup as `a <- b <- c` and truncated `a`, then `b` & `c` would also be truncated!".to_string(), cascade.text_range(), diff --git a/crates/squawk_linter/src/rules/changing_column_type.rs b/crates/squawk_linter/src/rules/changing_column_type.rs index ac6ee562..67f67119 100644 --- a/crates/squawk_linter/src/rules/changing_column_type.rs +++ b/crates/squawk_linter/src/rules/changing_column_type.rs @@ -12,10 +12,10 @@ pub(crate) fn changing_column_type(ctx: &mut Linter, parse: &Parse) for action in alter_table.actions() { if let ast::AlterTableAction::AlterColumn(alter_column) = action { if let Some(ast::AlterColumnOption::SetType(set_type)) = alter_column.option() { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::ChangingColumnType, "Changing a column type requires an `ACCESS EXCLUSIVE` lock on the table which blocks reads and writes while the table is rewritten. Changing the type of the column may also break other clients reading from the table.".into(), - set_type.syntax().text_range(), + set_type.syntax(), None, )); } diff --git a/crates/squawk_linter/src/rules/constraint_missing_not_valid.rs b/crates/squawk_linter/src/rules/constraint_missing_not_valid.rs index cb818452..dca23316 100644 --- a/crates/squawk_linter/src/rules/constraint_missing_not_valid.rs +++ b/crates/squawk_linter/src/rules/constraint_missing_not_valid.rs @@ -57,10 +57,10 @@ fn not_valid_validate_in_transaction( && not_valid_names.contains(&Identifier::new(&constraint_name)) { ctx.report( - Violation::new( + Violation::for_node( Rule::ConstraintMissingNotValid, "Using `NOT VALID` and `VALIDATE CONSTRAINT` in the same transaction will block all reads while the constraint is validated.".into(), - validate_constraint.syntax().text_range(), + validate_constraint.syntax(), "Add constraint as `NOT VALID` in one transaction and `VALIDATE CONSTRAINT` in a separate transaction.".to_string(), )) } @@ -127,10 +127,10 @@ pub(crate) fn constraint_missing_not_valid(ctx: &mut Linter, parse: &Parse) { if let Some(ty) = ty { if is_not_valid_int_type(&ty, &INT_TYPES) { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::PreferBigintOverInt, "Using 32-bit integer fields can result in hitting the max `int` limit.".into(), - ty.syntax().text_range(), + ty.syntax(), "Use 64-bit integer values instead to prevent hitting this limit.".to_string(), )); }; diff --git a/crates/squawk_linter/src/rules/prefer_bigint_over_smallint.rs b/crates/squawk_linter/src/rules/prefer_bigint_over_smallint.rs index 91c8a626..6251c421 100644 --- a/crates/squawk_linter/src/rules/prefer_bigint_over_smallint.rs +++ b/crates/squawk_linter/src/rules/prefer_bigint_over_smallint.rs @@ -23,10 +23,10 @@ lazy_static! { fn check_ty_for_small_int(ctx: &mut Linter, ty: Option) { if let Some(ty) = ty { if is_not_valid_int_type(&ty, &SMALL_INT_TYPES) { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::PreferBigintOverSmallint, "Using 16-bit integer fields can result in hitting the max `int` limit.".into(), - ty.syntax().text_range(), + ty.syntax(), "Use 64-bit integer values instead to prevent hitting this limit.".to_string(), )); }; diff --git a/crates/squawk_linter/src/rules/prefer_identity.rs b/crates/squawk_linter/src/rules/prefer_identity.rs index 7d151398..73e2bcb0 100644 --- a/crates/squawk_linter/src/rules/prefer_identity.rs +++ b/crates/squawk_linter/src/rules/prefer_identity.rs @@ -25,10 +25,10 @@ lazy_static! { fn check_ty_for_serial(ctx: &mut Linter, ty: Option) { if let Some(ty) = ty { if is_not_valid_int_type(&ty, &SERIAL_TYPES) { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::PreferIdentity, "Serial types make schema, dependency, and permission management difficult.".into(), - ty.syntax().text_range(), + ty.syntax(), "Use an `IDENTITY` column instead.".to_string(), )); }; diff --git a/crates/squawk_linter/src/rules/prefer_robust_stmts.rs b/crates/squawk_linter/src/rules/prefer_robust_stmts.rs index a2bf6f79..4ea16c69 100644 --- a/crates/squawk_linter/src/rules/prefer_robust_stmts.rs +++ b/crates/squawk_linter/src/rules/prefer_robust_stmts.rs @@ -142,10 +142,10 @@ pub(crate) fn prefer_robust_stmts(ctx: &mut Linter, parse: &Parse) { }; ctx.report( - Violation::new( + Violation::for_node( Rule::PreferRobustStmts, message, - action.syntax().text_range(), + action.syntax(), None, ) .with_fix(fix), @@ -164,10 +164,10 @@ pub(crate) fn prefer_robust_stmts(ctx: &mut Linter, parse: &Parse) { } else { None }; - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::PreferRobustStmts, "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.".into(), - create_index.syntax().text_range(), + create_index.syntax(), "Use an explicit name for a concurrently created index".to_string(), ).with_fix(fix)); } @@ -182,10 +182,10 @@ pub(crate) fn prefer_robust_stmts(ctx: &mut Linter, parse: &Parse) { None }; - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::PreferRobustStmts, "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.".into(), - create_table.syntax().text_range(), + create_table.syntax(), None, ).with_fix(fix)); } @@ -200,10 +200,10 @@ pub(crate) fn prefer_robust_stmts(ctx: &mut Linter, parse: &Parse) { None }; - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::PreferRobustStmts, "Missing `IF EXISTS`, the migration can't be rerun if it fails part way through.".into(), - drop_index.syntax().text_range(), + drop_index.syntax(), None, ).with_fix(fix)); } @@ -217,10 +217,10 @@ pub(crate) fn prefer_robust_stmts(ctx: &mut Linter, parse: &Parse) { } else { None }; - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::PreferRobustStmts, "Missing `IF EXISTS`, the migration can't be rerun if it fails part way through.".into(), - drop_table.syntax().text_range(), + drop_table.syntax(), None, ).with_fix(fix)); } @@ -235,10 +235,10 @@ pub(crate) fn prefer_robust_stmts(ctx: &mut Linter, parse: &Parse) { None }; - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::PreferRobustStmts, "Missing `IF EXISTS`, the migration can't be rerun if it fails part way through.".into(), - drop_type.syntax().text_range(), + drop_type.syntax(), None, ).with_fix(fix)); } @@ -258,7 +258,7 @@ mod test { assert_eq!(file.errors().len(), 0); assert_eq!(file.errors().len(), 0, "Shouldn't start with syntax errors"); let mut linter = Linter::from([Rule::PreferRobustStmts]); - let errors = linter.lint(file, sql); + let errors = linter.lint(&file, sql); assert!(!errors.is_empty(), "Should start with linter errors"); let fixes = errors.into_iter().flat_map(|x| x.fix).collect::>(); @@ -283,7 +283,7 @@ mod test { "Shouldn't introduce any syntax errors" ); let mut linter = Linter::from([Rule::PreferRobustStmts]); - let errors = linter.lint(file, &result); + let errors = linter.lint(&file, &result); assert_eq!( errors.len(), 0, diff --git a/crates/squawk_linter/src/rules/prefer_text_field.rs b/crates/squawk_linter/src/rules/prefer_text_field.rs index 42498b40..bd94cdbe 100644 --- a/crates/squawk_linter/src/rules/prefer_text_field.rs +++ b/crates/squawk_linter/src/rules/prefer_text_field.rs @@ -52,10 +52,10 @@ fn is_not_allowed_varchar(ty: &ast::Type) -> bool { fn check_ty_for_varchar(ctx: &mut Linter, ty: Option) { if let Some(ty) = ty { if is_not_allowed_varchar(&ty) { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::PreferTextField, "Changing the size of a `varchar` field requires an `ACCESS EXCLUSIVE` lock, that will prevent all reads and writes to the table.".to_string(), - ty.syntax().text_range(), + ty.syntax(), "Use a `TEXT` field with a `CHECK` constraint.".to_string(), )); }; diff --git a/crates/squawk_linter/src/rules/prefer_timestamptz.rs b/crates/squawk_linter/src/rules/prefer_timestamptz.rs index dcd5400c..8b8ad2ad 100644 --- a/crates/squawk_linter/src/rules/prefer_timestamptz.rs +++ b/crates/squawk_linter/src/rules/prefer_timestamptz.rs @@ -47,10 +47,10 @@ pub fn is_not_allowed_timestamp(ty: &ast::Type) -> bool { fn check_ty_for_timestamp(ctx: &mut Linter, ty: Option) { if let Some(ty) = ty { if is_not_allowed_timestamp(&ty) { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::PreferTimestampTz, "When Postgres stores a datetime in a `timestamp` field, Postgres drops the UTC offset. This means 2019-10-11 21:11:24+02 and 2019-10-11 21:11:24-06 will both be stored as 2019-10-11 21:11:24 in the database, even though they are eight hours apart in time.".into(), - ty.syntax().text_range(), + ty.syntax(), "Use timestamptz instead of timestamp for your column type.".to_string(), )); }; diff --git a/crates/squawk_linter/src/rules/renaming_column.rs b/crates/squawk_linter/src/rules/renaming_column.rs index 79779df9..e1d80410 100644 --- a/crates/squawk_linter/src/rules/renaming_column.rs +++ b/crates/squawk_linter/src/rules/renaming_column.rs @@ -11,10 +11,10 @@ pub(crate) fn renaming_column(ctx: &mut Linter, parse: &Parse) { if let ast::Stmt::AlterTable(alter_table) = stmt { for action in alter_table.actions() { if let ast::AlterTableAction::RenameColumn(rename_column) = action { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::RenamingColumn, "Renaming a column may break existing clients.".into(), - rename_column.syntax().text_range(), + rename_column.syntax(), None, )); } diff --git a/crates/squawk_linter/src/rules/renaming_table.rs b/crates/squawk_linter/src/rules/renaming_table.rs index b789220d..2fc2e1da 100644 --- a/crates/squawk_linter/src/rules/renaming_table.rs +++ b/crates/squawk_linter/src/rules/renaming_table.rs @@ -11,10 +11,10 @@ pub(crate) fn renaming_table(ctx: &mut Linter, parse: &Parse) { if let ast::Stmt::AlterTable(alter_table) = stmt { for action in alter_table.actions() { if let ast::AlterTableAction::RenameTable(rename_table) = action { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::RenamingTable, "Renaming a table may break existing clients.".into(), - rename_table.syntax().text_range(), + rename_table.syntax(), None, )); } diff --git a/crates/squawk_linter/src/rules/require_concurrent_index_creation.rs b/crates/squawk_linter/src/rules/require_concurrent_index_creation.rs index 8bfeaa59..89444d81 100644 --- a/crates/squawk_linter/src/rules/require_concurrent_index_creation.rs +++ b/crates/squawk_linter/src/rules/require_concurrent_index_creation.rs @@ -21,10 +21,10 @@ pub(crate) fn require_concurrent_index_creation(ctx: &mut Linter, parse: &Parse< if create_index.concurrently_token().is_none() && !tables_created.contains(&Identifier::new(&table_name.text())) { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::RequireConcurrentIndexCreation, "During normal index creation, table updates are blocked, but reads are still allowed.".into(), - create_index.syntax().text_range(), + create_index.syntax(), "Use `CONCURRENTLY` to avoid blocking writes.".to_string(), )); } diff --git a/crates/squawk_linter/src/rules/require_concurrent_index_deletion.rs b/crates/squawk_linter/src/rules/require_concurrent_index_deletion.rs index 7074c64f..40cdb4a8 100644 --- a/crates/squawk_linter/src/rules/require_concurrent_index_deletion.rs +++ b/crates/squawk_linter/src/rules/require_concurrent_index_deletion.rs @@ -10,10 +10,10 @@ pub(crate) fn require_concurrent_index_deletion(ctx: &mut Linter, parse: &Parse< for stmt in file.stmts() { if let ast::Stmt::DropIndex(drop_index) = stmt { if drop_index.concurrently_token().is_none() { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::RequireConcurrentIndexDeletion, "A normal `DROP INDEX` acquires an `ACCESS EXCLUSIVE` lock on the table, blocking other accesses until the index drop can complete.".into(), - drop_index.syntax().text_range(), + drop_index.syntax(), "Drop the index `CONCURRENTLY`.".to_string(), )); } diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__prefer_robust_stmts__test__create_table_err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__prefer_robust_stmts__test__create_table_err.snap index d395caef..e81bfa96 100644 --- a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__prefer_robust_stmts__test__create_table_err.snap +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__prefer_robust_stmts__test__create_table_err.snap @@ -6,7 +6,7 @@ expression: errors Violation { code: PreferRobustStmts, message: "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.", - text_range: 11..122, + text_range: 40..122, help: None, fix: Some( Fix { diff --git a/crates/squawk_linter/src/rules/transaction_nesting.rs b/crates/squawk_linter/src/rules/transaction_nesting.rs index 23e65b38..bbd1f794 100644 --- a/crates/squawk_linter/src/rules/transaction_nesting.rs +++ b/crates/squawk_linter/src/rules/transaction_nesting.rs @@ -14,17 +14,17 @@ pub(crate) fn transaction_nesting(ctx: &mut Linter, parse: &Parse) { match stmt { ast::Stmt::Begin(_) => { if ctx.settings.assume_in_transaction { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::TransactionNesting, "There is an existing transaction already in progress, managed by your migration tool.".to_string(), - stmt.syntax().text_range(), + stmt.syntax(), assume_in_transaction_help.to_string() )); } else if in_explicit_transaction { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::TransactionNesting, "There is an existing transaction already in progress.".to_string(), - stmt.syntax().text_range(), + stmt.syntax(), assume_in_transaction_help.to_string(), )); } @@ -32,18 +32,18 @@ pub(crate) fn transaction_nesting(ctx: &mut Linter, parse: &Parse) { } ast::Stmt::Commit(_) | ast::Stmt::Rollback(_) => { if ctx.settings.assume_in_transaction { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::TransactionNesting, "Attempting to end the transaction that is managed by your migration tool" .to_string(), - stmt.syntax().text_range(), + stmt.syntax(), assume_in_transaction_help.to_string(), )); } else if !in_explicit_transaction { - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::TransactionNesting, "There is no transaction to `COMMIT` or `ROLLBACK`.".to_string(), - stmt.syntax().text_range(), + stmt.syntax(), "`BEGIN` a transaction at an earlier point in the migration or remove this statement.".to_string() )); } @@ -71,7 +71,7 @@ COMMIT; "#; let file = SourceFile::parse(sql); let mut linter = Linter::from([Rule::TransactionNesting]); - let errors = linter.lint(file, sql); + let errors = linter.lint(&file, sql); assert_ne!(errors.len(), 0); assert_debug_snapshot!(errors); } @@ -86,7 +86,7 @@ COMMIT; "#; let file = SourceFile::parse(sql); let mut linter = Linter::from([Rule::TransactionNesting]); - let errors = linter.lint(file, sql); + let errors = linter.lint(&file, sql); assert_ne!(errors.len(), 0); assert_debug_snapshot!(errors); } @@ -100,7 +100,7 @@ COMMIT; let file = SourceFile::parse(sql); let mut linter = Linter::from([Rule::TransactionNesting]); linter.settings.assume_in_transaction = true; - let errors = linter.lint(file, sql); + let errors = linter.lint(&file, sql); assert_ne!(errors.len(), 0); assert_debug_snapshot!(errors); } @@ -115,7 +115,7 @@ ROLLBACK; let file = SourceFile::parse(sql); let mut linter = Linter::from([Rule::TransactionNesting]); linter.settings.assume_in_transaction = true; - let errors = linter.lint(file, sql); + let errors = linter.lint(&file, sql); assert_ne!(errors.len(), 0); assert_debug_snapshot!(errors); } @@ -131,7 +131,7 @@ COMMIT; let file = SourceFile::parse(sql); let mut linter = Linter::from([Rule::TransactionNesting]); linter.settings.assume_in_transaction = true; - let errors = linter.lint(file, sql); + let errors = linter.lint(&file, sql); assert_ne!(errors.len(), 0); assert_debug_snapshot!(errors); } @@ -145,7 +145,7 @@ COMMIT; "#; let file = SourceFile::parse(sql); let mut linter = Linter::from([Rule::TransactionNesting]); - let errors = linter.lint(file, sql); + let errors = linter.lint(&file, sql); assert_eq!(errors.len(), 0); } @@ -163,7 +163,7 @@ COMMIT; "#; let file = SourceFile::parse(sql); let mut linter = Linter::from([Rule::TransactionNesting]); - let errors = linter.lint(file, sql); + let errors = linter.lint(&file, sql); assert_eq!(errors.len(), 0); } @@ -175,7 +175,7 @@ SELECT 1; let file = SourceFile::parse(sql); let mut linter = Linter::from([Rule::TransactionNesting]); linter.settings.assume_in_transaction = true; - let errors = linter.lint(file, sql); + let errors = linter.lint(&file, sql); assert_eq!(errors.len(), 0); } } diff --git a/crates/squawk_linter/src/test_utils.rs b/crates/squawk_linter/src/test_utils.rs index e8029281..517cf5fe 100644 --- a/crates/squawk_linter/src/test_utils.rs +++ b/crates/squawk_linter/src/test_utils.rs @@ -4,7 +4,7 @@ pub(crate) fn lint(sql: &str, rule: Rule) -> Vec { let file = squawk_syntax::SourceFile::parse(sql); assert_eq!(file.errors().len(), 0); let mut linter = Linter::from([rule]); - linter.lint(file, sql) + linter.lint(&file, sql) } pub(crate) fn lint_with_assume_in_transaction(sql: &str, rule: Rule) -> Vec { @@ -12,5 +12,5 @@ pub(crate) fn lint_with_assume_in_transaction(sql: &str, rule: Rule) -> Vec Vec { let parse: Parse = SourceFile::parse(content); let parse_errors = parse.errors(); let mut linter = Linter::with_all_rules(); - let violations = linter.lint(parse, content); + let violations = linter.lint(&parse, content); let line_index = LineIndex::new(content); let mut diagnostics = Vec::with_capacity(violations.len() + parse_errors.len()); diff --git a/crates/squawk_wasm/src/lib.rs b/crates/squawk_wasm/src/lib.rs index b51f093c..15e4986b 100644 --- a/crates/squawk_wasm/src/lib.rs +++ b/crates/squawk_wasm/src/lib.rs @@ -117,7 +117,7 @@ pub fn lint(text: String) -> Result { } }); - let lint_errors = linter.lint(parse, &text); + let lint_errors = linter.lint(&parse, &text); let errors = lint_errors.into_iter().map(|x| { let start = line_index.line_col(x.text_range.start()); let end = line_index.line_col(x.text_range.end()); diff --git a/crates/xtask/src/new_rule.rs b/crates/xtask/src/new_rule.rs index 3a94ae58..0daa1544 100644 --- a/crates/xtask/src/new_rule.rs +++ b/crates/xtask/src/new_rule.rs @@ -25,7 +25,7 @@ pub(crate) fn {rule_name_snake}(ctx: &mut Linter, parse: &Parse) {{ ctx.report(Violation::new( Rule::{rule_name_pascal}, "todo".to_string(), - create_table.syntax().text_range(), + create_table.syntax(), "todo or none".to_string(), )); todo!(); From 87181d5e3cc6e6a3ced7802d2dd84c3b29b0ff8f Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Wed, 20 Aug 2025 20:08:34 -0400 Subject: [PATCH 2/2] fix --- crates/xtask/src/new_rule.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/xtask/src/new_rule.rs b/crates/xtask/src/new_rule.rs index 0daa1544..cdb03bbd 100644 --- a/crates/xtask/src/new_rule.rs +++ b/crates/xtask/src/new_rule.rs @@ -22,7 +22,7 @@ pub(crate) fn {rule_name_snake}(ctx: &mut Linter, parse: &Parse) {{ match stmt {{ // TODO: update to the stmt you want to check ast::Stmt::CreateTable(create_table) => {{ - ctx.report(Violation::new( + ctx.report(Violation::for_node( Rule::{rule_name_pascal}, "todo".to_string(), create_table.syntax(),