From e251f3a0b41c321fd35e118cf9b91df4ae855252 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maria=20Jos=C3=A9=20Solano?= <majosolano99@gmail.com>
Date: Wed, 26 Apr 2023 07:14:02 -0700
Subject: [PATCH] Change type of parameter default values to `Option<Value>`
 (#8940)

# Description

Fixes #8939.

# User-Facing Changes

- Parameter default values will now be parsed as constants.
- If the default value is not a constant, a parser error is displayed.

# Tests + Formatting

The [only affected
test](https://github.com/nushell/nushell/blob/d42c2b2dbc941d63c5a05404a0c22c4a7cd9f965/src/tests/test_engine.rs#L325-L328)
has been updated to reflect the new behavior.
---
 crates/nu-engine/src/eval.rs          |  5 ++---
 crates/nu-parser/src/parser.rs        | 13 ++++++++++++-
 crates/nu-protocol/src/parse_error.rs |  5 +++++
 crates/nu-protocol/src/signature.rs   |  3 ++-
 src/tests/test_engine.rs              | 12 ++++++++++--
 5 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs
index 7fa74c782..cb2cb8498 100644
--- a/crates/nu-engine/src/eval.rs
+++ b/crates/nu-engine/src/eval.rs
@@ -72,9 +72,8 @@ pub fn eval_call(
             if let Some(arg) = call.positional_nth(param_idx) {
                 let result = eval_expression(engine_state, caller_stack, arg)?;
                 callee_stack.add_var(var_id, result);
-            } else if let Some(arg) = &param.default_value {
-                let result = eval_expression(engine_state, caller_stack, arg)?;
-                callee_stack.add_var(var_id, result);
+            } else if let Some(value) = &param.default_value {
+                callee_stack.add_var(var_id, value.to_owned());
             } else {
                 callee_stack.add_var(var_id, Value::nothing(call.head));
             }
diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs
index 3fb0dcf5f..9f4127d12 100644
--- a/crates/nu-parser/src/parser.rs
+++ b/crates/nu-parser/src/parser.rs
@@ -3735,8 +3735,19 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
                                                 }
                                             }
                                         }
+
+                                        *default_value = if let Ok(constant) =
+                                            eval_constant(working_set, &expression)
+                                        {
+                                            Some(constant)
+                                        } else {
+                                            working_set.error(ParseError::NonConstantDefaultValue(
+                                                expression.span,
+                                            ));
+                                            None
+                                        };
+
                                         *shape = expression.ty.to_shape();
-                                        *default_value = Some(expression);
                                         *required = false;
                                     }
                                     Arg::RestPositional(..) => {
diff --git a/crates/nu-protocol/src/parse_error.rs b/crates/nu-protocol/src/parse_error.rs
index 935e272c2..2696e8a16 100644
--- a/crates/nu-protocol/src/parse_error.rs
+++ b/crates/nu-protocol/src/parse_error.rs
@@ -338,6 +338,10 @@ pub enum ParseError {
         #[label = "parameter {0} needs to be '{1}' instead of '{2}'"] Span,
     ),
 
+    #[error("Default values should be constant expressions.")]
+    #[diagnostic(code(nu::parser::non_constant_default_value))]
+    NonConstantDefaultValue(#[label = "expected a constant value"] Span),
+
     #[error("Extra columns.")]
     #[diagnostic(code(nu::parser::extra_columns))]
     ExtraColumns(
@@ -472,6 +476,7 @@ impl ParseError {
             ParseError::IncompleteParser(s) => *s,
             ParseError::RestNeedsName(s) => *s,
             ParseError::ParameterMismatchType(_, _, _, s) => *s,
+            ParseError::NonConstantDefaultValue(s) => *s,
             ParseError::ExtraColumns(_, s) => *s,
             ParseError::MissingColumns(_, s) => *s,
             ParseError::AssignmentMismatch(_, _, s) => *s,
diff --git a/crates/nu-protocol/src/signature.rs b/crates/nu-protocol/src/signature.rs
index 491ba06ad..8e2e35883 100644
--- a/crates/nu-protocol/src/signature.rs
+++ b/crates/nu-protocol/src/signature.rs
@@ -11,6 +11,7 @@ use crate::PipelineData;
 use crate::ShellError;
 use crate::SyntaxShape;
 use crate::Type;
+use crate::Value;
 use crate::VarId;
 use std::fmt::Write;
 
@@ -35,7 +36,7 @@ pub struct PositionalArg {
 
     // For custom commands
     pub var_id: Option<VarId>,
-    pub default_value: Option<Expression>,
+    pub default_value: Option<Value>,
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
diff --git a/src/tests/test_engine.rs b/src/tests/test_engine.rs
index df9b03aa9..a8cfd24d8 100644
--- a/src/tests/test_engine.rs
+++ b/src/tests/test_engine.rs
@@ -323,8 +323,16 @@ fn default_value12() -> TestResult {
 }
 
 #[test]
-fn default_value_expression() -> TestResult {
-    run_test(r#"def foo [x = ("foo" | str length)] { $x }; foo"#, "3")
+fn default_value_constant() -> TestResult {
+    run_test(r#"def foo [x = "foo"] { $x }; foo"#, "foo")
+}
+
+#[test]
+fn default_value_not_constant() -> TestResult {
+    fail_test(
+        r#"def foo [x = ("foo" | str length)] { $x }; foo"#,
+        "expected a constant",
+    )
 }
 
 #[test]