From f9c0d223c1a329e65d60e4e72a120724a2afd0c6 Mon Sep 17 00:00:00 2001 From: JT <547158+jntrnr@users.noreply.github.com> Date: Sat, 15 Jan 2022 10:26:52 -0500 Subject: [PATCH] Improve keyword parsing, including for (#747) * Improve keyword parsing, including for * touchup --- crates/nu-parser/src/errors.rs | 20 ++++---- crates/nu-parser/src/parse_keywords.rs | 14 +++--- crates/nu-parser/src/parser.rs | 68 +++++++++++++++++++++----- src/tests/test_engine.rs | 5 ++ src/tests/test_parser.rs | 2 +- 5 files changed, 80 insertions(+), 29 deletions(-) diff --git a/crates/nu-parser/src/errors.rs b/crates/nu-parser/src/errors.rs index ae7432d388..44987e3796 100644 --- a/crates/nu-parser/src/errors.rs +++ b/crates/nu-parser/src/errors.rs @@ -61,10 +61,20 @@ pub enum ParseError { #[diagnostic( code(nu::parser::unexpected_keyword), url(docsrs), - help("'export' keyword is allowed only in a module.") + help("'{0}' keyword is allowed only in a module.") )] UnexpectedKeyword(String, #[label("unexpected {0}")] Span), + #[error("Statement used in pipeline.")] + #[diagnostic( + code(nu::parser::unexpected_keyword), + url(docsrs), + help( + "'{0}' keyword is not allowed in pipeline. Use '{0}' by itself, outside of a pipeline." + ) + )] + StatementInPipeline(String, #[label("not allowed in pipeline")] Span), + #[error("Incorrect value")] #[diagnostic(code(nu::parser::incorrect_value), url(docsrs), help("{2}"))] IncorrectValue(String, #[label("unexpected {0}")] Span, String), @@ -203,14 +213,6 @@ pub enum ParseError { #[diagnostic(code(nu::parser::file_not_found), url(docsrs))] FileNotFound(String, #[label("File not found: {0}")] Span), - #[error("'let' statements can't be part of a pipeline")] - #[diagnostic( - code(nu::parser::let_not_statement), - url(docsrs), - help("use parens to assign to a variable\neg) let x = ('hello' | str length)") - )] - LetNotStatement(#[label = "let statement part of a pipeline"] Span), - #[error("{0}")] #[diagnostic()] LabeledError(String, String, #[label("{1}")] Span), diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 8350117fc7..9fe5d6a207 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -60,12 +60,12 @@ pub fn parse_def_predecl(working_set: &mut StateWorkingSet, spans: &[Span]) -> O pub fn parse_for( working_set: &mut StateWorkingSet, spans: &[Span], -) -> (Statement, Option) { +) -> (Expression, Option) { // Checking that the function is used with the correct name // Maybe this is not necessary but it is a sanity check if working_set.get_span_contents(spans[0]) != b"for" { return ( - garbage_statement(spans), + garbage(spans[0]), Some(ParseError::UnknownState( "internal error: Wrong call name for 'for' function".into(), span(spans), @@ -79,7 +79,7 @@ pub fn parse_for( let (call, call_span) = match working_set.find_decl(b"for") { None => { return ( - garbage_statement(spans), + garbage(spans[0]), Some(ParseError::UnknownState( "internal error: def declaration not found".into(), span(spans), @@ -117,12 +117,12 @@ pub fn parse_for( err = check_call(call_span, &sig, &call).or(err); if err.is_some() || call.has_flag("help") { return ( - Statement::Pipeline(Pipeline::from_vec(vec![Expression { + Expression { expr: Expr::Call(call), span: call_span, ty: Type::Unknown, custom_completion: None, - }])), + }, err, ); } @@ -162,12 +162,12 @@ pub fn parse_for( } ( - Statement::Pipeline(Pipeline::from_vec(vec![Expression { + Expression { expr: Expr::Call(call), span: call_span, ty: Type::Unknown, custom_completion: None, - }])), + }, error, ) } diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 2541deb506..85ffeec588 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -488,6 +488,15 @@ pub fn parse_multispan_value( (arg, error) } + SyntaxShape::MathExpression => { + trace!("parsing: math expression"); + + let (arg, err) = parse_math_expression(working_set, &spans[*spans_idx..], None); + error = error.or(err); + *spans_idx = spans.len() - 1; + + (arg, error) + } SyntaxShape::Expression => { trace!("parsing: expression"); @@ -3337,7 +3346,49 @@ pub fn parse_expression( let (output, err) = if is_math_expression_byte(bytes[0]) { parse_math_expression(working_set, &spans[pos..], None) } else { - parse_call(working_set, &spans[pos..], expand_aliases, spans[0]) + // For now, check for special parses of certain keywords + match bytes { + b"def" => ( + parse_call(working_set, &spans[pos..], expand_aliases, spans[0]).0, + Some(ParseError::StatementInPipeline("def".into(), spans[0])), + ), + b"let" => ( + parse_call(working_set, &spans[pos..], expand_aliases, spans[0]).0, + Some(ParseError::StatementInPipeline("let".into(), spans[0])), + ), + b"alias" => ( + parse_call(working_set, &spans[pos..], expand_aliases, spans[0]).0, + Some(ParseError::StatementInPipeline("alias".into(), spans[0])), + ), + b"module" => ( + parse_call(working_set, &spans[pos..], expand_aliases, spans[0]).0, + Some(ParseError::StatementInPipeline("module".into(), spans[0])), + ), + b"use" => ( + parse_call(working_set, &spans[pos..], expand_aliases, spans[0]).0, + Some(ParseError::StatementInPipeline("use".into(), spans[0])), + ), + b"source" => ( + parse_call(working_set, &spans[pos..], expand_aliases, spans[0]).0, + Some(ParseError::StatementInPipeline("source".into(), spans[0])), + ), + b"export" => ( + parse_call(working_set, &spans[pos..], expand_aliases, spans[0]).0, + Some(ParseError::UnexpectedKeyword("export".into(), spans[0])), + ), + b"hide" => ( + parse_call(working_set, &spans[pos..], expand_aliases, spans[0]).0, + Some(ParseError::StatementInPipeline("hide".into(), spans[0])), + ), + #[cfg(feature = "plugin")] + b"register" => ( + parse_call(working_set, &spans[pos..], expand_aliases, spans[0]).0, + Some(ParseError::StatementInPipeline("plugin".into(), spans[0])), + ), + + b"for" => parse_for(working_set, spans), + _ => parse_call(working_set, &spans[pos..], expand_aliases, spans[0]), + } }; let with_env = working_set.find_decl(b"with-env"); @@ -3425,7 +3476,10 @@ pub fn parse_statement( match name { b"def" => parse_def(working_set, spans), b"let" => parse_let(working_set, spans), - b"for" => parse_for(working_set, spans), + b"for" => { + let (expr, err) = parse_for(working_set, spans); + (Statement::Pipeline(Pipeline::from_vec(vec![expr])), err) + } b"alias" => parse_alias(working_set, spans), b"module" => parse_module(working_set, spans), b"use" => parse_use(working_set, spans), @@ -3563,16 +3617,6 @@ pub fn parse_block( }) .collect::>(); - if let Some(let_call_id) = working_set.find_decl(b"let") { - for expr in output.iter() { - if let Expr::Call(x) = &expr.expr { - if let_call_id == x.decl_id && output.len() != 1 && error.is_none() { - error = Some(ParseError::LetNotStatement(expr.span)); - } - } - } - } - for expr in output.iter_mut().skip(1) { if expr.has_in_variable(working_set) { *expr = wrap_expr_with_collect(working_set, expr); diff --git a/src/tests/test_engine.rs b/src/tests/test_engine.rs index 3325d2f8b8..b8cac9a972 100644 --- a/src/tests/test_engine.rs +++ b/src/tests/test_engine.rs @@ -143,3 +143,8 @@ fn proper_variable_captures_with_nesting() -> TestResult { "102", ) } + +#[test] +fn proper_variable_for() -> TestResult { + run_test(r#"for x in 1..3 { if $x == 2 { "bob" } } | get 1"#, "bob") +} diff --git a/src/tests/test_parser.rs b/src/tests/test_parser.rs index 64fa65a66c..a714491950 100644 --- a/src/tests/test_parser.rs +++ b/src/tests/test_parser.rs @@ -116,7 +116,7 @@ fn long_flag() -> TestResult { #[test] fn let_not_statement() -> TestResult { - fail_test(r#"let x = "hello" | str length"#, "can't") + fail_test(r#"let x = "hello" | str length"#, "used in pipeline") } #[test]