Include supported shapes in unsupported shape error messages

[Diwakar-Gupta]

I cannot match the kind of error because it is not public to external crate.

https://github.com/TedDriggs/darling/blob/bafada6f97fd67dfba9005e47934f80b66ec7977/core/src/error/kind.rs#L12-L13

I wanted to give custom error message when king = UnsupportedShape.

[TedDriggs]

That’s by design - I don’t want to commit to having the internals of darling errors public. What are you trying to override the message to say? Maybe we could instead improve the message darling emits?

[Diwakar-Gupta]

    let mut receiver = match DeriveModel::from_derive_input(&input) {
        Ok(model) => model,
        Err(error) => return Err(Error::Parsing(error)),
    };

I have this code and there are many reasons that can cause Err(error). I support only struct so wanted to give error message you can only derive DeriveModel on structs.

For now, I have used #[darling(supports(struct_named))]. Which gives error message Unsupported shape enum

I was thinking something like this will work for me

    let mut receiver = match DeriveModel::from_derive_input(&input) {
        Ok(model) => model,
        Err({kind: ErrorKind::UnsupportedShape, ...}) => todo,
    };

[TedDriggs]

Let's make the error message better - that way everyone who uses the crate will benefit, and you won't need to do any pattern matching on the error's contents in your project.

To do that, we'll need a new function on Error that takes both the observed unsupported shape and something that represents the allowed shapes. We'll change the ErrorKind::UnsupportedShape variant from a newtype variant to a named-fields variant, and store the expected value in an Option so we don't break compatibility for existing code. Then when we display the error, if that Option has a value, we'll add a sentence to the error message that says what shapes are accepted.

Does that meet SeaQL's needs?

[TedDriggs]

Looking at the code for this, I think the best path forward is something like...

1. Rework darling_core::options::shape::DataShape and expose the replacement under darling_core::util; this way we're able to use an actual method to do the validation instead of the current approach which relies entirely on generated code. This has the secondary benefit of enabling per-variant shape validation code like this to benefit from this work.
2. Rework darling_core::options::Shape to use the new DataShape in its generated validation code.

[Diwakar-Gupta]

Let's make the error message better - that way everyone who uses the crate will benefit, and you won't need to do any pattern matching on the error's contents in your project.

To do that, we'll need a new function on Error that takes both the observed unsupported shape and something that represents the allowed shapes. We'll change the ErrorKind::UnsupportedShape variant from a newtype variant to a named-fields variant, and store the expected value in an Option so we don't break compatibility for existing code. Then when we display the error, if that Option has a value, we'll add a sentence to the error message that says what shapes are accepted.

Does that meet SeaQL's needs?

Yes, this will meet SeaQL's needs. Thanks for the feature, this will help.

[TedDriggs]

I've been looking at this, but it's been more complex than I'd hoped.

Right now, I've got the following:

1. darling::util::Shape is an enum that represents the four shapes a struct or variant can have (named, tuple, newtype, unit)
2. darling::util::AsShape is a trait that enables getting the shape from a Fields (the syn one or the darling::ast one), as well as from a variant or a syn::DataStruct.
3. darling::util::ShapeSet is a struct that holds which Shape values (or "any") are permitted. Right now, this also includes pub fn check(&self, target: &impl AsShape) -> crate::Result<()> so that it can theoretically do the validation itself.

The plan then is to change the generated shape checking code to instead build two ShapeSet structs, one for checking variants in case we get an enum, and one in case we get a struct.

The issue right now is building the error message.

Scenario	Condition	Error Placement	Message
1	macro user passes an enum and we only take structs	One error, at call site	Unsupported shape: enum. Expected: any struct.
2	macro user passes an enum and we only take tuple structs or named structs	One error, at call site	Unsupported shape: enum. Expected: struct with named fields or unnamed fields.
3	macro user passes an enum with some named-field variants and we take enums, but we require all variants are newtype or unit	One error per offending variant, at the variant	Unsupported shape: named fields. Expected: one unnamed field or no fields.
4	macro user passes an enum with a newtype variant, and we only take structs with named fields or enums whose variants have named fields	One error per offending variant, at the variant	Unsupported shape: newtype. Expected: named fields.

Writing the error messages for this, I am having a hard time not using the term "struct" or "variant" in the message. This is a problem because it means that ShapeSet::check(&self, &impl AsShape) -> crate::Result<()> doesn't work as a signature - it doesn't know which word to use.

It might be possible to address this by saying that we only need the term "struct" or "enum" if the user has gotten it completely wrong, and presented the wrong container type. In that world, we could further simplify case 1 by not special-casing "any" and instead changing the message to Unsupported type: enum. Expected: struct with named fields, unnamed fields, or no fields. The reverse would be Unsupported type: struct. Expected: enum with variants containing named fields, unnamed fields, or no fields. Then we can more easily see how the generated check would go:

// Initializers for these would be generated by darling
let struct_checker = ShapeSet { /* ... */ };
let enum_checker = ShapeSet { /* ... */ };

match __body {
    Data::Struct(__data) => {
        if struct_checker.is_empty() {
            return Err(Error::unsupported_shape_with_expected("struct", format!("enum with variants {}", enum_checker)));
        }

        struct_checker.check(__data)
    }
    Data::Enum(__data) => {
        if enum_checker.is_empty() {
            return Err(Error::unsupported_shape_with_expected("enum", format!("struct with {}", struct_checker)));
        }

        let mut __shape_errors = ::darling::Error::accumulator();
        for __variant in &__data.variants {
            __shape_errors.handle(
                enum_checker
                    .check(__variant)
                    .map_err(|e| e.with_span(__variant.ident)
                )
            );
        }

        __shape_errors.finish()
    }
    // ... union branch elided
}

I think it's fine (or even good) that when a user makes a variant of the wrong shape, we don't tell them about the accepted struct shapes as long as some variant shapes are allowed - it seems unlikely to me that the user would change their mind between using a struct and an enum just because they can't use a named-fields variant and are only allowed to use a newtype variant.

This approach also makes it fairly easy for someone to implement this checking on their own, as in the from_variants use-case where I want to only enforce shape validation after checking if the macro user has told me to skip ignore that variant. For that use-case, I'd use #[darling(supports(enum_any))] on the receiver, and would then use the and_then attribute to add my custom validation; something along the lines of...

if self.ignore {
    Ok(self)
} else {
     ShapeSet::default()
        .insert(Shape::Newtype)
        .insert(Shape::Unit)
        .check(&self.fields)
        .map_err(|e| e.with_span(&self.ident)
}