From 1f2006f4dcbefe9bae6fb5a8b492dc251205e066 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Mon, 15 Jul 2024 09:37:27 -0700 Subject: [PATCH] feat: Improving error messages when parsin Exclude suggestions to add 'start of file' :) Also renames `parse_single` to `parse_source` --- prqlc/prqlc-parser/src/parser/expr.rs | 45 ++++++++- prqlc/prqlc-parser/src/parser/perror.rs | 9 +- prqlc/prqlc-parser/src/test.rs | 128 ++++++++++++------------ 3 files changed, 115 insertions(+), 67 deletions(-) diff --git a/prqlc/prqlc-parser/src/parser/expr.rs b/prqlc/prqlc-parser/src/parser/expr.rs index 9f88c2618688..bb3450f3e23d 100644 --- a/prqlc/prqlc-parser/src/parser/expr.rs +++ b/prqlc/prqlc-parser/src/parser/expr.rs @@ -537,13 +537,56 @@ fn operator_coalesce() -> impl Parser + Clone #[cfg(test)] mod tests { - use insta::assert_yaml_snapshot; + use insta::{assert_debug_snapshot, assert_yaml_snapshot}; use super::super::test::trim_start; use crate::test::parse_with_parser; use super::*; + #[test] + fn test_tuple() { + let tuple = || trim_start().ignore_then(tuple(expr())); + assert_yaml_snapshot!( + parse_with_parser(r#"{a = 5, b = 6}"#, tuple()).unwrap(), + @r###" + --- + Tuple: + - Literal: + Integer: 5 + span: "0:5-6" + alias: a + - Literal: + Integer: 6 + span: "0:12-13" + alias: b + "###); + + assert_debug_snapshot!( + parse_with_parser(r#" + {a = 5 + b = 6}"#, tuple()).unwrap_err(), + @r###" + [ + Error { + kind: Error, + span: Some( + 0:33-34, + ), + reason: Expected { + who: Some( + "new line", + ), + expected: "}", + found: "b", + }, + hints: [], + code: None, + }, + ] + "###); + } + #[test] fn test_expr() { assert_yaml_snapshot!( diff --git a/prqlc/prqlc-parser/src/parser/perror.rs b/prqlc/prqlc-parser/src/parser/perror.rs index 973aee315857..aebe54457cfc 100644 --- a/prqlc/prqlc-parser/src/parser/perror.rs +++ b/prqlc/prqlc-parser/src/parser/perror.rs @@ -222,8 +222,13 @@ impl From for Error { .all(|t| matches!(t, None | Some(TokenKind::NewLine))); let expected: Vec = e .expected() - // Only include whitespace if we're _only_ expecting whitespace - .filter(|t| is_all_whitespace || !matches!(t, None | Some(TokenKind::NewLine))) + // Only include whitespace if we're _only_ expecting whitespace, + // otherwise we get "expected start of line or new line or }" + // when there's no ending `}`, since a new line is allowed. + .filter(|t| { + is_all_whitespace + || !matches!(t, None | Some(TokenKind::NewLine) | Some(TokenKind::Start)) + }) .cloned() .map(token_to_string) .collect(); diff --git a/prqlc/prqlc-parser/src/test.rs b/prqlc/prqlc-parser/src/test.rs index d3e43302882c..40100281b742 100644 --- a/prqlc/prqlc-parser/src/test.rs +++ b/prqlc/prqlc-parser/src/test.rs @@ -27,7 +27,7 @@ pub(crate) fn parse_with_parser( } /// Parse into statements -pub(crate) fn parse_single(source: &str) -> Result, Vec> { +pub(crate) fn parse_source(source: &str) -> Result, Vec> { parse_with_parser(source, stmt::source()) } @@ -35,15 +35,15 @@ pub(crate) fn parse_single(source: &str) -> Result, Vec> { fn test_error_unicode_string() { // Test various unicode strings successfully parse errors. We were // getting loops in the lexer before. - parse_single("s’ ").unwrap_err(); - parse_single("s’").unwrap_err(); - parse_single(" s’").unwrap_err(); - parse_single(" ’ s").unwrap_err(); - parse_single("’s").unwrap_err(); - parse_single("👍 s’").unwrap_err(); + parse_source("s’ ").unwrap_err(); + parse_source("s’").unwrap_err(); + parse_source(" s’").unwrap_err(); + parse_source(" ’ s").unwrap_err(); + parse_source("’s").unwrap_err(); + parse_source("👍 s’").unwrap_err(); let source = "Mississippi has four S’s and four I’s."; - assert_debug_snapshot!(parse_single(source).unwrap_err(), @r###" + assert_debug_snapshot!(parse_source(source).unwrap_err(), @r###" [ Error { kind: Error, @@ -73,7 +73,7 @@ fn test_error_unicode_string() { #[test] fn test_error_unexpected() { - assert_debug_snapshot!(parse_single("Answer: T-H-A-T!").unwrap_err(), @r###" + assert_debug_snapshot!(parse_source("Answer: T-H-A-T!").unwrap_err(), @r###" [ Error { kind: Error, @@ -92,7 +92,7 @@ fn test_error_unexpected() { #[test] fn test_pipeline_parse_tree() { - assert_yaml_snapshot!(parse_single( + assert_yaml_snapshot!(parse_source( r#" from employees filter country == "USA" # Each line transforms the previous result. @@ -122,9 +122,9 @@ take 20 #[test] fn test_take() { - parse_single("take 10").unwrap(); + parse_source("take 10").unwrap(); - assert_yaml_snapshot!(parse_single(r#"take 10"#).unwrap(), @r###" + assert_yaml_snapshot!(parse_source(r#"take 10"#).unwrap(), @r###" --- - VarDef: kind: Main @@ -142,7 +142,7 @@ fn test_take() { span: "0:0-7" "###); - assert_yaml_snapshot!(parse_single(r#"take ..10"#).unwrap(), @r###" + assert_yaml_snapshot!(parse_source(r#"take ..10"#).unwrap(), @r###" --- - VarDef: kind: Main @@ -164,7 +164,7 @@ fn test_take() { span: "0:0-9" "###); - assert_yaml_snapshot!(parse_single(r#"take 1..10"#).unwrap(), @r###" + assert_yaml_snapshot!(parse_source(r#"take 1..10"#).unwrap(), @r###" --- - VarDef: kind: Main @@ -193,7 +193,7 @@ fn test_take() { #[test] fn test_filter() { assert_yaml_snapshot!( - parse_single(r#"filter country == "USA""#).unwrap(), @r###" + parse_source(r#"filter country == "USA""#).unwrap(), @r###" --- - VarDef: kind: Main @@ -219,7 +219,7 @@ fn test_filter() { "###); assert_yaml_snapshot!( - parse_single(r#"filter (text.upper country) == "USA""#).unwrap(), @r###" + parse_source(r#"filter (text.upper country) == "USA""#).unwrap(), @r###" --- - VarDef: kind: Main @@ -259,7 +259,7 @@ fn test_filter() { #[test] fn test_aggregate() { - let aggregate = parse_single( + let aggregate = parse_source( r"group {title} ( aggregate {sum salary, count} )", @@ -302,7 +302,7 @@ fn test_aggregate() { span: "0:0-77" span: "0:0-77" "###); - let aggregate = parse_single( + let aggregate = parse_source( r"group {title} ( aggregate {sum salary} )", @@ -348,7 +348,7 @@ fn test_aggregate() { #[test] fn test_basic_exprs() { // Currently not putting comments in our parse tree, so this is blank. - assert_yaml_snapshot!(parse_single( + assert_yaml_snapshot!(parse_source( r#"# this is a comment select a"# ).unwrap(), @r###" @@ -371,7 +371,7 @@ fn test_basic_exprs() { #[test] fn test_function() { - assert_yaml_snapshot!(parse_single("let plus_one = x -> x + 1\n").unwrap(), @r###" + assert_yaml_snapshot!(parse_source("let plus_one = x -> x + 1\n").unwrap(), @r###" --- - VarDef: kind: Let @@ -398,7 +398,7 @@ fn test_function() { span: "0:15-26" span: "0:0-26" "###); - assert_yaml_snapshot!(parse_single("let identity = x -> x\n").unwrap() + assert_yaml_snapshot!(parse_source("let identity = x -> x\n").unwrap() , @r###" --- - VarDef: @@ -418,7 +418,7 @@ fn test_function() { span: "0:15-22" span: "0:0-22" "###); - assert_yaml_snapshot!(parse_single("let plus_one = x -> (x + 1)\n").unwrap() + assert_yaml_snapshot!(parse_source("let plus_one = x -> (x + 1)\n").unwrap() , @r###" --- - VarDef: @@ -446,7 +446,7 @@ fn test_function() { span: "0:15-28" span: "0:0-28" "###); - assert_yaml_snapshot!(parse_single("let plus_one = x -> x + 1\n").unwrap() + assert_yaml_snapshot!(parse_source("let plus_one = x -> x + 1\n").unwrap() , @r###" --- - VarDef: @@ -475,7 +475,7 @@ fn test_function() { span: "0:0-26" "###); - assert_yaml_snapshot!(parse_single("let foo = x -> some_func (foo bar + 1) (plax) - baz\n").unwrap() + assert_yaml_snapshot!(parse_source("let foo = x -> some_func (foo bar + 1) (plax) - baz\n").unwrap() , @r###" --- - VarDef: @@ -525,7 +525,7 @@ fn test_function() { span: "0:0-51" "###); - assert_yaml_snapshot!(parse_single("func return_constant -> 42\n").unwrap(), @r###" + assert_yaml_snapshot!(parse_source("func return_constant -> 42\n").unwrap(), @r###" --- - VarDef: kind: Main @@ -546,7 +546,7 @@ fn test_function() { span: "0:0-27" "###); - assert_yaml_snapshot!(parse_single(r#"let count = X -> s"SUM({X})" + assert_yaml_snapshot!(parse_source(r#"let count = X -> s"SUM({X})" "#).unwrap(), @r###" --- - VarDef: @@ -574,7 +574,7 @@ fn test_function() { span: "0:0-28" "###); - assert_yaml_snapshot!(parse_single( + assert_yaml_snapshot!(parse_source( r#" let lag_day = x -> ( window x @@ -638,7 +638,7 @@ fn test_function() { span: "0:0-147" "###); - assert_yaml_snapshot!(parse_single("let add = x to:a -> x + to\n").unwrap(), @r###" + assert_yaml_snapshot!(parse_source("let add = x to:a -> x + to\n").unwrap(), @r###" --- - VarDef: kind: Let @@ -672,7 +672,7 @@ fn test_function() { #[test] fn test_var_def() { - assert_yaml_snapshot!(parse_single( + assert_yaml_snapshot!(parse_source( "let newest_employees = (from employees)" ).unwrap(), @r###" --- @@ -691,7 +691,7 @@ fn test_var_def() { span: "0:0-39" "###); - assert_yaml_snapshot!(parse_single( + assert_yaml_snapshot!(parse_source( r#" let newest_employees = ( from employees @@ -765,7 +765,7 @@ fn test_var_def() { span: "0:0-231" "###); - assert_yaml_snapshot!(parse_single(r#" + assert_yaml_snapshot!(parse_source(r#" let e = s"SELECT * FROM employees" "#).unwrap(), @r###" --- @@ -779,7 +779,7 @@ fn test_var_def() { span: "0:0-47" "###); - assert_yaml_snapshot!(parse_single( + assert_yaml_snapshot!(parse_source( "let x = ( from x_table @@ -834,7 +834,7 @@ fn test_var_def() { #[test] fn test_inline_pipeline() { - assert_yaml_snapshot!(parse_single("let median = x -> (x | percentile 50)\n").unwrap(), @r###" + assert_yaml_snapshot!(parse_source("let median = x -> (x | percentile 50)\n").unwrap(), @r###" --- - VarDef: kind: Let @@ -869,7 +869,7 @@ fn test_inline_pipeline() { #[test] fn test_sql_parameters() { - assert_yaml_snapshot!(parse_single(r#" + assert_yaml_snapshot!(parse_source(r#" from mytable filter { first_name == $1, @@ -925,7 +925,7 @@ fn test_sql_parameters() { #[test] fn test_tab_characters() { // #284 - parse_single( + parse_source( "from c_invoice join doc:c_doctype (==c_invoice_id) select [ @@ -946,7 +946,7 @@ join `my-proj.dataset.table` join `my-proj`.`dataset`.`table` "; - assert_yaml_snapshot!(parse_single(prql).unwrap(), @r###" + assert_yaml_snapshot!(parse_source(prql).unwrap(), @r###" --- - VarDef: kind: Main @@ -1025,7 +1025,7 @@ join `my-proj`.`dataset`.`table` #[test] fn test_sort() { - assert_yaml_snapshot!(parse_single(" + assert_yaml_snapshot!(parse_source(" from invoices sort issued_at sort (-issued_at) @@ -1121,7 +1121,7 @@ fn test_sort() { #[test] fn test_dates() { - assert_yaml_snapshot!(parse_single(" + assert_yaml_snapshot!(parse_source(" from employees derive {age_plus_two_years = (age + 2years)} ").unwrap(), @r###" @@ -1168,7 +1168,7 @@ fn test_dates() { #[test] fn test_multiline_string() { - assert_yaml_snapshot!(parse_single(r##" + assert_yaml_snapshot!(parse_source(r##" derive x = r"r-string test" "##).unwrap(), @r###" --- @@ -1194,7 +1194,7 @@ fn test_multiline_string() { fn test_empty_lines() { // The span of the Pipeline shouldn't include the empty lines; the VarDef // should have a larger span - assert_yaml_snapshot!(parse_single(r#" + assert_yaml_snapshot!(parse_source(r#" from artists derive x = 5 @@ -1233,7 +1233,7 @@ derive x = 5 #[test] fn test_coalesce() { - assert_yaml_snapshot!(parse_single(r###" + assert_yaml_snapshot!(parse_source(r###" from employees derive amount = amount ?? 0 "###).unwrap(), @r###" @@ -1276,7 +1276,7 @@ fn test_coalesce() { #[test] fn test_literal() { - assert_yaml_snapshot!(parse_single(r###" + assert_yaml_snapshot!(parse_source(r###" derive x = true "###).unwrap(), @r###" --- @@ -1300,7 +1300,7 @@ fn test_literal() { #[test] fn test_allowed_idents() { - assert_yaml_snapshot!(parse_single(r###" + assert_yaml_snapshot!(parse_source(r###" from employees join _salary (==employee_id) # table with leading underscore filter first_name == $1 @@ -1372,7 +1372,7 @@ fn test_allowed_idents() { #[test] fn test_gt_lt_gte_lte() { - assert_yaml_snapshot!(parse_single(r###" + assert_yaml_snapshot!(parse_source(r###" from people filter age >= 100 filter num_grandchildren <= 10 @@ -1465,7 +1465,7 @@ fn test_gt_lt_gte_lte() { #[test] fn test_assign() { - assert_yaml_snapshot!(parse_single(r###" + assert_yaml_snapshot!(parse_source(r###" from employees join s=salaries (==id) "###).unwrap(), @r###" @@ -1507,7 +1507,7 @@ join s=salaries (==id) #[test] fn test_unicode() { let source = "from tète"; - assert_yaml_snapshot!(parse_single(source).unwrap(), @r###" + assert_yaml_snapshot!(parse_source(source).unwrap(), @r###" --- - VarDef: kind: Main @@ -1527,7 +1527,7 @@ fn test_unicode() { #[test] fn test_var_defs() { - assert_yaml_snapshot!(parse_single(r#" + assert_yaml_snapshot!(parse_source(r#" let a = ( x ) @@ -1542,7 +1542,7 @@ fn test_var_defs() { span: "0:0-42" "###); - assert_yaml_snapshot!(parse_single(r#" + assert_yaml_snapshot!(parse_source(r#" x into a "#).unwrap(), @r###" @@ -1556,7 +1556,7 @@ fn test_var_defs() { span: "0:0-25" "###); - assert_yaml_snapshot!(parse_single(r#" + assert_yaml_snapshot!(parse_source(r#" x "#).unwrap(), @r###" --- @@ -1572,7 +1572,7 @@ fn test_var_defs() { #[test] fn test_array() { - assert_yaml_snapshot!(parse_single(r#" + assert_yaml_snapshot!(parse_source(r#" let a = [1, 2,] let a = [false, "hello"] "#).unwrap(), @r###" @@ -1608,7 +1608,7 @@ fn test_array() { #[test] fn test_annotation() { - assert_yaml_snapshot!(parse_single(r#" + assert_yaml_snapshot!(parse_source(r#" @{binding_strength=1} let add = a b -> a + b "#).unwrap(), @r###" @@ -1647,7 +1647,7 @@ fn test_annotation() { alias: binding_strength span: "0:10-30" "###); - parse_single( + parse_source( r#" @{binding_strength=1} let add = a b -> a + b @@ -1655,7 +1655,7 @@ fn test_annotation() { ) .unwrap(); - parse_single( + parse_source( r#" @{binding_strength=1} # comment @@ -1664,7 +1664,7 @@ fn test_annotation() { ) .unwrap(); - parse_single( + parse_source( r#" @{binding_strength=1} @@ -1683,7 +1683,7 @@ fn check_valid_version() { "#, env!("CARGO_PKG_VERSION_MAJOR") ); - assert!(parse_single(&stmt).is_ok()); + assert!(parse_source(&stmt).is_ok()); let stmt = format!( r#" @@ -1692,7 +1692,7 @@ fn check_valid_version() { env!("CARGO_PKG_VERSION_MAJOR"), env!("CARGO_PKG_VERSION_MINOR") ); - assert!(parse_single(&stmt).is_ok()); + assert!(parse_source(&stmt).is_ok()); let stmt = format!( r#" @@ -1702,7 +1702,7 @@ fn check_valid_version() { env!("CARGO_PKG_VERSION_MINOR"), env!("CARGO_PKG_VERSION_PATCH"), ); - assert!(parse_single(&stmt).is_ok()); + assert!(parse_source(&stmt).is_ok()); } #[test] @@ -1711,12 +1711,12 @@ fn check_invalid_version() { "prql version:{}\n", env!("CARGO_PKG_VERSION_MAJOR").parse::().unwrap() + 1 ); - assert!(parse_single(&stmt).is_err()); + assert!(parse_source(&stmt).is_err()); } #[test] fn test_target() { - assert_yaml_snapshot!(parse_single( + assert_yaml_snapshot!(parse_source( r#" prql target:sql.sqlite @@ -1761,7 +1761,7 @@ fn test_target() { #[test] fn test_number() { // We don't allow trailing periods - assert!(parse_single( + assert!(parse_source( r#" from artists derive x = 1."# @@ -1773,7 +1773,7 @@ fn test_number() { fn doc_comment() { use insta::assert_yaml_snapshot; - assert_yaml_snapshot!(parse_single(r###" + assert_yaml_snapshot!(parse_source(r###" from artists derive x = 5 "###).unwrap(), @r###" @@ -1806,7 +1806,7 @@ fn doc_comment() { span: "0:0-34" "###); - assert_yaml_snapshot!(parse_single(r###" + assert_yaml_snapshot!(parse_source(r###" from artists #! This is a doc comment @@ -1845,7 +1845,7 @@ fn doc_comment() { doc_comment: " This is a doc comment" "###); - assert_yaml_snapshot!(parse_single(r###" + assert_yaml_snapshot!(parse_source(r###" #! This is a doc comment from artists derive x = 5 @@ -1880,7 +1880,7 @@ fn doc_comment() { doc_comment: " This is a doc comment" "###); - assert_debug_snapshot!(parse_single(r###" + assert_debug_snapshot!(parse_source(r###" from artists #! This is a doc comment "###).unwrap_err(), @r###" [