From 7441d5587b0fcef0e7b4caa9e8f0a9516d774183 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 1 Nov 2026 05:19:56 +0000 Subject: [PATCH] fix(rules): let SQLCLEAN004 skip constant-only projections A query whose select list holds only constants returns identical rows whichever ones LIMIT or OFFSET keeps, so its row selection is not nondeterministic. Column references, function calls and stars still report. --- .../skill/references/docs/rules/reference.md | 3 +- .../sqlclean/src/sql_lint/_helpers/engine.rs | 33 ++------- .../src/sql_lint/_helpers/terminal_shape.rs | 70 +++++++++++++++++++ .../sql_lint/tests/test_lint_exemptions.rs | 45 +++++++++++- .../src/content/docs/docs/rules/reference.mdx | 3 +- 5 files changed, 123 insertions(+), 31 deletions(-) diff --git a/crates/sqlclean-cli/skill/references/docs/rules/reference.md b/crates/sqlclean-cli/skill/references/docs/rules/reference.md index b9b6a19..3f03d68 100644 --- a/crates/sqlclean-cli/skill/references/docs/rules/reference.md +++ b/crates/sqlclean-cli/skill/references/docs/rules/reference.md @@ -213,7 +213,8 @@ FROM customer_orders ``` `SQLCLEAN004` reports the same problem for `LIMIT` and `OFFSET`; see the example in -[Rules](overview.md). +[Rules](overview.md). It skips a query that selects only constants, such as +`SELECT 1 AS present FROM orders LIMIT 1`, because every row it could return is identical. ## SQLCLEAN019: NULL in NOT IN diff --git a/crates/sqlclean/src/sql_lint/_helpers/engine.rs b/crates/sqlclean/src/sql_lint/_helpers/engine.rs index 4d28192..93b4225 100644 --- a/crates/sqlclean/src/sql_lint/_helpers/engine.rs +++ b/crates/sqlclean/src/sql_lint/_helpers/engine.rs @@ -17,7 +17,10 @@ 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; +use crate::sql_lint::_helpers::terminal_shape::{ + collect_terminal_shape_facts, first_row_selection_clause, limits_to_no_rows, + projects_only_constants, +}; const NULL_COMPARISON: LintRuleMetadata = LintRuleMetadata { code: "SQLCLEAN001", @@ -539,6 +542,7 @@ fn collect_token_query_facts( if order.is_none() && let Some(index) = limit && !limits_to_no_rows(tokens, index) + && !projects_only_constants(tokens, &direct, index) { facts.unordered_limits.push(tokens[index].span); } @@ -704,33 +708,6 @@ fn first_type(tokens: &[Token], indices: &[usize], token_type: TokenType) -> Opt .find(|&index| tokens[index].token_type == token_type) } -fn first_row_selection_clause(tokens: &[Token], direct: &[usize]) -> Option { - let from = direct - .iter() - .copied() - .find(|&index| is_query_from(tokens, index)); - direct.iter().copied().find(|&index| { - matches!( - tokens[index].token_type, - TokenType::Limit | TokenType::Offset - ) && from.is_none_or(|from_index| index > from_index) - && significant_before(tokens, index).is_none_or(|previous| { - !matches!( - tokens[previous].token_type, - TokenType::Colon | TokenType::DColon | TokenType::Dot | TokenType::DotColon - ) - }) - }) -} - -/// Whether a row-selection clause is `LIMIT 0`, which selects no rows in any order. -fn limits_to_no_rows(tokens: &[Token], index: usize) -> bool { - tokens[index].token_type == TokenType::Limit - && tokens.get(index + 1).is_some_and(|count| { - count.token_type == TokenType::Number && count.text.parse::() == Ok(0) - }) -} - pub(super) fn is_query_from(tokens: &[Token], index: usize) -> bool { tokens[index].token_type == TokenType::From && significant_before(tokens, index) diff --git a/crates/sqlclean/src/sql_lint/_helpers/terminal_shape.rs b/crates/sqlclean/src/sql_lint/_helpers/terminal_shape.rs index 73637fa..3c377af 100644 --- a/crates/sqlclean/src/sql_lint/_helpers/terminal_shape.rs +++ b/crates/sqlclean/src/sql_lint/_helpers/terminal_shape.rs @@ -1,5 +1,7 @@ use polyglot_sql::tokens::{Span, Token, TokenType}; +use crate::sql_lint::_helpers::engine::{is_query_from, significant_before}; + const CEREMONIAL_SELECT_LITERAL: &str = "1"; pub(super) fn final_cte_name_spans(tokens: &[Token], depths: &[usize]) -> Vec { @@ -301,3 +303,71 @@ fn is_terminal_from(tokens: &[Token], significant: &[usize], index: usize) -> bo .and_then(|position| position.checked_sub(1)) .is_none_or(|position| tokens[significant[position]].token_type != TokenType::Distinct) } + +pub(super) fn first_row_selection_clause(tokens: &[Token], direct: &[usize]) -> Option { + let from = direct + .iter() + .copied() + .find(|&index| is_query_from(tokens, index)); + direct.iter().copied().find(|&index| { + matches!( + tokens[index].token_type, + TokenType::Limit | TokenType::Offset + ) && from.is_none_or(|from_index| index > from_index) + && significant_before(tokens, index).is_none_or(|previous| { + !matches!( + tokens[previous].token_type, + TokenType::Colon | TokenType::DColon | TokenType::Dot | TokenType::DotColon + ) + }) + }) +} + +/// Whether a row-selection clause is `LIMIT 0`, which selects no rows in any order. +pub(super) fn limits_to_no_rows(tokens: &[Token], index: usize) -> bool { + tokens[index].token_type == TokenType::Limit + && tokens.get(index + 1).is_some_and(|count| { + count.token_type == TokenType::Number && count.text.parse::() == Ok(0) + }) +} + +/// Whether the projection before `limit` holds only constants, so every row it could select is identical. +pub(super) fn projects_only_constants(tokens: &[Token], direct: &[usize], limit: usize) -> bool { + let Some(&first) = direct.first() else { + return false; + }; + let end: usize = direct + .iter() + .copied() + .find(|&index| is_query_from(tokens, index)) + .unwrap_or(limit); + (first..end).all(|index| match tokens[index].token_type { + TokenType::Var | TokenType::Identifier | TokenType::QuotedIdentifier => { + names_output_or_type(tokens, index) + } + TokenType::Star | TokenType::Select => false, + _ => true, + }) +} + +/// Whether the name at `index` is an alias or a type after `AS`, or a bare alias after a value. +fn names_output_or_type(tokens: &[Token], index: usize) -> bool { + let Some(previous) = index.checked_sub(1).map(|previous| &tokens[previous]) else { + return false; + }; + let followed_by_list_end: bool = tokens + .get(index + 1) + .is_none_or(|next| matches!(next.token_type, TokenType::Comma | TokenType::From)); + previous.token_type == TokenType::As + || (followed_by_list_end + && matches!( + previous.token_type, + TokenType::Number + | TokenType::String + | TokenType::Null + | TokenType::True + | TokenType::False + | TokenType::Parameter + | TokenType::RParen + )) +} diff --git a/crates/sqlclean/src/sql_lint/tests/test_lint_exemptions.rs b/crates/sqlclean/src/sql_lint/tests/test_lint_exemptions.rs index 94768c3..c872c27 100644 --- a/crates/sqlclean/src/sql_lint/tests/test_lint_exemptions.rs +++ b/crates/sqlclean/src/sql_lint/tests/test_lint_exemptions.rs @@ -1,8 +1,9 @@ use serde_json::{Value, json}; use crate::sql_lint::main::engine::lint_json; +use crate::sql_lint::tests::helpers; use crate::sql_lint::tests::test_types::{ - DynamicOutputStarLintTestCase, SqlMacroOutputLintTestCase, + DynamicOutputStarLintTestCase, LintTestCase, SqlMacroOutputLintTestCase, }; #[test] @@ -128,3 +129,45 @@ unrelated AS (SELECT * FROM pivot_input) SELECT * FROM final", } Ok(()) } + +#[test] +fn given_constant_projection_when_linting_row_selection_then_only_column_dependent_rows_fail() +-> Result<(), String> { + let test_cases = [ + LintTestCase { + description: "constant-only projection selects identical rows in any order", + sql: "SELECT CAST(100 AS DOUBLE) AS measured_value, CAST(NULL AS INTEGER), 0 samples FROM items LIMIT 1", + expected_codes: &[], + expected_anchors: &[], + }, + LintTestCase { + description: "a column reference inside a cast still makes row selection nondeterministic", + sql: "SELECT CAST(id AS DOUBLE) AS measured_value FROM items LIMIT 1", + expected_codes: &["SQLCLEAN004"], + expected_anchors: &[("SQLCLEAN004", "LIMIT")], + }, + LintTestCase { + description: "a function call in a constant-looking projection still needs ordering", + sql: "SELECT now() AS observed_at FROM items OFFSET 1", + expected_codes: &["SQLCLEAN004"], + expected_anchors: &[("SQLCLEAN004", "OFFSET")], + }, + ]; + for test_case in &test_cases { + let diagnostics = helpers::diagnostics(test_case.sql)?; + let found: Vec<(&str, &str)> = diagnostics + .iter() + .filter_map(|item| { + let start = item["start"].as_u64()? as usize; + let end = item["end"].as_u64()? as usize; + Some((item["code"].as_str()?, &test_case.sql[start..end])) + }) + .collect(); + assert_eq!( + found, test_case.expected_anchors, + "{}", + test_case.description + ); + } + Ok(()) +} diff --git a/website/src/content/docs/docs/rules/reference.mdx b/website/src/content/docs/docs/rules/reference.mdx index f75475a..f6cc252 100644 --- a/website/src/content/docs/docs/rules/reference.mdx +++ b/website/src/content/docs/docs/rules/reference.mdx @@ -199,7 +199,8 @@ FROM customer_orders ``` `SQLCLEAN004` reports the same problem for `LIMIT` and `OFFSET`; see the example in -[Rules](/docs/rules/overview/). +[Rules](/docs/rules/overview/). It skips a query that selects only constants, such as +`SELECT 1 AS present FROM orders LIMIT 1`, because every row it could return is identical. ## SQLCLEAN019: NULL in NOT IN