From d9a00a876bb5b470b7f4514ae0171908e3022632 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maria=20Jos=C3=A9=20Solano?= Date: Wed, 3 May 2023 14:09:36 -0700 Subject: [PATCH] Change type of flag defaults to Option (#9085) # Description Follow-up of #8940. As @bobhy pointed out, it makes sense for the behaviour of flags to match the one for positional arguments, where default values are of type `Option` instead of `Option`. # User-Facing Changes The same ones from the original PR: - Flag default values will now be parsed as constants. - If the default value is not a constant, a parser error is displayed. # Tests + Formatting A [new test](https://github.com/nushell/nushell/blob/e34e2d35f495000897d785bd31d8ace456d0da25/src/tests/test_engine.rs#L338-L344) has been added to verify the new restriction. --- crates/nu-engine/src/eval.rs | 18 ++++++------------ crates/nu-parser/src/parser.rs | 19 ++++++++++++++----- crates/nu-protocol/src/signature.rs | 3 +-- src/tests/test_engine.rs | 10 +++++++++- 4 files changed, 30 insertions(+), 20 deletions(-) diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index cb2cb84984..f809092b94 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -117,10 +117,8 @@ pub fn eval_call( let result = eval_expression(engine_state, caller_stack, arg)?; callee_stack.add_var(var_id, result); - } else if let Some(arg) = &named.default_value { - let result = eval_expression(engine_state, caller_stack, arg)?; - - callee_stack.add_var(var_id, result); + } else if let Some(value) = &named.default_value { + callee_stack.add_var(var_id, value.to_owned()); } else { callee_stack.add_var(var_id, Value::boolean(true, call.head)) } @@ -131,10 +129,8 @@ pub fn eval_call( let result = eval_expression(engine_state, caller_stack, arg)?; callee_stack.add_var(var_id, result); - } else if let Some(arg) = &named.default_value { - let result = eval_expression(engine_state, caller_stack, arg)?; - - callee_stack.add_var(var_id, result); + } else if let Some(value) = &named.default_value { + callee_stack.add_var(var_id, value.to_owned()); } else { callee_stack.add_var(var_id, Value::boolean(true, call.head)) } @@ -145,10 +141,8 @@ pub fn eval_call( if !found { if named.arg.is_none() { callee_stack.add_var(var_id, Value::boolean(false, call.head)) - } else if let Some(arg) = &named.default_value { - let result = eval_expression(engine_state, caller_stack, arg)?; - - callee_stack.add_var(var_id, result); + } else if let Some(value) = named.default_value { + callee_stack.add_var(var_id, value); } else { callee_stack.add_var(var_id, Value::Nothing { span: call.head }) } diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index ccc6bca93d..031c11fde9 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -3763,13 +3763,22 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> default_value, .. }) => { - let var_id = var_id.expect("internal error: all custom parameters must have var_ids"); - let var_type = &working_set.get_variable(var_id).ty; - - let expression_ty = expression.ty.clone(); let expression_span = expression.span; - *default_value = Some(expression); + *default_value = if let Ok(value) = + eval_constant(working_set, &expression) + { + Some(value) + } else { + working_set.error(ParseError::NonConstantDefaultValue( + expression_span, + )); + None + }; + + let var_id = var_id.expect("internal error: all custom parameters must have var_ids"); + let var_type = &working_set.get_variable(var_id).ty; + let expression_ty = expression.ty.clone(); // Flags with a boolean type are just present/not-present switches if var_type != &Type::Bool { diff --git a/crates/nu-protocol/src/signature.rs b/crates/nu-protocol/src/signature.rs index 8e2e35883f..071914d6ab 100644 --- a/crates/nu-protocol/src/signature.rs +++ b/crates/nu-protocol/src/signature.rs @@ -2,7 +2,6 @@ use serde::Deserialize; use serde::Serialize; use crate::ast::Call; -use crate::ast::Expression; use crate::engine::Command; use crate::engine::EngineState; use crate::engine::Stack; @@ -25,7 +24,7 @@ pub struct Flag { // For custom commands pub var_id: Option, - pub default_value: Option, + pub default_value: Option, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] diff --git a/src/tests/test_engine.rs b/src/tests/test_engine.rs index a8cfd24d8c..2c23d431f6 100644 --- a/src/tests/test_engine.rs +++ b/src/tests/test_engine.rs @@ -328,13 +328,21 @@ fn default_value_constant() -> TestResult { } #[test] -fn default_value_not_constant() -> TestResult { +fn default_value_not_constant1() -> TestResult { fail_test( r#"def foo [x = ("foo" | str length)] { $x }; foo"#, "expected a constant", ) } +#[test] +fn default_value_not_constant2() -> TestResult { + fail_test( + r#"def foo [--x = ("foo" | str length)] { $x }; foo"#, + "expected a constant", + ) +} + #[test] fn loose_each() -> TestResult { run_test(