diff --git a/.vscode/linter.code-snippets b/.vscode/linter.code-snippets new file mode 100644 index 00000000..63b32251 --- /dev/null +++ b/.vscode/linter.code-snippets @@ -0,0 +1,37 @@ +{ + // Place your tea workspace snippets here. Each snippet is defined under a snippet name and has a scope, prefix, body and + // description. Add comma separated ids of the languages where the snippet is applicable in the scope field. If scope + // is left empty or omitted, the snippet gets applied to all languages. The prefix is what is + // used to trigger the snippet and the body will be expanded and inserted. Possible variables are: + // $1, $2 for tab stops, $0 for the final cursor position, and ${1:label}, ${2:another} for placeholders. + // Placeholders with the same ids are connected. + // Example: + // "Print to console": { + // "scope": "javascript,typescript", + // "prefix": "log", + // "body": [ + // "console.log('$1');", + // "$2" + // ], + // "description": "Log output to console" + // } + + "linttest": { + "scope": "rust", + "prefix": "linttest", + "body": [ +"#[test]", +"fn $1() {", +" let sql = r#\"", +"$2", +" \"#;", +"", +" let file = SourceFile::parse(&sql);", +" let mut linter = Linter::new([Rule::$3]);", +" let errors = linter.lint(file, sql);", +" assert_debug_snapshot!(errors);", +"}", + + ] + } +} diff --git a/.vscode/syntax.code-snippets b/.vscode/syntax.code-snippets new file mode 100644 index 00000000..7ae4147e --- /dev/null +++ b/.vscode/syntax.code-snippets @@ -0,0 +1,83 @@ +{ + // Place your tea workspace snippets here. Each snippet is defined under a snippet name and has a scope, prefix, body and + // description. Add comma separated ids of the languages where the snippet is applicable in the scope field. If scope + // is left empty or omitted, the snippet gets applied to all languages. The prefix is what is + // used to trigger the snippet and the body will be expanded and inserted. Possible variables are: + // $1, $2 for tab stops, $0 for the final cursor position, and ${1:label}, ${2:another} for placeholders. + // Placeholders with the same ids are connected. + // Example: + // "Print to console": { + // "scope": "javascript,typescript", + // "prefix": "log", + // "body": [ + // "console.log('$1');", + // "$2" + // ], + // "description": "Log output to console" + // } + "astnode": { + "scope": "rust", + "prefix": "astnode", + "body": [ + +"#[derive(Debug, Clone, PartialEq, Eq, Hash)]", +"pub struct $1 {", +" pub(crate) syntax: SyntaxNode,", +"}", +"", +"impl AstNode for $1 {", +" #[inline]", +" fn can_cast(kind: SyntaxKind) -> bool {", +" kind == SyntaxKind::$2", +" }", +" #[inline]", +" fn cast(syntax: SyntaxNode) -> Option {", +" if Self::can_cast(syntax.kind()) {", +" Some(Self { syntax })", +" } else {", +" None", +" }", +" }", +" #[inline]", +" fn syntax(&self) -> &SyntaxNode {", +" &self.syntax", +" }", +"}", + + ] + }, + "astenum": { + "scope": "rust", + "prefix": "astenum", + "body": [ +"#[derive(Debug, Clone, PartialEq, Eq, Hash)]", +"pub enum $1 {", +" $3($3),", +"}", +"", +"impl AstNode for $1 {", +" #[inline]", +" fn can_cast(kind: SyntaxKind) -> bool {", +" matches!(kind, SyntaxKind::$2)", +" }", +" #[inline]", +" fn cast(syntax: SyntaxNode) -> Option {", +" let res = match syntax.kind() {", +" SyntaxKind::DEFAULT_CONSTRAINT => {", +" $1::$3($3 { syntax })", +" }", +" _ => return None,", +" };", +" Some(res)", +" }", +" #[inline]", +" fn syntax(&self) -> &SyntaxNode {", +" match self {", +" $1::$3(it) => &it.syntax,", +" }", +" }", +"}", + + ] + } +} diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 6fd3e28c..5a8ddab6 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -47,6 +47,7 @@ use rules::renaming_column; use rules::renaming_table; use rules::require_concurrent_index_creation; use rules::require_concurrent_index_deletion; +use rules::transaction_nesting; // xtask:new-lint:rule-import #[derive(Debug, PartialEq, Clone, Copy, Serialize, Hash, Eq, Deserialize, Sequence)] @@ -340,6 +341,9 @@ impl Linter { if self.rules.contains(&Rule::BanAlterDomainWithAddConstraint) { ban_alter_domain_with_add_constraint(self, &file); } + if self.rules.contains(&Rule::TransactionNesting) { + transaction_nesting(self, &file); + } // xtask:new-lint:rule-call // locate any ignores in the file diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index 7b909d85..3287c48f 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -24,6 +24,7 @@ pub(crate) mod renaming_column; pub(crate) mod renaming_table; pub(crate) mod require_concurrent_index_creation; pub(crate) mod require_concurrent_index_deletion; +pub(crate) mod transaction_nesting; // xtask:new-lint:mod-decl pub(crate) use adding_field_with_default::adding_field_with_default; @@ -52,4 +53,5 @@ pub(crate) use renaming_column::renaming_column; pub(crate) use renaming_table::renaming_table; pub(crate) use require_concurrent_index_creation::require_concurrent_index_creation; pub(crate) use require_concurrent_index_deletion::require_concurrent_index_deletion; +pub(crate) use transaction_nesting::transaction_nesting; // xtask:new-lint:export diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__begin_assume_transaction_err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__begin_assume_transaction_err.snap new file mode 100644 index 00000000..1514d4d8 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__begin_assume_transaction_err.snap @@ -0,0 +1,30 @@ +--- +source: crates/squawk_linter/src/rules/transaction_nesting.rs +expression: errors +--- +[ + Violation { + code: TransactionNesting, + message: "There is an existing transaction already in progress, managed by your migration tool.", + text_range: 1..6, + help: Some( + "Put migration statements in separate files to have them be in separate transactions or don't use the assume-in-transaction setting.", + ), + }, + Violation { + code: TransactionNesting, + message: "There is an existing transaction already in progress, managed by your migration tool.", + text_range: 8..13, + help: Some( + "Put migration statements in separate files to have them be in separate transactions or don't use the assume-in-transaction setting.", + ), + }, + Violation { + code: TransactionNesting, + message: "Attempting to end the transaction that is managed by your migration tool", + text_range: 25..31, + help: Some( + "Put migration statements in separate files to have them be in separate transactions or don't use the assume-in-transaction setting.", + ), + }, +] diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__begin_repeated_err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__begin_repeated_err.snap new file mode 100644 index 00000000..804d7973 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__begin_repeated_err.snap @@ -0,0 +1,14 @@ +--- +source: crates/squawk_linter/src/rules/transaction_nesting.rs +expression: errors +--- +[ + Violation { + code: TransactionNesting, + message: "There is an existing transaction already in progress.", + text_range: 8..13, + help: Some( + "Put migration statements in separate files to have them be in separate transactions or don't use the assume-in-transaction setting.", + ), + }, +] diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__commit_repeated_err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__commit_repeated_err.snap new file mode 100644 index 00000000..bdb1e945 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__commit_repeated_err.snap @@ -0,0 +1,14 @@ +--- +source: crates/squawk_linter/src/rules/transaction_nesting.rs +expression: errors +--- +[ + Violation { + code: TransactionNesting, + message: "There is no transaction to `COMMIT` or `ROLLBACK`.", + text_range: 26..32, + help: Some( + "`BEGIN` a transaction at an earlier point in the migration or remove this statement.", + ), + }, +] diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__commit_with_assume_in_transaction_err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__commit_with_assume_in_transaction_err.snap new file mode 100644 index 00000000..d05823c1 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__commit_with_assume_in_transaction_err.snap @@ -0,0 +1,14 @@ +--- +source: crates/squawk_linter/src/rules/transaction_nesting.rs +expression: errors +--- +[ + Violation { + code: TransactionNesting, + message: "Attempting to end the transaction that is managed by your migration tool", + text_range: 11..17, + help: Some( + "Put migration statements in separate files to have them be in separate transactions or don't use the assume-in-transaction setting.", + ), + }, +] diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__rollback_with_assume_in_transaction_err.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__rollback_with_assume_in_transaction_err.snap new file mode 100644 index 00000000..b371afd5 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__transaction_nesting__test__rollback_with_assume_in_transaction_err.snap @@ -0,0 +1,14 @@ +--- +source: crates/squawk_linter/src/rules/transaction_nesting.rs +expression: errors +--- +[ + Violation { + code: TransactionNesting, + message: "Attempting to end the transaction that is managed by your migration tool", + text_range: 92..100, + help: Some( + "Put migration statements in separate files to have them be in separate transactions or don't use the assume-in-transaction setting.", + ), + }, +] diff --git a/crates/squawk_linter/src/rules/transaction_nesting.rs b/crates/squawk_linter/src/rules/transaction_nesting.rs new file mode 100644 index 00000000..3e99e115 --- /dev/null +++ b/crates/squawk_linter/src/rules/transaction_nesting.rs @@ -0,0 +1,181 @@ +use squawk_syntax::{ + ast::{self, AstNode, HasModuleItem}, + Parse, SourceFile, +}; + +use crate::{Linter, Rule, Violation}; + +pub(crate) fn transaction_nesting(ctx: &mut Linter, parse: &Parse) { + let file = parse.tree(); + let mut in_explicit_transaction = false; + let assume_in_transaction_help = "Put migration statements in separate files to have them be in separate transactions or don't use the assume-in-transaction setting."; + + for item in file.items() { + match item { + ast::Item::Begin(_) => { + if ctx.settings.assume_in_transaction { + ctx.report(Violation::new( + Rule::TransactionNesting, + "There is an existing transaction already in progress, managed by your migration tool.".to_string(), + item.syntax().text_range(), + assume_in_transaction_help.to_string() + )); + } else if in_explicit_transaction { + ctx.report(Violation::new( + Rule::TransactionNesting, + "There is an existing transaction already in progress.".to_string(), + item.syntax().text_range(), + assume_in_transaction_help.to_string(), + )); + } + in_explicit_transaction = true; + } + ast::Item::Commit(_) | ast::Item::Rollback(_) => { + if ctx.settings.assume_in_transaction { + ctx.report(Violation::new( + Rule::TransactionNesting, + "Attempting to end the transaction that is managed by your migration tool" + .to_string(), + item.syntax().text_range(), + assume_in_transaction_help.to_string(), + )); + } else if !in_explicit_transaction { + ctx.report(Violation::new( + Rule::TransactionNesting, + "There is no transaction to `COMMIT` or `ROLLBACK`.".to_string(), + item.syntax().text_range(), + "`BEGIN` a transaction at an earlier point in the migration or remove this statement.".to_string() + )); + } + in_explicit_transaction = false; + } + _ => (), + } + } +} + +#[cfg(test)] +mod test { + use insta::assert_debug_snapshot; + + use crate::{Linter, Rule}; + use squawk_syntax::SourceFile; + + #[test] + fn begin_repeated_err() { + let sql = r#" +BEGIN; +BEGIN; +SELECT 1; +COMMIT; + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::TransactionNesting]); + let errors = linter.lint(file, sql); + assert_ne!(errors.len(), 0); + assert_debug_snapshot!(errors); + } + + #[test] + fn commit_repeated_err() { + let sql = r#" +BEGIN; +SELECT 1; +COMMIT; +COMMIT; + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::TransactionNesting]); + let errors = linter.lint(file, sql); + assert_ne!(errors.len(), 0); + assert_debug_snapshot!(errors); + } + + #[test] + fn commit_with_assume_in_transaction_err() { + let sql = r#" +SELECT 1; +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); + assert_ne!(errors.len(), 0); + assert_debug_snapshot!(errors); + } + + #[test] + fn rollback_with_assume_in_transaction_err() { + let sql = r#" +SELECT 1; +-- Not sure why rollback would be used in a migration, but test for completeness +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); + assert_ne!(errors.len(), 0); + assert_debug_snapshot!(errors); + } + + #[test] + fn begin_assume_transaction_err() { + let sql = r#" +BEGIN; +BEGIN; +SELECT 1; +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); + assert_ne!(errors.len(), 0); + assert_debug_snapshot!(errors); + } + + #[test] + fn no_nesting_ok() { + let sql = r#" +BEGIN; +SELECT 1; +COMMIT; + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::TransactionNesting]); + let errors = linter.lint(file, sql); + assert_eq!(errors.len(), 0); + } + + #[test] + fn no_nesting_repeated_ok() { + let sql = r#" +BEGIN; +SELECT 1; +COMMIT; +-- This probably shouldn't be done in a migration. However, Squawk may be linting several +-- migrations that are concatentated, so don't raise a warning here. +BEGIN; +SELECT 2; +COMMIT; + "#; + let file = SourceFile::parse(sql); + let mut linter = Linter::from([Rule::TransactionNesting]); + let errors = linter.lint(file, sql); + assert_eq!(errors.len(), 0); + } + + #[test] + fn no_nesting_with_assume_transaction_ok() { + let sql = r#" +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); + assert_eq!(errors.len(), 0); + } +} diff --git a/crates/squawk_syntax/src/ast/nodes.rs b/crates/squawk_syntax/src/ast/nodes.rs index e81c2fcf..ef1f5b61 100644 --- a/crates/squawk_syntax/src/ast/nodes.rs +++ b/crates/squawk_syntax/src/ast/nodes.rs @@ -3091,6 +3091,30 @@ impl AstNode for CreateAggregate { } } +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct Rollback { + pub(crate) syntax: SyntaxNode, +} + +impl AstNode for Rollback { + #[inline] + fn can_cast(kind: SyntaxKind) -> bool { + kind == SyntaxKind::ROLLBACK_STMT + } + #[inline] + fn cast(syntax: SyntaxNode) -> Option { + if Self::can_cast(syntax.kind()) { + Some(Self { syntax }) + } else { + None + } + } + #[inline] + fn syntax(&self) -> &SyntaxNode { + &self.syntax + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum Item { AlterTable(AlterTable), @@ -3099,6 +3123,7 @@ pub enum Item { CreateAggregate(CreateAggregate), Begin(Begin), Commit(Commit), + Rollback(Rollback), CreateFunc(CreateFunc), CreateIndex(CreateIndex), CreateTable(CreateTable), @@ -3134,6 +3159,7 @@ impl AstNode for Item { | SyntaxKind::ALTER_DOMAIN_STMT | SyntaxKind::ALTER_AGGREGATE_STMT | SyntaxKind::CREATE_AGGREGATE_STMT + | SyntaxKind::ROLLBACK_KW ) } #[inline] @@ -3155,6 +3181,7 @@ impl AstNode for Item { SyntaxKind::ALTER_AGGREGATE_STMT => Item::AlterAggregate(AlterAggregate { syntax }), SyntaxKind::CREATE_AGGREGATE_STMT => Item::CreateAggregate(CreateAggregate { syntax }), SyntaxKind::DROP_AGGREGATE_STMT => Item::DropAggregate(DropAggregate { syntax }), + SyntaxKind::ROLLBACK_STMT => Item::Rollback(Rollback { syntax }), _ => return None, }; Some(res) @@ -3178,6 +3205,7 @@ impl AstNode for Item { Item::AlterAggregate(it) => &it.syntax, Item::CreateAggregate(it) => &it.syntax, Item::DropAggregate(it) => &it.syntax, + Item::Rollback(it) => &it.syntax, } } } diff --git a/crates/xtask/src/new_lint.rs b/crates/xtask/src/new_lint.rs index 1774bba6..7a464736 100644 --- a/crates/xtask/src/new_lint.rs +++ b/crates/xtask/src/new_lint.rs @@ -15,13 +15,13 @@ use squawk_syntax::{{ Parse, SourceFile, }}; -use crate::{{Linter, Violation, ErrorCode}}; +use crate::{{Linter, Violation, Rule}}; pub(crate) fn {rule_name}(ctx: &mut Linter, parse: &Parse) {{ let file = parse.tree(); for item in file.items() {{ + todo!(); match item {{ - // TODO: _ => (), }} }} @@ -31,7 +31,7 @@ pub(crate) fn {rule_name}(ctx: &mut Linter, parse: &Parse) {{ mod test {{ use insta::assert_debug_snapshot; - use crate::{{Linter, Rule, ErrorCode}}; + use crate::{{Linter, Rule}}; use squawk_syntax::SourceFile; #[test] @@ -42,8 +42,8 @@ mod test {{ let file = SourceFile::parse(sql); let mut linter = Linter::from([Rule::{rule_name_pascal}]); let errors = linter.lint(file, sql); - assert_ne!(linter.errors.len(), 0); - assert_debug_snapshot!(linter.errors); + assert_ne!(errors.len(), 0); + assert_debug_snapshot!(errors); }} #[test] @@ -54,7 +54,7 @@ mod test {{ let file = SourceFile::parse(sql); let mut linter = Linter::from([Rule::{rule_name_pascal}]); let errors = linter.lint(file, sql); - assert_eq!(linter.errors.len(), 0); + assert_eq!(errors.len(), 0); }} }} "###, @@ -77,6 +77,7 @@ fn crates_path() -> Result { } fn create_rule_file(name: &str) -> Result<()> { + let name = name.to_case(Case::Snake); let crates = crates_path()?; let lint_path = crates.join(format!("squawk_linter/src/rules/{}.rs", name)); @@ -85,7 +86,7 @@ fn create_rule_file(name: &str) -> Result<()> { return Ok(()); } - let lint_data = make_lint(name); + let lint_data = make_lint(&name); fs::write(&lint_path, lint_data)?; println!("created file"); Ok(()) @@ -122,7 +123,7 @@ fn update_lib(name: &str) -> Result<()> { ( "// xtask:new-lint:str-name", format!( - r#""{name_kebab}" => Ok(ErrorCode::{name_pascal}), + r#""{name_kebab}" => Ok(Rule::{name_pascal}), "#, name_kebab = name_kebab, name_pascal = name_pascal,