Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option handling is verbose #161

Closed
CraftSpider opened this issue Mar 7, 2022 · 7 comments · Fixed by #177
Closed

Option handling is verbose #161

CraftSpider opened this issue Mar 7, 2022 · 7 comments · Fixed by #177

Comments

@CraftSpider
Copy link

Expectations:
Based on the phrasing in the documentation and the fact that the crate says it is inspired by serde, Option would default to None in a derive if the item isn't present.

Reality:
Missing items error, and need an explicit #[darling(default)] on them. This seems non-obvious, and at the least I think the documentation for Option in FromMeta should be clarified, as while the current documentation technically describes the behavior accurately, I misinterpreted it as I assumed it would work the same as syn.

@TedDriggs
Copy link
Owner

In serde, options aren't automatically defaulting; if you don't set #[serde(default)] an explicit null would be required in the input for Deserialize to work.

I assumed it would work the same as syn.

When I started darling, I don't think syn had any behavior here. I can see Option making something implicitly optional as useful for saving typing, but it does come at the expense of irregularity (since other types wouldn't implicitly be defaulted)

@CraftSpider
Copy link
Author

Thanks for the response. I just checked on the playground, at least with serde/serde_json it does automatically default:

#[derive(Debug, serde::Deserialize)]
struct Foo {
    item: Option<i32>,
}

fn main() {
    dbg!(serde_json::from_str::<Foo>("{}")
        .unwrap());
}

This succeeds, printing out Foo { item: None }. On the other hand, I can understand not wanting to add the irregularity. I think it would be nice to have a note in the documentation, but feel free to close the issue if you want.

TedDriggs added a commit that referenced this issue Mar 8, 2022
This is in support of #161, illustrating how defaults combine.
@TedDriggs
Copy link
Owner

Today I learned.

Knowing that, I'm certainly willing to entertain a PR that adds this, though I may sit agonizing about it for a week before merging. A couple things to consider:

  1. Determining that a field is an Option isn't quite as easy as it sounds. Here is an example from derive_builder - if you can find where serde does this check that'd be even better.
  2. See example below
#[derive(FromMeta)]
#[darling(default)]
struct Example {
    name: Option<String>,
    age: u8,
}

impl Default for Example {
    fn default() -> Self {
        Self {
            name: Some("Alice".into()),
            age: 0,
        }
    }
}

If given #[foo(age = 3)], the value of Example must be Example { name: Some("Alice"), age: 32 } rather than Example { name: None, age: 32 }. This means the behavior will be different than the case where someone adds #[darling(default)] to an optional field.

I've added some tests that illustrate behavior of stacking defaults, and the PR will need to extend those to show it behaves properly in those cases.

@TedDriggs
Copy link
Owner

TedDriggs commented Mar 8, 2022

I'd suggest implementing this as a new method on darling_core::codegen::field::Initializer, something like...

impl Initializer {
    fn option_aware_default_expression(&self) -> Option<Cow<'a, DefaultExpression>> {
        self.0.default_expression.map(Cow::Borrowed).or_else(|| /* ... */)
    }
}

The or_else would check if the field type is Option, and then if so would return a Some(Cow::Owned(DefaultExpression::Trait)). Otherwise it would return None.

You'd then replace the second reference to field.default_expression in Initializer::to_tokens with calls to self.option_aware_default_expression().

Edit: I realized that the multiple case shouldn't use this fallback.

TedDriggs added a commit that referenced this issue Mar 8, 2022
@TedDriggs TedDriggs changed the title Option handling is non-intuitive Option handling is verbose Mar 8, 2022
TedDriggs added a commit that referenced this issue Mar 9, 2022
This is in support of #161, illustrating how defaults combine.
TedDriggs added a commit that referenced this issue Apr 1, 2022
TedDriggs added a commit that referenced this issue Apr 1, 2022
@jonasbb
Copy link
Contributor

jonasbb commented Apr 6, 2022

[...] if you can find where serde does this check that'd be even better.

I can help with that part. The derive creates an internal struct which wraps every field in one extra layer of Option. While deserialization it sets them to Some if they appear in the serialized data. At the end to create the real struct each field is checked and when it is None the missing_field function is called.
https://github.com/serde-rs/serde/blob/6e94a27c76834c540e36fbe8bd881a60f1ad61e9/serde/src/private/de.rs#L18-L56
It tries to deserialize any type T with a special Deserializer, which errors in most cases (thus creating a missing field error message), except when deserialize_option is called. It now happens that if T = Option<V> this is called, thus no error is created and the default None is returned.
It is not a special handling of Option, but anything which calls deserialize_option on the Deserializer. It just happens that Option is the only type people usually come across which behaves in that way.

@TedDriggs
Copy link
Owner

@jonasbb that's extremely helpful; thanks for sharing it. As usual, serde has a really elegant solution for a gnarly problem.

I suppose we could map that to darling by adding FromMeta::from_none, though I'm not sure what the other consequences of that would be (and I have this idea that adding a trait method, even a provided one, is a breaking change).

TedDriggs added a commit that referenced this issue Apr 11, 2022
This removes the need to use #[darling(default)] on options
in deriving structs.

Fixes #161
@TedDriggs
Copy link
Owner

The change for this is in review. I'm trying to decide if from_none is the right name for this, or if something like fill_missing would be better, but I am generally happy with the change and would like to merge this for darling 0.14.0.

TedDriggs added a commit that referenced this issue Apr 12, 2022
This removes the need to use #[darling(default)] on options
in deriving structs.

Fixes #161
TedDriggs added a commit that referenced this issue Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants