Skip to content

Commit 07987be

Browse files
sbdchdsbdchd
authored
linter: fix violations to not include leading trivia (#612)
previously we'd mark leading comments and whitespace as being part of the violation instead of just the actual SQL nodes
1 parent 6c5e245 commit 07987be

36 files changed

+166
-108
lines changed

‎crates/squawk/src/reporter.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ fn check_sql(
3434
linter.settings.assume_in_transaction = assume_in_transaction;
3535
let parse = SourceFile::parse(sql);
3636
let parse_errors = parse.errors();
37-
let errors = linter.lint(parse, sql);
37+
let errors = linter.lint(&parse, sql);
3838
let line_index = LineIndex::new(sql);
3939

4040
let mut violations = Vec::with_capacity(parse_errors.len() + errors.len());

‎crates/squawk_linter/src/ignore.rs‎

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ pub(crate) fn find_ignores(ctx: &mut Linter, file: &SyntaxNode) {
9898
let end = start + TextSize::new(trimmed.len() as u32);
9999
let range = TextRange::new(start, end);
100100

101-
ctx.report(Violation::new(
101+
ctx.report(Violation::for_range(
102102
Rule::UnusedIgnore,
103103
format!("unknown name {trimmed}"),
104104
range,
@@ -144,6 +144,37 @@ alter table t drop column c cascade;
144144
assert!(ignore.violation_names.contains(&Rule::BanDropColumn));
145145
}
146146

147+
#[test]
148+
fn multiple_sql_comments_with_ignore_is_ok() {
149+
let sql = "
150+
-- fooo bar
151+
-- buzz
152+
-- squawk-ignore prefer-robust-stmts
153+
create table x();
154+
155+
select 1;
156+
";
157+
158+
let parse = squawk_syntax::SourceFile::parse(sql);
159+
let mut linter = Linter::with_all_rules();
160+
find_ignores(&mut linter, &parse.syntax_node());
161+
162+
assert_eq!(linter.ignores.len(), 1);
163+
let ignore = &linter.ignores[0];
164+
assert!(
165+
ignore.violation_names.contains(&Rule::PreferRobustStmts),
166+
"Make sure we picked up the ignore"
167+
);
168+
169+
let errors = linter.lint(&parse, sql);
170+
171+
assert_eq!(
172+
errors,
173+
vec![],
174+
"We shouldn't have any errors because we have the ignore setup"
175+
);
176+
}
177+
147178
#[test]
148179
fn single_ignore_c_style_comment() {
149180
let sql = r#"
@@ -217,7 +248,7 @@ create table users (
217248
"#;
218249

219250
let parse = squawk_syntax::SourceFile::parse(sql);
220-
let errors = linter.lint(parse, sql);
251+
let errors = linter.lint(&parse, sql);
221252
assert_eq!(errors.len(), 0);
222253
}
223254

@@ -227,7 +258,7 @@ create table users (
227258
let sql = r#"alter table t add column c char;"#;
228259

229260
let parse = squawk_syntax::SourceFile::parse(sql);
230-
let errors = linter.lint(parse, sql);
261+
let errors = linter.lint(&parse, sql);
231262
assert_eq!(errors.len(), 1);
232263
}
233264

@@ -244,7 +275,7 @@ create table test_table (
244275
"#;
245276

246277
let parse = squawk_syntax::SourceFile::parse(sql);
247-
let errors = linter.lint(parse, sql);
278+
let errors = linter.lint(&parse, sql);
248279
assert_eq!(errors.len(), 0);
249280
}
250281

@@ -282,7 +313,7 @@ alter table t drop column c cascade;
282313
assert!(ignore.violation_names.is_empty());
283314

284315
let errors: Vec<_> = linter
285-
.lint(parse, sql)
316+
.lint(&parse, sql)
286317
.into_iter()
287318
.map(|x| x.code)
288319
.collect();
@@ -353,7 +384,7 @@ alter table t2 drop column c2 cascade;
353384

354385
let parse = squawk_syntax::SourceFile::parse(sql);
355386
let errors: Vec<_> = linter
356-
.lint(parse, sql)
387+
.lint(&parse, sql)
357388
.into_iter()
358389
.map(|x| x.code)
359390
.collect();
@@ -377,7 +408,7 @@ alter table t2 drop column c2 cascade;
377408

378409
let parse = squawk_syntax::SourceFile::parse(sql);
379410
let errors: Vec<_> = linter
380-
.lint(parse, sql)
411+
.lint(&parse, sql)
381412
.into_iter()
382413
.map(|x| x.code)
383414
.collect();

‎crates/squawk_linter/src/lib.rs‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use rowan::TextRange;
1010
use rowan::TextSize;
1111
use serde::{Deserialize, Serialize};
1212

13+
use squawk_syntax::SyntaxNode;
1314
use squawk_syntax::{Parse, SourceFile};
1415

1516
pub use version::Version;
@@ -259,7 +260,33 @@ impl Edit {
259260

260261
impl Violation {
261262
#[must_use]
262-
pub fn new(
263+
pub fn for_node(
264+
code: Rule,
265+
message: String,
266+
node: &SyntaxNode,
267+
help: impl Into<Option<String>>,
268+
) -> Self {
269+
let range = node.text_range();
270+
271+
let start = node
272+
.children_with_tokens()
273+
.filter(|x| !x.kind().is_trivia())
274+
.next()
275+
.map(|x| x.text_range().start())
276+
// Not sure we actually hit this, but just being safe
277+
.unwrap_or_else(|| range.start());
278+
279+
Self {
280+
code,
281+
text_range: TextRange::new(start, range.end()),
282+
message,
283+
help: help.into(),
284+
fix: None,
285+
}
286+
}
287+
288+
#[must_use]
289+
pub fn for_range(
263290
code: Rule,
264291
message: String,
265292
text_range: TextRange,
@@ -303,7 +330,7 @@ impl Linter {
303330
}
304331

305332
#[must_use]
306-
pub fn lint(&mut self, file: Parse<SourceFile>, text: &str) -> Vec<Violation> {
333+
pub fn lint(&mut self, file: &Parse<SourceFile>, text: &str) -> Vec<Violation> {
307334
if self.rules.contains(&Rule::AddingFieldWithDefault) {
308335
adding_field_with_default(self, &file);
309336
}

‎crates/squawk_linter/src/rules/adding_field_with_default.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,18 +73,18 @@ pub(crate) fn adding_field_with_default(ctx: &mut Linter, parse: &Parse<SourceFi
7373
if is_const_expr(&expr) || is_non_volatile(&expr) {
7474
continue;
7575
}
76-
ctx.report(Violation::new(
76+
ctx.report(Violation::for_node(
7777
Rule::AddingFieldWithDefault,
7878
message.into(),
79-
expr.syntax().text_range(),
79+
expr.syntax(),
8080
help.to_string(),
8181
))
8282
}
8383
ast::Constraint::GeneratedConstraint(generated) => {
84-
ctx.report(Violation::new(
84+
ctx.report(Violation::for_node(
8585
Rule::AddingFieldWithDefault,
8686
message.into(),
87-
generated.syntax().text_range(),
87+
generated.syntax(),
8888
help.to_string(),
8989
));
9090
}

‎crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,10 @@ pub(crate) fn adding_foreign_key_constraint(ctx: &mut Linter, parse: &Parse<Sour
2525
ast::Constraint::ForeignKeyConstraint(_)
2626
| ast::Constraint::ReferencesConstraint(_)
2727
) {
28-
ctx.report(Violation::new(
28+
ctx.report(Violation::for_node(
2929
Rule::AddingForeignKeyConstraint,
3030
message.into(),
31-
constraint.syntax().text_range(),
31+
constraint.syntax(),
3232
help.to_string(),
3333
))
3434
}
@@ -41,10 +41,10 @@ pub(crate) fn adding_foreign_key_constraint(ctx: &mut Linter, parse: &Parse<Sour
4141
ast::Constraint::ForeignKeyConstraint(_)
4242
| ast::Constraint::ReferencesConstraint(_)
4343
) {
44-
ctx.report(Violation::new(
44+
ctx.report(Violation::for_node(
4545
Rule::AddingForeignKeyConstraint,
4646
message.into(),
47-
constraint.syntax().text_range(),
47+
constraint.syntax(),
4848
help.to_string(),
4949
))
5050
}

‎crates/squawk_linter/src/rules/adding_not_null_field.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,11 @@ pub(crate) fn adding_not_null_field(ctx: &mut Linter, parse: &Parse<SourceFile>)
1616
};
1717

1818
if matches!(option, ast::AlterColumnOption::SetNotNull(_)) {
19-
ctx.report(Violation::new(
19+
ctx.report(Violation::for_node(
2020
Rule::AddingNotNullableField,
2121
"Setting a column `NOT NULL` blocks reads while the table is scanned."
2222
.into(),
23-
option.syntax().text_range(),
23+
option.syntax(),
2424
"Make the field nullable and use a `CHECK` constraint instead."
2525
.to_string(),
2626
));

‎crates/squawk_linter/src/rules/adding_primary_key_constraint.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,10 @@ pub(crate) fn adding_primary_key_constraint(ctx: &mut Linter, parse: &Parse<Sour
1818
add_constraint.constraint()
1919
{
2020
if primary_key_constraint.using_index().is_none() {
21-
ctx.report(Violation::new(
21+
ctx.report(Violation::for_node(
2222
Rule::AddingSerialPrimaryKeyField,
2323
message.to_string(),
24-
primary_key_constraint.syntax().text_range(),
24+
primary_key_constraint.syntax(),
2525
help.to_string(),
2626
));
2727
}
@@ -33,10 +33,10 @@ pub(crate) fn adding_primary_key_constraint(ctx: &mut Linter, parse: &Parse<Sour
3333
constraint
3434
{
3535
if primary_key_constraint.using_index().is_none() {
36-
ctx.report(Violation::new(
36+
ctx.report(Violation::for_node(
3737
Rule::AddingSerialPrimaryKeyField,
3838
message.to_string(),
39-
primary_key_constraint.syntax().text_range(),
39+
primary_key_constraint.syntax(),
4040
help.to_string(),
4141
));
4242
}

‎crates/squawk_linter/src/rules/adding_required_field.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@ pub(crate) fn adding_required_field(ctx: &mut Linter, parse: &Parse<SourceFile>)
1515
continue;
1616
}
1717
if has_not_null_and_no_default_constraint(add_column.constraints()) {
18-
ctx.report(Violation::new(
18+
ctx.report(Violation::for_node(
1919
Rule::AddingRequiredField,
2020
"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.".into(),
21-
add_column.syntax().text_range(),
21+
add_column.syntax(),
2222
"Make the field nullable or add a non-VOLATILE DEFAULT".to_string(),
2323
));
2424
}

‎crates/squawk_linter/src/rules/ban_alter_domain_with_add_constraint.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,10 @@ pub(crate) fn ban_alter_domain_with_add_constraint(ctx: &mut Linter, parse: &Par
1212
if let Some(ast::AlterDomainAction::AddConstraint(add_constraint)) =
1313
alter_domain.action()
1414
{
15-
ctx.report(Violation::new(
15+
ctx.report(Violation::for_node(
1616
Rule::BanAlterDomainWithAddConstraint,
1717
"Domains with constraints have poor support for online migrations. Use table and column constraints instead.".into(),
18-
add_constraint.syntax().text_range(),
18+
add_constraint.syntax(),
1919
None,
2020
))
2121
}

‎crates/squawk_linter/src/rules/ban_char_field.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,10 @@ fn check_path_type(ctx: &mut Linter, path_type: ast::PathType) {
2020
.and_then(|x| x.name_ref())
2121
{
2222
if is_char_type(name_ref.text()) {
23-
ctx.report(Violation::new(
23+
ctx.report(Violation::for_node(
2424
Rule::BanCharField,
2525
"Using `character` is likely a mistake and should almost always be replaced by `text` or `varchar`.".into(),
26-
path_type.syntax().text_range(),
26+
path_type.syntax(),
2727
None,
2828
));
2929
}
@@ -32,10 +32,10 @@ fn check_path_type(ctx: &mut Linter, path_type: ast::PathType) {
3232

3333
fn check_char_type(ctx: &mut Linter, char_type: ast::CharType) {
3434
if is_char_type(char_type.text()) {
35-
ctx.report(Violation::new(
35+
ctx.report(Violation::for_node(
3636
Rule::BanCharField,
3737
"Using `character` is likey a mistake and should almost always be replaced by `text` or `varchar`.".into(),
38-
char_type.syntax().text_range(),
38+
char_type.syntax(),
3939
None,
4040
));
4141
}

0 commit comments

Comments
 (0)