extern/nushell
1
0
Fork 1
mirror of https://github.com/nushell/nushell.git synced 2025-08-18 03:09:53 +02:00
Code Issues Packages Projects Releases Wiki Activity

Make drop notification timing for plugin custom values more consistent (#12341)

Browse Source 
# Description
This keeps plugin custom values that have requested drop notification
around during the lifetime of a plugin call / stream by sending them to
a channel that gets persisted during the lifetime of the call.

Before this change, it was very likely that the drop notification would
be sent before the plugin ever had a chance to handle the value it
received.

Tests have been added to make sure this works - see the `custom_values`
plugin.

cc @ayax79 

# User-Facing Changes
This is basically just a bugfix, just a slightly big one.

However, I did add an `as_mut_any()` function for custom values, to
avoid having to clone them. This is a breaking change.
This commit is contained in:
Devyn Cairns
2024-04-04 00:13:25 -07:00
committed by GitHub
parent a6afc89338
commit cbf7feef22
33 changed files with 1115 additions and 368 deletions
Download Patch File Download Diff File Expand all files Collapse all files

139
crates/nu-plugin/src/protocol/plugin_custom_value.rs
View File

@@ -1,7 +1,11 @@
use crate::plugin::{PluginInterface, PluginSource};
use nu_protocol::{ast::Operator, CustomValue, IntoSpanned, ShellError, Span, Value};
use std::{cmp::Ordering, sync::Arc};
use crate::{
plugin::{PluginInterface, PluginSource},
util::with_custom_values_in,
};
use nu_protocol::{ast::Operator, CustomValue, IntoSpanned, ShellError, Span, Spanned, Value};
use serde::{Deserialize, Serialize};
use std::{cmp::Ordering, convert::Infallible, sync::Arc};
#[cfg(test)]
mod tests;
@@ -50,10 +54,16 @@ fn is_false(b: &bool) -> bool {
!b
}
impl PluginCustomValue {
pub fn into_value(self, span: Span) -> Value {
Value::custom(Box::new(self), span)
}
}
#[typetag::serde]
impl CustomValue for PluginCustomValue {
fn clone_value(&self, span: Span) -> Value {
Value::custom(Box::new(self.clone()), span)
self.clone().into_value(span)
}
fn type_name(&self) -> String {
@@ -127,6 +137,10 @@ impl CustomValue for PluginCustomValue {
fn as_any(&self) -> &dyn std::any::Any {
self
}
fn as_mut_any(&mut self) -> &mut dyn std::any::Any {
self
}
}
impl PluginCustomValue {
@@ -164,11 +178,15 @@ impl PluginCustomValue {
/// 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>> {
pub fn source(&self) -> &Option<Arc<PluginSource>> {
&self.source
}
/// Set the [`PluginSource`] for this [`PluginCustomValue`].
pub fn set_source(&mut self, source: Option<Arc<PluginSource>>) {
self.source = source;
}
/// Create the [`PluginCustomValue`] with the given source.
#[cfg(test)]
pub(crate) fn with_source(mut self, source: Option<Arc<PluginSource>>) -> PluginCustomValue {
@@ -234,84 +252,55 @@ impl PluginCustomValue {
})
}
/// Add a [`PluginSource`] to all [`PluginCustomValue`]s within a value, recursively.
pub fn add_source(value: &mut Value, source: &Arc<PluginSource>) {
// This can't cause an error.
let _: Result<(), Infallible> = value.recurse_mut(&mut |value| {
let span = value.span();
match value {
// Set source on custom value
Value::Custom { ref val, .. } => {
if let Some(custom_value) = val.as_any().downcast_ref::<PluginCustomValue>() {
// Since there's no `as_mut_any()`, we have to copy the whole thing
let mut custom_value = custom_value.clone();
custom_value.source = Some(source.clone());
*value = Value::custom(Box::new(custom_value), span);
}
Ok(())
}
// LazyRecord could generate other values, but we shouldn't be receiving it anyway
//
// It's better to handle this as a bug
Value::LazyRecord { .. } => unimplemented!("add_source for LazyRecord"),
_ => Ok(()),
}
});
/// Add a [`PluginSource`] to the given [`CustomValue`] if it is a [`PluginCustomValue`].
pub fn add_source(value: &mut dyn CustomValue, source: &Arc<PluginSource>) {
if let Some(custom_value) = value.as_mut_any().downcast_mut::<PluginCustomValue>() {
custom_value.set_source(Some(source.clone()));
}
}
/// Check that all [`CustomValue`]s present within the `value` are [`PluginCustomValue`]s that
/// come from the given `source`, and return an error if not.
/// Add a [`PluginSource`] to all [`PluginCustomValue`]s within the value, recursively.
pub fn add_source_in(value: &mut Value, source: &Arc<PluginSource>) -> Result<(), ShellError> {
with_custom_values_in(value, |custom_value| {
Self::add_source(custom_value.item, source);
Ok::<_, ShellError>(())
})
}
/// Check that a [`CustomValue`] is a [`PluginCustomValue`] that come from the given `source`,
/// and return an error if not.
///
/// 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(
value: &mut Value,
value: Spanned<&dyn CustomValue>,
source: &PluginSource,
) -> Result<(), ShellError> {
value.recurse_mut(&mut |value| {
let span = value.span();
match value {
// Set source on custom value
Value::Custom { val, .. } => {
if let Some(custom_value) = val.as_any().downcast_ref::<PluginCustomValue>() {
if custom_value
.source
.as_ref()
.map(|s| s.is_compatible(source))
.unwrap_or(false)
{
Ok(())
} else {
Err(ShellError::CustomValueIncorrectForPlugin {
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()),
})
}
} else {
// Only PluginCustomValues can be sent
Err(ShellError::CustomValueIncorrectForPlugin {
name: val.type_name(),
span,
dest_plugin: source.name().to_owned(),
src_plugin: None,
})
}
}
// LazyRecord would be a problem for us, since it could return something else the
// next time, and we have to collect it anyway to serialize it. Collect it in place,
// and then verify the source of the result
Value::LazyRecord { val, .. } => {
*value = val.collect()?;
Ok(())
}
_ => Ok(()),
if let Some(custom_value) = value.item.as_any().downcast_ref::<PluginCustomValue>() {
if custom_value
.source
.as_ref()
.map(|s| s.is_compatible(source))
.unwrap_or(false)
{
Ok(())
} else {
Err(ShellError::CustomValueIncorrectForPlugin {
name: custom_value.name().to_owned(),
span: value.span,
dest_plugin: source.name().to_owned(),
src_plugin: custom_value.source.as_ref().map(|s| s.name().to_owned()),
})
}
})
} else {
// Only PluginCustomValues can be sent
Err(ShellError::CustomValueIncorrectForPlugin {
name: value.item.type_name(),
span: value.span,
dest_plugin: source.name().to_owned(),
src_plugin: None,
})
}
}
/// Convert all plugin-native custom values to [`PluginCustomValue`] within the given `value`,

184
crates/nu-plugin/src/protocol/plugin_custom_value/tests.rs
View File

@@ -7,7 +7,8 @@ use crate::{
},
};
use nu_protocol::{
ast::RangeInclusion, engine::Closure, record, CustomValue, Range, ShellError, Span, Value,
ast::RangeInclusion, engine::Closure, record, CustomValue, IntoSpanned, Range, ShellError,
Span, Value,
};
use std::sync::Arc;
@@ -42,10 +43,10 @@ fn expected_serialize_output() -> Result<(), ShellError> {
}
#[test]
fn add_source_at_root() -> Result<(), ShellError> {
fn add_source_in_at_root() -> Result<(), ShellError> {
let mut val = Value::test_custom_value(Box::new(test_plugin_custom_value()));
let source = Arc::new(PluginSource::new_fake("foo"));
PluginCustomValue::add_source(&mut val, &source);
PluginCustomValue::add_source_in(&mut val, &source)?;
let custom_value = val.as_custom_value()?;
let plugin_custom_value: &PluginCustomValue = custom_value
@@ -78,7 +79,7 @@ fn check_range_custom_values(
}
#[test]
fn add_source_nested_range() -> Result<(), ShellError> {
fn add_source_in_nested_range() -> Result<(), ShellError> {
let orig_custom_val = Value::test_custom_value(Box::new(test_plugin_custom_value()));
let mut val = Value::test_range(Range {
from: orig_custom_val.clone(),
@@ -87,7 +88,7 @@ fn add_source_nested_range() -> Result<(), ShellError> {
inclusion: RangeInclusion::Inclusive,
});
let source = Arc::new(PluginSource::new_fake("foo"));
PluginCustomValue::add_source(&mut val, &source);
PluginCustomValue::add_source_in(&mut val, &source)?;
check_range_custom_values(&val, |name, custom_value| {
let plugin_custom_value: &PluginCustomValue = custom_value
@@ -122,14 +123,14 @@ fn check_record_custom_values(
}
#[test]
fn add_source_nested_record() -> Result<(), ShellError> {
fn add_source_in_nested_record() -> Result<(), ShellError> {
let orig_custom_val = Value::test_custom_value(Box::new(test_plugin_custom_value()));
let mut val = Value::test_record(record! {
"foo" => orig_custom_val.clone(),
"bar" => orig_custom_val.clone(),
});
let source = Arc::new(PluginSource::new_fake("foo"));
PluginCustomValue::add_source(&mut val, &source);
PluginCustomValue::add_source_in(&mut val, &source)?;
check_record_custom_values(&val, &["foo", "bar"], |key, custom_value| {
let plugin_custom_value: &PluginCustomValue = custom_value
@@ -164,11 +165,11 @@ fn check_list_custom_values(
}
#[test]
fn add_source_nested_list() -> Result<(), ShellError> {
fn add_source_in_nested_list() -> Result<(), ShellError> {
let orig_custom_val = Value::test_custom_value(Box::new(test_plugin_custom_value()));
let mut val = Value::test_list(vec![orig_custom_val.clone(), orig_custom_val.clone()]);
let source = Arc::new(PluginSource::new_fake("foo"));
PluginCustomValue::add_source(&mut val, &source);
PluginCustomValue::add_source_in(&mut val, &source)?;
check_list_custom_values(&val, 0..=1, |index, custom_value| {
let plugin_custom_value: &PluginCustomValue = custom_value
@@ -205,14 +206,14 @@ fn check_closure_custom_values(
}
#[test]
fn add_source_nested_closure() -> Result<(), ShellError> {
fn add_source_in_nested_closure() -> Result<(), ShellError> {
let orig_custom_val = Value::test_custom_value(Box::new(test_plugin_custom_value()));
let mut val = Value::test_closure(Closure {
block_id: 0,
captures: vec![(0, orig_custom_val.clone()), (1, orig_custom_val.clone())],
});
let source = Arc::new(PluginSource::new_fake("foo"));
PluginCustomValue::add_source(&mut val, &source);
PluginCustomValue::add_source_in(&mut val, &source)?;
check_closure_custom_values(&val, 0..=1, |index, custom_value| {
let plugin_custom_value: &PluginCustomValue = custom_value
@@ -231,21 +232,25 @@ fn add_source_nested_closure() -> Result<(), ShellError> {
#[test]
fn verify_source_error_message() -> Result<(), ShellError> {
let span = Span::new(5, 7);
let mut ok_val = Value::custom(Box::new(test_plugin_custom_value_with_source()), span);
let mut native_val = Value::custom(Box::new(TestCustomValue(32)), span);
let mut foreign_val = {
let ok_val = test_plugin_custom_value_with_source();
let native_val = TestCustomValue(32);
let foreign_val = {
let mut val = test_plugin_custom_value();
val.source = Some(Arc::new(PluginSource::new_fake("other")));
Value::custom(Box::new(val), span)
val
};
let source = PluginSource::new_fake("test");
PluginCustomValue::verify_source(&mut ok_val, &source).expect("ok_val should be verified ok");
PluginCustomValue::verify_source((&ok_val as &dyn CustomValue).into_spanned(span), &source)
.expect("ok_val should be verified ok");
for (val, src_plugin) in [(&mut native_val, None), (&mut foreign_val, Some("other"))] {
let error = PluginCustomValue::verify_source(val, &source).expect_err(&format!(
"a custom value from {src_plugin:?} should result in an error"
));
for (val, src_plugin) in [
(&native_val as &dyn CustomValue, None),
(&foreign_val as &dyn CustomValue, Some("other")),
] {
let error = PluginCustomValue::verify_source(val.into_spanned(span), &source).expect_err(
&format!("a custom value from {src_plugin:?} should result in an error"),
);
if let ShellError::CustomValueIncorrectForPlugin {
name,
span: err_span,
@@ -265,145 +270,6 @@ fn verify_source_error_message() -> Result<(), ShellError> {
Ok(())
}
#[test]
fn verify_source_nested_range() -> Result<(), ShellError> {
let native_val = Value::test_custom_value(Box::new(TestCustomValue(32)));
let source = PluginSource::new_fake("test");
for (name, mut val) in [
(
"from",
Value::test_range(Range {
from: native_val.clone(),
incr: Value::test_nothing(),
to: Value::test_nothing(),
inclusion: RangeInclusion::RightExclusive,
}),
),
(
"incr",
Value::test_range(Range {
from: Value::test_nothing(),
incr: native_val.clone(),
to: Value::test_nothing(),
inclusion: RangeInclusion::RightExclusive,
}),
),
(
"to",
Value::test_range(Range {
from: Value::test_nothing(),
incr: Value::test_nothing(),
to: native_val.clone(),
inclusion: RangeInclusion::RightExclusive,
}),
),
] {
PluginCustomValue::verify_source(&mut val, &source)
.expect_err(&format!("error not generated on {name}"));
}
let mut ok_range = Value::test_range(Range {
from: Value::test_nothing(),
incr: Value::test_nothing(),
to: Value::test_nothing(),
inclusion: RangeInclusion::RightExclusive,
});
PluginCustomValue::verify_source(&mut ok_range, &source)
.expect("ok_range should not generate error");
Ok(())
}
#[test]
fn verify_source_nested_record() -> Result<(), ShellError> {
let native_val = Value::test_custom_value(Box::new(TestCustomValue(32)));
let source = PluginSource::new_fake("test");
for (name, mut val) in [
(
"first element foo",
Value::test_record(record! {
"foo" => native_val.clone(),
"bar" => Value::test_nothing(),
}),
),
(
"second element bar",
Value::test_record(record! {
"foo" => Value::test_nothing(),
"bar" => native_val.clone(),
}),
),
] {
PluginCustomValue::verify_source(&mut val, &source)
.expect_err(&format!("error not generated on {name}"));
}
let mut ok_record = Value::test_record(record! {"foo" => Value::test_nothing()});
PluginCustomValue::verify_source(&mut ok_record, &source)
.expect("ok_record should not generate error");
Ok(())
}
#[test]
fn verify_source_nested_list() -> Result<(), ShellError> {
let native_val = Value::test_custom_value(Box::new(TestCustomValue(32)));
let source = PluginSource::new_fake("test");
for (name, mut val) in [
(
"first element",
Value::test_list(vec![native_val.clone(), Value::test_nothing()]),
),
(
"second element",
Value::test_list(vec![Value::test_nothing(), native_val.clone()]),
),
] {
PluginCustomValue::verify_source(&mut val, &source)
.expect_err(&format!("error not generated on {name}"));
}
let mut ok_list = Value::test_list(vec![Value::test_nothing()]);
PluginCustomValue::verify_source(&mut ok_list, &source)
.expect("ok_list should not generate error");
Ok(())
}
#[test]
fn verify_source_nested_closure() -> Result<(), ShellError> {
let native_val = Value::test_custom_value(Box::new(TestCustomValue(32)));
let source = PluginSource::new_fake("test");
for (name, mut val) in [
(
"first capture",
Value::test_closure(Closure {
block_id: 0,
captures: vec![(0, native_val.clone()), (1, Value::test_nothing())],
}),
),
(
"second capture",
Value::test_closure(Closure {
block_id: 0,
captures: vec![(0, Value::test_nothing()), (1, native_val.clone())],
}),
),
] {
PluginCustomValue::verify_source(&mut val, &source)
.expect_err(&format!("error not generated on {name}"));
}
let mut ok_closure = Value::test_closure(Closure {
block_id: 0,
captures: vec![(0, Value::test_nothing())],
});
PluginCustomValue::verify_source(&mut ok_closure, &source)
.expect("ok_closure should not generate error");
Ok(())
}
#[test]
fn serialize_in_root() -> Result<(), ShellError> {
let span = Span::new(4, 10);

4
crates/nu-plugin/src/protocol/test_util.rs
View File

@@ -23,6 +23,10 @@ impl CustomValue for TestCustomValue {
fn as_any(&self) -> &dyn std::any::Any {
self
}
fn as_mut_any(&mut self) -> &mut dyn std::any::Any {
self
}
}
pub(crate) fn test_plugin_custom_value() -> PluginCustomValue {