From 53330c5676277848845863a0cc28604326345a5d Mon Sep 17 00:00:00 2001 From: Fernando Herrera Date: Mon, 27 Dec 2021 19:13:52 +0000 Subject: [PATCH] def argument check (#604) * def argument check * corrected test * clippy error --- Cargo.lock | 2 +- crates/nu-parser/src/parse_keywords.rs | 214 +++++++++++-------------- crates/nu-parser/src/parser.rs | 21 ++- crates/nu-protocol/src/syntax_shape.rs | 2 +- crates/nu-protocol/src/ty.rs | 2 + src/tests/test_custom_commands.rs | 2 +- 6 files changed, 116 insertions(+), 127 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1158ad75d..d8f104268 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2562,7 +2562,7 @@ dependencies = [ [[package]] name = "reedline" version = "0.2.0" -source = "git+https://github.com/nushell/reedline?branch=main#3acf7da71a3cbcb7f9a9aaf5b1ab10f77e4e0f6e" +source = "git+https://github.com/nushell/reedline?branch=main#7c75ed6c627b18c78117746e8b5055853930aaa1" dependencies = [ "chrono", "crossterm", diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index e16172def..a4c170e23 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -12,7 +12,7 @@ use std::collections::{HashMap, HashSet}; use crate::{ lex, lite_parse, parser::{ - check_name, garbage, garbage_statement, parse, parse_block_expression, + check_call, check_name, garbage, garbage_statement, parse, parse_block_expression, parse_import_pattern, parse_internal_call, parse_multispan_value, parse_signature, parse_string, parse_var_with_opt_type, trim_quotes, }, @@ -61,132 +61,101 @@ pub fn parse_def( working_set: &mut StateWorkingSet, spans: &[Span], ) -> (Statement, Option) { - let mut error = None; - let name = working_set.get_span_contents(spans[0]); - - if name == b"def" { - // TODO: Convert all 'expect("internal error: ...")' to ParseError::InternalError - let def_decl_id = working_set - .find_decl(b"def") - .expect("internal error: missing def command"); - - let mut call = Box::new(Call { - head: spans[0], - decl_id: def_decl_id, - positional: vec![], - named: vec![], - }); - - if let Some(name_span) = spans.get(1) { - let (name_expr, err) = parse_string(working_set, *name_span); - error = error.or(err); - - let name = name_expr.as_string(); - call.positional.push(name_expr); - - if let Some(sig_span) = spans.get(2) { - working_set.enter_scope(); - let (sig, err) = parse_signature(working_set, *sig_span); - error = error.or(err); - - let signature = sig.as_signature(); - - call.positional.push(sig); - - if let Some(block_span) = spans.get(3) { - let (block, err) = parse_block_expression( - working_set, - &SyntaxShape::Block(Some(vec![])), - *block_span, - ); - error = error.or(err); - - let block_id = block.as_block(); - - call.positional.push(block); - - if let (Some(name), Some(mut signature), Some(block_id)) = - (&name, signature, block_id) - { - if let Some(decl_id) = working_set.find_decl(name.as_bytes()) { - let declaration = working_set.get_decl_mut(decl_id); - - signature.name = name.clone(); - - *declaration = signature.into_block_command(block_id); - } else { - error = error.or_else(|| { - Some(ParseError::InternalError( - "Predeclaration failed to add declaration".into(), - spans[1], - )) - }); - }; - } - } else { - let err_span = Span { - start: sig_span.end, - end: sig_span.end, - }; - - error = error - .or_else(|| Some(ParseError::MissingPositional("block".into(), err_span))); - } - working_set.exit_scope(); - - if let Some(name) = name { - // It's OK if it returns None: The decl was already merged in previous parse - // pass. - working_set.merge_predecl(name.as_bytes()); - } else { - error = error.or_else(|| { - Some(ParseError::UnknownState( - "Could not get string from string expression".into(), - *name_span, - )) - }); - } - } else { - let err_span = Span { - start: name_span.end, - end: name_span.end, - }; - - error = error - .or_else(|| Some(ParseError::MissingPositional("parameters".into(), err_span))); - } - } else { - let err_span = Span { - start: spans[0].end, - end: spans[0].end, - }; - - error = error.or_else(|| { - Some(ParseError::MissingPositional( - "definition name".into(), - err_span, - )) - }); - } - - ( - Statement::Pipeline(Pipeline::from_vec(vec![Expression { - expr: Expr::Call(call), - span: span(spans), - ty: Type::Unknown, - custom_completion: None, - }])), - error, - ) - } else { - ( + // 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"def" { + return ( garbage_statement(spans), Some(ParseError::UnknownState( - "Expected structure: def [] {}".into(), + "internal error: Wrong call name for def function".into(), span(spans), )), - ) + ); } + + // Parsing the spans and checking that they match the register signature + // Using a parsed call makes more sense than checking for how many spans are in the call + // Also, by creating a call, it can be checked if it matches the declaration signature + let (call, call_span) = match working_set.find_decl(b"def") { + None => { + return ( + garbage_statement(spans), + Some(ParseError::UnknownState( + "internal error: def declaration not found".into(), + span(spans), + )), + ) + } + Some(decl_id) => { + working_set.enter_scope(); + let (call, mut err) = parse_internal_call(working_set, spans[0], &spans[1..], decl_id); + working_set.exit_scope(); + + let call_span = span(spans); + let decl = working_set.get_decl(decl_id); + + err = check_call(call_span, &decl.signature(), &call).or(err); + if err.is_some() || call.has_flag("help") { + return ( + Statement::Pipeline(Pipeline::from_vec(vec![Expression { + expr: Expr::Call(call), + span: call_span, + ty: Type::Unknown, + custom_completion: None, + }])), + err, + ); + } + + (call, call_span) + } + }; + + // All positional arguments must be in the call positional vector by this point + let name_expr = call.positional.get(0).expect("def call already checked"); + let sig = call.positional.get(1).expect("def call already checked"); + let block = call.positional.get(2).expect("def call already checked"); + + let mut error = None; + if let (Some(name), Some(mut signature), Some(block_id)) = + (&name_expr.as_string(), sig.as_signature(), block.as_block()) + { + if let Some(decl_id) = working_set.find_decl(name.as_bytes()) { + let declaration = working_set.get_decl_mut(decl_id); + + signature.name = name.clone(); + *declaration = signature.into_block_command(block_id); + } else { + error = error.or_else(|| { + Some(ParseError::InternalError( + "Predeclaration failed to add declaration".into(), + spans[1], + )) + }); + }; + } + + if let Some(name) = name_expr.as_string() { + // It's OK if it returns None: The decl was already merged in previous parse pass. + working_set.merge_predecl(name.as_bytes()); + } else { + error = error.or_else(|| { + Some(ParseError::UnknownState( + "Could not get string from string expression".into(), + name_expr.span, + )) + }); + } + + ( + Statement::Pipeline(Pipeline::from_vec(vec![Expression { + expr: Expr::Call(call), + span: call_span, + ty: Type::Unknown, + custom_completion: None, + }])), + error, + ) } pub fn parse_alias( @@ -1137,7 +1106,6 @@ pub fn parse_register( working_set: &mut StateWorkingSet, spans: &[Span], ) -> (Statement, Option) { - use crate::parser::check_call; use nu_plugin::{get_signature, EncodingType, PluginDeclaration}; use nu_protocol::Signature; diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 143c8dd01..8e54d536b 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -89,6 +89,25 @@ pub fn check_call(command: Span, sig: &Signature, call: &Call) -> Option Type::Unknown, SyntaxShape::RowCondition => Type::Bool, SyntaxShape::Boolean => Type::Bool, - SyntaxShape::Signature => Type::Unknown, + SyntaxShape::Signature => Type::Signature, SyntaxShape::String => Type::String, SyntaxShape::Table => Type::List(Box::new(Type::Unknown)), // FIXME: Tables should have better types SyntaxShape::VarWithOptType => Type::Unknown, diff --git a/crates/nu-protocol/src/ty.rs b/crates/nu-protocol/src/ty.rs index 27893d26f..b01d74dc8 100644 --- a/crates/nu-protocol/src/ty.rs +++ b/crates/nu-protocol/src/ty.rs @@ -24,6 +24,7 @@ pub enum Type { Error, Binary, Custom, + Signature, } impl Display for Type { @@ -57,6 +58,7 @@ impl Display for Type { Type::Error => write!(f, "error"), Type::Binary => write!(f, "binary"), Type::Custom => write!(f, "custom"), + Type::Signature => write!(f, "signature"), } } } diff --git a/src/tests/test_custom_commands.rs b/src/tests/test_custom_commands.rs index 4a8089f42..1d4ff39f9 100644 --- a/src/tests/test_custom_commands.rs +++ b/src/tests/test_custom_commands.rs @@ -47,7 +47,7 @@ fn def_twice_should_fail() -> TestResult { #[test] fn missing_parameters() -> TestResult { - fail_test(r#"def foo {}"#, "expected [") + fail_test(r#"def foo {}"#, "Missing required positional") } #[test]