Change type of flag defaults to Option<Value> (#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<Value>` instead of
`Option<Expression>`.

# 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](e34e2d35f4/src/tests/test_engine.rs (L338-L344))
has been added to verify the new restriction.
This commit is contained in:
Maria José Solano 2023-05-03 14:09:36 -07:00 committed by GitHub
parent e4625acf24
commit d9a00a876b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 30 additions and 20 deletions

View file

@ -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 })
}

View file

@ -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 {

View file

@ -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<VarId>,
pub default_value: Option<Expression>,
pub default_value: Option<Value>,
}
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]

View file

@ -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(