Reuse Closure type in Value::Closure (#10894)

# Description
Reuses the existing `Closure` type in `Value::Closure`. This will help
with the span refactoring for `Value`. Additionally, this allows us to
more easily box or unbox the `Closure` case should we chose to do so in
the future.

# User-Facing Changes
Breaking API change for `nu_protocol`.

crates/nu-cli/src/prompt_update.rs

@@ -40,13 +40,9 @@ fn get_prompt_string(
     stack
         .get_env_var(engine_state, prompt)
         .and_then(|v| match v {
-            Value::Closure {
+            Value::Closure { val, .. } => {
-                val: block_id,
+                let block = engine_state.get_block(val.block_id);
-                captures,
+                let mut stack = stack.captures_to_stack(&val.captures);
-                ..
-            } => {
-                let block = engine_state.get_block(block_id);
-                let mut stack = stack.captures_to_stack(&captures);
                 // Use eval_subexpression to force a redirection of output, so we can use everything in prompt
                 let ret_val =
                     eval_subexpression(engine_state, &mut stack, block, PipelineData::empty());

crates/nu-cli/src/reedline_config.rs

@@ -248,11 +248,11 @@ pub(crate) fn add_columnar_menu(
         Value::Nothing { .. } => {
             Ok(line_editor.with_menu(ReedlineMenu::EngineCompleter(Box::new(columnar_menu))))
         }
-        Value::Closure { val, captures, .. } => {
+        Value::Closure { val, .. } => {
             let menu_completer = NuMenuCompleter::new(
-                *val,
+                val.block_id,
                 span,
-                stack.captures_to_stack(captures),
+                stack.captures_to_stack(&val.captures),
                 engine_state,
                 only_buffer_difference,
             );
@@ -330,11 +330,11 @@ pub(crate) fn add_list_menu(
         Value::Nothing { .. } => {
             Ok(line_editor.with_menu(ReedlineMenu::HistoryMenu(Box::new(list_menu))))
         }
-        Value::Closure { val, captures, .. } => {
+        Value::Closure { val, .. } => {
             let menu_completer = NuMenuCompleter::new(
-                *val,
+                val.block_id,
                 span,
-                stack.captures_to_stack(captures),
+                stack.captures_to_stack(&val.captures),
                 engine_state,
                 only_buffer_difference,
             );
@@ -448,11 +448,11 @@ pub(crate) fn add_description_menu(
                 completer,
             }))
         }
-        Value::Closure { val, captures, .. } => {
+        Value::Closure { val, .. } => {
             let menu_completer = NuMenuCompleter::new(
-                *val,
+                val.block_id,
                 span,
-                stack.captures_to_stack(captures),
+                stack.captures_to_stack(&val.captures),
                 engine_state,
                 only_buffer_difference,
             );

crates/nu-cmd-base/src/hook.rs

@@ -166,41 +166,37 @@ pub fn eval_hook(
                 value.clone().follow_cell_path(&[condition_path], false)
             {
                 let other_span = condition.span();
-                match condition {
+                if let Ok(block_id) = condition.as_block() {
-                    Value::Block { val: block_id, .. } | Value::Closure { val: block_id, .. } => {
+                    match run_hook_block(
-                        match run_hook_block(
+                        engine_state,
-                            engine_state,
+                        stack,
-                            stack,
+                        block_id,
-                            block_id,
+                        None,
-                            None,
+                        arguments.clone(),
-                            arguments.clone(),
+                        other_span,
-                            other_span,
+                    ) {
-                        ) {
+                        Ok(pipeline_data) => {
-                            Ok(pipeline_data) => {
+                            if let PipelineData::Value(Value::Bool { val, .. }, ..) = pipeline_data
-                                if let PipelineData::Value(Value::Bool { val, .. }, ..) =
+                            {
-                                    pipeline_data
+                                val
-                                {
+                            } else {
-                                    val
+                                return Err(ShellError::UnsupportedConfigValue(
-                                } else {
+                                    "boolean output".to_string(),
-                                    return Err(ShellError::UnsupportedConfigValue(
+                                    "other PipelineData variant".to_string(),
-                                        "boolean output".to_string(),
+                                    other_span,
-                                        "other PipelineData variant".to_string(),
+                                ));
-                                        other_span,
-                                    ));
-                                }
-                            }
-                            Err(err) => {
-                                return Err(err);
                             }
                         }
+                        Err(err) => {
+                            return Err(err);
+                        }
                     }
-                    other => {
+                } else {
-                        return Err(ShellError::UnsupportedConfigValue(
+                    return Err(ShellError::UnsupportedConfigValue(
-                            "block".to_string(),
+                        "block".to_string(),
-                            format!("{}", other.get_type()),
+                        format!("{}", condition.get_type()),
-                            other_span,
+                        other_span,
-                        ));
+                    ));
-                    }
                 }
             } else {
                 // always run the hook
@@ -280,11 +276,11 @@ pub fn eval_hook(
                             source_span,
                         )?;
                     }
-                    Value::Closure { val: block_id, .. } => {
+                    Value::Closure { val, .. } => {
                         run_hook_block(
                             engine_state,
                             stack,
-                            block_id,
+                            val.block_id,
                             input,
                             arguments,
                             source_span,
@@ -303,8 +299,8 @@ pub fn eval_hook(
         Value::Block { val: block_id, .. } => {
             output = run_hook_block(engine_state, stack, *block_id, input, arguments, span)?;
         }
-        Value::Closure { val: block_id, .. } => {
+        Value::Closure { val, .. } => {
-            output = run_hook_block(engine_state, stack, *block_id, input, arguments, span)?;
+            output = run_hook_block(engine_state, stack, val.block_id, input, arguments, span)?;
         }
         other => {
             return Err(ShellError::UnsupportedConfigValue(

crates/nu-cmd-lang/src/core_commands/describe.rs

@@ -1,6 +1,6 @@
 use nu_protocol::{
     ast::Call,
-    engine::{Command, EngineState, Stack, StateWorkingSet},
+    engine::{Closure, Command, EngineState, Stack, StateWorkingSet},
     record, Category, Example, IntoPipelineData, PipelineData, PipelineMetadata, Record,
     ShellError, Signature, Type, Value,
 };
@@ -328,7 +328,11 @@ fn describe_value(
             ),
             head,
         ),
-        Value::Block { val, .. } | Value::Closure { val, .. } => {
+        Value::Block { val, .. }
+        | Value::Closure {
+            val: Closure { block_id: val, .. },
+            ..
+        } => {
             let block = engine_state.map(|engine_state| engine_state.get_block(val));
 
             if let Some(block) = block {

crates/nu-color-config/src/style_computer.rs

@@ -57,15 +57,11 @@ impl<'a> StyleComputer<'a> {
             Some(ComputableStyle::Closure(v)) => {
                 let span = v.span();
                 match v {
-                    Value::Closure {
+                    Value::Closure { val, .. } => {
-                        val: block_id,
+                        let block = self.engine_state.get_block(val.block_id).clone();
-                        captures,
-                        ..
-                    } => {
-                        let block = self.engine_state.get_block(*block_id).clone();
                         // Because captures_to_stack() clones, we don't need to use with_env() here
                         // (contrast with_env() usage in `each` or `do`).
-                        let mut stack = self.stack.captures_to_stack(captures);
+                        let mut stack = self.stack.captures_to_stack(&val.captures);
 
                         // Support 1-argument blocks as well as 0-argument blocks.
                         if let Some(var) = block.signature.get_positional(0) {

crates/nu-command/src/charting/hashable_value.rs

@@ -178,7 +178,10 @@ impl PartialEq for HashableValue {
 #[cfg(test)]
 mod test {
     use super::*;
-    use nu_protocol::ast::{CellPath, PathMember};
+    use nu_protocol::{
+        ast::{CellPath, PathMember},
+        engine::Closure,
+    };
     use std::collections::{HashMap, HashSet};
 
     #[test]
@@ -237,7 +240,13 @@ mod test {
         let span = Span::test_data();
         let values = [
             Value::list(vec![Value::bool(true, span)], span),
-            Value::closure(0, HashMap::new(), span),
+            Value::closure(
+                Closure {
+                    block_id: 0,
+                    captures: HashMap::new(),
+                },
+                span,
+            ),
             Value::nothing(span),
             Value::error(ShellError::DidYouMean("what?".to_string(), span), span),
             Value::cell_path(

crates/nu-command/src/database/commands/into_sqlite.rs

@@ -259,7 +259,7 @@ fn nu_value_to_string(value: Value, separator: &str) -> String {
             Err(error) => format!("{error:?}"),
         },
         Value::Block { val, .. } => format!("<Block {val}>"),
-        Value::Closure { val, .. } => format!("<Closure {val}>"),
+        Value::Closure { val, .. } => format!("<Closure {}>", val.block_id),
         Value::Nothing { .. } => String::new(),
         Value::Error { error, .. } => format!("{error:?}"),
         Value::Binary { val, .. } => format!("{val:?}"),

crates/nu-command/src/debug/explain.rs

@@ -253,7 +253,7 @@ pub fn debug_string_without_formatting(value: &Value) -> String {
         },
         //TODO: It would be good to drill in deeper to blocks and closures.
         Value::Block { val, .. } => format!("<Block {val}>"),
-        Value::Closure { val, .. } => format!("<Closure {val}>"),
+        Value::Closure { val, .. } => format!("<Closure {}>", val.block_id),
         Value::Nothing { .. } => String::new(),
         Value::Error { error, .. } => format!("{error:?}"),
         Value::Binary { val, .. } => format!("{val:?}"),

crates/nu-command/src/debug/view_source.rs

@@ -37,17 +37,6 @@ impl Command for ViewSource {
         let arg_span = arg.span();
 
         match arg {
-            Value::Block { val: block_id, .. } | Value::Closure { val: block_id, .. } => {
-                let block = engine_state.get_block(block_id);
-
-                if let Some(span) = block.span {
-                    let contents = engine_state.get_span_contents(span);
-                    Ok(Value::string(String::from_utf8_lossy(contents), call.head)
-                        .into_pipeline_data())
-                } else {
-                    Ok(Value::string("<internal command>", call.head).into_pipeline_data())
-                }
-            }
             Value::String { val, .. } => {
                 if let Some(decl_id) = engine_state.find_decl(val.as_bytes(), &[]) {
                     // arg is a command
@@ -139,13 +128,27 @@ impl Command for ViewSource {
                     ))
                 }
             }
-            _ => Err(ShellError::GenericError(
+            value => {
-                "Cannot view value".to_string(),
+                if let Ok(block_id) = value.as_block() {
-                "this value cannot be viewed".to_string(),
+                    let block = engine_state.get_block(block_id);
-                Some(arg_span),
+
-                None,
+                    if let Some(span) = block.span {
-                Vec::new(),
+                        let contents = engine_state.get_span_contents(span);
-            )),
+                        Ok(Value::string(String::from_utf8_lossy(contents), call.head)
+                            .into_pipeline_data())
+                    } else {
+                        Ok(Value::string("<internal command>", call.head).into_pipeline_data())
+                    }
+                } else {
+                    Err(ShellError::GenericError(
+                        "Cannot view value".to_string(),
+                        "this value cannot be viewed".to_string(),
+                        Some(arg_span),
+                        None,
+                        Vec::new(),
+                    ))
+                }
+            }
         }
     }
 

crates/nu-command/src/formats/to/text.rs

@@ -142,7 +142,7 @@ fn local_into_string(value: Value, separator: &str, config: &Config) -> String {
             Err(error) => format!("{error:?}"),
         },
         Value::Block { val, .. } => format!("<Block {val}>"),
-        Value::Closure { val, .. } => format!("<Closure {val}>"),
+        Value::Closure { val, .. } => format!("<Closure {}>", val.block_id),
         Value::Nothing { .. } => String::new(),
         Value::Error { error, .. } => format!("{error:?}"),
         Value::Binary { val, .. } => format!("{val:?}"),

crates/nu-engine/src/env.rs

@@ -387,8 +387,8 @@ fn get_converted_value(
         if let Ok(v) = env_conversions.follow_cell_path_not_from_user_input(path_members, false) {
             let from_span = v.span();
             match v {
-                Value::Closure { val: block_id, .. } => {
+                Value::Closure { val, .. } => {
-                    let block = engine_state.get_block(block_id);
+                    let block = engine_state.get_block(val.block_id);
 
                     if let Some(var) = block.signature.get_positional(0) {
                         let mut stack = stack.gather_captures(engine_state, &block.captures);

crates/nu-engine/src/eval.rs

@@ -5,7 +5,7 @@ use nu_protocol::{
         eval_operator, Argument, Assignment, Bits, Block, Boolean, Call, Comparison, Expr,
         Expression, Math, Operator, PathMember, PipelineElement, Redirection,
     },
-    engine::{EngineState, Stack},
+    engine::{Closure, EngineState, Stack},
     IntoInterruptiblePipelineData, IntoPipelineData, PipelineData, Range, Record, ShellError, Span,
     Spanned, Unit, Value, VarId, ENV_VARIABLE_ID,
 };
@@ -530,7 +530,13 @@ pub fn eval_expression(
             for var_id in &block.captures {
                 captures.insert(*var_id, stack.get_var(*var_id, expr.span)?);
             }
-            Ok(Value::closure(*block_id, captures, expr.span))
+            Ok(Value::closure(
+                Closure {
+                    block_id: *block_id,
+                    captures,
+                },
+                expr.span,
+            ))
         }
         Expr::Block(block_id) => Ok(Value::block(*block_id, expr.span)),
         Expr::List(x) => {

crates/nu-protocol/src/engine/capture_block.rs

@@ -2,7 +2,9 @@ use std::collections::HashMap;
 
 use crate::{BlockId, Value, VarId};
 
-#[derive(Clone, Debug)]
+use serde::{Deserialize, Serialize};
+
+#[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct Closure {
     pub block_id: BlockId,
     pub captures: HashMap<VarId, Value>,

crates/nu-protocol/src/value/from_value.rs

@@ -500,10 +500,7 @@ impl FromValue for Record {
 impl FromValue for Closure {
     fn from_value(v: &Value) -> Result<Self, ShellError> {
         match v {
-            Value::Closure { val, captures, .. } => Ok(Closure {
+            Value::Closure { val, .. } => Ok(val.clone()),
-                block_id: *val,
-                captures: captures.clone(),
-            }),
             Value::Block { val, .. } => Ok(Closure {
                 block_id: *val,
                 captures: HashMap::new(),
@@ -536,11 +533,8 @@ impl FromValue for Spanned<Closure> {
     fn from_value(v: &Value) -> Result<Self, ShellError> {
         let span = v.span();
         match v {
-            Value::Closure { val, captures, .. } => Ok(Spanned {
+            Value::Closure { val, .. } => Ok(Spanned {
-                item: Closure {
+                item: val.clone(),
-                    block_id: *val,
-                    captures: captures.clone(),
-                },
                 span,
             }),
             v => Err(ShellError::CantConvert {

crates/nu-protocol/src/value/mod.rs

@@ -9,9 +9,9 @@ mod unit;
 
 use crate::ast::{Bits, Boolean, CellPath, Comparison, MatchPattern, PathMember};
 use crate::ast::{Math, Operator};
-use crate::engine::EngineState;
+use crate::engine::{Closure, EngineState};
 use crate::ShellError;
-use crate::{did_you_mean, BlockId, Config, Span, Spanned, Type, VarId};
+use crate::{did_you_mean, BlockId, Config, Span, Spanned, Type};
 
 use byte_unit::ByteUnit;
 use chrono::{DateTime, Datelike, Duration, FixedOffset, Locale, TimeZone};
@@ -26,7 +26,6 @@ use num_format::ToFormattedString;
 pub use range::*;
 pub use record::Record;
 use serde::{Deserialize, Serialize};
-use std::collections::HashMap;
 use std::fmt::Write;
 use std::{
     borrow::Cow,
@@ -109,8 +108,7 @@ pub enum Value {
         internal_span: Span,
     },
     Closure {
-        val: BlockId,
+        val: Closure,
-        captures: HashMap<VarId, Value>,
         // note: spans are being refactored out of Value
         // please use .span() instead of matching this span value
         internal_span: Span,
@@ -202,13 +200,8 @@ impl Clone for Value {
                 val: *val,
                 internal_span: *internal_span,
             },
-            Value::Closure {
+            Value::Closure { val, internal_span } => Value::Closure {
-                val,
+                val: val.clone(),
-                captures,
-                internal_span,
-            } => Value::Closure {
-                val: *val,
-                captures: captures.clone(),
                 internal_span: *internal_span,
             },
             Value::Nothing { internal_span } => Value::Nothing {
@@ -443,7 +436,7 @@ impl Value {
     pub fn as_block(&self) -> Result<BlockId, ShellError> {
         match self {
             Value::Block { val, .. } => Ok(*val),
-            Value::Closure { val, .. } => Ok(*val),
+            Value::Closure { val, .. } => Ok(val.block_id),
             x => Err(ShellError::CantConvert {
                 to_type: "block".into(),
                 from_type: x.get_type().to_string(),
@@ -453,9 +446,9 @@ impl Value {
         }
     }
 
-    pub fn as_closure(&self) -> Result<(BlockId, &HashMap<VarId, Value>), ShellError> {
+    pub fn as_closure(&self) -> Result<&Closure, ShellError> {
         match self {
-            Value::Closure { val, captures, .. } => Ok((*val, captures)),
+            Value::Closure { val, .. } => Ok(val),
             x => Err(ShellError::CantConvert {
                 to_type: "closure".into(),
                 from_type: x.get_type().to_string(),
@@ -732,7 +725,7 @@ impl Value {
                 collected.into_string(separator, config)
             }
             Value::Block { val, .. } => format!("<Block {val}>"),
-            Value::Closure { val, .. } => format!("<Closure {val}>"),
+            Value::Closure { val, .. } => format!("<Closure {}>", val.block_id),
             Value::Nothing { .. } => String::new(),
             Value::Error { error, .. } => format!("{error:?}"),
             Value::Binary { val, .. } => format!("{val:?}"),
@@ -787,7 +780,7 @@ impl Value {
                 Err(error) => format!("{error:?}"),
             },
             Value::Block { val, .. } => format!("<Block {val}>"),
-            Value::Closure { val, .. } => format!("<Closure {val}>"),
+            Value::Closure { val, .. } => format!("<Closure {}>", val.block_id),
             Value::Nothing { .. } => String::new(),
             Value::Error { error, .. } => format!("{error:?}"),
             Value::Binary { val, .. } => format!("{val:?}"),
@@ -895,7 +888,7 @@ impl Value {
                 Err(error) => format!("{error:?}"),
             },
             Value::Block { val, .. } => format!("<Block {val}>"),
-            Value::Closure { val, .. } => format!("<Closure {val}>"),
+            Value::Closure { val, .. } => format!("<Closure {}>", val.block_id),
             Value::Nothing { .. } => String::new(),
             Value::Error { error, .. } => format!("{error:?}"),
             Value::Binary { val, .. } => format!("{val:?}"),
@@ -1836,10 +1829,9 @@ impl Value {
         }
     }
 
-    pub fn closure(val: BlockId, captures: HashMap<VarId, Value>, span: Span) -> Value {
+    pub fn closure(val: Closure, span: Span) -> Value {
         Value::Closure {
             val,
-            captures,
             internal_span: span,
         }
     }
@@ -1961,8 +1953,8 @@ impl Value {
 
     /// Note: Only use this for test data, *not* live data, as it will point into unknown source
     /// when used in errors.
-    pub fn test_closure(val: BlockId, captures: HashMap<VarId, Value>) -> Value {
+    pub fn test_closure(val: Closure) -> Value {
-        Value::closure(val, captures, Span::test_data())
+        Value::closure(val, Span::test_data())
     }
 
     /// Note: Only use this for test data, *not* live data, as it will point into unknown source
@@ -2288,7 +2280,7 @@ impl PartialOrd for Value {
                 Value::LazyRecord { .. } => Some(Ordering::Greater),
                 Value::List { .. } => Some(Ordering::Greater),
                 Value::Block { .. } => Some(Ordering::Greater),
-                Value::Closure { val: rhs, .. } => lhs.partial_cmp(rhs),
+                Value::Closure { val: rhs, .. } => lhs.block_id.partial_cmp(&rhs.block_id),
                 Value::Nothing { .. } => Some(Ordering::Less),
                 Value::Error { .. } => Some(Ordering::Less),
                 Value::Binary { .. } => Some(Ordering::Less),