Unify closure serializing logic for to nuon, to msgpack, and to json (#15285)

# Description
Before this PR, `to msgpack`/`to msgpackz` and `to json` serialize
closures as `nil`/`null` respectively, when the `--serialize` option
isn't passed. This PR makes it an error to serialize closures to msgpack
or JSON without the `--serialize` flag, which is the behavior of `to
nuon`.

This PR also adds the `--serialize` flag to `to msgpack`.

This PR also changes `to nuon` and `to json` to return an error if they
cannot find the block contents of a closure, rather than serializing an
empty string or an error string, respectively. This behavior is
replicated for `to msgpack`.

It also changes `to nuon`'s error message for serializing closures
without `--serialize` to be the same as the new errors for `to json` and
`to msgpack`.

# User-Facing Changes

* Add `--serialize` flag to `to msgpack`, similar to the `--serialize`
flag for `to nuon` and `to json`.
* Serializing closures to JSON or msgpack without `--serialize`

Partially fixes #11738

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

@@ -57,7 +57,7 @@ impl Command for ToJson {
         // allow ranges to expand and turn into array
         let input = input.try_expand_range()?;
         let value = input.into_value(span)?;
-        let json_value = value_to_json_value(engine_state, &value, serialize_types)?;
+        let json_value = value_to_json_value(engine_state, &value, span, serialize_types)?;
 
         let json_result = if raw {
             nu_json::to_string_raw(&json_value)
@@ -78,16 +78,12 @@ impl Command for ToJson {
                 };
                 Ok(PipelineData::Value(res, Some(metadata)))
             }
-            _ => Ok(Value::error(
+            _ => Err(ShellError::CantConvert {
-                ShellError::CantConvert {
+                to_type: "JSON".into(),
-                    to_type: "JSON".into(),
+                from_type: value.get_type().to_string(),
-                    from_type: value.get_type().to_string(),
-                    span,
-                    help: None,
-                },
                 span,
-            )
+                help: None,
-            .into_pipeline_data()),
+            }),
         }
     }
 
@@ -118,6 +114,7 @@ impl Command for ToJson {
 pub fn value_to_json_value(
     engine_state: &EngineState,
     v: &Value,
+    call_span: Span,
     serialize_types: bool,
 ) -> Result<nu_json::Value, ShellError> {
     let span = v.span();
@@ -142,24 +139,20 @@ pub fn value_to_json_value(
         ),
 
         Value::List { vals, .. } => {
-            nu_json::Value::Array(json_list(engine_state, vals, serialize_types)?)
+            nu_json::Value::Array(json_list(engine_state, vals, call_span, serialize_types)?)
         }
         Value::Error { error, .. } => return Err(*error.clone()),
         Value::Closure { val, .. } => {
             if serialize_types {
-                let block = engine_state.get_block(val.block_id);
+                let closure_string = val.coerce_into_string(engine_state, span)?;
-                if let Some(span) = block.span {
+                nu_json::Value::String(closure_string.to_string())
-                    let contents_bytes = engine_state.get_span_contents(span);
-                    let contents_string = String::from_utf8_lossy(contents_bytes);
-                    nu_json::Value::String(contents_string.to_string())
-                } else {
-                    nu_json::Value::String(format!(
-                        "unable to retrieve block contents for json block_id {}",
-                        val.block_id.get()
-                    ))
-                }
             } else {
-                nu_json::Value::Null
+                return Err(ShellError::UnsupportedInput {
+                    msg: "closures are currently not deserializable (use --serialize to serialize as a string)".into(),
+                    input: "value originates from here".into(),
+                    msg_span: call_span,
+                    input_span: span,
+                });
             }
         }
         Value::Range { .. } => nu_json::Value::Null,
@@ -171,14 +164,14 @@ pub fn value_to_json_value(
             for (k, v) in &**val {
                 m.insert(
                     k.clone(),
-                    value_to_json_value(engine_state, v, serialize_types)?,
+                    value_to_json_value(engine_state, v, call_span, serialize_types)?,
                 );
             }
             nu_json::Value::Object(m)
         }
         Value::Custom { val, .. } => {
             let collected = val.to_base_value(span)?;
-            value_to_json_value(engine_state, &collected, serialize_types)?
+            value_to_json_value(engine_state, &collected, call_span, serialize_types)?
         }
     })
 }
@@ -186,12 +179,18 @@ pub fn value_to_json_value(
 fn json_list(
     engine_state: &EngineState,
     input: &[Value],
+    call_span: Span,
     serialize_types: bool,
 ) -> Result<Vec<nu_json::Value>, ShellError> {
     let mut out = vec![];
 
     for value in input {
-        out.push(value_to_json_value(engine_state, value, serialize_types)?);
+        out.push(value_to_json_value(
+            engine_state,
+            value,
+            call_span,
+            serialize_types,
+        )?);
     }
 
     Ok(out)

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

@@ -22,6 +22,11 @@ impl Command for ToMsgpack {
     fn signature(&self) -> Signature {
         Signature::build(self.name())
             .input_output_type(Type::Any, Type::Binary)
+            .switch(
+                "serialize",
+                "serialize nushell types that cannot be deserialized",
+                Some('s'),
+            )
             .category(Category::Formats)
     }
 
@@ -69,8 +74,8 @@ MessagePack: https://msgpack.org/
 
     fn run(
         &self,
-        _engine_state: &EngineState,
+        engine_state: &EngineState,
-        _stack: &mut Stack,
+        stack: &mut Stack,
         call: &Call,
         input: PipelineData,
     ) -> Result<PipelineData, ShellError> {
@@ -83,7 +88,16 @@ MessagePack: https://msgpack.org/
         let value = input.into_value(value_span)?;
         let mut out = vec![];
 
-        write_value(&mut out, &value, 0)?;
+        let serialize_types = call.has_flag(engine_state, stack, "serialize")?;
+
+        write_value(
+            &mut out,
+            &value,
+            0,
+            engine_state,
+            call.head,
+            serialize_types,
+        )?;
 
         Ok(Value::binary(out, call.head).into_pipeline_data_with_metadata(Some(metadata)))
     }
@@ -148,6 +162,9 @@ pub(crate) fn write_value(
     out: &mut impl io::Write,
     value: &Value,
     depth: usize,
+    engine_state: &EngineState,
+    call_span: Span,
+    serialize_types: bool,
 ) -> Result<(), WriteError> {
     use mp::ValueWriteError::InvalidMarkerWrite;
     let span = value.span();
@@ -196,6 +213,9 @@ pub(crate) fn write_value(
                 out,
                 &Value::list(val.into_range_iter(span, Signals::empty()).collect(), span),
                 depth,
+                engine_state,
+                call_span,
+                serialize_types,
             )?;
         }
         Value::String { val, .. } => {
@@ -208,13 +228,20 @@ pub(crate) fn write_value(
             mp::write_map_len(out, convert(val.len(), span)?).err_span(span)?;
             for (k, v) in val.iter() {
                 mp::write_str(out, k).err_span(span)?;
-                write_value(out, v, depth + 1)?;
+                write_value(out, v, depth + 1, engine_state, call_span, serialize_types)?;
             }
         }
         Value::List { vals, .. } => {
             mp::write_array_len(out, convert(vals.len(), span)?).err_span(span)?;
             for val in vals {
-                write_value(out, val, depth + 1)?;
+                write_value(
+                    out,
+                    val,
+                    depth + 1,
+                    engine_state,
+                    call_span,
+                    serialize_types,
+                )?;
             }
         }
         Value::Nothing { .. } => {
@@ -222,11 +249,20 @@ pub(crate) fn write_value(
                 .map_err(InvalidMarkerWrite)
                 .err_span(span)?;
         }
-        Value::Closure { .. } => {
+        Value::Closure { val, .. } => {
-            // Closures can't be converted
+            if serialize_types {
-            mp::write_nil(out)
+                let closure_string = val
-                .map_err(InvalidMarkerWrite)
+                    .coerce_into_string(engine_state, span)
-                .err_span(span)?;
+                    .map_err(|err| WriteError::Shell(Box::new(err)))?;
+                mp::write_str(out, &closure_string).err_span(span)?;
+            } else {
+                return Err(WriteError::Shell(Box::new(ShellError::UnsupportedInput {
+                    msg: "closures are currently not deserializable (use --serialize to serialize as a string)".into(),
+                    input: "value originates from here".into(),
+                    msg_span: call_span,
+                    input_span: span,
+                })));
+            }
         }
         Value::Error { error, .. } => {
             return Err(WriteError::Shell(error.clone()));
@@ -249,7 +285,14 @@ pub(crate) fn write_value(
             mp::write_bin(out, val).err_span(span)?;
         }
         Value::Custom { val, .. } => {
-            write_value(out, &val.to_base_value(span)?, depth)?;
+            write_value(
+                out,
+                &val.to_base_value(span)?,
+                depth,
+                engine_state,
+                call_span,
+                serialize_types,
+            )?;
         }
     }
     Ok(())

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

@@ -32,6 +32,11 @@ impl Command for ToMsgpackz {
                 "Window size for brotli compression (default 20)",
                 Some('w'),
             )
+            .switch(
+                "serialize",
+                "serialize nushell types that cannot be deserialized",
+                Some('s'),
+            )
             .category(Category::Formats)
     }
 
@@ -69,6 +74,7 @@ impl Command for ToMsgpackz {
             .get_flag(engine_state, stack, "window-size")?
             .map(to_u32)
             .transpose()?;
+        let serialize_types = call.has_flag(engine_state, stack, "serialize")?;
 
         let value_span = input.span().unwrap_or(call.head);
         let value = input.into_value(value_span)?;
@@ -80,7 +86,14 @@ impl Command for ToMsgpackz {
             window_size.map(|w| w.item).unwrap_or(DEFAULT_WINDOW_SIZE),
         );
 
-        write_value(&mut out, &value, 0)?;
+        write_value(
+            &mut out,
+            &value,
+            0,
+            engine_state,
+            call.head,
+            serialize_types,
+        )?;
         out.flush()
             .map_err(|err| IoError::new(err.kind(), call.head, None))?;
         drop(out);

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

@@ -69,16 +69,9 @@ impl Command for ToNuon {
         match nuon::to_nuon(engine_state, &value, style, Some(span), serialize_types) {
             Ok(serde_nuon_string) => Ok(Value::string(serde_nuon_string, span)
                 .into_pipeline_data_with_metadata(Some(metadata))),
-            _ => Ok(Value::error(
+            Err(error) => {
-                ShellError::CantConvert {
+                Ok(Value::error(error, span).into_pipeline_data_with_metadata(Some(metadata)))
-                    to_type: "NUON".into(),
+            }
-                    from_type: value.get_type().to_string(),
-                    span,
-                    help: None,
-                },
-                span,
-            )
-            .into_pipeline_data_with_metadata(Some(metadata))),
         }
     }
 

crates/nu-command/src/network/http/client.rs

@@ -276,7 +276,7 @@ fn send_json_request(
 ) -> Result<Response, ShellErrorOrRequestError> {
     match body {
         Value::Int { .. } | Value::Float { .. } | Value::List { .. } | Value::Record { .. } => {
-            let data = value_to_json_value(engine_state, &body, serialize_types)?;
+            let data = value_to_json_value(engine_state, &body, span, serialize_types)?;
             send_cancellable_request(request_url, Box::new(|| req.send_json(data)), span, signals)
         }
         // If the body type is string, assume it is string json content.

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

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

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

@@ -0,0 +1,35 @@
+use std::borrow::Cow;
+
+use crate::{engine::EngineState, BlockId, ShellError, Span, Value, VarId};
+
+use serde::{Deserialize, Serialize};
+
+#[derive(Clone, Debug, Serialize, Deserialize)]
+pub struct Closure {
+    pub block_id: BlockId,
+    pub captures: Vec<(VarId, Value)>,
+}
+
+impl Closure {
+    pub fn coerce_into_string<'a>(
+        &self,
+        engine_state: &'a EngineState,
+        span: Span,
+    ) -> Result<Cow<'a, str>, ShellError> {
+        let block = engine_state.get_block(self.block_id);
+        if let Some(span) = block.span {
+            let contents_bytes = engine_state.get_span_contents(span);
+            Ok(String::from_utf8_lossy(contents_bytes))
+        } else {
+            Err(ShellError::CantConvert {
+                to_type: "string".into(),
+                from_type: "closure".into(),
+                span,
+                help: Some(format!(
+                    "unable to retrieve block contents for closure with id {}",
+                    self.block_id.get()
+                )),
+            })
+        }
+    }
+}

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

@@ -3,7 +3,7 @@ mod argument;
 mod cached_file;
 mod call;
 mod call_info;
-mod capture_block;
+mod closure;
 mod command;
 mod description;
 mod engine_state;
@@ -23,7 +23,7 @@ pub use cached_file::CachedFile;
 pub use argument::*;
 pub use call::*;
 pub use call_info::*;
-pub use capture_block::*;
+pub use closure::*;
 pub use command::*;
 pub use engine_state::*;
 pub use error_handler::*;

crates/nuon/src/to.rs

@@ -99,17 +99,10 @@ fn value_to_string(
         }
         Value::Closure { val, .. } => {
             if serialize_types {
-                let block = engine_state.get_block(val.block_id);
+                Ok(val.coerce_into_string(engine_state, span)?.to_string())
-                if let Some(span) = block.span {
-                    let contents_bytes = engine_state.get_span_contents(span);
-                    let contents_string = String::from_utf8_lossy(contents_bytes);
-                    Ok(contents_string.to_string())
-                } else {
-                    Ok(String::new())
-                }
             } else {
                 Err(ShellError::UnsupportedInput {
-                    msg: "closures are currently not nuon-compatible".into(),
+                    msg: "closures are currently not deserializable (use --serialize to serialize as a string)".into(),
                     input: "value originates from here".into(),
                     msg_span: span,
                     input_span: v.span(),