feat(scenarios): reject quoted and materialized scenario CTE names like SQL tests (#1052)

* feat(scenarios): reject quoted and materialized scenario CTE names like SQL tests

Scenarios and macro-test scope scans no longer fall back to a Polyglot
re-parse. Quoted CTE names and AS [NOT] MATERIALIZED are located compile
errors with help on every engine, matching SQL tests.

* test(scenarios): cover scenario body spans and pass them in unit helpers

Author: kvlonge

crates/sqlbuild-analysis/src/compiler/_helpers/sql_tests/extraction.rs

@@ -35,6 +35,8 @@ const DIRECT_NAMES: [(&str, &str, &str); 3] = [
         "__table_fn_expected__",
     ),
 ];
+const TEST_CONTEXT: &str = "SQL test";
+const SCENARIO_CONTEXT: &str = "SQL scenario";
 const DECLARATION_CALLS: [&str; 4] = ["enum", "const", "var", "param"];
 /// Longest projection the alias help repeats verbatim.
 const ALIAS_HELP_EXPRESSION_LIMIT: usize = 80;
@@ -67,6 +69,9 @@ struct TestRequest {
     /// Read only the CTE names and body offsets of the authored block, before any expansion.
     #[serde(default)]
     authored: bool,
+    /// The authored text is a scenario rather than a SQL test block.
+    #[serde(default)]
+    scenario: bool,
 }
 
 /// One extraction error, with help and the offending text where the author can act on it.
@@ -189,7 +194,7 @@ fn extract_test(test: &TestRequest, syntax: &LexicalSyntax) -> Result<Classified
     if test.authored {
         return authored_ctes(test, syntax);
     }
-    let ctes: Vec<Cte> = scan_ctes(&test.sql, &test.file_label, syntax)?.ctes;
+    let ctes: Vec<Cte> = scan_ctes(&test.sql, &test.file_label, TEST_CONTEXT, syntax)?.ctes;
     let scope = TestScope {
         file: &test.file_label,
         syntax,
@@ -204,7 +209,12 @@ fn extract_test(test: &TestRequest, syntax: &LexicalSyntax) -> Result<Classified
 
 /// The authored block's CTEs, or the located error of a CTE name; other errors read as no CTEs.
 fn authored_ctes(test: &TestRequest, syntax: &LexicalSyntax) -> Result<Classified, Failure> {
-    match scan_ctes(&test.sql, &test.file_label, syntax) {
+    let context: &str = if test.scenario {
+        SCENARIO_CONTEXT
+    } else {
+        TEST_CONTEXT
+    };
+    match scan_ctes(&test.sql, &test.file_label, context, syntax) {
         Ok(scanned) => Ok(Classified::Authored {
             ctes: scanned
                 .ctes
@@ -218,13 +228,30 @@ fn authored_ctes(test: &TestRequest, syntax: &LexicalSyntax) -> Result<Classifie
     }
 }
 
+/// Who owns the CTEs being read, which names them in errors, and the adapter's lexical rules.
+struct CteOwner<'a> {
+    file: &'a str,
+    context: &'a str,
+    syntax: &'a LexicalSyntax,
+}
+
 /// The top-level CTEs of one test, with the code-point offset of each stripped body.
 struct ScannedCtes {
     ctes: Vec<Cte>,
     body_starts: Vec<usize>,
 }
 
-fn scan_ctes(sql: &str, file: &str, syntax: &LexicalSyntax) -> Result<ScannedCtes, Failure> {
+fn scan_ctes(
+    sql: &str,
+    file: &str,
+    context: &str,
+    syntax: &LexicalSyntax,
+) -> Result<ScannedCtes, Failure> {
+    let owner = CteOwner {
+        file,
+        context,
+        syntax,
+    };
     let mut body_starts: Vec<usize> = Vec::new();
     let mut index = skip_ignorable(sql, 0, syntax)?;
     index = consume_keyword(sql, index, "WITH").ok_or_else(|| {
@@ -237,7 +264,7 @@ fn scan_ctes(sql: &str, file: &str, syntax: &LexicalSyntax) -> Result<ScannedCte
     let mut ctes: Vec<Cte> = Vec::new();
     let mut seen: HashSet<String> = HashSet::new();
     loop {
-        let (name, end) = read_cte_name(sql, index, file, syntax)?;
+        let (name, end) = read_cte_name(sql, index, &owner)?;
         if !seen.insert(name.clone()) {
             return Err(format!("SQL test '{file}' defines duplicate CTE '{name}'").into());
         }
@@ -252,8 +279,11 @@ fn scan_ctes(sql: &str, file: &str, syntax: &LexicalSyntax) -> Result<ScannedCte
         index = consume_keyword(sql, index, "AS")
             .ok_or_else(|| format!("SQL test '{file}' expected keyword AS"))?;
         index = skip_ignorable(sql, index, syntax)?;
+        if let Some(failure) = materialization_failure(sql, index, &owner, &name)? {
+            return Err(failure);
+        }
         if byte_at(sql, index) != Some(b'(') {
-            return Err(format!("SQL test '{file}' CTE '{name}' must use AS (...)").into());
+            return Err(format!("{context} '{file}' CTE '{name}' must use AS (...)").into());
         }
         let close = matching_paren(sql, index, "SQL test", syntax)?;
         let body = &sql[index + 1..close];
@@ -279,9 +309,9 @@ fn scan_ctes(sql: &str, file: &str, syntax: &LexicalSyntax) -> Result<ScannedCte
 fn read_cte_name(
     sql: &str,
     start: usize,
-    file: &str,
-    syntax: &LexicalSyntax,
+    owner: &CteOwner<'_>,
 ) -> Result<(String, usize), Failure> {
+    let (file, context, syntax) = (owner.file, owner.context, owner.syntax);
     if let Some(quote) = byte_at(sql, start).filter(|byte| matches!(byte, b'"' | b'`' | b'[')) {
         let end = if quote == b'[' {
             sql[start..]
@@ -290,28 +320,59 @@ fn read_cte_name(
         } else {
             skip_non_code(sql, start, syntax)?
         };
-        return Err(cte_name_failure(sql, start, end, file));
+        return Err(cte_name_failure(sql, start, end, owner));
     }
-    let Some(first) = char_at(sql, start).filter(|first| *first == '_' || first.is_alphabetic())
-    else {
-        return Err(format!("SQL test '{file}' expected a CTE name").into());
+    let Some(first) = char_at(sql, start).filter(|first| is_identifier_continue(*first)) else {
+        return Err(format!("{context} '{file}' expected a CTE name").into());
     };
     let mut end = start + first.len_utf8();
     while let Some(character) =
         char_at(sql, end).filter(|character| is_identifier_continue(*character))
     {
         end += character.len_utf8();
     }
-    if sql[start..end]
-        .bytes()
-        .all(|byte| byte.is_ascii_alphanumeric() || byte == b'_')
+    if (first == '_' || first.is_ascii_alphabetic())
+        && sql[start..end]
+            .bytes()
+            .all(|byte| byte.is_ascii_alphanumeric() || byte == b'_')
     {
         return Ok((sql[start..end].to_owned(), end));
     }
-    Err(cte_name_failure(sql, start, end, file))
+    Err(cte_name_failure(sql, start, end, owner))
+}
+
+/// Python's error for `AS MATERIALIZED` or `AS NOT MATERIALIZED`, located at the hint.
+fn materialization_failure(
+    sql: &str,
+    start: usize,
+    owner: &CteOwner<'_>,
+    name: &str,
+) -> Result<Option<Failure>, Failure> {
+    let (hint, end) = match consume_keyword(sql, start, "NOT") {
+        Some(not_end) => {
+            let index = skip_ignorable(sql, not_end, owner.syntax)?;
+            match consume_keyword(sql, index, "MATERIALIZED") {
+                Some(end) => ("NOT MATERIALIZED", end),
+                None => return Ok(None),
+            }
+        }
+        None => match consume_keyword(sql, start, "MATERIALIZED") {
+            Some(end) => ("MATERIALIZED", end),
+            None => return Ok(None),
+        },
+    };
+    Ok(Some(Failure {
+        message: format!(
+            "{} '{}' CTE '{name}' must not use AS {hint}; materialization hints are not supported in {} CTEs",
+            owner.context, owner.file, owner.context
+        ),
+        help: Some(format!("remove {hint} and write {name} AS (...)")),
+        token: Some((sql[start..end].to_owned(), sql[..start].chars().count())),
+    }))
 }
 
-fn cte_name_failure(sql: &str, start: usize, end: usize, file: &str) -> Failure {
+fn cte_name_failure(sql: &str, start: usize, end: usize, owner: &CteOwner<'_>) -> Failure {
+    let (file, context) = (owner.file, owner.context);
     let token = &sql[start..end];
     let shown = if matches!(byte_at(sql, start), Some(b'"' | b'`' | b'[')) {
         token.to_owned()
@@ -338,7 +399,7 @@ fn cte_name_failure(sql: &str, start: usize, end: usize, file: &str) -> Failure
     }
     Failure {
         message: format!(
-            "SQL test '{file}' CTE name {shown} must be an unquoted identifier of ASCII letters, digits and underscores"
+            "{context} '{file}' CTE name {shown} must be an unquoted identifier of ASCII letters, digits and underscores"
         ),
         help: Some(format!(
             "rename the CTE, for example {suggestion}; quoted CTE names and names with $ or non-ASCII characters are not supported"

crates/sqlbuild-analysis/src/compiler/_helpers/sql_tests/scenario_extraction.rs

@@ -31,12 +31,10 @@ struct ClassifiedScenario {
     assertion_names: Vec<String>,
 }
 
-/// One scenario's scanner error, or its classification after any Polyglot independence check.
+/// One scenario's error, or its classification after any Polyglot independence check.
 #[derive(Debug, Default, Serialize)]
 #[serde(rename_all = "camelCase")]
 struct ScenarioOutcome {
-    #[serde(skip_serializing_if = "Option::is_none")]
-    scan_error: Option<String>,
     #[serde(skip_serializing_if = "Option::is_none")]
     independence: Option<Vec<Cte>>,
     #[serde(skip_serializing_if = "Option::is_none")]
@@ -54,7 +52,7 @@ pub(crate) fn extract_scenario_json(
         match scan_top_level_ctes(sql, file, RelationshipSource::Scenario, syntax) {
             TopLevelCtes::Deferred => return Ok(None),
             TopLevelCtes::Failed(message) => ScenarioOutcome {
-                scan_error: Some(message),
+                error: Some(message),
                 ..ScenarioOutcome::default()
             },
             TopLevelCtes::Scanned(ctes) => {

crates/sqlbuild-analysis/src/compiler/tests/helpers.rs

@@ -1991,8 +1991,8 @@ pub(crate) fn rejected_outcome(
 }
 
 /// The authored-block reading of `sql`: its CTEs with body offsets, or its first error.
-pub(crate) fn authored_ctes_response(sql: &str) -> Value {
+pub(crate) fn authored_ctes_response(sql: &str, scenario: bool) -> Value {
     batch_response(
-        &json!({"tests": [{"sql": sql, "fileLabel": "tests/t.sql", "mode": "model", "authored": true}]}),
+        &json!({"tests": [{"sql": sql, "fileLabel": "tests/t.sql", "mode": "model", "authored": true, "scenario": scenario}]}),
     )
 }

crates/sqlbuild-analysis/src/compiler/tests/test_sql_scenario_extraction.rs

@@ -64,16 +64,23 @@ fn given_scenarios_when_extracting_natively_then_python_outcomes_or_deferrals_ar
             ),
         },
         ScenarioExtractionTestCase {
-            description: "a quoted CTE name is a scanner error Python may still accept with Polyglot",
+            description: "a quoted CTE name is Python's scanner error",
             sql: "WITH \"__source__raw\" AS (SELECT 1), __expected__orders AS (SELECT 1 AS id)",
             expected_json: Some(
-                r#"{"scanError":"SQL scenario 'tests/scenarios/orders.sql' expected a CTE name"}"#,
+                r#"{"error":"SQL scenario 'tests/scenarios/orders.sql' expected a CTE name"}"#,
             ),
         },
         ScenarioExtractionTestCase {
-            description: "an unclosed body is a scanner error naming the scenario context",
+            description: "an unclosed body is Python's scanner error naming the scenario context",
             sql: "WITH __source__raw AS (SELECT 1",
-            expected_json: Some(r#"{"scanError":"SQL scenario contains an unclosed parenthesis"}"#),
+            expected_json: Some(r#"{"error":"SQL scenario contains an unclosed parenthesis"}"#),
+        },
+        ScenarioExtractionTestCase {
+            description: "a materialization hint is Python's scanner error",
+            sql: "WITH __source__raw AS NOT MATERIALIZED (SELECT 1), __expected__orders AS (SELECT 1)",
+            expected_json: Some(
+                r#"{"error":"SQL scenario 'tests/scenarios/orders.sql' CTE '__source__raw' must not use AS NOT MATERIALIZED; materialization hints are not supported in SQL scenario CTEs"}"#,
+            ),
         },
         ScenarioExtractionTestCase {
             description: "a keyword Python would match by case mapping defers",

crates/sqlbuild-analysis/src/compiler/tests/test_sql_test_extraction_rules.rs

@@ -385,6 +385,7 @@ fn given_authored_sql_tests_when_extracting_then_each_rule_holds() {
 fn given_authored_blocks_when_reading_ctes_then_offsets_and_name_errors_are_exact() {
     let test_cases = [
         AuthoredCtesTestCase {
+            scenario: false,
             description: "body offsets count code points and skip leading whitespace",
             sql: "-- café\nWITH h AS (  SELECT 'é' AS a), __ref__orders AS (\n SELECT 1)",
             expected_response: json!({"tests": [{"kind": "authored", "ctes": [
@@ -393,6 +394,7 @@ fn given_authored_blocks_when_reading_ctes_then_offsets_and_name_errors_are_exac
             ]}]}),
         },
         AuthoredCtesTestCase {
+            scenario: false,
             description: "a CTE name error keeps its code-point offset",
             sql: "-- é\nWITH h AS (SELECT 1), café AS (SELECT 2)",
             expected_response: json!({"error": {
@@ -404,6 +406,55 @@ fn given_authored_blocks_when_reading_ctes_then_offsets_and_name_errors_are_exac
             }}),
         },
         AuthoredCtesTestCase {
+            scenario: false,
+            description: "a materialization hint is located at the hint with help",
+            sql: "WITH h AS (SELECT 1),\n__ref__orders AS not\nmaterialized (SELECT 2)",
+            expected_response: json!({"error": {
+                "index": 0,
+                "message": "SQL test 'tests/t.sql' CTE '__ref__orders' must not use AS NOT MATERIALIZED; materialization hints are not supported in SQL test CTEs",
+                "help": "remove NOT MATERIALIZED and write __ref__orders AS (...)",
+                "token": "not\nmaterialized",
+                "tokenOffset": 39,
+            }}),
+        },
+        AuthoredCtesTestCase {
+            scenario: true,
+            description: "a scenario's quoted CTE name names the scenario",
+            sql: "WITH \"__source__raw_orders\" AS (SELECT 1), __expected__orders AS (SELECT 1)",
+            expected_response: json!({"error": {
+                "index": 0,
+                "message": "SQL scenario 'tests/t.sql' CTE name \"__source__raw_orders\" must be an unquoted identifier of ASCII letters, digits and underscores",
+                "help": "rename the CTE, for example __source__raw_orders; quoted CTE names and names with $ or non-ASCII characters are not supported",
+                "token": "\"__source__raw_orders\"",
+                "tokenOffset": 5,
+            }}),
+        },
+        AuthoredCtesTestCase {
+            scenario: true,
+            description: "a scenario's materialization hint names the scenario",
+            sql: "WITH __source__raw_orders AS MATERIALIZED (SELECT 1)",
+            expected_response: json!({"error": {
+                "index": 0,
+                "message": "SQL scenario 'tests/t.sql' CTE '__source__raw_orders' must not use AS MATERIALIZED; materialization hints are not supported in SQL scenario CTEs",
+                "help": "remove MATERIALIZED and write __source__raw_orders AS (...)",
+                "token": "MATERIALIZED",
+                "tokenOffset": 29,
+            }}),
+        },
+        AuthoredCtesTestCase {
+            scenario: false,
+            description: "a CTE name starting with a digit is located with help",
+            sql: "WITH 2024_orders AS (SELECT 1)",
+            expected_response: json!({"error": {
+                "index": 0,
+                "message": "SQL test 'tests/t.sql' CTE name '2024_orders' must be an unquoted identifier of ASCII letters, digits and underscores",
+                "help": "rename the CTE, for example cte_2024_orders; quoted CTE names and names with $ or non-ASCII characters are not supported",
+                "token": "2024_orders",
+                "tokenOffset": 5,
+            }}),
+        },
+        AuthoredCtesTestCase {
+            scenario: false,
             description: "other errors are left to extraction and read as no CTEs",
             sql: "WITH h AS (SELECT 1) SELECT * FROM h",
             expected_response: json!({"tests": [{"kind": "authored", "ctes": []}]}),
@@ -412,7 +463,7 @@ fn given_authored_blocks_when_reading_ctes_then_offsets_and_name_errors_are_exac
 
     for test_case in test_cases {
         assert_eq!(
-            authored_ctes_response(test_case.sql),
+            authored_ctes_response(test_case.sql, test_case.scenario),
             test_case.expected_response,
             "{}",
             test_case.description

crates/sqlbuild-analysis/src/compiler/tests/test_types.rs

@@ -162,6 +162,7 @@ pub(crate) struct ExtractionRuleTestCase {
 
 pub(crate) struct AuthoredCtesTestCase {
     pub(crate) description: &'static str,
+    pub(crate) scenario: bool,
     pub(crate) sql: &'static str,
     pub(crate) expected_response: serde_json::Value,
 }

crates/sqlbuild-discovery/src/sql_tests/_helpers/scenario_parsing.rs

@@ -38,9 +38,15 @@ pub(crate) fn parse_scenario_file(
         contents[..header_start].matches('\n').count() + 1,
     )?;
     let sql_body: String = python_cleandoc(options.python, &contents[sql_start..]);
+    let span_start: usize = contents[..sql_start].chars().count();
+    let body_span: (usize, usize) = (
+        span_start,
+        span_start + contents[sql_start..].chars().count(),
+    );
     Ok(DiscoveredScenarioFile {
         contents,
         header_values,
         sql_body,
+        body_span,
     })
 }

crates/sqlbuild-discovery/src/sql_tests/models.rs

@@ -36,4 +36,6 @@ pub struct DiscoveredScenarioFile {
     pub contents: String,
     pub header_values: Vec<(String, AuthoredValue)>,
     pub sql_body: String,
+    /// Code-point span of the authored SQL after the header, which `sql_body` cleans.
+    pub body_span: (usize, usize),
 }

crates/sqlbuild-discovery/src/sql_tests/tests/helpers.rs

@@ -94,3 +94,10 @@ pub(super) fn statement_failures(contents: &str) -> [Option<(String, Option<Stri
             .map(failure),
     ]
 }
+
+/// The code-point span of a parsed scenario's SQL after its header, or `None` if parsing failed.
+pub(super) fn scenario_body_span(contents: &str) -> Option<(usize, usize)> {
+    parse_scenario_text(FILE_PATH, contents.to_owned(), &options())
+        .ok()
+        .map(|file| file.body_span)
+}