Skip to content

Commit

Permalink
Allow missing fields in derived FromValue::from_value calls (#13206)
Browse files Browse the repository at this point in the history
# Description
In #13031 I added the derive macros for `FromValue` and `IntoValue`. In
that implementation, in particular for structs with named fields, it was
not possible to omit fields while loading them from a value, when the
field is an `Option`. This PR adds extra handling for this behavior, so
if a field is an `Option` and that field is missing in the `Value`, then
the field becomes `None`. This behavior is also tested in
`nu_protocol::value::test_derive::missing_options`.

# User-Facing Changes
When using structs for options or similar, users can now just emit
fields in the record and the derive `from_value` method will be able to
understand this, if the struct has an `Option` type for that field.

# Tests + Formatting
- :green_circle: `toolkit fmt`
- :green_circle: `toolkit clippy`
- :green_circle: `toolkit test`
- :green_circle: `toolkit test stdlib`

# After Submitting
A showcase for this feature would be great, I tried to use the current
derive macro in a plugin of mine for a config but without this addition,
they are annoying to use. So, when this is done, I would add an example
for such plugin configs that may be loaded via `FromValue`.
  • Loading branch information
cptpiepmatz committed Jun 22, 2024
1 parent b6bdadb commit 9b7f899
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 19 deletions.
66 changes: 47 additions & 19 deletions crates/nu-derive-value/src/from.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use proc_macro2::TokenStream as TokenStream2;
use quote::{quote, ToTokens};
use syn::{
spanned::Spanned, Attribute, Data, DataEnum, DataStruct, DeriveInput, Fields, Generics, Ident,
Type,
};

use crate::attributes::{self, ContainerAttributes};
Expand Down Expand Up @@ -116,15 +117,11 @@ fn derive_struct_from_value(
/// src_span: span
/// })?,
/// )?,
/// favorite_toy: <Option<String> as nu_protocol::FromValue>::from_value(
/// record
/// .remove("favorite_toy")
/// .ok_or_else(|| nu_protocol::ShellError::CantFindColumn {
/// col_name: std::string::ToString::to_string("favorite_toy"),
/// span: std::option::Option::None,
/// src_span: span
/// })?,
/// )?,
/// favorite_toy: record
/// .remove("favorite_toy")
/// .map(|v| <#ty as nu_protocol::FromValue>::from_value(v))
/// .transpose()?
/// .flatten(),
/// })
/// }
/// }
Expand Down Expand Up @@ -480,20 +477,29 @@ fn parse_value_via_fields(fields: &Fields, self_ident: impl ToTokens) -> TokenSt
match fields {
Fields::Named(fields) => {
let fields = fields.named.iter().map(|field| {
// TODO: handle missing fields for Options as None
let ident = field.ident.as_ref().expect("named has idents");
let ident_s = ident.to_string();
let ty = &field.ty;
quote! {
#ident: <#ty as nu_protocol::FromValue>::from_value(
record
match type_is_option(ty) {
true => quote! {
#ident: record
.remove(#ident_s)
.ok_or_else(|| nu_protocol::ShellError::CantFindColumn {
col_name: std::string::ToString::to_string(#ident_s),
span: std::option::Option::None,
src_span: span
})?,
)?
.map(|v| <#ty as nu_protocol::FromValue>::from_value(v))
.transpose()?
.flatten()
},

false => quote! {
#ident: <#ty as nu_protocol::FromValue>::from_value(
record
.remove(#ident_s)
.ok_or_else(|| nu_protocol::ShellError::CantFindColumn {
col_name: std::string::ToString::to_string(#ident_s),
span: std::option::Option::None,
src_span: span
})?,
)?
},
}
});
quote! {
Expand Down Expand Up @@ -537,3 +543,25 @@ fn parse_value_via_fields(fields: &Fields, self_ident: impl ToTokens) -> TokenSt
},
}
}

const FULLY_QUALIFIED_OPTION: &str = "std::option::Option";
const PARTIALLY_QUALIFIED_OPTION: &str = "option::Option";
const PRELUDE_OPTION: &str = "Option";

/// Check if the field type is an `Option`.
///
/// This function checks if a given type is an `Option`.
/// We assume that an `Option` is [`std::option::Option`] because we can't see the whole code and
/// can't ask the compiler itself.
/// If the `Option` type isn't `std::option::Option`, the user will get a compile error due to a
/// type mismatch.
/// It's very unusual for people to override `Option`, so this should rarely be an issue.
///
/// When [rust#63084](https://github.com/rust-lang/rust/issues/63084) is resolved, we can use
/// [`std::any::type_name`] for a static assertion check to get a more direct error messages.
fn type_is_option(ty: &Type) -> bool {
let s = ty.to_token_stream().to_string();
s.starts_with(PRELUDE_OPTION)
|| s.starts_with(PARTIALLY_QUALIFIED_OPTION)
|| s.starts_with(FULLY_QUALIFIED_OPTION)
}
56 changes: 56 additions & 0 deletions crates/nu-protocol/src/value/test_derive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,62 @@ fn named_fields_struct_incorrect_type() {
assert!(res.is_err());
}

#[derive(IntoValue, FromValue, Debug, PartialEq, Default)]
struct ALotOfOptions {
required: bool,
float: Option<f64>,
int: Option<i64>,
value: Option<Value>,
nested: Option<Nestee>,
}

#[test]
fn missing_options() {
let value = Value::test_record(Record::new());
let res: Result<ALotOfOptions, _> = ALotOfOptions::from_value(value);
assert!(res.is_err());

let value = Value::test_record(record! {"required" => Value::test_bool(true)});
let expected = ALotOfOptions {
required: true,
..Default::default()
};
let actual = ALotOfOptions::from_value(value).unwrap();
assert_eq!(expected, actual);

let value = Value::test_record(record! {
"required" => Value::test_bool(true),
"float" => Value::test_float(std::f64::consts::PI),
});
let expected = ALotOfOptions {
required: true,
float: Some(std::f64::consts::PI),
..Default::default()
};
let actual = ALotOfOptions::from_value(value).unwrap();
assert_eq!(expected, actual);

let value = Value::test_record(record! {
"required" => Value::test_bool(true),
"int" => Value::test_int(12),
"nested" => Value::test_record(record! {
"u32" => Value::test_int(34),
}),
});
let expected = ALotOfOptions {
required: true,
int: Some(12),
nested: Some(Nestee {
u32: 34,
some: None,
none: None,
}),
..Default::default()
};
let actual = ALotOfOptions::from_value(value).unwrap();
assert_eq!(expected, actual);
}

#[derive(IntoValue, FromValue, Debug, PartialEq)]
struct UnnamedFieldsStruct<T>(u32, String, T)
where
Expand Down

0 comments on commit 9b7f899

Please sign in to comment.