Skip to content

Commit 96aca8e

Browse files
sbdchdsbdchd
committed
parser: improve error handling for type args
1 parent 126c1cb commit 96aca8e

19 files changed

+231
-112
lines changed

‎PLAN.md‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,21 @@ suggest using an aggregate or grouping by
434434

435435
Provide options to select from in quick fix
436436

437+
```sql
438+
create table t (t_id int, name text, created timestampz);
439+
create table u (u_id int, t_id int, created timestampz);
440+
441+
select name, created from u join t using (t_id);
442+
-- ^^^^^^^ error: ambiguous column name `created`, prefix with either `t` or `u`
443+
-- action: Prefix with...
444+
-- Prefix with `t`
445+
-- Prefix with `u`
446+
447+
-- gives
448+
449+
select name, u.created from u join t using (t_id);
450+
```
451+
437452
### Rule: column label is the same as an existing column
438453

439454
```sql
@@ -792,7 +807,7 @@ becomes after filling in alias name with `b`
792807
select b.name, b.email from bar
793808
```
794809

795-
should prompt for table name for each entry when there is an ambigous column
810+
should prompt for table name for each entry when there is an ambiguous column
796811

797812
related:
798813

‎crates/squawk_parser/src/grammar.rs‎

Lines changed: 62 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -548,21 +548,23 @@ fn json_table_fn(p: &mut Parser<'_>) -> CompletedMarker {
548548

549549
fn custom_fn(
550550
p: &mut Parser<'_>,
551-
kind: SyntaxKind,
551+
name: SyntaxKind,
552552
mut body: impl FnMut(&mut Parser<'_>),
553553
) -> CompletedMarker {
554-
assert!(p.at(kind));
554+
assert!(p.at(name));
555555
let m = p.start();
556556
let name_ref = p.start();
557-
p.expect(kind);
557+
p.expect(name);
558558
name_ref.complete(p, NAME_REF);
559+
559560
let args = p.start();
560561
p.expect(L_PAREN);
561562
if !p.at(R_PAREN) {
562563
body(p);
563564
}
564565
p.expect(R_PAREN);
565566
args.complete(p, ARG_LIST);
567+
566568
opt_agg_clauses(p);
567569
m.complete(p, CALL_EXPR)
568570
}
@@ -1613,21 +1615,26 @@ fn type_mods(
16131615
return Some(m.complete(p, PERCENT_TYPE));
16141616
}
16151617
if p.at(L_PAREN) && type_args_enabled {
1616-
p.bump(L_PAREN);
1617-
let type_args = p.start();
1618-
while !p.at(EOF) && !p.at(R_PAREN) {
1619-
let arg = p.start();
1620-
if expr(p).is_none() {
1621-
arg.abandon(p);
1622-
break;
1623-
}
1624-
arg.complete(p, ARG);
1625-
if !p.eat(COMMA) {
1626-
break;
1627-
}
1628-
}
1629-
p.expect(R_PAREN);
1630-
type_args.complete(p, ARG_LIST);
1618+
let m = p.start();
1619+
delimited(
1620+
p,
1621+
L_PAREN,
1622+
R_PAREN,
1623+
COMMA,
1624+
|| "unexpected comma".to_string(),
1625+
EXPR_FIRST,
1626+
|p| {
1627+
let m = p.start();
1628+
if expr(p).is_some() {
1629+
m.complete(p, ARG);
1630+
true
1631+
} else {
1632+
m.abandon(p);
1633+
false
1634+
}
1635+
},
1636+
);
1637+
m.complete(p, ARG_LIST);
16311638
}
16321639
let cm = m.complete(p, kind);
16331640
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
17151722
p.bump(PRECISION_KW);
17161723
DOUBLE_TYPE
17171724
}
1725+
// Column constraint start sequence that can also overlap with a type
1726+
// since `generated` is a valid type name. Special case this so we can
1727+
// be more generous in our parsing.
1728+
GENERATED_KW if p.nth_at(1, ALWAYS_KW) => {
1729+
m.abandon(p);
1730+
return None;
1731+
}
17181732
_ if p.at_ts(TYPE_KEYWORDS) || p.at(IDENT) => {
17191733
path_name_ref(p);
17201734
PATH_TYPE
@@ -3785,12 +3799,6 @@ fn index_elem(p: &mut Parser<'_>) {
37853799
}
37863800
}
37873801

3788-
#[derive(PartialEq, Eq, Clone, Copy)]
3789-
enum ColDefType {
3790-
WithData,
3791-
ColumnNameOnly,
3792-
}
3793-
37943802
fn opt_operator(p: &mut Parser<'_>) -> bool {
37953803
let (power, kind, _) = current_op(p, &Restrictions::default());
37963804
if power == 0 {
@@ -4122,38 +4130,33 @@ const COLUMN_NAME_KEYWORDS: TokenSet = TokenSet::new(&[
41224130
XMLTABLE_KW,
41234131
]);
41244132

4133+
const COL_DEF_FIRST: TokenSet = TokenSet::new(&[LIKE_KW])
4134+
.union(TABLE_CONSTRAINT_FIRST)
4135+
.union(NAME_FIRST);
4136+
41254137
// column_name data_type [ STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN | DEFAULT } ] [ COMPRESSION compression_method ] [ COLLATE collation ] [ column_constraint [ ... ] ]
41264138
// { column_name data_type [ STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN | DEFAULT } ] [ COMPRESSION compression_method ] [ COLLATE collation ] [ column_constraint [ ... ] ]
41274139
// | table_constraint
41284140
// | LIKE source_table [ like_option ... ] }
4129-
fn col_def(p: &mut Parser<'_>, t: ColDefType) -> Option<CompletedMarker> {
4130-
if !(p.at(LIKE_KW)
4131-
|| p.at_ts(TABLE_CONSTRAINT_FIRST)
4132-
// ColId
4133-
|| p.at_ts(NAME_FIRST))
4134-
{
4141+
fn opt_col_def(p: &mut Parser<'_>) -> Option<CompletedMarker> {
4142+
if !p.at_ts(COL_DEF_FIRST) {
41354143
return None;
41364144
}
4145+
// TODO: add validation to check we only specify this when data types are allowed for args
41374146
// LIKE source_table [ like_option ... ]
4138-
if t == ColDefType::WithData && p.at(LIKE_KW) {
4147+
if p.at(LIKE_KW) {
41394148
return Some(like_clause(p));
41404149
}
41414150
if p.at_ts(TABLE_CONSTRAINT_FIRST) {
41424151
return Some(table_constraint(p));
41434152
}
41444153
let m = p.start();
41454154
name(p);
4146-
match t {
4147-
ColDefType::WithData => {
4148-
if opt_type_name(p) {
4149-
opt_storage(p);
4150-
opt_compression_method(p);
4151-
}
4152-
}
4153-
ColDefType::ColumnNameOnly => {
4154-
opt_with_options(p);
4155-
}
4155+
if opt_type_name(p) {
4156+
opt_storage(p);
4157+
opt_compression_method(p);
41564158
}
4159+
opt_with_options(p);
41574160
opt_collate(p);
41584161
opt_column_constraint_list(p);
41594162
Some(m.complete(p, COLUMN))
@@ -4558,6 +4561,8 @@ const LHS_FIRST: TokenSet = TokenSet::new(&[
45584561
ARRAY_KW,
45594562
ROW_KW,
45604563
DEFAULT_KW,
4564+
// for non-standard params, like :foo
4565+
COLON,
45614566
])
45624567
.union(OPERATOR_FIRST)
45634568
.union(LITERAL_FIRST)
@@ -4789,31 +4794,18 @@ fn opt_nulls_order(p: &mut Parser<'_>) {
47894794
}
47904795
}
47914796

4792-
fn table_arg_list(p: &mut Parser<'_>, t: ColDefType) -> Option<CompletedMarker> {
4797+
fn table_arg_list(p: &mut Parser<'_>) -> Option<CompletedMarker> {
47934798
assert!(p.at(L_PAREN));
47944799
let m = p.start();
4795-
match t {
4796-
ColDefType::WithData => {
4797-
p.expect(L_PAREN);
4798-
}
4799-
ColDefType::ColumnNameOnly => {
4800-
if !p.eat(L_PAREN) {
4801-
m.abandon(p);
4802-
return None;
4803-
}
4804-
}
4805-
}
4806-
while !p.at(EOF) && !p.at(R_PAREN) {
4807-
col_def(p, t);
4808-
if p.at(COMMA) && p.nth_at(1, R_PAREN) {
4809-
p.err_and_bump("unexpected trailing comma");
4810-
break;
4811-
}
4812-
if !p.eat(COMMA) {
4813-
break;
4814-
}
4815-
}
4816-
p.expect(R_PAREN);
4800+
delimited(
4801+
p,
4802+
L_PAREN,
4803+
R_PAREN,
4804+
COMMA,
4805+
|| "unexpected comma".to_string(),
4806+
COL_DEF_FIRST,
4807+
|p| opt_col_def(p).is_some(),
4808+
);
48174809
Some(m.complete(p, TABLE_ARG_LIST))
48184810
}
48194811

@@ -4918,28 +4910,25 @@ fn create_table(p: &mut Parser<'_>) -> CompletedMarker {
49184910
p.expect(TABLE_KW);
49194911
opt_if_not_exists(p);
49204912
path_name(p);
4921-
let mut col_def_t = ColDefType::WithData;
49224913
let mut is_partition = false;
49234914
// OF type_name
49244915
if p.at(OF_KW) {
49254916
of_type(p);
4926-
col_def_t = ColDefType::ColumnNameOnly;
4927-
// PARTITION OF parent_table
4917+
// TODO: add validation to check that table args don't have data types
4918+
// PARTITION OF parent_table
49284919
} else if p.at(PARTITION_KW) {
49294920
partition_of(p);
4930-
col_def_t = ColDefType::ColumnNameOnly;
4921+
// TODO: add validation to check that table args don't have data types
49314922
is_partition = true;
49324923
}
49334924
if p.at(L_PAREN) {
4934-
table_arg_list(p, col_def_t);
4925+
table_arg_list(p);
49354926
}
49364927
if is_partition {
49374928
partition_option(p);
49384929
}
4939-
// [ INHERITS ( parent_table [, ... ] ) ]
4940-
if col_def_t == ColDefType::WithData {
4941-
opt_inherits_tables(p);
4942-
}
4930+
// TODO: add validation to check we don't specify this when data types aren't allowed
4931+
opt_inherits_tables(p);
49434932
opt_partition_by(p);
49444933
opt_using_method(p);
49454934
if opt_with_params(p).is_none() {

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,11 @@ select array[1, ,3];
3232
-- trailing comma
3333
select array[1,2,3,];
3434

35+
-- cast with malformed type mod args
36+
select cast(x as varchar(100 200));
37+
select cast(x as varchar(100, , 200));
38+
select cast(x as t(a, b,));
39+
3540
-- regression test: this would cause the parser to get stuck & panic, now it
3641
-- warns about a missing semicolon
3742
select select;

‎crates/squawk_parser/tests/snapshots/tests__alter_extension_ok.snap‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -231,8 +231,8 @@ SOURCE_FILE
231231
L_PAREN "("
232232
CHAR_TYPE
233233
VARCHAR_KW "varchar"
234-
L_PAREN "("
235234
ARG_LIST
235+
L_PAREN "("
236236
ARG
237237
LITERAL
238238
INT_NUMBER "100"
@@ -671,8 +671,8 @@ SOURCE_FILE
671671
WHITESPACE " "
672672
CHAR_TYPE
673673
VARCHAR_KW "varchar"
674-
L_PAREN "("
675674
ARG_LIST
675+
L_PAREN "("
676676
ARG
677677
LITERAL
678678
INT_NUMBER "100"
@@ -752,8 +752,8 @@ SOURCE_FILE
752752
WHITESPACE " "
753753
CHAR_TYPE
754754
VARCHAR_KW "varchar"
755-
L_PAREN "("
756755
ARG_LIST
756+
L_PAREN "("
757757
ARG
758758
LITERAL
759759
INT_NUMBER "100"
@@ -881,8 +881,8 @@ SOURCE_FILE
881881
WHITESPACE " "
882882
CHAR_TYPE
883883
VARCHAR_KW "varchar"
884-
L_PAREN "("
885884
ARG_LIST
885+
L_PAREN "("
886886
ARG
887887
LITERAL
888888
INT_NUMBER "100"
@@ -916,8 +916,8 @@ SOURCE_FILE
916916
WHITESPACE " "
917917
CHAR_TYPE
918918
VARCHAR_KW "varchar"
919-
L_PAREN "("
920919
ARG_LIST
920+
L_PAREN "("
921921
ARG
922922
LITERAL
923923
INT_NUMBER "100"
@@ -1140,8 +1140,8 @@ SOURCE_FILE
11401140
WHITESPACE " "
11411141
CHAR_TYPE
11421142
VARCHAR_KW "varchar"
1143-
L_PAREN "("
11441143
ARG_LIST
1144+
L_PAREN "("
11451145
ARG
11461146
LITERAL
11471147
INT_NUMBER "100"
@@ -1221,8 +1221,8 @@ SOURCE_FILE
12211221
WHITESPACE " "
12221222
CHAR_TYPE
12231223
VARCHAR_KW "varchar"
1224-
L_PAREN "("
12251224
ARG_LIST
1225+
L_PAREN "("
12261226
ARG
12271227
LITERAL
12281228
INT_NUMBER "100"
@@ -1279,8 +1279,8 @@ SOURCE_FILE
12791279
WHITESPACE " "
12801280
CHAR_TYPE
12811281
VARCHAR_KW "varchar"
1282-
L_PAREN "("
12831282
ARG_LIST
1283+
L_PAREN "("
12841284
ARG
12851285
LITERAL
12861286
INT_NUMBER "100"
@@ -1360,8 +1360,8 @@ SOURCE_FILE
13601360
WHITESPACE " "
13611361
CHAR_TYPE
13621362
VARCHAR_KW "varchar"
1363-
L_PAREN "("
13641363
ARG_LIST
1364+
L_PAREN "("
13651365
ARG
13661366
LITERAL
13671367
INT_NUMBER "100"

‎crates/squawk_parser/tests/snapshots/tests__alter_operator_family_ok.snap‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,8 +153,8 @@ SOURCE_FILE
153153
WHITESPACE " "
154154
CHAR_TYPE
155155
VARCHAR_KW "varchar"
156-
L_PAREN "("
157156
ARG_LIST
157+
L_PAREN "("
158158
ARG
159159
LITERAL
160160
INT_NUMBER "100"

‎crates/squawk_parser/tests/snapshots/tests__alter_sequence_ok.snap‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@ SOURCE_FILE
2929
WHITESPACE " "
3030
CHAR_TYPE
3131
VARCHAR_KW "varchar"
32-
L_PAREN "("
3332
ARG_LIST
33+
L_PAREN "("
3434
ARG
3535
LITERAL
3636
INT_NUMBER "100"

0 commit comments

Comments
 (0)