Support for all custom value operations on plugin custom values (#12088)

# Description

Adds support for the following operations on plugin custom values, in
addition to `to_base_value` which was already present:

- `follow_path_int()`
- `follow_path_string()`
- `partial_cmp()`
- `operation()`
- `Drop` (notification, if opted into with
`CustomValue::notify_plugin_on_drop`)

There are additionally customizable methods within the `Plugin` and
`StreamingPlugin` traits for implementing these functions in a way that
requires access to the plugin state, as a registered handle model such
as might be used in a dataframes plugin would.

`Value::append` was also changed to handle custom values correctly.

# User-Facing Changes

- Signature of `CustomValue::follow_path_string` and
`CustomValue::follow_path_int` changed to give access to the span of the
custom value itself, useful for some errors.
- Plugins using custom values have to be recompiled because the engine
will try to do custom value operations that aren't supported
- Plugins can do more things 🎉 

# Tests + Formatting
Tests were added for all of the new custom values functionality.

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting
- [ ] Document protocol reference `CustomValueOp` variants:
  - [ ] `FollowPathInt`
  - [ ] `FollowPathString`
  - [ ] `PartialCmp`
  - [ ] `Operation`
  - [ ] `Dropped`
- [ ] Document `notify_on_drop` optional field in `PluginCustomValue`
This commit is contained in:
Devyn Cairns
2024-03-12 02:37:08 -07:00
committed by GitHub
parent 8a250d2e08
commit 73f3c0b60b
19 changed files with 1065 additions and 156 deletions

View File

@@ -10,8 +10,8 @@ pub(crate) mod test_util;
pub use evaluated_call::EvaluatedCall;
use nu_protocol::{
engine::Closure, Config, PipelineData, PluginSignature, RawStream, ShellError, Span, Spanned,
Value,
ast::Operator, engine::Closure, Config, PipelineData, PluginSignature, RawStream, ShellError,
Span, Spanned, Value,
};
pub use plugin_custom_value::PluginCustomValue;
pub use protocol_info::ProtocolInfo;
@@ -131,6 +131,31 @@ pub enum PluginCall<D> {
pub enum CustomValueOp {
/// [`to_base_value()`](nu_protocol::CustomValue::to_base_value)
ToBaseValue,
/// [`follow_path_int()`](nu_protocol::CustomValue::follow_path_int)
FollowPathInt(Spanned<usize>),
/// [`follow_path_string()`](nu_protocol::CustomValue::follow_path_string)
FollowPathString(Spanned<String>),
/// [`partial_cmp()`](nu_protocol::CustomValue::partial_cmp)
PartialCmp(Value),
/// [`operation()`](nu_protocol::CustomValue::operation)
Operation(Spanned<Operator>, Value),
/// Notify that the custom value has been dropped, if
/// [`notify_plugin_on_drop()`](nu_protocol::CustomValue::notify_plugin_on_drop) is true
Dropped,
}
impl CustomValueOp {
/// Get the name of the op, for error messages.
pub(crate) fn name(&self) -> &'static str {
match self {
CustomValueOp::ToBaseValue => "to_base_value",
CustomValueOp::FollowPathInt(_) => "follow_path_int",
CustomValueOp::FollowPathString(_) => "follow_path_string",
CustomValueOp::PartialCmp(_) => "partial_cmp",
CustomValueOp::Operation(_, _) => "operation",
CustomValueOp::Dropped => "dropped",
}
}
}
/// Any data sent to the plugin
@@ -306,6 +331,7 @@ impl From<ShellError> for LabeledError {
pub enum PluginCallResponse<D> {
Error(LabeledError),
Signature(Vec<PluginSignature>),
Ordering(Option<Ordering>),
PipelineData(D),
}
@@ -330,6 +356,34 @@ pub enum PluginOption {
GcDisabled(bool),
}
/// This is just a serializable version of [std::cmp::Ordering], and can be converted 1:1
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
pub enum Ordering {
Less,
Equal,
Greater,
}
impl From<std::cmp::Ordering> for Ordering {
fn from(value: std::cmp::Ordering) -> Self {
match value {
std::cmp::Ordering::Less => Ordering::Less,
std::cmp::Ordering::Equal => Ordering::Equal,
std::cmp::Ordering::Greater => Ordering::Greater,
}
}
}
impl From<Ordering> for std::cmp::Ordering {
fn from(value: Ordering) -> Self {
match value {
Ordering::Less => std::cmp::Ordering::Less,
Ordering::Equal => std::cmp::Ordering::Equal,
Ordering::Greater => std::cmp::Ordering::Greater,
}
}
}
/// Information received from the plugin
///
/// Note: exported for internal use, not public.

View File

@@ -1,9 +1,9 @@
use std::sync::Arc;
use std::{cmp::Ordering, sync::Arc};
use nu_protocol::{CustomValue, ShellError, Span, Spanned, Value};
use nu_protocol::{ast::Operator, CustomValue, IntoSpanned, ShellError, Span, Value};
use serde::{Deserialize, Serialize};
use crate::plugin::PluginSource;
use crate::plugin::{PluginInterface, PluginSource};
#[cfg(test)]
mod tests;
@@ -22,41 +22,171 @@ mod tests;
/// values sent matches the plugin it is being sent to.
#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct PluginCustomValue {
/// The name of the custom value as defined by the plugin (`value_string()`)
pub name: String,
/// The bincoded representation of the custom value on the plugin side
pub data: Vec<u8>,
#[serde(flatten)]
shared: SerdeArc<SharedContent>,
/// Which plugin the custom value came from. This is not defined on the plugin side. The engine
/// side is responsible for maintaining it, and it is not sent over the serialization boundary.
#[serde(skip, default)]
pub(crate) source: Option<Arc<PluginSource>>,
source: Option<Arc<PluginSource>>,
}
/// Content shared across copies of a plugin custom value.
#[derive(Clone, Debug, Serialize, Deserialize)]
struct SharedContent {
/// The name of the custom value as defined by the plugin (`value_string()`)
name: String,
/// The bincoded representation of the custom value on the plugin side
data: Vec<u8>,
/// True if the custom value should notify the source if all copies of it are dropped.
///
/// This is not serialized if `false`, since most custom values don't need it.
#[serde(default, skip_serializing_if = "is_false")]
notify_on_drop: bool,
}
fn is_false(b: &bool) -> bool {
!b
}
#[typetag::serde]
impl CustomValue for PluginCustomValue {
fn clone_value(&self, span: nu_protocol::Span) -> nu_protocol::Value {
fn clone_value(&self, span: Span) -> Value {
Value::custom_value(Box::new(self.clone()), span)
}
fn value_string(&self) -> String {
self.name.clone()
self.name().to_owned()
}
fn to_base_value(
fn to_base_value(&self, span: Span) -> Result<Value, ShellError> {
self.get_plugin(Some(span), "get base value")?
.custom_value_to_base_value(self.clone().into_spanned(span))
}
fn follow_path_int(
&self,
span: nu_protocol::Span,
) -> Result<nu_protocol::Value, nu_protocol::ShellError> {
self_span: Span,
index: usize,
path_span: Span,
) -> Result<Value, ShellError> {
self.get_plugin(Some(self_span), "follow cell path")?
.custom_value_follow_path_int(
self.clone().into_spanned(self_span),
index.into_spanned(path_span),
)
}
fn follow_path_string(
&self,
self_span: Span,
column_name: String,
path_span: Span,
) -> Result<Value, ShellError> {
self.get_plugin(Some(self_span), "follow cell path")?
.custom_value_follow_path_string(
self.clone().into_spanned(self_span),
column_name.into_spanned(path_span),
)
}
fn partial_cmp(&self, other: &Value) -> Option<Ordering> {
self.get_plugin(Some(other.span()), "perform comparison")
.and_then(|plugin| {
// We're passing Span::unknown() here because we don't have one, and it probably
// shouldn't matter here and is just a consequence of the API
plugin.custom_value_partial_cmp(self.clone(), other.clone())
})
.unwrap_or_else(|err| {
// We can't do anything with the error other than log it.
log::warn!(
"Error in partial_cmp on plugin custom value (source={source:?}): {err}",
source = self.source
);
None
})
.map(|ordering| ordering.into())
}
fn operation(
&self,
lhs_span: Span,
operator: Operator,
op_span: Span,
right: &Value,
) -> Result<Value, ShellError> {
self.get_plugin(Some(lhs_span), "invoke operator")?
.custom_value_operation(
self.clone().into_spanned(lhs_span),
operator.into_spanned(op_span),
right.clone(),
)
}
fn as_any(&self) -> &dyn std::any::Any {
self
}
}
impl PluginCustomValue {
/// Create a new [`PluginCustomValue`].
pub(crate) fn new(
name: String,
data: Vec<u8>,
notify_on_drop: bool,
source: Option<Arc<PluginSource>>,
) -> PluginCustomValue {
PluginCustomValue {
shared: SerdeArc(Arc::new(SharedContent {
name,
data,
notify_on_drop,
})),
source,
}
}
/// The name of the custom value as defined by the plugin (`value_string()`)
pub fn name(&self) -> &str {
&self.shared.name
}
/// The bincoded representation of the custom value on the plugin side
pub fn data(&self) -> &[u8] {
&self.shared.data
}
/// True if the custom value should notify the source if all copies of it are dropped.
pub fn notify_on_drop(&self) -> bool {
self.shared.notify_on_drop
}
/// Which plugin the custom value came from. This is not defined on the plugin side. The engine
/// side is responsible for maintaining it, and it is not sent over the serialization boundary.
#[cfg(test)]
pub(crate) fn source(&self) -> &Option<Arc<PluginSource>> {
&self.source
}
/// Create the [`PluginCustomValue`] with the given source.
#[cfg(test)]
pub(crate) fn with_source(mut self, source: Option<Arc<PluginSource>>) -> PluginCustomValue {
self.source = source;
self
}
/// Helper to get the plugin to implement an op
fn get_plugin(&self, span: Option<Span>, for_op: &str) -> Result<PluginInterface, ShellError> {
let wrap_err = |err: ShellError| ShellError::GenericError {
error: format!(
"Unable to spawn plugin `{}` to get base value",
"Unable to spawn plugin `{}` to {for_op}",
self.source
.as_ref()
.map(|s| s.name())
.unwrap_or("<unknown>")
),
msg: err.to_string(),
span: Some(span),
span,
help: None,
inner: vec![err],
};
@@ -69,25 +199,13 @@ impl CustomValue for PluginCustomValue {
// Envs probably should be passed here, but it's likely that the plugin is already running
let empty_envs = std::iter::empty::<(&str, &str)>();
let plugin = source
.persistent(Some(span))
.and_then(|p| p.get(|| Ok(empty_envs)))
.map_err(wrap_err)?;
plugin
.custom_value_to_base_value(Spanned {
item: self.clone(),
span,
})
source
.persistent(span)
.and_then(|p| p.get(|| Ok(empty_envs)))
.map_err(wrap_err)
}
fn as_any(&self) -> &dyn std::any::Any {
self
}
}
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(
@@ -95,12 +213,9 @@ impl PluginCustomValue {
span: Span,
) -> Result<PluginCustomValue, ShellError> {
let name = custom_value.value_string();
let notify_on_drop = custom_value.notify_plugin_on_drop();
bincode::serialize(custom_value)
.map(|data| PluginCustomValue {
name,
data,
source: None,
})
.map(|data| PluginCustomValue::new(name, data, notify_on_drop, None))
.map_err(|err| ShellError::CustomValueFailedToEncode {
msg: err.to_string(),
span,
@@ -113,7 +228,7 @@ impl PluginCustomValue {
&self,
span: Span,
) -> Result<Box<dyn CustomValue>, ShellError> {
bincode::deserialize::<Box<dyn CustomValue>>(&self.data).map_err(|err| {
bincode::deserialize::<Box<dyn CustomValue>>(self.data()).map_err(|err| {
ShellError::CustomValueFailedToDecode {
msg: err.to_string(),
span,
@@ -199,7 +314,7 @@ impl PluginCustomValue {
Ok(())
} else {
Err(ShellError::CustomValueIncorrectForPlugin {
name: custom_value.name.clone(),
name: custom_value.name().to_owned(),
span,
dest_plugin: source.name().to_owned(),
src_plugin: custom_value.source.as_ref().map(|s| s.name().to_owned()),
@@ -363,3 +478,62 @@ impl PluginCustomValue {
}
}
}
impl Drop for PluginCustomValue {
fn drop(&mut self) {
// If the custom value specifies notify_on_drop and this is the last copy, we need to let
// the plugin know about it if we can.
if self.source.is_some() && self.notify_on_drop() && Arc::strong_count(&self.shared) == 1 {
self.get_plugin(None, "drop")
// While notifying drop, we don't need a copy of the source
.and_then(|plugin| {
plugin.custom_value_dropped(PluginCustomValue {
shared: self.shared.clone(),
source: None,
})
})
.unwrap_or_else(|err| {
// We shouldn't do anything with the error except log it
let name = self.name();
log::warn!("Failed to notify drop of custom value ({name}): {err}")
});
}
}
}
/// A serializable `Arc`, to avoid having to have the serde `rc` feature enabled.
#[derive(Clone, Debug)]
#[repr(transparent)]
struct SerdeArc<T>(Arc<T>);
impl<T> Serialize for SerdeArc<T>
where
T: Serialize,
{
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
self.0.serialize(serializer)
}
}
impl<'de, T> Deserialize<'de> for SerdeArc<T>
where
T: Deserialize<'de>,
{
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
T::deserialize(deserializer).map(Arc::new).map(SerdeArc)
}
}
impl<T> std::ops::Deref for SerdeArc<T> {
type Target = Arc<T>;
fn deref(&self) -> &Self::Target {
&self.0
}
}

View File

@@ -19,7 +19,7 @@ fn serialize_deserialize() -> Result<(), ShellError> {
let original_value = TestCustomValue(32);
let span = Span::test_data();
let serialized = PluginCustomValue::serialize_from_custom_value(&original_value, span)?;
assert_eq!(original_value.value_string(), serialized.name);
assert_eq!(original_value.value_string(), serialized.name());
assert!(serialized.source.is_none());
let deserialized = serialized.deserialize_to_custom_value(span)?;
let downcasted = deserialized
@@ -36,8 +36,8 @@ fn expected_serialize_output() -> Result<(), ShellError> {
let span = Span::test_data();
let serialized = PluginCustomValue::serialize_from_custom_value(&original_value, span)?;
assert_eq!(
test_plugin_custom_value().data,
serialized.data,
test_plugin_custom_value().data(),
serialized.data(),
"The bincode configuration is probably different from what we expected. \
Fix test_plugin_custom_value() to match it"
);
@@ -417,8 +417,11 @@ fn serialize_in_root() -> Result<(), ShellError> {
let custom_value = val.as_custom_value()?;
if let Some(plugin_custom_value) = custom_value.as_any().downcast_ref::<PluginCustomValue>() {
assert_eq!("TestCustomValue", plugin_custom_value.name);
assert_eq!(test_plugin_custom_value().data, plugin_custom_value.data);
assert_eq!("TestCustomValue", plugin_custom_value.name());
assert_eq!(
test_plugin_custom_value().data(),
plugin_custom_value.data()
);
assert!(plugin_custom_value.source.is_none());
} else {
panic!("Failed to downcast to PluginCustomValue");
@@ -443,7 +446,8 @@ fn serialize_in_range() -> Result<(), ShellError> {
.downcast_ref()
.unwrap_or_else(|| panic!("{name} not PluginCustomValue"));
assert_eq!(
"TestCustomValue", plugin_custom_value.name,
"TestCustomValue",
plugin_custom_value.name(),
"{name} name not set correctly"
);
Ok(())
@@ -465,7 +469,8 @@ fn serialize_in_record() -> Result<(), ShellError> {
.downcast_ref()
.unwrap_or_else(|| panic!("'{key}' not PluginCustomValue"));
assert_eq!(
"TestCustomValue", plugin_custom_value.name,
"TestCustomValue",
plugin_custom_value.name(),
"'{key}' name not set correctly"
);
Ok(())
@@ -484,7 +489,8 @@ fn serialize_in_list() -> Result<(), ShellError> {
.downcast_ref()
.unwrap_or_else(|| panic!("[{index}] not PluginCustomValue"));
assert_eq!(
"TestCustomValue", plugin_custom_value.name,
"TestCustomValue",
plugin_custom_value.name(),
"[{index}] name not set correctly"
);
Ok(())
@@ -506,7 +512,8 @@ fn serialize_in_closure() -> Result<(), ShellError> {
.downcast_ref()
.unwrap_or_else(|| panic!("[{index}] not PluginCustomValue"));
assert_eq!(
"TestCustomValue", plugin_custom_value.name,
"TestCustomValue",
plugin_custom_value.name(),
"[{index}] name not set correctly"
);
Ok(())

View File

@@ -31,11 +31,7 @@ pub(crate) fn test_plugin_custom_value() -> PluginCustomValue {
let data = bincode::serialize(&expected_test_custom_value() as &dyn CustomValue)
.expect("bincode serialization of the expected_test_custom_value() failed");
PluginCustomValue {
name: "TestCustomValue".into(),
data,
source: None,
}
PluginCustomValue::new("TestCustomValue".into(), data, false, None)
}
pub(crate) fn expected_test_custom_value() -> TestCustomValue {
@@ -43,8 +39,5 @@ pub(crate) fn expected_test_custom_value() -> TestCustomValue {
}
pub(crate) fn test_plugin_custom_value_with_source() -> PluginCustomValue {
PluginCustomValue {
source: Some(PluginSource::new_fake("test").into()),
..test_plugin_custom_value()
}
test_plugin_custom_value().with_source(Some(PluginSource::new_fake("test").into()))
}