fix(rules): lint plain relation slot group reads in SQL macro bodies as bare columns

A plain read of a COLUMNS group stands as one bare column, as expansion pastes it, and every reference to one RELATION parameter as a relation shares one placeholder, so plain single-slot and undeclared reads through it are in scope. Real mixed-qualification and out-of-scope findings beside them are still reported.

Author: kvlonge

crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_parameter_spans.rs

@@ -1,28 +1,62 @@
 //! The byte spans of a SQL macro body's `@name` parameter references, as expansion reads them.
 
+use crate::sql_macros::_helpers::parsing::header::unquoted;
 use crate::sql_macros::_helpers::parsing::lexing::{Token, TokenKind, tokens};
 use crate::sql_macros::_helpers::parsing::parameters::parameter_references;
 use crate::sql_macros::_helpers::parsing::slots::prefix_start;
+use crate::sql_macros::models::{ParameterSpan, SqlMacroParameter};
 
-/// The span of each `@name` expansion replaces, and of each slot name whose column a call maps.
+/// Each `@name` span expansion replaces: aliased and plain group reads whole, other plain reads as relation and slot.
 #[must_use]
-pub fn sql_macro_parameter_spans(body: &str) -> Vec<(usize, usize)> {
+pub fn sql_macro_parameter_spans(
+    body: &str,
+    parameters: &[SqlMacroParameter],
+) -> Vec<ParameterSpan> {
     let Ok(scanned) = tokens(body) else {
         return Vec::new();
     };
-    let mut spans: Vec<(usize, usize)> = Vec::new();
+    let relation = |name: &str| -> Option<&SqlMacroParameter> {
+        parameters
+            .iter()
+            .find(|parameter| parameter.name.eq_ignore_ascii_case(name))
+            .filter(|parameter| parameter.kind.declares_relation())
+    };
+    let mut spans: Vec<ParameterSpan> = Vec::new();
     for reference in parameter_references(body, &scanned) {
         let name: usize = reference.token;
+        let declared: Option<&SqlMacroParameter> = relation(&reference.name);
+        let named_relation: Option<String> =
+            declared.map(|parameter| parameter.name.to_lowercase());
         let Some(slot) = slot_word(body, &scanned, name) else {
-            spans.push((reference.start, reference.end));
+            spans.push(ParameterSpan {
+                start: reference.start,
+                end: reference.end,
+                relation: named_relation,
+            });
             continue;
         };
-        if name >= 1 && prefix_start(body, &scanned, name - 1).is_some() {
-            spans.push((reference.start, slot.end));
+        let slot_name: String = unquoted(slot.text(body));
+        let qualified: bool = name >= 1 && prefix_start(body, &scanned, name - 1).is_some();
+        let group: bool =
+            declared.is_some_and(|parameter| parameter.kind.declares_group(&slot_name));
+        if qualified || group {
+            spans.push(ParameterSpan {
+                start: reference.start,
+                end: slot.end,
+                relation: None,
+            });
             continue;
         }
-        spans.push((reference.start, reference.end));
-        spans.push((slot.start, slot.end));
+        spans.push(ParameterSpan {
+            start: reference.start,
+            end: reference.end,
+            relation: named_relation,
+        });
+        spans.push(ParameterSpan {
+            start: slot.start,
+            end: slot.end,
+            relation: None,
+        });
     }
     spans
 }

crates/sqlbuild-analysis/src/sql_macros/models.rs

@@ -77,6 +77,11 @@ impl ParameterKind {
         groups.iter().any(|group| group.eq_ignore_ascii_case(slot))
     }
 
+    /// Whether this is a `RELATION`.
+    pub fn declares_relation(&self) -> bool {
+        matches!(self, Self::Relation { .. })
+    }
+
     /// Whether this is a `RELATION` that declares a slot called `slot`.
     pub fn declares_slot(&self, slot: &str) -> bool {
         let Self::Relation { columns, groups } = self else {
@@ -468,6 +473,15 @@ pub struct SqlMacroFinding {
     pub declared_at: SqlMacroLocation,
 }
 
+/// The byte span of one parameter reference a SQL macro body's lint replaces.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct ParameterSpan {
+    pub start: usize,
+    pub end: usize,
+    /// The `RELATION` parameter this span names as a relation, as `@weekly` in `FROM @weekly`.
+    pub relation: Option<String>,
+}
+
 /// What type inference knows of an expression macro body over its declared arguments.
 #[derive(Clone, Debug)]
 pub(crate) enum BodyType {

crates/sqlbuild-python/src/bindings/_helpers/rules/lint_pipeline.rs

@@ -8,8 +8,9 @@ use std::sync::atomic::{AtomicBool, Ordering};
 use pyo3::exceptions::{PyException, PyValueError};
 use pyo3::prelude::{Bound, IntoPyObject, PyAny, PyAnyMethods, PyErr, PyResult, Python};
 use pyo3::types::{PyDict, PyDictMethods, PyString, PyTuple};
+use sqlbuild_analysis::sql_macros::main::parse_sql_macro::parse_sql_macro;
 use sqlbuild_analysis::sql_macros::main::sql_macro_body_form::sql_macro_body_form;
-use sqlbuild_analysis::sql_macros::models::SqlMacroForm;
+use sqlbuild_analysis::sql_macros::models::{SqlMacroForm, SqlMacroParameter};
 use sqlbuild_core::panics::main::catch_compiler_panic::catch_compiler_panic;
 use sqlbuild_core::text::main::python_isspace::python_isspace;
 use sqlbuild_core::text::main::python_str_repr::python_str_repr;
@@ -755,9 +756,13 @@ fn plan_bodies(
         ranges = sql_body_ranges(text, headers);
     }
     let sql_test: bool = headers.iter().any(|header| header.kind == SQL_TEST_KIND);
-    let neutralize: Option<fn(&str) -> Neutralized> = if sql_macro {
-        Some(neutralize_sql_macro_parameters)
-    } else if sql_test {
+    let parameters: Vec<SqlMacroParameter> = if sql_macro {
+        sql_macro_parameters(file)
+    } else {
+        Vec::new()
+    };
+    let neutralize_macro = |body: &str| neutralize_sql_macro_parameters(body, &parameters);
+    let other: Option<fn(&str) -> Neutralized> = if sql_test {
         Some(neutralize_test_parameters)
     } else {
         neutralizer(&BodyRole {
@@ -766,6 +771,13 @@ fn plan_bodies(
             in_hook_directory,
         })
     };
+    let neutralize: Option<&dyn Fn(&str) -> Neutralized> = if sql_macro {
+        Some(&neutralize_macro)
+    } else {
+        other
+            .as_ref()
+            .map(|function| function as &dyn Fn(&str) -> Neutralized)
+    };
     let mut bodies: Vec<PlannedBody> = ranges
         .into_iter()
         .map(|range| plan_body(text, range, neutralize, compiled))
@@ -838,7 +850,15 @@ pub(crate) fn prepare_single_body(request: &SingleBodyRequest<'_, '_>) -> PyResu
     let dialect: &str = dialect;
     let py: Python<'_> = file_path.py();
     let text: Vec<char> = contents.chars().collect();
-    let planned: PlannedBody = plan_body(&text, range, neutralizer(role), None);
+    let neutralize: Option<fn(&str) -> Neutralized> = neutralizer(role);
+    let planned: PlannedBody = plan_body(
+        &text,
+        range,
+        neutralize
+            .as_ref()
+            .map(|function| function as &dyn Fn(&str) -> Neutralized),
+        None,
+    );
     let (expanded, passes) = match planned.source {
         BodySource::Expand => {
             let expansion: AuthoredExpansion =
@@ -895,10 +915,22 @@ fn not_prepared(py: Python<'_>, path: &Path) -> PyResult<PyErr> {
     ))
 }
 
+/// The declared parameters of a SQL macro file, or none when its header does not parse.
+fn sql_macro_parameters(file: &LintFile) -> Vec<SqlMacroParameter> {
+    let name: String = file
+        .path
+        .file_stem()
+        .map(|stem| stem.to_string_lossy().into_owned())
+        .unwrap_or_default();
+    parse_sql_macro(&name, &file.path.to_string_lossy(), &file.contents)
+        .map(|definition| definition.parameters)
+        .unwrap_or_default()
+}
+
 fn plan_body(
     text: &[char],
     (mut start, end): (usize, usize),
-    neutralize: Option<fn(&str) -> Neutralized>,
+    neutralize: Option<&dyn Fn(&str) -> Neutralized>,
     compiled: Option<&CompiledExpansion>,
 ) -> PlannedBody {
     let body: String = text[start..end].iter().collect();

crates/sqlbuild-rules/src/lint_text/main/neutralize_sql_macro_parameters.rs

@@ -1,22 +1,34 @@
 //! `neutralize_sql_macro_parameters`: a SQL macro body's `@name` parameters replaced by sentinels.
 
+use std::collections::HashMap;
+
 use sqlbuild_analysis::sql_macros::main::sql_macro_parameter_spans::sql_macro_parameter_spans;
+use sqlbuild_analysis::sql_macros::models::{ParameterSpan, SqlMacroParameter};
 
 use crate::lint_text::_helpers::sites::{ByteSite, assemble, sentinel};
 use crate::lint_text::constants::SQL_MACRO_PARAMETER_SENTINEL_PREFIX;
 use crate::lint_text::models::Neutralized;
 
-/// Replace each parameter reference expansion would replace, and nothing else, with a sentinel.
-pub fn neutralize_sql_macro_parameters(body: &str) -> Neutralized {
-    let byte_sites: Vec<ByteSite> = sql_macro_parameter_spans(body)
+/// Replace each parameter reference with a sentinel; one `RELATION` parameter's relation spans share one.
+pub fn neutralize_sql_macro_parameters(
+    body: &str,
+    parameters: &[SqlMacroParameter],
+) -> Neutralized {
+    let mut relations: HashMap<String, String> = HashMap::new();
+    let mut next: usize = 0;
+    let mut fresh = || {
+        let made: String = sentinel(SQL_MACRO_PARAMETER_SENTINEL_PREFIX, next);
+        next += 1;
+        made
+    };
+    let byte_sites: Vec<ByteSite> = sql_macro_parameter_spans(body, parameters)
         .into_iter()
-        .enumerate()
-        .map(|(index, (start, end))| {
-            (
-                start,
-                end,
-                sentinel(SQL_MACRO_PARAMETER_SENTINEL_PREFIX, index),
-            )
+        .map(|span: ParameterSpan| {
+            let replacement: String = match span.relation {
+                Some(relation) => relations.entry(relation).or_insert_with(&mut fresh).clone(),
+                None => fresh(),
+            };
+            (span.start, span.end, replacement)
         })
         .collect();
     assemble(body, byte_sites)

crates/sqlbuild-rules/src/lint_text/tests/test_lint_text.rs

@@ -17,9 +17,10 @@ use crate::lint_text::models::{ExpansionSpan, FormatFile, LintSqlBody, LintSqlOp
 use crate::lint_text::tests::helpers::directive_finding;
 use crate::lint_text::tests::test_types::{
     Finding, FormatBodiesTestCase, LintBodiesTestCase, NeutralizeTestCase, ParameterTestCase,
-    RestoreTestCase, SitePatternTestCase, SuppressionTestCase, TokenAtTestCase,
+    RestoreTestCase, SitePatternTestCase, SlotReadTestCase, SuppressionTestCase, TokenAtTestCase,
     UnwrappedExpressionTestCase,
 };
+use sqlbuild_analysis::sql_macros::main::parse_sql_macro::parse_sql_macro;
 
 const SUPPRESSED_FILE: &str =
     "-- sqb: ignore SQBRSQL005 because kept\n\n-- note\nselect 1\n-- sqb: ignore nope\nselect 2\n";
@@ -144,25 +145,25 @@ fn given_generated_parameters_when_neutralizing_then_only_bare_parameters_are_re
         ParameterTestCase {
             description: "SQL macro parameters followed by words and a parenthesis",
             body: "@value IS NULL OR TRIM(@value) = ''",
-            neutralize: neutralize_sql_macro_parameters,
+            neutralize: |body| neutralize_sql_macro_parameters(body, &[]),
             expected_text: "__sqlbuild_macro_parameter_0__ IS NULL OR TRIM(__sqlbuild_macro_parameter_1__) = ''",
         },
         ParameterTestCase {
             description: "SQL macro parameters before IN and a parenthesised predicate",
             body: "CASE WHEN @x IN ('a', 'b') THEN 1 END + CASE WHEN @x IS NULL AND (@y IS NULL) THEN 1 END",
-            neutralize: neutralize_sql_macro_parameters,
+            neutralize: |body| neutralize_sql_macro_parameters(body, &[]),
             expected_text: "CASE WHEN __sqlbuild_macro_parameter_0__ IN ('a', 'b') THEN 1 END + CASE WHEN __sqlbuild_macro_parameter_1__ IS NULL AND (__sqlbuild_macro_parameter_2__ IS NULL) THEN 1 END",
         },
         ParameterTestCase {
             description: "SQL macro slot reads hide the slot name, whole after an alias",
             body: "SELECT s.@src.amount AS amount, @src.id, @src.* FROM @src AS s",
-            neutralize: neutralize_sql_macro_parameters,
+            neutralize: |body| neutralize_sql_macro_parameters(body, &[]),
             expected_text: "SELECT s.__sqlbuild_macro_parameter_0__ AS amount, __sqlbuild_macro_parameter_1__.__sqlbuild_macro_parameter_2__, __sqlbuild_macro_parameter_3__.* FROM __sqlbuild_macro_parameter_4__ AS s",
         },
         ParameterTestCase {
             description: "SQL macro calls, enum references and quoted text keep their at-signs",
             body: "@sql.other(@v) + @enum(\"s\").A + LENGTH('@v') + @@x",
-            neutralize: neutralize_sql_macro_parameters,
+            neutralize: |body| neutralize_sql_macro_parameters(body, &[]),
             expected_text: "@sql.other(__sqlbuild_macro_parameter_0__) + @enum(\"s\").A + LENGTH('@v') + @@x",
         },
         ParameterTestCase {
@@ -190,6 +191,43 @@ fn given_generated_parameters_when_neutralizing_then_only_bare_parameters_are_re
     }
 }
 
+#[test]
+fn given_relation_slot_reads_when_neutralizing_then_they_stand_as_their_expansion_does() {
+    let header: &str = "MACRO (\n  description \"Weekly rates\",\n  \
+         arguments (weekly RELATION (keys COLUMNS, refund_count BIGINT), cutoff BIGINT),\n);\n\n";
+    let test_cases = [
+        SlotReadTestCase {
+            description: "a plain group read is one bare value",
+            body: "SELECT @weekly.keys, orders FROM counted GROUP BY @weekly.keys",
+            expected_text: "SELECT __sqlbuild_macro_parameter_0__, orders FROM counted GROUP BY __sqlbuild_macro_parameter_1__",
+        },
+        SlotReadTestCase {
+            description: "plain single-slot and undeclared reads go through the one relation",
+            body: "SELECT @weekly.refund_count, @weekly.week FROM @weekly WHERE @cutoff > @cutoff",
+            expected_text: "SELECT __sqlbuild_macro_parameter_0__.__sqlbuild_macro_parameter_1__, __sqlbuild_macro_parameter_0__.__sqlbuild_macro_parameter_2__ FROM __sqlbuild_macro_parameter_0__ WHERE __sqlbuild_macro_parameter_3__ > __sqlbuild_macro_parameter_4__",
+        },
+        SlotReadTestCase {
+            description: "aliased reads stay whole after their alias",
+            body: "SELECT s.@weekly.keys, s.@weekly.refund_count FROM @weekly AS s",
+            expected_text: "SELECT s.__sqlbuild_macro_parameter_0__, s.__sqlbuild_macro_parameter_1__ FROM __sqlbuild_macro_parameter_2__ AS s",
+        },
+    ];
+
+    for test_case in test_cases {
+        let source: String = format!("{header}{}\n", test_case.body);
+        let parameters = parse_sql_macro("weekly_rates", "macros/sql/weekly_rates.sql", &source)
+            .map(|definition| definition.parameters)
+            .unwrap_or_default();
+
+        assert_eq!(
+            neutralize_sql_macro_parameters(test_case.body, &parameters).text,
+            test_case.expected_text,
+            "{}",
+            test_case.description
+        );
+    }
+}
+
 #[test]
 fn given_formatted_sql_when_restoring_then_each_sentinel_must_appear_once()
 -> Result<(), LintTextError> {

crates/sqlbuild-rules/src/lint_text/tests/test_types.rs

@@ -8,6 +8,12 @@ pub(crate) struct NeutralizeTestCase {
     pub(crate) expected_sites: Vec<(&'static str, usize, usize)>,
 }
 
+pub(crate) struct SlotReadTestCase {
+    pub(crate) description: &'static str,
+    pub(crate) body: &'static str,
+    pub(crate) expected_text: &'static str,
+}
+
 pub(crate) struct ParameterTestCase {
     pub(crate) description: &'static str,
     pub(crate) body: &'static str,

tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/_test_types.py

@@ -125,3 +125,14 @@ class SqlMacroFormatPairTestCase:
     slot_text: str
     hand_text: str
     expected_check_returncode: int
+
+
+@dataclass(frozen=True)
+class SqlMacroBodyRulesTestCase:
+    """One project linting a SQL macro body, and the findings it must and must not print."""
+
+    description: str
+    files: dict[str, str]
+    expected_returncode: int
+    expected_fragments: tuple[str, ...]
+    expected_absent_fragments: tuple[str, ...]

tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/helpers.py

@@ -554,3 +554,43 @@ def scoped_model(macros: dict[str, str], body: str) -> dict[str, str]:
             f"{body}\n"
         ),
     }
+
+
+SQL_RULES: dict[str, str] = {
+    "sqlbuild_project.toml": BASE_FILES["sqlbuild_project.toml"]
+    + '\n[rules]\nselect = ["SQBRSQL028", "SQBRSQL029"]\n'
+}
+
+
+def rates_with_middle_cte(middle: str) -> dict[str, str]:
+    """The docs' weekly refund rates macro with `middle` as a CTE between its two, and a model."""
+
+    return {
+        "macros/sql/weekly_rates.sql": (
+            'MACRO (\n  description "Weekly refund rate per grain",\n'
+            "  arguments (weekly RELATION (keys COLUMNS, order_count BIGINT, refund_count BIGINT)),"
+            "\n);\n\n"
+            "WITH counted AS (\n"
+            "  SELECT\n"
+            "    s.@weekly.keys,\n"
+            "    s.@weekly.order_count AS orders,\n"
+            "    s.@weekly.refund_count AS refunds\n"
+            "  FROM @weekly AS s\n"
+            "),\n\n"
+            f"{middle},\n\n"
+            "rated AS (\n"
+            "  SELECT\n"
+            "    @weekly.keys,\n"
+            "    SUM(refunds) / NULLIF(SUM(orders), 0) AS refund_rate\n"
+            "  FROM plain\n"
+            "  GROUP BY @weekly.keys\n"
+            ")\n"
+        ),
+        "models/marts/rates.sql": (
+            'MODEL (description "Refund rates");\n\n'
+            'WITH weekly AS (SELECT * FROM __seed("weekly_orders")),\n'
+            "@sql.weekly_rates(weekly, weekly.keys := (product_id, region), "
+            "weekly.order_count := orders_placed)\n"
+            "SELECT * FROM rated\n"
+        ),
+    }