parser: improve error handling for type args

PLAN.md

@@ -434,6 +434,21 @@ suggest using an aggregate or grouping by
 
 Provide options to select from in quick fix
 
+```sql
+create table t (t_id int, name text, created timestampz);
+create table u (u_id int, t_id int, created timestampz);
+
+select name, created from u join t using (t_id);
+--           ^^^^^^^ error: ambiguous column name `created`, prefix with either `t` or `u`
+--                   action: Prefix with...
+--                            Prefix with `t`
+--                            Prefix with `u`
+
+-- gives
+
+select name, u.created from u join t using (t_id);
+```
+
 ### Rule: column label is the same as an existing column
 
 ```sql
@@ -792,7 +807,7 @@ becomes after filling in alias name with `b`
 select b.name, b.email from bar
 ```
 
-should prompt for table name for each entry when there is an ambigous column
+should prompt for table name for each entry when there is an ambiguous column
 
 related:
 

crates/squawk_parser/src/grammar.rs

@@ -548,21 +548,23 @@ fn json_table_fn(p: &mut Parser<'_>) -> CompletedMarker {
 
 fn custom_fn(
     p: &mut Parser<'_>,
-    kind: SyntaxKind,
+    name: SyntaxKind,
     mut body: impl FnMut(&mut Parser<'_>),
 ) -> CompletedMarker {
-    assert!(p.at(kind));
+    assert!(p.at(name));
     let m = p.start();
     let name_ref = p.start();
-    p.expect(kind);
+    p.expect(name);
     name_ref.complete(p, NAME_REF);
+
     let args = p.start();
     p.expect(L_PAREN);
     if !p.at(R_PAREN) {
         body(p);
     }
     p.expect(R_PAREN);
     args.complete(p, ARG_LIST);
+
     opt_agg_clauses(p);
     m.complete(p, CALL_EXPR)
 }
@@ -1613,21 +1615,26 @@ fn type_mods(
         return Some(m.complete(p, PERCENT_TYPE));
     }
     if p.at(L_PAREN) && type_args_enabled {
-        p.bump(L_PAREN);
-        let type_args = p.start();
-        while !p.at(EOF) && !p.at(R_PAREN) {
-            let arg = p.start();
-            if expr(p).is_none() {
-                arg.abandon(p);
-                break;
-            }
-            arg.complete(p, ARG);
-            if !p.eat(COMMA) {
-                break;
-            }
-        }
-        p.expect(R_PAREN);
-        type_args.complete(p, ARG_LIST);
+        let m = p.start();
+        delimited(
+            p,
+            L_PAREN,
+            R_PAREN,
+            COMMA,
+            || "unexpected comma".to_string(),
+            EXPR_FIRST,
+            |p| {
+                let m = p.start();
+                if expr(p).is_some() {
+                    m.complete(p, ARG);
+                    true
+                } else {
+                    m.abandon(p);
+                    false
+                }
+            },
+        );
+        m.complete(p, ARG_LIST);
     }
     let cm = m.complete(p, kind);
     if !p.at(L_BRACK) && !p.at(ARRAY_KW) {
@@ -1715,6 +1722,13 @@ fn opt_type_name_with(p: &mut Parser<'_>, type_args_enabled: bool) -> Option<Com
             p.bump(PRECISION_KW);
             DOUBLE_TYPE
         }
+        // Column constraint start sequence that can also overlap with a type
+        // since `generated` is a valid type name. Special case this so we can
+        // be more generous in our parsing.
+        GENERATED_KW if p.nth_at(1, ALWAYS_KW) => {
+            m.abandon(p);
+            return None;
+        }
         _ if p.at_ts(TYPE_KEYWORDS) || p.at(IDENT) => {
             path_name_ref(p);
             PATH_TYPE
@@ -3785,12 +3799,6 @@ fn index_elem(p: &mut Parser<'_>) {
     }
 }
 
-#[derive(PartialEq, Eq, Clone, Copy)]
-enum ColDefType {
-    WithData,
-    ColumnNameOnly,
-}
-
 fn opt_operator(p: &mut Parser<'_>) -> bool {
     let (power, kind, _) = current_op(p, &Restrictions::default());
     if power == 0 {
@@ -4122,38 +4130,33 @@ const COLUMN_NAME_KEYWORDS: TokenSet = TokenSet::new(&[
     XMLTABLE_KW,
 ]);
 
+const COL_DEF_FIRST: TokenSet = TokenSet::new(&[LIKE_KW])
+    .union(TABLE_CONSTRAINT_FIRST)
+    .union(NAME_FIRST);
+
 // column_name data_type [ STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN | DEFAULT } ] [ COMPRESSION compression_method ] [ COLLATE collation ] [ column_constraint [ ... ] ]
 //   { column_name data_type [ STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN | DEFAULT } ] [ COMPRESSION compression_method ] [ COLLATE collation ] [ column_constraint [ ... ] ]
 //     | table_constraint
 //     | LIKE source_table [ like_option ... ] }
-fn col_def(p: &mut Parser<'_>, t: ColDefType) -> Option<CompletedMarker> {
-    if !(p.at(LIKE_KW)
-        || p.at_ts(TABLE_CONSTRAINT_FIRST)
-        // ColId
-        || p.at_ts(NAME_FIRST))
-    {
+fn opt_col_def(p: &mut Parser<'_>) -> Option<CompletedMarker> {
+    if !p.at_ts(COL_DEF_FIRST) {
         return None;
     }
+    // TODO: add validation to check we only specify this when data types are allowed for args
     // LIKE source_table [ like_option ... ]
-    if t == ColDefType::WithData && p.at(LIKE_KW) {
+    if p.at(LIKE_KW) {
         return Some(like_clause(p));
     }
     if p.at_ts(TABLE_CONSTRAINT_FIRST) {
         return Some(table_constraint(p));
     }
     let m = p.start();
     name(p);
-    match t {
-        ColDefType::WithData => {
-            if opt_type_name(p) {
-                opt_storage(p);
-                opt_compression_method(p);
-            }
-        }
-        ColDefType::ColumnNameOnly => {
-            opt_with_options(p);
-        }
+    if opt_type_name(p) {
+        opt_storage(p);
+        opt_compression_method(p);
     }
+    opt_with_options(p);
     opt_collate(p);
     opt_column_constraint_list(p);
     Some(m.complete(p, COLUMN))
@@ -4558,6 +4561,8 @@ const LHS_FIRST: TokenSet = TokenSet::new(&[
     ARRAY_KW,
     ROW_KW,
     DEFAULT_KW,
+    // for non-standard params, like :foo
+    COLON,
 ])
 .union(OPERATOR_FIRST)
 .union(LITERAL_FIRST)
@@ -4789,31 +4794,18 @@ fn opt_nulls_order(p: &mut Parser<'_>) {
     }
 }
 
-fn table_arg_list(p: &mut Parser<'_>, t: ColDefType) -> Option<CompletedMarker> {
+fn table_arg_list(p: &mut Parser<'_>) -> Option<CompletedMarker> {
     assert!(p.at(L_PAREN));
     let m = p.start();
-    match t {
-        ColDefType::WithData => {
-            p.expect(L_PAREN);
-        }
-        ColDefType::ColumnNameOnly => {
-            if !p.eat(L_PAREN) {
-                m.abandon(p);
-                return None;
-            }
-        }
-    }
-    while !p.at(EOF) && !p.at(R_PAREN) {
-        col_def(p, t);
-        if p.at(COMMA) && p.nth_at(1, R_PAREN) {
-            p.err_and_bump("unexpected trailing comma");
-            break;
-        }
-        if !p.eat(COMMA) {
-            break;
-        }
-    }
-    p.expect(R_PAREN);
+    delimited(
+        p,
+        L_PAREN,
+        R_PAREN,
+        COMMA,
+        || "unexpected comma".to_string(),
+        COL_DEF_FIRST,
+        |p| opt_col_def(p).is_some(),
+    );
     Some(m.complete(p, TABLE_ARG_LIST))
 }
 
@@ -4918,28 +4910,25 @@ fn create_table(p: &mut Parser<'_>) -> CompletedMarker {
     p.expect(TABLE_KW);
     opt_if_not_exists(p);
     path_name(p);
-    let mut col_def_t = ColDefType::WithData;
     let mut is_partition = false;
     // OF type_name
     if p.at(OF_KW) {
         of_type(p);
-        col_def_t = ColDefType::ColumnNameOnly;
-    // PARTITION OF parent_table
+        // TODO: add validation to check that table args don't have data types
+        // PARTITION OF parent_table
     } else if p.at(PARTITION_KW) {
         partition_of(p);
-        col_def_t = ColDefType::ColumnNameOnly;
+        // TODO: add validation to check that table args don't have data types
         is_partition = true;
     }
     if p.at(L_PAREN) {
-        table_arg_list(p, col_def_t);
+        table_arg_list(p);
     }
     if is_partition {
         partition_option(p);
     }
-    // [ INHERITS ( parent_table [, ... ] ) ]
-    if col_def_t == ColDefType::WithData {
-        opt_inherits_tables(p);
-    }
+    // TODO: add validation to check we don't specify this when data types aren't allowed
+    opt_inherits_tables(p);
     opt_partition_by(p);
     opt_using_method(p);
     if opt_with_params(p).is_none() {

crates/squawk_parser/tests/data/err/select.sql

@@ -32,6 +32,11 @@ select array[1, ,3];
 -- trailing comma
 select array[1,2,3,];
 
+-- cast with malformed type mod args
+select cast(x as varchar(100 200));
+select cast(x as varchar(100, , 200));
+select cast(x as t(a, b,));
+
 -- regression test: this would cause the parser to get stuck & panic, now it
 -- warns about a missing semicolon
 select select;

crates/squawk_parser/tests/snapshots/tests__alter_extension_ok.snap

@@ -231,8 +231,8 @@ SOURCE_FILE
     L_PAREN "("
     CHAR_TYPE
       VARCHAR_KW "varchar"
-      L_PAREN "("
       ARG_LIST
+        L_PAREN "("
         ARG
           LITERAL
             INT_NUMBER "100"
@@ -671,8 +671,8 @@ SOURCE_FILE
         WHITESPACE " "
         CHAR_TYPE
           VARCHAR_KW "varchar"
-          L_PAREN "("
           ARG_LIST
+            L_PAREN "("
             ARG
               LITERAL
                 INT_NUMBER "100"
@@ -752,8 +752,8 @@ SOURCE_FILE
         WHITESPACE " "
         CHAR_TYPE
           VARCHAR_KW "varchar"
-          L_PAREN "("
           ARG_LIST
+            L_PAREN "("
             ARG
               LITERAL
                 INT_NUMBER "100"
@@ -881,8 +881,8 @@ SOURCE_FILE
     WHITESPACE " "
     CHAR_TYPE
       VARCHAR_KW "varchar"
-      L_PAREN "("
       ARG_LIST
+        L_PAREN "("
         ARG
           LITERAL
             INT_NUMBER "100"
@@ -916,8 +916,8 @@ SOURCE_FILE
     WHITESPACE " "
     CHAR_TYPE
       VARCHAR_KW "varchar"
-      L_PAREN "("
       ARG_LIST
+        L_PAREN "("
         ARG
           LITERAL
             INT_NUMBER "100"
@@ -1140,8 +1140,8 @@ SOURCE_FILE
         WHITESPACE " "
         CHAR_TYPE
           VARCHAR_KW "varchar"
-          L_PAREN "("
           ARG_LIST
+            L_PAREN "("
             ARG
               LITERAL
                 INT_NUMBER "100"
@@ -1221,8 +1221,8 @@ SOURCE_FILE
         WHITESPACE " "
         CHAR_TYPE
           VARCHAR_KW "varchar"
-          L_PAREN "("
           ARG_LIST
+            L_PAREN "("
             ARG
               LITERAL
                 INT_NUMBER "100"
@@ -1279,8 +1279,8 @@ SOURCE_FILE
         WHITESPACE " "
         CHAR_TYPE
           VARCHAR_KW "varchar"
-          L_PAREN "("
           ARG_LIST
+            L_PAREN "("
             ARG
               LITERAL
                 INT_NUMBER "100"
@@ -1360,8 +1360,8 @@ SOURCE_FILE
         WHITESPACE " "
         CHAR_TYPE
           VARCHAR_KW "varchar"
-          L_PAREN "("
           ARG_LIST
+            L_PAREN "("
             ARG
               LITERAL
                 INT_NUMBER "100"

crates/squawk_parser/tests/snapshots/tests__alter_operator_family_ok.snap

@@ -153,8 +153,8 @@ SOURCE_FILE
     WHITESPACE " "
     CHAR_TYPE
       VARCHAR_KW "varchar"
-      L_PAREN "("
       ARG_LIST
+        L_PAREN "("
         ARG
           LITERAL
             INT_NUMBER "100"

crates/squawk_parser/tests/snapshots/tests__alter_sequence_ok.snap

@@ -29,8 +29,8 @@ SOURCE_FILE
     WHITESPACE " "
     CHAR_TYPE
       VARCHAR_KW "varchar"
-      L_PAREN "("
       ARG_LIST
+        L_PAREN "("
         ARG
           LITERAL
             INT_NUMBER "100"