feat(linter rule): no-not-in, no-wildcard-projection

Cargo.toml

@@ -18,6 +18,7 @@ repository = "https://github.com/future-architect/uroborosql-fmt"
 [workspace.dependencies]
 # Internal crates
 uroborosql-fmt = { path = "./crates/uroborosql-fmt" }
+postgresql-cst-parser = { git = "https://github.com/future-architect/postgresql-cst-parser", branch = "feat/new-apis-for-linter" }
 
 [profile.release]
 lto = true

crates/uroborosql-fmt/Cargo.toml

@@ -21,7 +21,7 @@ serde_json = "1.0.91"
 thiserror = "1.0.38"
 
 # git config --global core.longpaths true を管理者権限で実行しないとけない
-postgresql-cst-parser = { git = "https://github.com/future-architect/postgresql-cst-parser" }
+postgresql-cst-parser = { workspace = true }
 
 [dev-dependencies]
 console = "0.15.10"

crates/uroborosql-lint/Cargo.toml

@@ -7,4 +7,4 @@ license.workspace = true
 repository.workspace = true
 
 [dependencies]
-postgresql-cst-parser = { git = "https://github.com/future-architect/postgresql-cst-parser", branch = "feat/new-apis-for-linter" }
+postgresql-cst-parser = { workspace = true }

crates/uroborosql-lint/src/linter.rs

@@ -2,7 +2,7 @@ use crate::{
     context::LintContext,
     diagnostic::{Diagnostic, Severity},
     rule::Rule,
-    rules::{NoDistinct, NoUnionDistinct, TooLargeInList},
+    rules::{NoDistinct, NoNotIn, NoUnionDistinct, NoWildcardProjection, TooLargeInList},
     tree::collect_preorder,
 };
 use postgresql_cst_parser::tree_sitter;
@@ -105,7 +105,9 @@ impl Linter {
 fn default_rules() -> Vec<Box<dyn Rule>> {
     vec![
         Box::new(NoDistinct),
+        Box::new(NoNotIn),
         Box::new(NoUnionDistinct),
+        Box::new(NoWildcardProjection),
         Box::new(TooLargeInList),
     ]
 }

crates/uroborosql-lint/src/rules.rs

@@ -1,7 +1,11 @@
 mod no_distinct;
+mod no_not_in;
 mod no_union_distinct;
+mod no_wildcard_projection;
 mod too_large_in_list;
 
 pub use no_distinct::NoDistinct;
+pub use no_not_in::NoNotIn;
 pub use no_union_distinct::NoUnionDistinct;
+pub use no_wildcard_projection::NoWildcardProjection;
 pub use too_large_in_list::TooLargeInList;

crates/uroborosql-lint/src/rules/no_not_in.rs

@@ -0,0 +1,125 @@
+use crate::{
+    context::LintContext,
+    diagnostic::{Diagnostic, Severity},
+    rule::Rule,
+    tree::prev_node_skipping_comments,
+};
+use postgresql_cst_parser::{
+    syntax_kind::SyntaxKind,
+    tree_sitter::{Node, Range},
+};
+
+/// Detects NOT IN expressions.
+/// Rule source: https://future-architect.github.io/coding-standards/documents/forSQL/SQL%E3%82%B3%E3%83%BC%E3%83%87%E3%82%A3%E3%83%B3%E3%82%B0%E8%A6%8F%E7%B4%84%EF%BC%88PostgreSQL%EF%BC%89.html#not-in-%E5%8F%A5
+pub struct NoNotIn;
+
+impl Rule for NoNotIn {
+    fn name(&self) -> &'static str {
+        "no-not-in"
+    }
+
+    fn default_severity(&self) -> Severity {
+        Severity::Warning
+    }
+
+    fn target_kinds(&self) -> &'static [SyntaxKind] {
+        &[SyntaxKind::in_expr]
+    }
+
+    fn run_on_node<'tree>(&self, node: &Node<'tree>, ctx: &mut LintContext, severity: Severity) {
+        let Some(range) = detect_not_in(node) else {
+            return;
+        };
+
+        let diagnostic = Diagnostic::new(
+            self.name(),
+            severity,
+            "Avoid using NOT IN; prefer NOT EXISTS or other alternatives to handle NULL correctly.",
+            &range,
+        );
+        ctx.report(diagnostic);
+    }
+}
+
+fn detect_not_in(node: &Node<'_>) -> Option<Range> {
+    // Detects `NOT_LA IN_P in_expr` sequense.
+    // We traverse siblings backwards, so the expected order is `in_expr`, `IN_P`, `NOT_LA`.
+
+    let in_expr_node = node;
+    if in_expr_node.kind() != SyntaxKind::in_expr {
+        return None;
+    }
+
+    // IN_P
+    let in_node = prev_node_skipping_comments(in_expr_node)?;
+    if in_node.kind() != SyntaxKind::IN_P {
+        return None;
+    }
+
+    // NOT_LA
+    let not_node = prev_node_skipping_comments(&in_node)?;
+    if not_node.kind() != SyntaxKind::NOT_LA {
+        return None;
+    }
+
+    Some(not_node.range().extended_by(&in_expr_node.range()))
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{linter::tests::run_with_rules, SqlSpan};
+
+    #[test]
+    fn detects_simple_not_in() {
+        let sql = "SELECT value FROM users WHERE id NOT IN (1, 2);";
+        let diagnostics = run_with_rules(sql, vec![Box::new(NoNotIn)]);
+
+        let diagnostic = diagnostics
+            .iter()
+            .find(|diag| diag.rule_id == "no-not-in")
+            .expect("expected NOT IN to be detected");
+
+        let SqlSpan { start, end } = diagnostic.span;
+        assert_eq!(&sql[start.byte..end.byte], "NOT IN (1, 2)");
+    }
+
+    #[test]
+    fn detects_not_in_with_comment() {
+        let sql = "SELECT value FROM users WHERE id NOT /* comment */ IN (1);";
+        let diagnostics = run_with_rules(sql, vec![Box::new(NoNotIn)]);
+
+        let diagnostic = diagnostics
+            .iter()
+            .find(|diag| diag.rule_id == "no-not-in")
+            .expect("expected NOT IN to be detected");
+
+        let SqlSpan { start, end } = diagnostic.span;
+        assert_eq!(&sql[start.byte..end.byte], "NOT /* comment */ IN (1)");
+    }
+
+    #[test]
+    fn detects_not_in_with_subquery() {
+        let sql = "SELECT value FROM users WHERE id NOT IN (SELECT id FROM admins);";
+        let diagnostics = run_with_rules(sql, vec![Box::new(NoNotIn)]);
+
+        assert!(
+            diagnostics.iter().any(|diag| diag.rule_id == "no-not-in"),
+            "expected NOT IN subquery to be detected"
+        );
+    }
+
+    #[test]
+    fn allows_in_without_not() {
+        let sql = "SELECT value FROM users WHERE id IN (1, 2);";
+        let diagnostics = run_with_rules(sql, vec![Box::new(NoNotIn)]);
+        assert!(diagnostics.is_empty());
+    }
+
+    #[test]
+    fn allows_not_between() {
+        let sql = "SELECT value FROM users WHERE id NOT BETWEEN 1 AND 5;";
+        let diagnostics = run_with_rules(sql, vec![Box::new(NoNotIn)]);
+        assert!(diagnostics.is_empty(), "NOT BETWEEN should be allowed");
+    }
+}

crates/uroborosql-lint/src/rules/no_wildcard_projection.rs

@@ -0,0 +1,132 @@
+use crate::{
+    context::LintContext,
+    diagnostic::{Diagnostic, Severity},
+    rule::Rule,
+};
+use postgresql_cst_parser::{
+    syntax_kind::SyntaxKind,
+    tree_sitter::{Node, Range},
+};
+
+/// Detects wildcard projections. (e.g. `SELECT *`, `SELECT u.*`, `RETURNING *`)
+/// Rule source: https://future-architect.github.io/coding-standards/documents/forSQL/SQL%E3%82%B3%E3%83%BC%E3%83%87%E3%82%A3%E3%83%B3%E3%82%B0%E8%A6%8F%E7%B4%84%EF%BC%88PostgreSQL%EF%BC%89.html#%E6%A4%9C%E7%B4%A2:~:text=%E3%82%92%E6%8C%87%E5%AE%9A%E3%81%99%E3%82%8B-,%E5%85%A8%E5%88%97%E3%83%AF%E3%82%A4%E3%83%AB%E3%83%89%E3%82%AB%E3%83%BC%E3%83%89%E3%80%8C*%E3%80%8D%E3%81%AE%E4%BD%BF%E7%94%A8%E3%81%AF%E3%81%9B%E3%81%9A%E3%80%81%E3%82%AB%E3%83%A9%E3%83%A0%E5%90%8D%E3%82%92%E6%98%8E%E8%A8%98%E3%81%99%E3%82%8B,-%E3%82%A4%E3%83%B3%E3%83%87%E3%83%83%E3%82%AF%E3%82%B9%E3%81%AB%E3%82%88%E3%82%8B%E6%A4%9C%E7%B4%A2
+pub struct NoWildcardProjection;
+
+impl Rule for NoWildcardProjection {
+    fn name(&self) -> &'static str {
+        "no-wildcard-projection"
+    }
+
+    fn default_severity(&self) -> Severity {
+        Severity::Warning
+    }
+
+    fn target_kinds(&self) -> &'static [SyntaxKind] {
+        &[SyntaxKind::target_el]
+    }
+
+    fn run_on_node<'tree>(&self, node: &Node<'tree>, ctx: &mut LintContext, severity: Severity) {
+        let Some(range) = detect_wildcard(node) else {
+            return;
+        };
+
+        let diagnostic = Diagnostic::new(
+            self.name(),
+            severity,
+            "Wildcard projections are not allowed; list the columns explicitly.",
+            &range,
+        );
+        ctx.report(diagnostic);
+    }
+}
+
+fn detect_wildcard(target_el_node: &Node<'_>) -> Option<Range> {
+    assert_eq!(target_el_node.kind(), SyntaxKind::target_el);
+
+    // If the last node (including the entire subtree) under target_el is '*', it is considered a wildcard.
+    let last_node = target_el_node.last_node()?;
+
+    if last_node.kind() == SyntaxKind::Star {
+        return Some(last_node.range());
+    }
+
+    None
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{linter::tests::run_with_rules, SqlSpan};
+
+    fn run(sql: &str) -> Vec<Diagnostic> {
+        run_with_rules(sql, vec![Box::new(NoWildcardProjection)])
+    }
+
+    #[test]
+    fn detects_select_star() {
+        let sql = "SELECT * FROM users;";
+        let diagnostics = run(sql);
+
+        let diagnostic = diagnostics
+            .iter()
+            .find(|diag| diag.rule_id == "no-wildcard-projection")
+            .expect("should detect SELECT *");
+
+        let SqlSpan { start, end } = diagnostic.span;
+        assert_eq!(&sql[start.byte..end.byte], "*");
+    }
+
+    #[test]
+    fn detects_returning_star() {
+        let sql = "INSERT INTO users(id) VALUES (1) RETURNING *;";
+        let diagnostics = run(sql);
+
+        let diagnostic = diagnostics
+            .iter()
+            .find(|diag| diag.rule_id == "no-wildcard-projection")
+            .expect("should detect RETURNING *");
+
+        let SqlSpan { start, end } = diagnostic.span;
+        assert_eq!(&sql[start.byte..end.byte], "*");
+    }
+
+    #[test]
+    fn detects_table_star() {
+        let sql = "SELECT u.* FROM users u;";
+        let diagnostics = run(sql);
+        let diagnostic = diagnostics
+            .iter()
+            .find(|diag| diag.rule_id == "no-wildcard-projection")
+            .expect("should detect *");
+
+        let SqlSpan { start, end } = diagnostic.span;
+        assert_eq!(&sql[start.byte..end.byte], "*");
+    }
+
+    #[test]
+    fn detects_parenthesized_star() {
+        let sql = "SELECT (u).* FROM users u;";
+        let diagnostics = run(sql);
+        let diagnostic = diagnostics
+            .iter()
+            .find(|diag| diag.rule_id == "no-wildcard-projection")
+            .expect("should detect *");
+
+        let SqlSpan { start, end } = diagnostic.span;
+        assert_eq!(&sql[start.byte..end.byte], "*");
+    }
+
+    #[test]
+    fn allows_explicit_columns() {
+        let sql = "SELECT id, name FROM users;";
+        let diagnostics = run(sql);
+        assert!(diagnostics.is_empty());
+    }
+
+    #[test]
+    fn allows_count_star() {
+        let sql = "SELECT count(*) FROM users;";
+        let diagnostics = run(sql);
+        assert!(diagnostics.is_empty(), "count(*) should be allowed");
+    }
+}

crates/uroborosql-lint/src/tree.rs

@@ -32,3 +32,16 @@ fn walk_preorder<'tree, T>(
         }
     }
 }
+
+/// Returns the previous sibling node, skipping comment nodes.
+/// Returns `None` if no non-comment sibling is found.
+pub fn prev_node_skipping_comments<'a>(node: &Node<'a>) -> Option<Node<'a>> {
+    let mut current = node.clone();
+    loop {
+        let prev = current.prev_sibling()?;
+        if !prev.is_comment() {
+            return Some(prev);
+        }
+        current = prev;
+    }
+}