Skip to content

Commit 0fe3fa5

Browse files
authored
fix: close correctness gaps in Rules, SQL scanners and sqb query (#848)
* fix: report unused table aliases written without AS SQBRSQL023 only recognised aliases introduced by AS, so an unused implicit alias such as `FROM orders o` went unreported. Implicit aliases after relation names, subqueries and table functions now follow the same unused-alias contract. Refs CHI-376 * fix: report numeric BETWEEN bounds and IN lists as named decisions SQBRDECLARATION102 only inspected binary comparison operators, so literal BETWEEN bounds and numeric IN/NOT IN members bypassed the rule. Range and value-list predicates are now evaluated like the equivalent comparisons, and value lists point to a list or set constant used through IN @const(...). Refs CHI-397 * fix: treat dollar-quoted strings as opaque literals in compiler scanners The compiler's reference, macro, declaration, interpolation and resolution scanners only recognised quote characters, so an apostrophe inside a $$...$$ literal opened a phantom quote and compilation failed with an unclosed quoted string, while reference-shaped text inside dollar quotes could be extracted as a dependency. The Python scanners and the native compiler scan policy now skip $$ and $tag$ literals like single-quoted strings. Refs CHI-375 * fix: report sqb query warehouse errors as CLI errors A warehouse error from sqb query escaped the CLI error projection and printed a Python traceback. Query execution failures now raise a coded CLI error, error[C108], carrying the warehouse message on stderr with exit code 2 and no stdout output. Refs CHI-343 * fix: skip type-proof rules when SQL analysis is disabled SQBRCONTRACT105 needs inferred type proofs, which only exist when SQL analysis runs, so compile --no-sql-analysis (or sql_analysis = false) reported it for essentially every typed contract column. Type-proof rules are now not evaluated when the run or project disables SQL analysis, and compile prints one note naming the skipped rules on stderr before the final summary. With analysis enabled, behaviour is unchanged. Refs CHI-353 * fix: keep join modifiers and whole-row alias uses out of unused-alias findings SQBRSQL023 treated an unaliased relation's join or relation modifier, such as POSITIONAL in `FROM orders POSITIONAL JOIN customers`, as an implicit alias and offered to delete it. It also counted an alias as used only when followed by a dot, so whole-row uses such as `to_json(o)`, `SELECT o` or `WHERE o IS NOT NULL` were reported and their deletion broke binding. Modifier and clause keywords are no longer implicit-alias candidates, and any bare identifier reference to the alias counts as a use for both implicit and AS aliases. Refs CHI-376 * fix: keep WINDOW clauses out of unused-alias findings * fix: report aliases that repeat their relation name when unused
1 parent fde7574 commit 0fe3fa5

54 files changed

Lines changed: 1518 additions & 67 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎crates/sqlbuild-rules-native/src/compiler/tests/helpers.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,13 +67,22 @@ pub(crate) fn comments_and_quoted_text_hide_references() -> bool {
6767
== Some(vec![("ref".to_owned(), "orders".to_owned(), None, None)])
6868
}
6969

70+
pub(crate) fn dollar_quoted_text_hides_references() -> bool {
71+
extract(concat!(
72+
"SELECT $$Customer's order -- __ref(\"ignored\") $5$$ AS label, ",
73+
"$tag$ $$ __seed(\"also_ignored\") $tag$ AS note, price$1$ ",
74+
"FROM __ref(orders)"
75+
)) == Some(vec![("ref".to_owned(), "orders".to_owned(), None, None)])
76+
}
77+
7078
pub(crate) fn complex_or_malformed_sql_requests_fallback() -> bool {
7179
[
7280
"SELECT * FROM __table_fn(\"orders\")(1)",
7381
"SELECT * FROM __ref(concat('ord', 'ers'))",
7482
"SELECT * FROM __ref(\"orders\"",
7583
"SELECT * FROM __ref(\"orders\") /* unterminated",
7684
"SELECT * FROM __ref(\"orders\") WHERE note = 'unterminated",
85+
"SELECT * FROM __ref(\"orders\") WHERE note = $$unterminated",
7786
"SELECT * FROM __ref(örders)",
7887
]
7988
.into_iter()

‎crates/sqlbuild-rules-native/src/compiler/tests/test_sql_references.rs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use crate::compiler::tests::helpers::{
22
comments_and_quoted_text_hide_references, complex_or_malformed_sql_requests_fallback,
3-
simple_references_preserve_authored_order,
3+
dollar_quoted_text_hides_references, simple_references_preserve_authored_order,
44
};
55
use crate::compiler::tests::test_types::StaticSqlOperationTestCase;
66

@@ -17,6 +17,11 @@ fn given_sql_reference_cases_when_extracting_then_expected_behavior_holds() {
1717
run: comments_and_quoted_text_hide_references,
1818
expected_success: true,
1919
},
20+
StaticSqlOperationTestCase {
21+
description: "dollar-quoted text hides embedded references",
22+
run: dollar_quoted_text_hides_references,
23+
expected_success: true,
24+
},
2025
StaticSqlOperationTestCase {
2126
description: "complex and malformed SQL requests Python fallback",
2227
run: complex_or_malformed_sql_requests_fallback,

‎crates/sqlbuild-rules-native/src/engine/tests/test_evaluation.rs‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -954,6 +954,60 @@ fn given_numeric_comparisons_when_evaluating_named_decisions_then_only_authored_
954954
authored_sql: ("SELECT CASE WHEN amount > 30 THEN 'large' END AS amount_30 FROM orders"),
955955
expected_fault_count: 1,
956956
},
957+
test_types::NumericDecisionTestCase {
958+
description: "authored between bounds are a named decision finding",
959+
query_sql: "SELECT code FROM orders WHERE TRY_CAST(code AS INTEGER) BETWEEN 30 AND 61",
960+
authored_sql: "SELECT code FROM orders WHERE TRY_CAST(code AS INTEGER) BETWEEN 30 AND 61",
961+
expected_fault_count: 1,
962+
},
963+
test_types::NumericDecisionTestCase {
964+
description: "authored not between bounds are a named decision finding",
965+
query_sql: "SELECT code FROM orders WHERE amount NOT BETWEEN 5 AND 10",
966+
authored_sql: "SELECT code FROM orders WHERE amount NOT BETWEEN 5 AND 10",
967+
expected_fault_count: 1,
968+
},
969+
test_types::NumericDecisionTestCase {
970+
description: "canonical between bounds pass",
971+
query_sql: "SELECT code FROM orders WHERE amount BETWEEN 0 AND 1",
972+
authored_sql: "SELECT code FROM orders WHERE amount BETWEEN 0 AND 1",
973+
expected_fault_count: 0,
974+
},
975+
test_types::NumericDecisionTestCase {
976+
description: "expanded between constants are not authored literal findings",
977+
query_sql: "SELECT code FROM orders WHERE amount BETWEEN 30 AND 61",
978+
authored_sql: "SELECT code FROM orders WHERE amount BETWEEN @const(\"_low\") AND @const(\"_high\")",
979+
expected_fault_count: 0,
980+
},
981+
test_types::NumericDecisionTestCase {
982+
description: "authored numeric in list is a named decision finding",
983+
query_sql: "SELECT code FROM orders WHERE TRY_CAST(code AS INTEGER) IN (30, 31, 45)",
984+
authored_sql: "SELECT code FROM orders WHERE TRY_CAST(code AS INTEGER) IN (30, 31, 45)",
985+
expected_fault_count: 1,
986+
},
987+
test_types::NumericDecisionTestCase {
988+
description: "authored numeric not in list is a named decision finding",
989+
query_sql: "SELECT code FROM orders WHERE amount NOT IN (30, 31)",
990+
authored_sql: "SELECT code FROM orders WHERE amount NOT IN (30, 31)",
991+
expected_fault_count: 1,
992+
},
993+
test_types::NumericDecisionTestCase {
994+
description: "canonical numeric in list passes",
995+
query_sql: "SELECT code FROM orders WHERE amount IN (-1, 0, 1)",
996+
authored_sql: "SELECT code FROM orders WHERE amount IN (-1, 0, 1)",
997+
expected_fault_count: 0,
998+
},
999+
test_types::NumericDecisionTestCase {
1000+
description: "expanded list constant is not an authored literal finding",
1001+
query_sql: "SELECT code FROM orders WHERE amount IN (30, 31, 45)",
1002+
authored_sql: "SELECT code FROM orders WHERE amount IN @const(\"_codes\")",
1003+
expected_fault_count: 0,
1004+
},
1005+
test_types::NumericDecisionTestCase {
1006+
description: "between in projected case is a named decision finding",
1007+
query_sql: "SELECT CASE WHEN amount BETWEEN 30 AND 61 THEN 'mid' END AS band FROM orders",
1008+
authored_sql: "SELECT CASE WHEN amount BETWEEN 30 AND 61 THEN 'mid' END AS band FROM orders",
1009+
expected_fault_count: 1,
1010+
},
9571011
test_types::NumericDecisionTestCase {
9581012
description: "nested comparison reports only its leaf decision",
9591013
query_sql: ("SELECT CASE WHEN amount > 5 THEN status END = 'ready' AS selected FROM orders"),

‎crates/sqlbuild-rules-native/src/engine/tests/test_sql_scanning.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ fn given_quoted_commented_and_malformed_fragments_when_scanning_then_every_scann
170170
SqlScannerTestCase {
171171
description: "dollar quote",
172172
fragment: "$$ ) $$",
173-
expected_compiler_paren: Ok(4),
173+
expected_compiler_paren: Ok(10),
174174
expected_table_function: "SELECT * FROM __table_fn(\"o\", $$ ) $$ x)(1)",
175175
expected_table_function_token: "SELECT $$ ) $$ __table_fn(\"o\", 1)",
176176
expected_snowflake_exclude: "SELECT * $$ x) , a FROM t",

‎crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs‎

Lines changed: 40 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ use std::ops::ControlFlow;
3030

3131
const MIXED_STAR_REMEDIATION: &str = "SELECT *, a, b mixes a passthrough star with derived columns. The * exemption permits a lone SELECT * only. Move a and b into an earlier CTE so they are computed upstream, leaving the final select a pure SELECT *. If you need those columns in the output, drop the * exemption and enumerate every column explicitly.";
3232
const RULES_PARSER_RECURSION_LIMIT: usize = 128;
33+
const NUMERIC_VALUE_LIST_REMEDIATION: &str = "Declare the values as a list or set CONSTANT and compare through [NOT] IN @const(\"<name>\"); only -1, 0, and 1 are self-explanatory.";
3334

3435
#[derive(Clone)]
3536
struct Position {
@@ -42,6 +43,8 @@ struct ComparisonFact {
4243
position: Option<Position>,
4344
sql: String,
4445
equality: bool,
46+
value_list: bool,
47+
binary: bool,
4548
output_name: Option<String>,
4649
columns: BTreeSet<String>,
4750
string_literals: Vec<String>,
@@ -312,6 +315,7 @@ pub(crate) fn rules_quote_policy(dialect_name: &str) -> QuotePolicy {
312315
single_quote_backslash_escapes: backslash_escapes,
313316
double_quote_backslash_escapes: backslash_escapes
314317
&& !dialect.is_delimited_identifier_start('"'),
318+
dollar_quotes: false,
315319
}
316320
}
317321

@@ -1226,7 +1230,9 @@ fn evaluate_literal_rules(
12261230
) {
12271231
let facts = select_facts(&parsed.query);
12281232
for comparison in facts.into_iter().flat_map(|select| select.comparisons) {
1229-
if let Some(rule) = selected.get("SQBRDECLARATION101") {
1233+
if comparison.binary
1234+
&& let Some(rule) = selected.get("SQBRDECLARATION101")
1235+
{
12301236
let enum_column = comparison
12311237
.columns
12321238
.iter()
@@ -1265,7 +1271,9 @@ fn evaluate_literal_rules(
12651271
rule,
12661272
comparison.position.as_ref(),
12671273
format!("non-canonical numeric comparison: {}", comparison.sql),
1268-
None,
1274+
comparison
1275+
.value_list
1276+
.then(|| NUMERIC_VALUE_LIST_REMEDIATION.to_owned()),
12691277
));
12701278
}
12711279
}
@@ -1669,17 +1677,20 @@ fn comparison_facts(root: &Expr, source_context: &SourceContext, output: &mut Ve
16691677
impl Visitor for Comparisons<'_> {
16701678
type Break = ();
16711679
fn pre_visit_expr(&mut self, expression: &Expr) -> ControlFlow<Self::Break> {
1672-
if let Expr::BinaryOp { op, .. } = expression
1673-
&& matches!(
1680+
let is_comparison = match expression {
1681+
Expr::BinaryOp { op, .. } => matches!(
16741682
op,
16751683
BinaryOperator::Eq
16761684
| BinaryOperator::NotEq
16771685
| BinaryOperator::Gt
16781686
| BinaryOperator::GtEq
16791687
| BinaryOperator::Lt
16801688
| BinaryOperator::LtEq
1681-
)
1682-
{
1689+
),
1690+
Expr::Between { .. } | Expr::InList { .. } => true,
1691+
_ => false,
1692+
};
1693+
if is_comparison {
16831694
self.output
16841695
.push(comparison_fact(expression, self.source_context));
16851696
}
@@ -1776,29 +1787,40 @@ fn comparison_fact(root: &Expr, source_context: &SourceContext) -> ComparisonFac
17761787
Expr::BinaryOp {
17771788
op: BinaryOperator::Eq,
17781789
..
1779-
}
1790+
} | Expr::InList { negated: false, .. }
17801791
),
1792+
value_list: matches!(root, Expr::InList { .. }),
1793+
binary: matches!(root, Expr::BinaryOp { .. }),
17811794
source_context: source_context.clone(),
17821795
..ComparisonFact::default()
17831796
},
17841797
};
17851798
let _ = root.visit(&mut visitor);
1786-
if let Expr::BinaryOp { left, right, .. } = root {
1787-
for operand in [left.as_ref(), right.as_ref()] {
1788-
if let Some(number) = numeric_value(operand) {
1789-
visitor.fact.numeric_literals.push(number);
1790-
}
1791-
if direct_column(operand).is_none() {
1792-
visitor.fact.modified_columns.extend(column_facts(operand));
1793-
}
1794-
if direct_string_literal(operand).is_none() && contains_string_literal(operand) {
1795-
visitor.fact.modified_string_literal = true;
1796-
}
1799+
for operand in comparison_operands(root) {
1800+
if let Some(number) = numeric_value(operand) {
1801+
visitor.fact.numeric_literals.push(number);
1802+
}
1803+
if direct_column(operand).is_none() {
1804+
visitor.fact.modified_columns.extend(column_facts(operand));
1805+
}
1806+
if direct_string_literal(operand).is_none() && contains_string_literal(operand) {
1807+
visitor.fact.modified_string_literal = true;
17971808
}
17981809
}
17991810
visitor.fact
18001811
}
18011812

1813+
fn comparison_operands(root: &Expr) -> Vec<&Expr> {
1814+
match root {
1815+
Expr::BinaryOp { left, right, .. } => vec![left.as_ref(), right.as_ref()],
1816+
Expr::Between {
1817+
expr, low, high, ..
1818+
} => vec![expr.as_ref(), low.as_ref(), high.as_ref()],
1819+
Expr::InList { expr, list, .. } => std::iter::once(expr.as_ref()).chain(list).collect(),
1820+
_ => Vec::new(),
1821+
}
1822+
}
1823+
18021824
fn direct_column(expression: &Expr) -> Option<ColumnFact> {
18031825
match unwrap_nested(expression) {
18041826
Expr::Identifier(value) => Some(ColumnFact {

‎crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs‎

Lines changed: 71 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use crate::sql_lint::_helpers::engine::{
1010
direct_indices, is_comment, is_layout, is_query_from, query_end, significant_after,
1111
significant_before, token_depths,
1212
};
13+
use crate::sql_lint::constants::RELATION_MODIFIER_KEYWORDS;
1314
use crate::sql_lint::models::{AdditionalFactOptions, AdditionalQueryFacts};
1415

1516
const COUNT_ONE_LITERAL: &str = "1";
@@ -1242,34 +1243,92 @@ fn unused_alias_spans(query: &QuerySlice<'_>) -> Vec<Span> {
12421243
let query_end = *query_end;
12431244
let depth = *depth;
12441245
let mut spans: Vec<Span> = Vec::new();
1245-
for window in relation_clause.windows(2) {
1246-
if tokens[window[0]].token_type != TokenType::As || !is_identifier(&tokens[window[1]]) {
1247-
continue;
1248-
}
1249-
if significant_after(tokens, window[1])
1246+
for (span_start, alias_index) in relation_alias_positions(tokens, relation_clause) {
1247+
if significant_after(tokens, alias_index)
12501248
.is_some_and(|index| tokens[index].token_type == TokenType::LParen)
12511249
{
12521250
continue;
12531251
}
1254-
let alias = &tokens[window[1]].text;
1252+
let alias = &tokens[alias_index].text;
1253+
let relation_name = significant_before(tokens, span_start);
12551254
let used = (query_start..query_end).any(|index| {
1256-
depths[index] >= depth
1255+
index != alias_index
1256+
&& Some(index) != relation_name
1257+
&& depths[index] >= depth
1258+
&& is_identifier(&tokens[index])
12571259
&& tokens[index].text.eq_ignore_ascii_case(alias)
1260+
&& significant_before(tokens, index).is_none_or(|previous| {
1261+
!matches!(tokens[previous].token_type, TokenType::Dot | TokenType::As)
1262+
})
12581263
&& significant_after(tokens, index)
1259-
.is_some_and(|next| tokens[next].token_type == TokenType::Dot)
1264+
.is_none_or(|next| tokens[next].token_type != TokenType::LParen)
12601265
});
12611266
if !used {
12621267
spans.push(Span {
1263-
start: tokens[window[0]].span.start,
1264-
end: tokens[window[1]].span.end,
1265-
line: tokens[window[0]].span.line,
1266-
column: tokens[window[0]].span.column,
1268+
start: tokens[span_start].span.start,
1269+
end: tokens[alias_index].span.end,
1270+
line: tokens[span_start].span.line,
1271+
column: tokens[span_start].span.column,
12671272
});
12681273
}
12691274
}
12701275
spans
12711276
}
12721277

1278+
fn relation_alias_positions(tokens: &[Token], relation_clause: &[usize]) -> Vec<(usize, usize)> {
1279+
let mut aliases: Vec<(usize, usize)> = Vec::new();
1280+
let mut segment_start = 0_usize;
1281+
let mut in_condition = false;
1282+
for (position, &index) in relation_clause.iter().enumerate() {
1283+
if matches!(tokens[index].token_type, TokenType::Join | TokenType::Comma) {
1284+
segment_start = position + 1;
1285+
in_condition = false;
1286+
continue;
1287+
}
1288+
if in_condition || is_relation_condition_start(&tokens[index]) {
1289+
in_condition = true;
1290+
continue;
1291+
}
1292+
if tokens[index].token_type == TokenType::As {
1293+
if let Some(&alias) = relation_clause.get(position + 1)
1294+
&& is_identifier(&tokens[alias])
1295+
{
1296+
aliases.push((index, alias));
1297+
}
1298+
continue;
1299+
}
1300+
if is_implicit_relation_alias(
1301+
tokens,
1302+
&relation_clause[segment_start..],
1303+
position - segment_start,
1304+
) {
1305+
aliases.push((index, index));
1306+
}
1307+
}
1308+
aliases
1309+
}
1310+
1311+
fn is_implicit_relation_alias(tokens: &[Token], segment: &[usize], position: usize) -> bool {
1312+
let candidate = &tokens[segment[position]];
1313+
if position == 0
1314+
|| !is_identifier(candidate)
1315+
|| candidate.token_type != TokenType::QuotedIdentifier
1316+
&& RELATION_MODIFIER_KEYWORDS
1317+
.iter()
1318+
.any(|keyword| candidate.text.eq_ignore_ascii_case(keyword))
1319+
{
1320+
return false;
1321+
}
1322+
let previous = &tokens[segment[position - 1]];
1323+
if previous.token_type == TokenType::LParen {
1324+
return true;
1325+
}
1326+
is_identifier(previous)
1327+
&& position
1328+
.checked_sub(QUALIFIER_PAIR_LENGTH)
1329+
.is_none_or(|before| tokens[segment[before]].token_type == TokenType::Dot)
1330+
}
1331+
12731332
fn null_rejected_left_join_spans(
12741333
tokens: &[Token],
12751334
direct: &[usize],

‎crates/sqlbuild-rules-native/src/sql_lint/constants.rs‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,48 @@ pub(crate) const VALUES_RELATION_PREFIX_TOKEN_COUNT: usize = 2;
66
pub(crate) const POSITION_ARGUMENT_SEPARATOR: &str = ",";
77
pub(crate) const CTE_MACRO_PLACEHOLDER_LITERAL: &str = "1";
88
pub(crate) const POSTFIX_CAST_PREFIX_TOKEN_COUNT: usize = 2;
9+
pub(crate) const RELATION_MODIFIER_KEYWORDS: [&str; 43] = [
10+
"ANTI",
11+
"APPLY",
12+
"ASOF",
13+
"AT",
14+
"BEFORE",
15+
"CHANGES",
16+
"CONNECT",
17+
"CROSS",
18+
"EXCEPT",
19+
"FETCH",
20+
"FINAL",
21+
"FOR",
22+
"FULL",
23+
"GROUP",
24+
"HAVING",
25+
"INNER",
26+
"INTERSECT",
27+
"JOIN",
28+
"LATERAL",
29+
"LEFT",
30+
"LIMIT",
31+
"MATCH_RECOGNIZE",
32+
"MINUS",
33+
"NATURAL",
34+
"OFFSET",
35+
"ON",
36+
"ORDER",
37+
"OUTER",
38+
"PASTE",
39+
"PIVOT",
40+
"POSITIONAL",
41+
"QUALIFY",
42+
"RETURNING",
43+
"RIGHT",
44+
"SAMPLE",
45+
"SEMI",
46+
"START",
47+
"TABLESAMPLE",
48+
"UNION",
49+
"UNPIVOT",
50+
"USING",
51+
"WHERE",
52+
"WINDOW",
53+
];

0 commit comments

Comments
 (0)