From 72cb4b6032cb4322aa3650c969eac5680d0ee276 Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Mon, 30 Oct 2023 22:34:23 +0000 Subject: [PATCH] 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 | 10 +-- crates/nu-cli/src/reedline_config.rs | 18 ++--- crates/nu-cmd-base/src/hook.rs | 68 +++++++++---------- .../nu-cmd-lang/src/core_commands/describe.rs | 8 ++- crates/nu-color-config/src/style_computer.rs | 10 +-- .../nu-command/src/charting/hashable_value.rs | 13 +++- .../src/database/commands/into_sqlite.rs | 2 +- crates/nu-command/src/debug/explain.rs | 2 +- crates/nu-command/src/debug/view_source.rs | 39 ++++++----- crates/nu-command/src/formats/to/text.rs | 2 +- crates/nu-engine/src/env.rs | 4 +- crates/nu-engine/src/eval.rs | 10 ++- .../nu-protocol/src/engine/capture_block.rs | 4 +- crates/nu-protocol/src/value/from_value.rs | 12 +--- crates/nu-protocol/src/value/mod.rs | 38 ++++------- 15 files changed, 119 insertions(+), 121 deletions(-) diff --git a/crates/nu-cli/src/prompt_update.rs b/crates/nu-cli/src/prompt_update.rs index d212ab2de..4eed1e7d6 100644 --- 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 index 9e758920f..cc372ee36 100644 --- 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 index fa79a1a2e..4f326960a 100644 --- 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 index 6f77c42bc..5a2dbf76a 100644 --- 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 index 12f7b0199..9ac37670c 100644 --- 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 index 1e7ff8b37..5d2314653 100644 --- 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 index bdaa135a3..a420eb7bb 100644 --- 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!(""), - Value::Closure { val, .. } => format!(""), + Value::Closure { val, .. } => format!("", 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 index a88fc78c1..86dcd8518 100644 --- 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!(""), - Value::Closure { val, .. } => format!(""), + Value::Closure { val, .. } => format!("", 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 index df636a1b5..7db2eeab3 100644 --- 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("", 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("", 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 index cb3092d45..0161d7ed2 100644 --- 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!(""), - Value::Closure { val, .. } => format!(""), + Value::Closure { val, .. } => format!("", 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 index 67a94c85b..af3a7177e 100644 --- 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 index 39d988125..df48bf8f2 100644 --- 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 index 291a009e0..974b037fb 100644 --- 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, diff --git a/crates/nu-protocol/src/value/from_value.rs b/crates/nu-protocol/src/value/from_value.rs index 63c535ecd..75a87bfca 100644 --- 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 { 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 { fn from_value(v: &Value) -> Result { 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 { diff --git a/crates/nu-protocol/src/value/mod.rs b/crates/nu-protocol/src/value/mod.rs index 88528bd8f..f495b7cf8 100644 --- a/crates/nu-protocol/src/value/mod.rs +++ b/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, - captures: HashMap, + val: Closure, // 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 { - val, - captures, - internal_span, - } => Value::Closure { - val: *val, - captures: captures.clone(), + Value::Closure { val, internal_span } => Value::Closure { + val: val.clone(), internal_span: *internal_span, }, Value::Nothing { internal_span } => Value::Nothing { @@ -443,7 +436,7 @@ impl Value { pub fn as_block(&self) -> Result { 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), 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!(""), - Value::Closure { val, .. } => format!(""), + Value::Closure { val, .. } => format!("", 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!(""), - Value::Closure { val, .. } => format!(""), + Value::Closure { val, .. } => format!("", 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!(""), - Value::Closure { val, .. } => format!(""), + Value::Closure { val, .. } => format!("", 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, 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) -> Value { - Value::closure(val, captures, Span::test_data()) + pub fn test_closure(val: Closure) -> Value { + 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),