Make FromValue take owned Values (#10900)

# Description Changes `FromValue` to take owned `Value`s instead of borrowed `Value`s. This eliminates some unnecessary clones (e.g., in `call_ext.rs`). # User-Facing Changes Breaking API change for `nu_protocol`.
nushell · Oct 31, 2023 · 15c22db · 15c22db
1 parent 1c52b11
commit 15c22db
Show file tree

Hide file tree

Showing 9 changed files with 102 additions and 116 deletions.
diff --git a/crates/nu-cmd-dataframe/src/dataframe/utils.rs b/crates/nu-cmd-dataframe/src/dataframe/utils.rs
@@ -1,15 +1,16 @@
 use nu_protocol::{FromValue, ShellError, Value};
 
 pub fn extract_strings(value: Value) -> Result<Vec<String>, ShellError> {
+    let span = value.span();
     match (
-        <String as FromValue>::from_value(&value),
-        <Vec<String> as FromValue>::from_value(&value),
+        <String as FromValue>::from_value(value.clone()),
+        <Vec<String> as FromValue>::from_value(value),
     ) {
         (Ok(col), Err(_)) => Ok(vec![col]),
         (Err(_), Ok(cols)) => Ok(cols),
         _ => Err(ShellError::IncompatibleParametersSingle {
             msg: "Expected a string or list of strings".into(),
-            span: value.span(),
+            span,
         }),
     }
 }
diff --git a/crates/nu-command/src/filters/drop/column.rs b/crates/nu-command/src/filters/drop/column.rs
@@ -157,7 +157,7 @@ fn get_cellpath_columns(keep_cols: Vec<String>, span: Span) -> Vec<CellPath> {
     let mut output = vec![];
     for keep_col in keep_cols {
         let val = Value::string(keep_col, span);
-        let cell_path = match CellPath::from_value(&val) {
+        let cell_path = match CellPath::from_value(val) {
             Ok(v) => v,
             Err(_) => return vec![],
         };

diff --git a/crates/nu-command/src/filters/drop/nth.rs b/crates/nu-command/src/filters/drop/nth.rs
@@ -3,8 +3,8 @@ use nu_engine::CallExt;
 use nu_protocol::ast::{Call, RangeInclusion};
 use nu_protocol::engine::{Command, EngineState, Stack};
 use nu_protocol::{
-    Category, Example, FromValue, IntoInterruptiblePipelineData, PipelineData, PipelineIterator,
-    Range, ShellError, Signature, Span, Spanned, SyntaxShape, Type, Value,
+    Category, Example, IntoInterruptiblePipelineData, PipelineData, PipelineIterator, Range,
+    ShellError, Signature, Span, Spanned, SyntaxShape, Type, Value,
 };
 
 #[derive(Clone)]
@@ -185,9 +185,7 @@ fn extract_int_or_range(
     let value = call.req::<Value>(engine_state, stack, 0)?;
 
     let int_opt = value.as_int().map(Either::Left).ok();
-    let range_opt: Result<nu_protocol::Range, ShellError> = FromValue::from_value(&value);
-
-    let range_opt = range_opt.map(Either::Right).ok();
+    let range_opt = value.as_range().map(|r| Either::Right(r.clone())).ok();
 
     int_opt
         .or(range_opt)

diff --git a/crates/nu-command/src/filters/insert.rs b/crates/nu-command/src/filters/insert.rs
@@ -122,7 +122,7 @@ fn insert(
 
     // Replace is a block, so set it up and run it instead of using it as the replacement
     if replacement.as_block().is_ok() {
-        let capture_block: Closure = FromValue::from_value(&replacement)?;
+        let capture_block = Closure::from_value(replacement)?;
         let block = engine_state.get_block(capture_block.block_id).clone();
 
         let mut stack = stack.captures_to_stack(&capture_block.captures);

diff --git a/crates/nu-command/src/filters/update.rs b/crates/nu-command/src/filters/update.rs
@@ -119,7 +119,7 @@ fn update(
 
     // Replace is a block, so set it up and run it instead of using it as the replacement
     if replacement.as_block().is_ok() {
-        let capture_block: Closure = FromValue::from_value(&replacement)?;
+        let capture_block = Closure::from_value(replacement)?;
         let block = engine_state.get_block(capture_block.block_id).clone();
 
         let mut stack = stack.captures_to_stack(&capture_block.captures);

diff --git a/crates/nu-command/src/filters/upsert.rs b/crates/nu-command/src/filters/upsert.rs
@@ -141,7 +141,7 @@ fn upsert(
 
     // Replace is a block, so set it up and run it instead of using it as the replacement
     if replacement.as_block().is_ok() {
-        let capture_block: Closure = FromValue::from_value(&replacement)?;
+        let capture_block = Closure::from_value(replacement)?;
         let block = engine_state.get_block(capture_block.block_id).clone();
 
         let mut stack = stack.captures_to_stack(&capture_block.captures);

diff --git a/crates/nu-engine/src/call_ext.rs b/crates/nu-engine/src/call_ext.rs
@@ -71,7 +71,7 @@ impl CallExt for Call {
     ) -> Result<Option<T>, ShellError> {
         if let Some(expr) = self.get_flag_expr(name) {
             let result = eval_expression(engine_state, stack, &expr)?;
-            FromValue::from_value(&result).map(Some)
+            FromValue::from_value(result).map(Some)
         } else {
             Ok(None)
         }
@@ -84,7 +84,7 @@ impl CallExt for Call {
     ) -> Result<Option<T>, ShellError> {
         if let Some(expr) = self.get_flag_expr(name) {
             let result = eval_constant(working_set, &expr)?;
-            FromValue::from_value(&result).map(Some)
+            FromValue::from_value(result).map(Some)
         } else {
             Ok(None)
         }
@@ -100,7 +100,7 @@ impl CallExt for Call {
 
         for expr in self.positional_iter().skip(starting_pos) {
             let result = eval_expression(engine_state, stack, expr)?;
-            output.push(FromValue::from_value(&result)?);
+            output.push(FromValue::from_value(result)?);
         }
 
         Ok(output)
@@ -115,7 +115,7 @@ impl CallExt for Call {
 
         for expr in self.positional_iter().skip(starting_pos) {
             let result = eval_constant(working_set, expr)?;
-            output.push(FromValue::from_value(&result)?);
+            output.push(FromValue::from_value(result)?);
         }
 
         Ok(output)
@@ -129,7 +129,7 @@ impl CallExt for Call {
     ) -> Result<Option<T>, ShellError> {
         if let Some(expr) = self.positional_nth(pos) {
             let result = eval_expression(engine_state, stack, expr)?;
-            FromValue::from_value(&result).map(Some)
+            FromValue::from_value(result).map(Some)
         } else {
             Ok(None)
         }
@@ -143,7 +143,7 @@ impl CallExt for Call {
     ) -> Result<T, ShellError> {
         if let Some(expr) = self.positional_nth(pos) {
             let result = eval_expression(engine_state, stack, expr)?;
-            FromValue::from_value(&result)
+            FromValue::from_value(result)
         } else if self.positional_len() == 0 {
             Err(ShellError::AccessEmptyContent { span: self.head })
         } else {
@@ -161,7 +161,7 @@ impl CallExt for Call {
     ) -> Result<T, ShellError> {
         if let Some(expr) = self.positional_nth(pos) {
             let result = eval_constant(working_set, expr)?;
-            FromValue::from_value(&result)
+            FromValue::from_value(result)
         } else if self.positional_len() == 0 {
             Err(ShellError::AccessEmptyContent { span: self.head })
         } else {
@@ -180,7 +180,7 @@ impl CallExt for Call {
     ) -> Result<T, ShellError> {
         if let Some(expr) = self.get_parser_info(name) {
             let result = eval_expression(engine_state, stack, expr)?;
-            FromValue::from_value(&result)
+            FromValue::from_value(result)
         } else if self.parser_info.is_empty() {
             Err(ShellError::AccessEmptyContent { span: self.head })
         } else {

diff --git a/crates/nu-plugin/src/protocol/evaluated_call.rs b/crates/nu-plugin/src/protocol/evaluated_call.rs
@@ -238,7 +238,7 @@ impl EvaluatedCall {
     /// ```
     pub fn get_flag<T: FromValue>(&self, name: &str) -> Result<Option<T>, ShellError> {
         if let Some(value) = self.get_flag_value(name) {
-            FromValue::from_value(&value).map(Some)
+            FromValue::from_value(value).map(Some)
         } else {
             Ok(None)
         }
@@ -272,7 +272,7 @@ impl EvaluatedCall {
         self.positional
             .iter()
             .skip(starting_pos)
-            .map(|value| FromValue::from_value(value))
+            .map(|value| FromValue::from_value(value.clone()))
             .collect()
     }
 
@@ -283,7 +283,7 @@ impl EvaluatedCall {
     /// or an error that can be passed back to the shell on error.
     pub fn opt<T: FromValue>(&self, pos: usize) -> Result<Option<T>, ShellError> {
         if let Some(value) = self.nth(pos) {
-            FromValue::from_value(&value).map(Some)
+            FromValue::from_value(value).map(Some)
         } else {
             Ok(None)
         }
@@ -296,7 +296,7 @@ impl EvaluatedCall {
     /// be passed back to the shell.
     pub fn req<T: FromValue>(&self, pos: usize) -> Result<T, ShellError> {
         if let Some(value) = self.nth(pos) {
-            FromValue::from_value(&value)
+            FromValue::from_value(value)
         } else if self.positional.is_empty() {
             Err(ShellError::AccessEmptyContent { span: self.head })
         } else {