diff --git a/crates/nu-plugin-test-support/src/plugin_test.rs b/crates/nu-plugin-test-support/src/plugin_test.rs index 54ab84d3aa3..d2df2d861a5 100644 --- a/crates/nu-plugin-test-support/src/plugin_test.rs +++ b/crates/nu-plugin-test-support/src/plugin_test.rs @@ -1,4 +1,4 @@ -use std::{convert::Infallible, sync::Arc}; +use std::{cmp::Ordering, convert::Infallible, sync::Arc}; use nu_ansi_term::Style; use nu_engine::eval_block; @@ -222,7 +222,7 @@ impl PluginTest { }); // Check for equality with the result - if *expectation != value { + if !self.value_eq(expectation, &value)? { // If they're not equal, print a diff of the debug format let expectation_formatted = format!("{:#?}", expectation); let value_formatted = format!("{:#?}", value); @@ -272,4 +272,82 @@ impl PluginTest { ) -> Result<(), ShellError> { self.test_examples(&command.examples()) } + + /// This implements custom value comparison with `plugin.custom_value_partial_cmp()` to behave + /// as similarly as possible to comparison in the engine. + /// + /// NOTE: Try to keep these reflecting the same comparison as `Value::partial_cmp` does under + /// normal circumstances. Otherwise people will be very confused. + fn value_eq(&self, a: &Value, b: &Value) -> Result { + match (a, b) { + (Value::Custom { val, .. }, _) => { + // We have to serialize both custom values before handing them to the plugin + let mut serialized = + PluginCustomValue::serialize_from_custom_value(val.as_ref(), a.span())?; + serialized.set_source(Some(self.source.clone())); + let mut b_serialized = b.clone(); + PluginCustomValue::serialize_custom_values_in(&mut b_serialized)?; + PluginCustomValue::add_source_in(&mut b_serialized, &self.source)?; + // Now get the plugin reference and execute the comparison + let persistent = self.source.persistent(None)?.get_plugin(None)?; + let ordering = persistent.custom_value_partial_cmp(serialized, b_serialized)?; + Ok(matches!( + ordering.map(Ordering::from), + Some(Ordering::Equal) + )) + } + // All container types need to be here except Closure. + (Value::List { vals: a_vals, .. }, Value::List { vals: b_vals, .. }) => { + // Must be the same length, with all elements equivalent + Ok(a_vals.len() == b_vals.len() && { + for (a_el, b_el) in a_vals.iter().zip(b_vals) { + if !self.value_eq(a_el, b_el)? { + return Ok(false); + } + } + true + }) + } + (Value::Record { val: a_rec, .. }, Value::Record { val: b_rec, .. }) => { + // Must be the same length + if a_rec.len() != b_rec.len() { + return Ok(false); + } + + // reorder cols and vals to make more logically compare. + // more general, if two record have same col and values, + // the order of cols shouldn't affect the equal property. + let mut a_rec = a_rec.clone(); + let mut b_rec = b_rec.clone(); + a_rec.sort_cols(); + b_rec.sort_cols(); + + // Check columns first + for (a, b) in a_rec.columns().zip(b_rec.columns()) { + if a != b { + return Ok(false); + } + } + // Then check the values + for (a, b) in a_rec.values().zip(b_rec.values()) { + if !self.value_eq(a, b)? { + return Ok(false); + } + } + // All equal, and same length + Ok(true) + } + (Value::Range { val: a_rng, .. }, Value::Range { val: b_rng, .. }) => { + Ok(a_rng.inclusion == b_rng.inclusion + && self.value_eq(&a_rng.from, &b_rng.from)? + && self.value_eq(&a_rng.to, &b_rng.to)? + && self.value_eq(&a_rng.incr, &b_rng.incr)?) + } + // Must collect lazy records to compare. + (Value::LazyRecord { val: a_val, .. }, _) => self.value_eq(&a_val.collect()?, b), + (_, Value::LazyRecord { val: b_val, .. }) => self.value_eq(a, &b_val.collect()?), + // Fall back to regular eq. + _ => Ok(a == b), + } + } } diff --git a/crates/nu-plugin/src/plugin/command.rs b/crates/nu-plugin/src/plugin/command.rs index bfad385d6c2..86787435245 100644 --- a/crates/nu-plugin/src/plugin/command.rs +++ b/crates/nu-plugin/src/plugin/command.rs @@ -1,5 +1,6 @@ use nu_protocol::{ - Example, LabeledError, PipelineData, PluginExample, PluginSignature, Signature, Value, + Example, IntoSpanned, LabeledError, PipelineData, PluginExample, PluginSignature, ShellError, + Signature, Value, }; use crate::{EngineInterface, EvaluatedCall, Plugin}; @@ -359,3 +360,36 @@ pub fn create_plugin_signature(command: &(impl PluginCommand + ?Sized)) -> Plugi .collect(), ) } + +/// Render examples to their base value so they can be sent in the response to `Signature`. +pub(crate) fn render_examples( + plugin: &impl Plugin, + engine: &EngineInterface, + examples: &mut [PluginExample], +) -> Result<(), ShellError> { + for example in examples { + if let Some(ref mut value) = example.result { + value.recurse_mut(&mut |value| { + let span = value.span(); + match value { + Value::Custom { .. } => { + let value_taken = std::mem::replace(value, Value::nothing(span)); + let Value::Custom { val, .. } = value_taken else { + unreachable!() + }; + *value = + plugin.custom_value_to_base_value(engine, val.into_spanned(span))?; + Ok::<_, ShellError>(()) + } + // Collect LazyRecord before proceeding + Value::LazyRecord { ref val, .. } => { + *value = val.collect()?; + Ok(()) + } + _ => Ok(()), + } + })?; + } + } + Ok(()) +} diff --git a/crates/nu-plugin/src/plugin/interface/engine.rs b/crates/nu-plugin/src/plugin/interface/engine.rs index 79433ae9e77..90f3e051c96 100644 --- a/crates/nu-plugin/src/plugin/interface/engine.rs +++ b/crates/nu-plugin/src/plugin/interface/engine.rs @@ -403,16 +403,8 @@ impl EngineInterface { /// Any custom values in the examples will be rendered using `to_base_value()`. pub(crate) fn write_signature( &self, - mut signature: Vec, + signature: Vec, ) -> Result<(), ShellError> { - // Render any custom values in the examples to plain values so that the engine doesn't - // have to keep custom values around just to render the help pages. - for sig in signature.iter_mut() { - for value in sig.examples.iter_mut().flat_map(|e| e.result.as_mut()) { - PluginCustomValue::render_to_base_value_in(value)?; - } - } - let response = PluginCallResponse::Signature(signature); self.write(PluginOutput::CallResponse(self.context()?, response))?; self.flush() diff --git a/crates/nu-plugin/src/plugin/interface/engine/tests.rs b/crates/nu-plugin/src/plugin/interface/engine/tests.rs index fb98ac123f7..22b721cd97b 100644 --- a/crates/nu-plugin/src/plugin/interface/engine/tests.rs +++ b/crates/nu-plugin/src/plugin/interface/engine/tests.rs @@ -11,7 +11,7 @@ use crate::{ }; use nu_protocol::{ engine::Closure, Config, CustomValue, IntoInterruptiblePipelineData, LabeledError, - PipelineData, PluginExample, PluginSignature, ShellError, Signature, Span, Spanned, Value, + PipelineData, PluginSignature, ShellError, Span, Spanned, Value, }; use std::{ collections::HashMap, @@ -782,49 +782,6 @@ fn interface_write_signature() -> Result<(), ShellError> { Ok(()) } -#[test] -fn interface_write_signature_custom_value() -> Result<(), ShellError> { - let test = TestCase::new(); - let interface = test.engine().interface_for_context(38); - let signatures = vec![PluginSignature::new( - Signature::build("test command"), - vec![PluginExample { - example: "test command".into(), - description: "a test".into(), - result: Some(Value::test_custom_value(Box::new( - expected_test_custom_value(), - ))), - }], - )]; - interface.write_signature(signatures.clone())?; - - let written = test.next_written().expect("nothing written"); - - match written { - PluginOutput::CallResponse(id, response) => { - assert_eq!(38, id, "id"); - match response { - PluginCallResponse::Signature(sigs) => { - assert_eq!(1, sigs.len(), "sigs.len"); - - let sig = &sigs[0]; - assert_eq!(1, sig.examples.len(), "sig.examples.len"); - - assert_eq!( - Some(Value::test_int(expected_test_custom_value().0 as i64)), - sig.examples[0].result, - ); - } - _ => panic!("unexpected response: {response:?}"), - } - } - _ => panic!("unexpected message written: {written:?}"), - } - - assert!(!test.has_unconsumed_write()); - Ok(()) -} - #[test] fn interface_write_engine_call_registers_subscription() -> Result<(), ShellError> { let mut manager = TestCase::new().engine(); diff --git a/crates/nu-plugin/src/plugin/mod.rs b/crates/nu-plugin/src/plugin/mod.rs index 40cecd9011b..90253ab7a33 100644 --- a/crates/nu-plugin/src/plugin/mod.rs +++ b/crates/nu-plugin/src/plugin/mod.rs @@ -33,8 +33,8 @@ use std::os::unix::process::CommandExt; #[cfg(windows)] use std::os::windows::process::CommandExt; -use self::gc::PluginGc; pub use self::interface::{PluginRead, PluginWrite}; +use self::{command::render_examples, gc::PluginGc}; mod command; mod context; @@ -651,7 +651,12 @@ where let sigs = commands .values() .map(|command| create_plugin_signature(command.deref())) - .collect(); + .map(|mut sig| { + render_examples(plugin, &engine, &mut sig.examples)?; + Ok(sig) + }) + .collect::, ShellError>>() + .try_to_report(&engine)?; engine.write_signature(sigs).try_to_report(&engine)?; } // Run the plugin on a background thread, handling any input or output streams diff --git a/crates/nu-plugin/src/plugin/source.rs b/crates/nu-plugin/src/plugin/source.rs index 4a10a7ed836..522694968b6 100644 --- a/crates/nu-plugin/src/plugin/source.rs +++ b/crates/nu-plugin/src/plugin/source.rs @@ -40,7 +40,10 @@ impl PluginSource { /// Try to upgrade the persistent reference, and return an error referencing `span` as the /// object that referenced it otherwise - pub(crate) fn persistent(&self, span: Option) -> Result, ShellError> { + /// + /// This is not a public API. + #[doc(hidden)] + pub fn persistent(&self, span: Option) -> Result, ShellError> { self.persistent .upgrade() .ok_or_else(|| ShellError::GenericError { diff --git a/crates/nu-plugin/src/protocol/plugin_custom_value.rs b/crates/nu-plugin/src/protocol/plugin_custom_value.rs index b4bab9f0135..1b3316675a5 100644 --- a/crates/nu-plugin/src/protocol/plugin_custom_value.rs +++ b/crates/nu-plugin/src/protocol/plugin_custom_value.rs @@ -224,7 +224,7 @@ impl PluginCustomValue { /// Serialize a custom value into a [`PluginCustomValue`]. This should only be done on the /// plugin side. - pub(crate) fn serialize_from_custom_value( + pub fn serialize_from_custom_value( custom_value: &dyn CustomValue, span: Span, ) -> Result { @@ -240,7 +240,7 @@ impl PluginCustomValue { /// Deserialize a [`PluginCustomValue`] into a `Box`. This should only be done /// on the plugin side. - pub(crate) fn deserialize_to_custom_value( + pub fn deserialize_to_custom_value( &self, span: Span, ) -> Result, ShellError> { @@ -272,7 +272,7 @@ impl PluginCustomValue { /// /// This method will collapse `LazyRecord` in-place as necessary to make the guarantee, /// since `LazyRecord` could return something different the next time it is called. - pub(crate) fn verify_source( + pub fn verify_source( value: Spanned<&dyn CustomValue>, source: &PluginSource, ) -> Result<(), ShellError> { @@ -356,7 +356,7 @@ impl PluginCustomValue { } /// Render any custom values in the `Value` using `to_base_value()` - pub(crate) fn render_to_base_value_in(value: &mut Value) -> Result<(), ShellError> { + pub fn render_to_base_value_in(value: &mut Value) -> Result<(), ShellError> { value.recurse_mut(&mut |value| { let span = value.span(); match value { diff --git a/crates/nu-protocol/src/value/mod.rs b/crates/nu-protocol/src/value/mod.rs index 385a2b06814..1136c59d25f 100644 --- a/crates/nu-protocol/src/value/mod.rs +++ b/crates/nu-protocol/src/value/mod.rs @@ -2441,15 +2441,28 @@ impl PartialOrd for Value { // reorder cols and vals to make more logically compare. // more general, if two record have same col and values, // the order of cols shouldn't affect the equal property. - let (lhs_cols_ordered, lhs_vals_ordered) = reorder_record_inner(lhs); - let (rhs_cols_ordered, rhs_vals_ordered) = reorder_record_inner(rhs); + let mut lhs = lhs.clone(); + let mut rhs = rhs.clone(); + lhs.sort_cols(); + rhs.sort_cols(); - let result = lhs_cols_ordered.partial_cmp(&rhs_cols_ordered); - if result == Some(Ordering::Equal) { - lhs_vals_ordered.partial_cmp(&rhs_vals_ordered) - } else { - result + // Check columns first + for (a, b) in lhs.columns().zip(rhs.columns()) { + let result = a.partial_cmp(b); + if result != Some(Ordering::Equal) { + return result; + } } + // Then check the values + for (a, b) in lhs.values().zip(rhs.values()) { + let result = a.partial_cmp(b); + if result != Some(Ordering::Equal) { + return result; + } + } + // If all of the comparisons were equal, then lexicographical order dictates + // that the shorter sequence is less than the longer one + lhs.len().partial_cmp(&rhs.len()) } Value::LazyRecord { val, .. } => { if let Ok(rhs) = val.collect() { @@ -3766,12 +3779,6 @@ impl Value { } } -fn reorder_record_inner(record: &Record) -> (Vec<&String>, Vec<&Value>) { - let mut kv_pairs = record.iter().collect::>(); - kv_pairs.sort_by_key(|(col, _)| *col); - kv_pairs.into_iter().unzip() -} - // TODO: The name of this function is overly broad with partial compatibility // Should be replaced by an explicitly named helper on `Type` (take `Any` into account) fn type_compatible(a: Type, b: Type) -> bool { diff --git a/crates/nu-protocol/src/value/record.rs b/crates/nu-protocol/src/value/record.rs index 9a3ad319027..1d08fbf7440 100644 --- a/crates/nu-protocol/src/value/record.rs +++ b/crates/nu-protocol/src/value/record.rs @@ -257,6 +257,32 @@ impl Record { iter: self.inner.drain(range), } } + + /// Sort the record by its columns. + /// + /// ```rust + /// use nu_protocol::{record, Value}; + /// + /// let mut rec = record!( + /// "c" => Value::test_string("foo"), + /// "b" => Value::test_int(42), + /// "a" => Value::test_nothing(), + /// ); + /// + /// rec.sort_cols(); + /// + /// assert_eq!( + /// Value::test_record(rec), + /// Value::test_record(record!( + /// "a" => Value::test_nothing(), + /// "b" => Value::test_int(42), + /// "c" => Value::test_string("foo"), + /// )) + /// ); + /// ``` + pub fn sort_cols(&mut self) { + self.inner.sort_by(|(k1, _), (k2, _)| k1.cmp(k2)) + } } impl FromIterator<(String, Value)> for Record {