Specific errors for missing or repeated fields

[seb-bl]

Description

As described in the README, errors when a field has not been set when build is called or when the setter is called twice are just no method errors.

Implementation

The suggested implementation is based on the proposed idea from #3.

It does not provide very clean errors, but still more specific than the (confusing) no method.

Errors for missing fields when the build method is called only report the first missing field. This allows to keep a number of impl equal to the number of fields and avoid an exponential number.

Setting a field twice

For each field, there is an impl containing setter for the field, for builders who already have that field set.

That setter takes as argument an empty enum to cause a compilation error, instead of the field.
This enum has a particular name to explain a bit the error.
The setter is also marked as deprecated for an error message a bit cleaner.

The error for the example from the README gives:

warning: use of deprecated item 'FooBuilder::<(__x, (std::option::Option<i32>,), __z, __w)>::y': Repeated field y
  --> src/main.rs:42:30
   |
42 |     Foo::builder().x(1).y(2).y(3); // y is specified twice
   |                              ^
   |
   = note: `#[warn(deprecated)]` on by default

error[E0308]: mismatched types
  --> src/main.rs:42:32
   |
42 |     Foo::builder().x(1).y(2).y(3); // y is specified twice
   |                                ^ expected enum `FooBuilder_Error_Repeated_field_y`, found integer
   |
   = note: expected type `FooBuilder_Error_Repeated_field_y`
              found type `{integer}`

Calling build when a field is missing

For each field, there is an impl containing a build method for builders that have a required field missing but the previous (in the struct declaration order) required fields present.

The method takes an additional argument which is an empty enum to cause a compilation error.
The method is also marked as deprecated for an error message a bit cleaner.

The error for the example from the README gives:

warning: use of deprecated item 'FooBuilder::<((), __y, __z, __w)>::build': Missing required field x
  --> src/main.rs:41:20
   |
41 |     Foo::builder().build(); // missing x
   |                    ^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

error[E0061]: this function takes 1 parameter but 0 parameters were supplied
  --> src/main.rs:41:20
   |
6  | #[derive(TypedBuilder)]
   |          ------------ defined here
...
41 |     Foo::builder().build(); // missing x
   |                    ^^^^^ expected 1 parameter

---

I am not sure that the impl causing errors for setting a field twice is a really good idea. As the method implemented could be used as a getter to mutate an already set parameter. I don't know if anyone is doing this. The interest of the specific error should be compared against that.

[idanarye]

I'm not sure I understand what's going on here. Why are you using into_iter() and then use #( #required_fields )* in the returned quote!?

[seb-bl]

I just reused the same pattern as just above (but filtering for required fields)

[idanarye]

Bit of a nitpick, but is there a particular reason why you use &FieldInfo here but *self when destructuring the StructIInfo?

[idanarye]

let ident = &type_param.ident; 

And do the same for GenericParam::Const.

[idanarye]

This part is tricky to understand. Could you please add comments to each branch explaining why we do it? Something like:

if f.builder_attr.default.is_some() {
    // `f` is not mandatory - it does not have it's own fake `build` method, so `field` will need
    // to warn about missing `field` whether or not `f` is set.
    assert!(f.ordinal != field.ordinal, "`required_field_impl` called for optional field {}", field.name);
    g.params.push(f.generic_ty_param());
    builder_generics_tuple.elems.push_value(f.type_ident().into());
} else if f.ordinal < field.ordinal {
    // Only add a `build` method that warns about missing `field` if `f` is set. If `f` is not set,
    // `f`'s `build` method will warn, since it appears earlier in the argument list.
    builder_generics_tuple.elems.push_value(f.tuplized_type_ty_param());
} else if f.ordinal == field.ordinal {
    builder_generics_tuple.elems.push_value(empty_type());
} else {
    // `f` appears later in the argument list after `field`, so if they are both missing we will
    // show a warning for `field` and not for `f` - which means this warning should appear whether
    // or not `f` is set.
    g.params.push(f.generic_ty_param());
    builder_generics_tuple.elems.push_value(f.type_ident().into());
}

I've split the optional field case and the fields that come after case - it requires a bit of code duplication but I think it's more readable that way.

[idanarye]

Nice work!

I understand the empty enums are just bottom types, right? If ! was stable we could just use that? We don't rely on them for the build error - the deprecation warning is responsible for that?

The reason I'm asking is that in #31 I want to split the proc macro to another crate so we could export all the helper types from the crate instead of generating new ones for each builder. When I do that, will it be OK to unite all these empty enums to a single enum BottomType {}?

[idanarye]

Merged for now because I want to do more changes that might conflict with this. I'll fix my remarks myself.

[seb-bl]

Hi, (sorry I took so much time to come back),

I have to admit I don't have a lot of experience with procedural macros. So I was not very confident to diverge a lot from the code I wrote by mimicking yours or other example.

The empty enums could indeed be replaced by ! or something similar. I gave them complete names because I first tried to use them as the way to explain the actual error. As I couldn't find a good way to have the type name to show up in the error message for the build methods, I added the deprecation warning. The deprecation warnings make the names of the empty enums redundant for explaining the error, so I think a simpler name would be just fine.