Bug in builder/tests/07-repeated-field.rs about proc-macro-workshop HOT 5 CLOSED

@leordev: I would expect that on something like #[builder(each = "arg")] args: Vec<String>, if .arg(...) is called 2 times then args gets built as a vector containing 2 elements; if .arg(...) is called 0 times then args gets built as a vector containing 0 elements.

from proc-macro-workshop.

This sounds like a bug in your implementation of the macro? The invocation looks like it should work to me. There is only one env field.

from proc-macro-workshop.

I think the test expectations could be better documented. When the "each" name is the same as the field name (as is the case with "env" here), it's not possible to generate both a one-at-a-time setter AND an all-at-once setter.

The correct result could be any of the following:

1. Don't emit an all-at-once setter function if the one-at-a-time function is requested
2. Emit both functions unless there's a name collision, in which case emit only the one-at-a-time fn.
3. Emit both functions unless there's a name collision, in which case emit only the all-at-once fn.
4. Throw a compiler error if there's a name collision.

If you meant for the name collision to be allowed, then (4) is no good. (3) seems at odds with the programmer's attempt to add something. (1) is maybe too limiting, and (2) seems like a weird side-effect-at-a-distance ("I added B, why did A go away?").

I think @jonhoo found this confusing during his stream also; he went with (2).

from proc-macro-workshop.

Ah, makes sense. I think I had 1 in mind in the reference implementation but 2 would be fine too. I would accept a PR to clarify this in the test case. Thanks!

from proc-macro-workshop.

@dtolnay thank you very much for this amazing workshop! Great job and it's being totally amazing for me!

Aside from the above clarification, I think there's still a bug or I didn't implement as desired since past exercises. The builder is called like this:

    let command = Command::builder()
        .executable("cargo".to_owned())
        .arg("build".to_owned())
        .arg("--release".to_owned())
        .build()
        .unwrap();

We are not calling the builder method to instantiate env at any moment. So my macro is throwing field "env" is missing since we are attempting to build() and env is not an optional field - which I believe is behaving correctly. Did I miss anything?

from proc-macro-workshop.