fix(rules): treat a parenthesized JOIN condition as a condition

SQLCLEAN003 read only the direct tokens after ON, so ON (a = b OR c = d) looked empty and was reported. It now reads nested tokens too, while ON (1 = 1) is still reported.

Author: kvlonge

crates/sqlclean/src/sql_lint/_helpers/engine.rs

@@ -13,7 +13,9 @@ use crate::sql_lint::models::{
 
 use crate::sql_lint::_helpers::additional::collect_additional_facts;
 use crate::sql_lint::_helpers::inline_relations::collect_inline_query_relation_spans;
-use crate::sql_lint::_helpers::join_predicates::cross_join_diagnostics;
+use crate::sql_lint::_helpers::join_predicates::{
+    condition_tokens, cross_join_diagnostics, trivial_on_condition,
+};
 use crate::sql_lint::_helpers::parsing::parse;
 use crate::sql_lint::_helpers::terminal_shape::collect_terminal_shape_facts;
 
@@ -213,7 +215,6 @@ const INLINE_QUERY_RELATION: LintRuleMetadata = LintRuleMetadata {
     message: "Inline query relation obscures data flow",
     remediation: "Extract the derived relation into a named top-level CTE; use explicit LATERAL only when the relation must remain correlated.",
 };
-const TRIVIAL_EQUALITY_TOKEN_COUNT: usize = 3;
 const JOIN_EXPRESSION: LintRuleMetadata = LintRuleMetadata {
     code: "SQLCLEAN040",
     message: "JOIN ON contains a computed expression",
@@ -901,8 +902,10 @@ fn collect_from_and_join_facts(tokens: &[Token], direct: &[usize]) -> (Vec<Span>
             .iter()
             .any(|&candidate| tokens[candidate].token_type == TokenType::Using);
         if !has_using
-            && on_position
-                .is_none_or(|offset| trivial_on_condition(tokens, &condition[offset + 1..]))
+            && on_position.is_none_or(|offset| {
+                let end = direct.get(condition_end).copied().unwrap_or(tokens.len());
+                trivial_on_condition(tokens, &condition_tokens(tokens, condition[offset], end))
+            })
         {
             missing_conditions.push(tokens[index].span);
         }
@@ -943,24 +946,6 @@ fn is_conditionless_join(tokens: &[Token], direct: &[usize], join_position: usiz
         })
 }
 
-fn trivial_on_condition(tokens: &[Token], condition: &[usize]) -> bool {
-    let significant: Vec<&Token> = condition
-        .iter()
-        .map(|&index| &tokens[index])
-        .filter(|token| !matches!(token.token_type, TokenType::LParen | TokenType::RParen))
-        .collect();
-    significant.is_empty()
-        || significant.len() == 1 && significant[0].token_type == TokenType::True
-        || significant.len() == TRIVIAL_EQUALITY_TOKEN_COUNT
-            && significant[1].token_type == TokenType::Eq
-            && matches!(
-                significant[0].token_type,
-                TokenType::Number | TokenType::String | TokenType::True | TokenType::False
-            )
-            && significant[0].token_type == significant[2].token_type
-            && significant[0].text == significant[2].text
-}
-
 fn collect_set_operation_facts(tokens: &[Token], depths: &[usize]) -> Vec<Span> {
     let mut spans: Vec<Span> = Vec::new();
     for (index, token) in tokens.iter().enumerate() {

crates/sqlclean/src/sql_lint/_helpers/join_predicates.rs

@@ -9,13 +9,17 @@ use polyglot_sql::tokens::{Span, Token, TokenType};
 use polyglot_sql::{DialectType, ExpressionWalk};
 
 use crate::constants::SETTING_SNIPPET_INDENT;
+use crate::sql_lint::_helpers::engine::{is_comment, is_layout};
 use crate::sql_lint::constants::{
     CROSS_JOIN_CTE_MARKER, CROSS_JOIN_PATH_PLACEHOLDER, CROSS_JOIN_PREFIX,
     CROSS_JOIN_REASON_PLACEHOLDER, CROSS_JOIN_SUFFIX, CROSS_JOIN_UNNAMED_MESSAGE,
 };
 use crate::sql_lint::models::{CrossJoinRequest, LintDiagnostic, LintRuleMetadata};
 
 const CROSS_JOIN_FIX_REASON: &str = "a keyed relationship requires user intent";
+
+/// Tokens in a constant `ON` condition such as `1 = 1`.
+const TRIVIAL_EQUALITY_TOKEN_COUNT: usize = 3;
 const FIRST_ROW: &str = "1";
 const ROW_GENERATOR_FUNCTIONS: [&str; 16] = [
     "unnest",
@@ -692,3 +696,39 @@ fn relation_anchor_candidates(tokens: &[Token], significant: &[usize], cross: Sp
     }
     candidates
 }
+
+/// Every token after `ON` before `end`, including those nested in parentheses, stopping at a parenthesis the condition did not open.
+pub(super) fn condition_tokens(tokens: &[Token], on: usize, end: usize) -> Vec<usize> {
+    let mut open: usize = 0;
+    let mut found: Vec<usize> = Vec::new();
+    for (index, token) in tokens.iter().enumerate().take(end).skip(on + 1) {
+        match token.token_type {
+            TokenType::LParen => open += 1,
+            TokenType::RParen if open == 0 => break,
+            TokenType::RParen => open -= 1,
+            _ => {}
+        }
+        if !is_layout(token) && !is_comment(token) {
+            found.push(index);
+        }
+    }
+    found
+}
+
+pub(super) fn trivial_on_condition(tokens: &[Token], condition: &[usize]) -> bool {
+    let significant: Vec<&Token> = condition
+        .iter()
+        .map(|&index| &tokens[index])
+        .filter(|token| !matches!(token.token_type, TokenType::LParen | TokenType::RParen))
+        .collect();
+    significant.is_empty()
+        || significant.len() == 1 && significant[0].token_type == TokenType::True
+        || significant.len() == TRIVIAL_EQUALITY_TOKEN_COUNT
+            && significant[1].token_type == TokenType::Eq
+            && matches!(
+                significant[0].token_type,
+                TokenType::Number | TokenType::String | TokenType::True | TokenType::False
+            )
+            && significant[0].token_type == significant[2].token_type
+            && significant[0].text == significant[2].text
+}

crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs

@@ -78,6 +78,18 @@ fn given_sql_cases_when_linting_then_diagnostics_match() -> Result<(), String> {
             expected_codes: &["SQLCLEAN003"],
             expected_anchors: &[("SQLCLEAN003", "JOIN")],
         },
+        test_types::LintTestCase {
+            description: "parenthesized join condition is a condition",
+            sql: "SELECT a.id FROM a JOIN b ON (a.id = b.id OR a.status = 'new')",
+            expected_codes: &[],
+            expected_anchors: &[],
+        },
+        test_types::LintTestCase {
+            description: "parenthesized constant join condition is no condition",
+            sql: "SELECT a.id FROM a JOIN b ON (1 = 1)",
+            expected_codes: &["SQLCLEAN003"],
+            expected_anchors: &[("SQLCLEAN003", "JOIN")],
+        },
         test_types::LintTestCase {
             description: "unordered limit",
             sql: "SELECT id FROM items LIMIT 1",