fix(rules): keep SQLCLEAN013 and SQLCLEAN030 fixes meaning-preserving

crates/sqlclean-cli/skill/references/docs/rules/reference.md

@@ -38,18 +38,18 @@ Run `sqlclean rules <CODE>` for a Rule's remediation.
 | `SQLCLEAN009` | Table alias is duplicated in the same query scope | Yes | — |
 | `SQLCLEAN010` | Output alias is duplicated in the same select list | Yes | — |
 | `SQLCLEAN011` | Clause mixes positional and named references | No | — |
-| `SQLCLEAN012` | ELSE NULL is redundant | No | — |
+| `SQLCLEAN012` | ELSE NULL is redundant | No | Safe |
 | `SQLCLEAN013` | DISTINCT is written as though it were a function | No | Safe |
 | `SQLCLEAN014` | Predicate compares identical constant values | No | — |
-| `SQLCLEAN015` | Empty statement follows another terminator | No | — |
+| `SQLCLEAN015` | Empty statement follows another terminator | No | Safe |
 | `SQLCLEAN016` | Expression is aliased to its existing name | No | — |
 | `SQLCLEAN017` | Row count uses a constant expression | No | Safe |
 | `SQLCLEAN018` | ROW_NUMBER has no deterministic ordering | Yes | — |
 | `SQLCLEAN019` | NOT IN list contains NULL and can reject every row | Yes | — |
 | `SQLCLEAN020` | Set-operation branches project different column counts | Yes | — |
 | `SQLCLEAN021` | Query output has an uncontrolled column shape | No | — |
 | `SQLCLEAN022` | Calculated projection has no stable output name | No | — |
-| `SQLCLEAN023` | Table alias is never referenced | No | — |
+| `SQLCLEAN023` | Table alias is never referenced | No | Safe |
 | `SQLCLEAN024` | WHERE predicate rejects NULL rows from a LEFT JOIN | No | — |
 | `SQLCLEAN025` | JOIN type is implicit | No | Safe |
 | `SQLCLEAN026` | ORDER BY mixes explicit and implicit directions | No | — |
@@ -68,7 +68,7 @@ Run `sqlclean rules <CODE>` for a Rule's remediation.
 | `SQLCLEAN039` | Inline query relation obscures data flow | Yes | — |
 | `SQLCLEAN040` | JOIN ON contains a computed expression | No | — |
 | `SQLCLEAN041` | Only the last top-level CTE may be named final, and it must use that name | No | — |
-| `SQLCLEAN042` | CTE output column is never read later in the query | No | — |
+| `SQLCLEAN042` | CTE output column is never read later in the query | No | Safe |
 | `SQLCLEAN043` | Ranking window sorts by more expressions than max_ranking_order_by | No | — |
 | `SQLCLEAN044` | String literal is longer than max_literal_length | No | — |
 | `SQLCLEAN045` | JSON payload is parsed more than once in one SELECT | No | — |

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

@@ -1706,16 +1706,25 @@ fn simple_boolean_case_span(
     let when_position = body
         .iter()
         .position(|&index| tokens[index].token_type == TokenType::When)?;
-    let then_position = body
-        .iter()
-        .position(|&index| tokens[index].token_type == TokenType::Then)?;
     let else_position = body
         .iter()
         .position(|&index| tokens[index].token_type == TokenType::Else)?;
-    let then_value = *body.get(then_position + 1)?;
     let else_value = *body.get(else_position + 1)?;
+    let mut thens = body
+        .iter()
+        .enumerate()
+        .filter(|&(_, &index)| tokens[index].token_type == TokenType::Then)
+        .peekable();
+    thens.peek()?;
+    let every_branch_returns_true = thens.all(|(offset, _)| {
+        body.get(offset + 1)
+            .is_some_and(|&value| tokens[value].token_type == TokenType::True)
+            && body.get(offset + 2).is_some_and(|&next| {
+                matches!(tokens[next].token_type, TokenType::When | TokenType::Else)
+            })
+    });
     if when_position != 0
-        || tokens[then_value].token_type != TokenType::True
+        || !every_branch_returns_true
         || tokens[else_value].token_type != TokenType::False
         || else_position + 2 != body.len() - 1
     {

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

@@ -5,15 +5,17 @@ use polyglot_sql::expressions::With;
 use polyglot_sql::tokens::{Span, Token, TokenType};
 use polyglot_sql::{DialectType, Expression, ExpressionWalk};
 
-use crate::sql_lint::constants::LINT_API_VERSION;
+use crate::sql_lint::constants::{LINT_API_VERSION, SELECT_ITEM_END_TOKENS};
 use crate::sql_lint::models::{
     AdditionalFactOptions, CrossJoinRequest, FixtureHarness, LintDiagnostic, LintEdit, LintRequest,
     LintResponse, LintRuleMetadata, ParsedSql, QueryFacts, SqlMacroOutput,
 };
 
 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() {
@@ -1461,6 +1446,7 @@ fn additional_style_diagnostics(context: &DiagnosticContext<'_>) -> Vec<LintDiag
     let DiagnosticContext {
         sql,
         facts,
+        tokens,
         enabled,
         ..
     } = context;
@@ -1501,8 +1487,8 @@ fn additional_style_diagnostics(context: &DiagnosticContext<'_>) -> Vec<LintDiag
             diagnostic(
                 &PARENTHESIZED_DISTINCT,
                 Some(*span),
-                parenthesized_distinct_fix(sql, *span),
-                Some("comments or malformed parentheses prevent a lossless rewrite"),
+                parenthesized_distinct_fix(sql, tokens, *span),
+                Some("the parentheses group an expression that continues after them, or hold comments"),
             )
         }));
     }
@@ -1736,9 +1722,13 @@ fn deletion(span: Span) -> LintEdit {
     }
 }
 
-fn parenthesized_distinct_fix(sql: &str, span: Span) -> Option<LintEdit> {
+/// `DISTINCT <expression>`, only when the parentheses hold the whole select item, so removing them cannot regroup an operator.
+fn parenthesized_distinct_fix(sql: &str, tokens: &[Token], span: Span) -> Option<LintEdit> {
     let source = char_slice(sql, span.start, span.end)?;
-    if contains_comment(source) {
+    let close = tokens.iter().position(|token| token.span.end == span.end)?;
+    let item_ends = significant_after(tokens, close)
+        .is_none_or(|next| SELECT_ITEM_END_TOKENS.contains(&tokens[next].token_type));
+    if contains_comment(source) || !item_ends {
         return None;
     }
     let open = source.find('(')?;

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/constants.rs

@@ -1,3 +1,5 @@
+use polyglot_sql::tokens::TokenType;
+
 pub(crate) const CAST_TYPE_SEPARATOR_KEYWORD: &str = "AS";
 pub(crate) const CLOSE_PARENTHESIS: &str = ")";
 pub const LINT_API_VERSION: u32 = 1;
@@ -59,3 +61,14 @@ pub(crate) const CROSS_JOIN_UNNAMED_MESSAGE: &str = "CROSS JOIN creates an expli
 /// Stands for the finding's project-relative path until the caller that knows it fills it in.
 pub(crate) const CROSS_JOIN_PATH_PLACEHOLDER: &str = "<path of this file>";
 pub(crate) const CROSS_JOIN_REASON_PLACEHOLDER: &str = "<why this deviation is intentional>";
+/// Tokens that end a select item, so parentheses closed just before one cannot be regrouping an operator.
+pub(crate) const SELECT_ITEM_END_TOKENS: [TokenType; 8] = [
+    TokenType::Comma,
+    TokenType::From,
+    TokenType::As,
+    TokenType::Semicolon,
+    TokenType::RParen,
+    TokenType::Var,
+    TokenType::Identifier,
+    TokenType::QuotedIdentifier,
+];

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",
@@ -876,6 +888,13 @@ fn given_additional_rule_cases_when_linting_then_findings_and_fixes_match() -> R
             expected_anchor: Some("DISTINCT(a)"),
             expected_replacement: Some("DISTINCT a"),
         },
+        test_types::AdditionalLintRuleTestCase {
+            description: "function-like distinct whose expression continues keeps its grouping",
+            sql: "SELECT DISTINCT(price + tax) * 2 AS total FROM orders",
+            rule: "SQLCLEAN013",
+            expected_anchor: Some("DISTINCT(price + tax)"),
+            expected_replacement: None,
+        },
         test_types::AdditionalLintRuleTestCase {
             description: "constant scaffold predicate",
             sql: "SELECT a FROM items WHERE 1 = 1",
@@ -1375,6 +1394,13 @@ fn given_additional_rule_cases_when_linting_then_findings_and_fixes_match() -> R
             ),
             expected_replacement: None,
         },
+        test_types::AdditionalLintRuleTestCase {
+            description: "boolean case whose branch continues after TRUE is not a plain conversion",
+            sql: "SELECT CASE WHEN is_paid THEN TRUE AND is_shipped ELSE FALSE END AS done FROM orders",
+            rule: "SQLCLEAN030",
+            expected_anchor: None,
+            expected_replacement: None,
+        },
         test_types::AdditionalLintRuleTestCase {
             description: "case nested directly in else",
             sql: "SELECT CASE WHEN a THEN 1 ELSE CASE WHEN b THEN 2 END END FROM items",

website/src/content/docs/docs/rules/reference.mdx

@@ -24,18 +24,18 @@ Run `sqlclean rules <CODE>` for a Rule's remediation.
 | `SQLCLEAN009` | Table alias is duplicated in the same query scope | Yes | — |
 | `SQLCLEAN010` | Output alias is duplicated in the same select list | Yes | — |
 | `SQLCLEAN011` | Clause mixes positional and named references | No | — |
-| `SQLCLEAN012` | ELSE NULL is redundant | No | — |
+| `SQLCLEAN012` | ELSE NULL is redundant | No | Safe |
 | `SQLCLEAN013` | DISTINCT is written as though it were a function | No | Safe |
 | `SQLCLEAN014` | Predicate compares identical constant values | No | — |
-| `SQLCLEAN015` | Empty statement follows another terminator | No | — |
+| `SQLCLEAN015` | Empty statement follows another terminator | No | Safe |
 | `SQLCLEAN016` | Expression is aliased to its existing name | No | — |
 | `SQLCLEAN017` | Row count uses a constant expression | No | Safe |
 | `SQLCLEAN018` | ROW_NUMBER has no deterministic ordering | Yes | — |
 | `SQLCLEAN019` | NOT IN list contains NULL and can reject every row | Yes | — |
 | `SQLCLEAN020` | Set-operation branches project different column counts | Yes | — |
 | `SQLCLEAN021` | Query output has an uncontrolled column shape | No | — |
 | `SQLCLEAN022` | Calculated projection has no stable output name | No | — |
-| `SQLCLEAN023` | Table alias is never referenced | No | — |
+| `SQLCLEAN023` | Table alias is never referenced | No | Safe |
 | `SQLCLEAN024` | WHERE predicate rejects NULL rows from a LEFT JOIN | No | — |
 | `SQLCLEAN025` | JOIN type is implicit | No | Safe |
 | `SQLCLEAN026` | ORDER BY mixes explicit and implicit directions | No | — |
@@ -54,7 +54,7 @@ Run `sqlclean rules <CODE>` for a Rule's remediation.
 | `SQLCLEAN039` | Inline query relation obscures data flow | Yes | — |
 | `SQLCLEAN040` | JOIN ON contains a computed expression | No | — |
 | `SQLCLEAN041` | Only the last top-level CTE may be named final, and it must use that name | No | — |
-| `SQLCLEAN042` | CTE output column is never read later in the query | No | — |
+| `SQLCLEAN042` | CTE output column is never read later in the query | No | Safe |
 | `SQLCLEAN043` | Ranking window sorts by more expressions than max_ranking_order_by | No | — |
 | `SQLCLEAN044` | String literal is longer than max_literal_length | No | — |
 | `SQLCLEAN045` | JSON payload is parsed more than once in one SELECT | No | — |