linter: fix violations to not include leading trivia

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());

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();

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<Option<String>>,
+    ) -> 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<SourceFile>, text: &str) -> Vec<Violation> {
+    pub fn lint(&mut self, file: &Parse<SourceFile>, text: &str) -> Vec<Violation> {
         if self.rules.contains(&Rule::AddingFieldWithDefault) {
             adding_field_with_default(self, &file);
         }

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<SourceFi
                                 if is_const_expr(&expr) || is_non_volatile(&expr) {
                                     continue;
                                 }
-                                ctx.report(Violation::new(
+                                ctx.report(Violation::for_node(
                                     Rule::AddingFieldWithDefault,
                                     message.into(),
-                                    expr.syntax().text_range(),
+                                    expr.syntax(),
                                     help.to_string(),
                                 ))
                             }
                             ast::Constraint::GeneratedConstraint(generated) => {
-                                ctx.report(Violation::new(
+                                ctx.report(Violation::for_node(
                                     Rule::AddingFieldWithDefault,
                                     message.into(),
-                                    generated.syntax().text_range(),
+                                    generated.syntax(),
                                     help.to_string(),
                                 ));
                             }

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<Sour
                                 ast::Constraint::ForeignKeyConstraint(_)
                                     | ast::Constraint::ReferencesConstraint(_)
                             ) {
-                                ctx.report(Violation::new(
+                                ctx.report(Violation::for_node(
                                     Rule::AddingForeignKeyConstraint,
                                     message.into(),
-                                    constraint.syntax().text_range(),
+                                    constraint.syntax(),
                                     help.to_string(),
                                 ))
                             }
@@ -41,10 +41,10 @@ pub(crate) fn adding_foreign_key_constraint(ctx: &mut Linter, parse: &Parse<Sour
                                 ast::Constraint::ForeignKeyConstraint(_)
                                     | ast::Constraint::ReferencesConstraint(_)
                             ) {
-                                ctx.report(Violation::new(
+                                ctx.report(Violation::for_node(
                                     Rule::AddingForeignKeyConstraint,
                                     message.into(),
-                                    constraint.syntax().text_range(),
+                                    constraint.syntax(),
                                     help.to_string(),
                                 ))
                             }

crates/squawk_linter/src/rules/adding_not_null_field.rs

@@ -16,11 +16,11 @@ pub(crate) fn adding_not_null_field(ctx: &mut Linter, parse: &Parse<SourceFile>)
                     };
 
                     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(),
                         ));

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<Sour
                             add_constraint.constraint()
                         {
                             if primary_key_constraint.using_index().is_none() {
-                                ctx.report(Violation::new(
+                                ctx.report(Violation::for_node(
                                     Rule::AddingSerialPrimaryKeyField,
                                     message.to_string(),
-                                    primary_key_constraint.syntax().text_range(),
+                                    primary_key_constraint.syntax(),
                                     help.to_string(),
                                 ));
                             }
@@ -33,10 +33,10 @@ pub(crate) fn adding_primary_key_constraint(ctx: &mut Linter, parse: &Parse<Sour
                                 constraint
                             {
                                 if primary_key_constraint.using_index().is_none() {
-                                    ctx.report(Violation::new(
+                                    ctx.report(Violation::for_node(
                                         Rule::AddingSerialPrimaryKeyField,
                                         message.to_string(),
-                                        primary_key_constraint.syntax().text_range(),
+                                        primary_key_constraint.syntax(),
                                         help.to_string(),
                                     ));
                                 }

crates/squawk_linter/src/rules/adding_required_field.rs

@@ -15,10 +15,10 @@ pub(crate) fn adding_required_field(ctx: &mut Linter, parse: &Parse<SourceFile>)
                         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(),
                         ));
                     }

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,
                     ))
             }

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,
         ));
     }

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(),

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,

crates/squawk_linter/src/rules/ban_drop_column.rs

@@ -11,10 +11,10 @@ pub(crate) fn ban_drop_column(ctx: &mut Linter, parse: &Parse<SourceFile>) {
         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,
                     ));
                 }

crates/squawk_linter/src/rules/ban_drop_database.rs

@@ -10,10 +10,10 @@ pub(crate) fn ban_drop_database(ctx: &mut Linter, parse: &Parse<SourceFile>) {
     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,
             ));
         }

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<SourceFile>) {
                     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,
                         ));
                     }

crates/squawk_linter/src/rules/ban_drop_table.rs

@@ -9,10 +9,10 @@ pub(crate) fn ban_drop_table(ctx: &mut Linter, parse: &Parse<SourceFile>) {
     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,
             ));
         }

crates/squawk_linter/src/rules/ban_truncate_cascade.rs

@@ -10,7 +10,7 @@ pub(crate) fn ban_truncate_cascade(ctx: &mut Linter, parse: &Parse<SourceFile>)
                 // 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(),

crates/squawk_linter/src/rules/changing_column_type.rs

@@ -12,10 +12,10 @@ pub(crate) fn changing_column_type(ctx: &mut Linter, parse: &Parse<SourceFile>)
             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,
                         ));
                     }