From 62652cf8c13ef6561d733885474bbf2241408bcc Mon Sep 17 00:00:00 2001 From: WindSoilder Date: Wed, 22 Feb 2023 20:14:20 +0800 Subject: [PATCH] simplify `parse_expression` function code. (#8149) # Description When reading parser code to see how it works, I found that `parse_expression` function contains some duplicate code about function call, we can match several values at once to simplify code # User-Facing Changes None # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --- crates/nu-parser/src/errors.rs | 30 +----- crates/nu-parser/src/parser.rs | 162 ++------------------------------- 2 files changed, 14 insertions(+), 178 deletions(-) diff --git a/crates/nu-parser/src/errors.rs b/crates/nu-parser/src/errors.rs index a2425e9395..e71c948627 100644 --- a/crates/nu-parser/src/errors.rs +++ b/crates/nu-parser/src/errors.rs @@ -122,35 +122,15 @@ pub enum ParseError { )] BuiltinCommandInPipeline(String, #[label("not allowed in pipeline")] Span), - #[error("Let statement used in pipeline.")] + #[error("{0} statement used in pipeline.")] #[diagnostic( code(nu::parser::unexpected_keyword), url(docsrs), help( - "Assigning '{0}' to '{1}' does not produce a value to be piped. If the pipeline result is meant to be assigned to '{1}', use 'let {1} = ({0} | ...)'." + "Assigning '{1}' to '{2}' does not produce a value to be piped. If the pipeline result is meant to be assigned to '{2}', use '{0} {2} = ({1} | ...)'." ) )] - LetInPipeline(String, String, #[label("let in pipeline")] Span), - - #[error("Const statement used in pipeline.")] - #[diagnostic( - code(nu::parser::unexpected_keyword), - url(docsrs), - help( - "Assigning '{0}' to '{1}' does not produce a value to be piped. If the pipeline result is meant to be assigned to '{1}', use 'const {1} = ({0} | ...)'." - ) - )] - ConstInPipeline(String, String, #[label("const in pipeline")] Span), - - #[error("Mut statement used in pipeline.")] - #[diagnostic( - code(nu::parser::unexpected_keyword), - url(docsrs), - help( - "Assigning '{0}' to '{1}' does not produce a value to be piped. If the pipeline result is meant to be assigned to '{1}', use 'mut {1} = ({0} | ...)'." - ) - )] - MutInPipeline(String, String, #[label("mut in pipeline")] Span), + AssignInPipeline(String, String, String, #[label("'{0}' in pipeline")] Span), #[error("Let used with builtin variable name.")] #[diagnostic( @@ -465,9 +445,7 @@ impl ParseError { ParseError::ExpectedKeyword(_, s) => *s, ParseError::UnexpectedKeyword(_, s) => *s, ParseError::BuiltinCommandInPipeline(_, s) => *s, - ParseError::LetInPipeline(_, _, s) => *s, - ParseError::MutInPipeline(_, _, s) => *s, - ParseError::ConstInPipeline(_, _, s) => *s, + ParseError::AssignInPipeline(_, _, _, s) => *s, ParseError::LetBuiltinVar(_, s) => *s, ParseError::MutBuiltinVar(_, s) => *s, ParseError::ConstBuiltinVar(_, s) => *s, diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 302d11bc4b..43c32661cd 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -5015,22 +5015,12 @@ pub fn parse_expression( { parse_math_expression(working_set, &spans[pos..], None, expand_aliases_denylist) } else { - let bytes = working_set.get_span_contents(spans[pos]); + let bytes = working_set.get_span_contents(spans[pos]).to_vec(); // For now, check for special parses of certain keywords - match bytes { - b"def" => ( - parse_call( - working_set, - &spans[pos..], - spans[0], - expand_aliases_denylist, - is_subexpression, - ) - .0, - Some(ParseError::BuiltinCommandInPipeline("def".into(), spans[0])), - ), - b"extern" => ( + match bytes.as_slice() { + b"def" | b"extern" | b"for" | b"module" | b"use" | b"source" | b"alias" | b"export" + | b"hide" => ( parse_call( working_set, &spans[pos..], @@ -5040,11 +5030,12 @@ pub fn parse_expression( ) .0, Some(ParseError::BuiltinCommandInPipeline( - "extern".into(), + String::from_utf8(bytes) + .expect("builtin commands bytes should be able to convert to string"), spans[0], )), ), - b"for" => ( + b"let" | b"const" | b"mut" => ( parse_call( working_set, &spans[pos..], @@ -5053,18 +5044,9 @@ pub fn parse_expression( is_subexpression, ) .0, - Some(ParseError::BuiltinCommandInPipeline("for".into(), spans[0])), - ), - b"let" => ( - parse_call( - working_set, - &spans[pos..], - spans[0], - expand_aliases_denylist, - is_subexpression, - ) - .0, - Some(ParseError::LetInPipeline( + Some(ParseError::AssignInPipeline( + String::from_utf8(bytes) + .expect("builtin commands bytes should be able to convert to string"), String::from_utf8_lossy(match spans.len() { 1 | 2 | 3 => b"value", _ => working_set.get_span_contents(spans[3]), @@ -5078,91 +5060,6 @@ pub fn parse_expression( spans[0], )), ), - b"const" => ( - parse_call( - working_set, - &spans[pos..], - spans[0], - expand_aliases_denylist, - is_subexpression, - ) - .0, - Some(ParseError::ConstInPipeline( - String::from_utf8_lossy(match spans.len() { - 1 | 2 | 3 => b"value", - _ => working_set.get_span_contents(spans[3]), - }) - .to_string(), - String::from_utf8_lossy(match spans.len() { - 1 => b"variable", - _ => working_set.get_span_contents(spans[1]), - }) - .to_string(), - spans[0], - )), - ), - b"mut" => ( - parse_call( - working_set, - &spans[pos..], - spans[0], - expand_aliases_denylist, - is_subexpression, - ) - .0, - Some(ParseError::MutInPipeline( - String::from_utf8_lossy(match spans.len() { - 1 | 2 | 3 => b"value", - _ => working_set.get_span_contents(spans[3]), - }) - .to_string(), - String::from_utf8_lossy(match spans.len() { - 1 => b"variable", - _ => working_set.get_span_contents(spans[1]), - }) - .to_string(), - spans[0], - )), - ), - b"alias" => ( - parse_call( - working_set, - &spans[pos..], - spans[0], - expand_aliases_denylist, - is_subexpression, - ) - .0, - Some(ParseError::BuiltinCommandInPipeline( - "alias".into(), - spans[0], - )), - ), - b"module" => ( - parse_call( - working_set, - &spans[pos..], - spans[0], - expand_aliases_denylist, - is_subexpression, - ) - .0, - Some(ParseError::BuiltinCommandInPipeline( - "module".into(), - spans[0], - )), - ), - b"use" => ( - parse_call( - working_set, - &spans[pos..], - spans[0], - expand_aliases_denylist, - is_subexpression, - ) - .0, - Some(ParseError::BuiltinCommandInPipeline("use".into(), spans[0])), - ), b"overlay" => { if spans.len() > 1 && working_set.get_span_contents(spans[1]) == b"list" { // whitelist 'overlay list' @@ -5190,45 +5087,6 @@ pub fn parse_expression( ) } } - b"source" => ( - parse_call( - working_set, - &spans[pos..], - spans[0], - expand_aliases_denylist, - is_subexpression, - ) - .0, - Some(ParseError::BuiltinCommandInPipeline( - "source".into(), - spans[0], - )), - ), - b"export" => ( - parse_call( - working_set, - &spans[pos..], - spans[0], - expand_aliases_denylist, - is_subexpression, - ) - .0, - Some(ParseError::UnexpectedKeyword("export".into(), spans[0])), - ), - b"hide" => ( - parse_call( - working_set, - &spans[pos..], - spans[0], - expand_aliases_denylist, - is_subexpression, - ) - .0, - Some(ParseError::BuiltinCommandInPipeline( - "hide".into(), - spans[0], - )), - ), b"where" => parse_where_expr(working_set, &spans[pos..], expand_aliases_denylist), #[cfg(feature = "plugin")] b"register" => (