From cec0d38a95c5cd0a7521ceb1c858884c30d51c5a Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sat, 31 Oct 2026 12:59:20 +0000 Subject: [PATCH 1/2] fix(rules): keep SQLCLEAN013 and SQLCLEAN030 fixes meaning-preserving SQLCLEAN013 removed the parentheses of DISTINCT(price + tax) * 2, which regrouped the expression as price + tax * 2. It now offers the fix only when the parentheses hold the whole select item. SQLCLEAN030 matched CASE WHEN a THEN TRUE AND b ELSE FALSE END and rewrote it to COALESCE(a, FALSE), dropping AND b. It now reports only a CASE whose every branch returns exactly TRUE. The Rule reference also lists the safe fixes of SQLCLEAN012, 015, 023 and 042, which it omitted. --- .../skill/references/docs/rules/reference.md | 8 ++++---- .../src/sql_lint/_helpers/additional.rs | 19 ++++++++++++++----- .../sqlclean/src/sql_lint/_helpers/engine.rs | 15 ++++++++++----- crates/sqlclean/src/sql_lint/constants.rs | 13 +++++++++++++ .../src/sql_lint/tests/test_sql_lint.rs | 14 ++++++++++++++ .../src/content/docs/docs/rules/reference.mdx | 8 ++++---- 6 files changed, 59 insertions(+), 18 deletions(-) diff --git a/crates/sqlclean-cli/skill/references/docs/rules/reference.md b/crates/sqlclean-cli/skill/references/docs/rules/reference.md index 8299a30..b9b6a19 100644 --- a/crates/sqlclean-cli/skill/references/docs/rules/reference.md +++ b/crates/sqlclean-cli/skill/references/docs/rules/reference.md @@ -38,10 +38,10 @@ Run `sqlclean rules ` 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 | — | @@ -49,7 +49,7 @@ Run `sqlclean rules ` for a Rule's remediation. | `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 ` 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 | — | diff --git a/crates/sqlclean/src/sql_lint/_helpers/additional.rs b/crates/sqlclean/src/sql_lint/_helpers/additional.rs index 369923f..f18e557 100644 --- a/crates/sqlclean/src/sql_lint/_helpers/additional.rs +++ b/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 { diff --git a/crates/sqlclean/src/sql_lint/_helpers/engine.rs b/crates/sqlclean/src/sql_lint/_helpers/engine.rs index cb3b3d2..26c234d 100644 --- a/crates/sqlclean/src/sql_lint/_helpers/engine.rs +++ b/crates/sqlclean/src/sql_lint/_helpers/engine.rs @@ -5,7 +5,7 @@ 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, @@ -1461,6 +1461,7 @@ fn additional_style_diagnostics(context: &DiagnosticContext<'_>) -> Vec) -> Vec LintEdit { } } -fn parenthesized_distinct_fix(sql: &str, span: Span) -> Option { +/// `DISTINCT `, 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 { 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('(')?; diff --git a/crates/sqlclean/src/sql_lint/constants.rs b/crates/sqlclean/src/sql_lint/constants.rs index eead024..908275a 100644 --- a/crates/sqlclean/src/sql_lint/constants.rs +++ b/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 = ""; pub(crate) const CROSS_JOIN_REASON_PLACEHOLDER: &str = ""; +/// 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, +]; diff --git a/crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs b/crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs index 909602d..2fe5b11 100644 --- a/crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs +++ b/crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs @@ -876,6 +876,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 +1382,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", diff --git a/website/src/content/docs/docs/rules/reference.mdx b/website/src/content/docs/docs/rules/reference.mdx index 97149a1..f75475a 100644 --- a/website/src/content/docs/docs/rules/reference.mdx +++ b/website/src/content/docs/docs/rules/reference.mdx @@ -24,10 +24,10 @@ Run `sqlclean rules ` 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 | — | @@ -35,7 +35,7 @@ Run `sqlclean rules ` for a Rule's remediation. | `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 ` 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 | — | From 443587a4e22c6a4cb4ca8ea86a09f09e4baec0bb Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sat, 31 Oct 2026 13:28:16 +0000 Subject: [PATCH 2/2] 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. --- .../sqlclean/src/sql_lint/_helpers/engine.rs | 29 ++++---------- .../src/sql_lint/_helpers/join_predicates.rs | 40 +++++++++++++++++++ .../src/sql_lint/tests/test_sql_lint.rs | 12 ++++++ 3 files changed, 59 insertions(+), 22 deletions(-) diff --git a/crates/sqlclean/src/sql_lint/_helpers/engine.rs b/crates/sqlclean/src/sql_lint/_helpers/engine.rs index 26c234d..5ad2889 100644 --- a/crates/sqlclean/src/sql_lint/_helpers/engine.rs +++ b/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 .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 { let mut spans: Vec = Vec::new(); for (index, token) in tokens.iter().enumerate() { diff --git a/crates/sqlclean/src/sql_lint/_helpers/join_predicates.rs b/crates/sqlclean/src/sql_lint/_helpers/join_predicates.rs index 2b5453d..a20b443 100644 --- a/crates/sqlclean/src/sql_lint/_helpers/join_predicates.rs +++ b/crates/sqlclean/src/sql_lint/_helpers/join_predicates.rs @@ -9,6 +9,7 @@ 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, @@ -16,6 +17,9 @@ use crate::sql_lint::constants::{ 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 { + let mut open: usize = 0; + let mut found: Vec = 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 +} diff --git a/crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs b/crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs index 2fe5b11..ae92f58 100644 --- a/crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs +++ b/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",