Reuse Closure type in Value::Closure (nushell#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`.
hardfau1t · Dec 14, 2023 · 0378fe6 · 0378fe6
1 parent 6aa97ce
commit 0378fe6
Show file tree

Hide file tree

Showing 15 changed files with 119 additions and 121 deletions.
diff --git a/crates/nu-cli/src/prompt_update.rs b/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 {
-                val: block_id,
-                captures,
-                ..
-            } => {
-                let block = engine_state.get_block(block_id);
-                let mut stack = stack.captures_to_stack(&captures);
+            Value::Closure { val, .. } => {
+                let block = engine_state.get_block(val.block_id);
+                let mut stack = stack.captures_to_stack(&val.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());

diff --git a/crates/nu-cli/src/reedline_config.rs b/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,
             );

diff --git a/crates/nu-cmd-base/src/hook.rs b/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 {
-                    Value::Block { val: block_id, .. } | Value::Closure { val: block_id, .. } => {
-                        match run_hook_block(
-                            engine_state,
-                            stack,
-                            block_id,
-                            None,
-                            arguments.clone(),
-                            other_span,
-                        ) {
-                            Ok(pipeline_data) => {
-                                if let PipelineData::Value(Value::Bool { val, .. }, ..) =
-                                    pipeline_data
-                                {
-                                    val
-                                } else {
-                                    return Err(ShellError::UnsupportedConfigValue(
-                                        "boolean output".to_string(),
-                                        "other PipelineData variant".to_string(),
-                                        other_span,
-                                    ));
-                                }
-                            }
-                            Err(err) => {
-                                return Err(err);
+                if let Ok(block_id) = condition.as_block() {
+                    match run_hook_block(
+                        engine_state,
+                        stack,
+                        block_id,
+                        None,
+                        arguments.clone(),
+                        other_span,
+                    ) {
+                        Ok(pipeline_data) => {
+                            if let PipelineData::Value(Value::Bool { val, .. }, ..) = pipeline_data
+                            {
+                                val
+                            } else {
+                                return Err(ShellError::UnsupportedConfigValue(
+                                    "boolean output".to_string(),
+                                    "other PipelineData variant".to_string(),
+                                    other_span,
+                                ));
                             }
                         }
+                        Err(err) => {
+                            return Err(err);
+                        }
                     }
-                    other => {
-                        return Err(ShellError::UnsupportedConfigValue(
-                            "block".to_string(),
-                            format!("{}", other.get_type()),
-                            other_span,
-                        ));
-                    }
+                } else {
+                    return Err(ShellError::UnsupportedConfigValue(
+                        "block".to_string(),
+                        format!("{}", condition.get_type()),
+                        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, .. } => {
-            output = run_hook_block(engine_state, stack, *block_id, input, arguments, span)?;
+        Value::Closure { val, .. } => {
+            output = run_hook_block(engine_state, stack, val.block_id, input, arguments, span)?;
         }
         other => {
             return Err(ShellError::UnsupportedConfigValue(

diff --git a/crates/nu-cmd-lang/src/core_commands/describe.rs b/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 {

diff --git a/crates/nu-color-config/src/style_computer.rs b/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 {
-                        val: block_id,
-                        captures,
-                        ..
-                    } => {
-                        let block = self.engine_state.get_block(*block_id).clone();
+                    Value::Closure { val, .. } => {
+                        let block = self.engine_state.get_block(val.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) {

diff --git a/crates/nu-command/src/charting/hashable_value.rs b/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(

diff --git a/crates/nu-command/src/database/commands/into_sqlite.rs b/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:?}"),

diff --git a/crates/nu-command/src/debug/explain.rs b/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:?}"),

diff --git a/crates/nu-command/src/debug/view_source.rs b/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(
-                "Cannot view value".to_string(),
-                "this value cannot be viewed".to_string(),
-                Some(arg_span),
-                None,
-                Vec::new(),
-            )),
+            value => {
+                if let Ok(block_id) = value.as_block() {
+                    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())
+                    }
+                } else {
+                    Err(ShellError::GenericError(
+                        "Cannot view value".to_string(),
+                        "this value cannot be viewed".to_string(),
+                        Some(arg_span),
+                        None,
+                        Vec::new(),
+                    ))
+                }
+            }
         }
     }
 

diff --git a/crates/nu-command/src/formats/to/text.rs b/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:?}"),

diff --git a/crates/nu-engine/src/env.rs b/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, .. } => {
-                    let block = engine_state.get_block(block_id);
+                Value::Closure { val, .. } => {
+                    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);

diff --git a/crates/nu-engine/src/eval.rs b/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) => {

diff --git a/crates/nu-protocol/src/engine/capture_block.rs b/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>,

diff --git a/crates/nu-protocol/src/value/from_value.rs b/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 {
-                block_id: *val,
-                captures: captures.clone(),
-            }),
+            Value::Closure { val, .. } => Ok(val.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 {
-                item: Closure {
-                    block_id: *val,
-                    captures: captures.clone(),
-                },
+            Value::Closure { val, .. } => Ok(Spanned {
+                item: val.clone(),
                 span,
             }),
             v => Err(ShellError::CantConvert {