Skip to content

Conversation

@schneems
Copy link
Contributor

Removing duplicate attributes won't change the code emitted, we can confidently write an impl + the error. Similar to how the current macro emits an impl + errors when the leaf attribute is used on an enum (as it can have no effect on the generated code).

Background

I identified two other possible locations for this behavior in https://schneems.com/2025/03/26/a-daft-procmacro-trick-how-to-emit-partialcode-errors/. This PR addresses one of them: duplicate attributes on a struct.

Related conversation https://ruby.social/@Schneems/114230457184998727

…onfidently write an impl + the error. Similar to how the current macro emits an impl + errors when the `leaf` attribute is used on an enum (as it can have no effect on the generated code).

## Background

I identified two other possible locations for this behavior in https://schneems.com/2025/03/26/a-daft-procmacro-trick-how-to-emit-partialcode-errors/. This PR addresses one of them: duplicate attributes on a struct.

Related conversation https://ruby.social/@Schneems/114230457184998727
@schneems schneems marked this pull request as draft March 26, 2025 21:32
} else {
for error in duplicates {
errors.push(error);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to bypass errors.has_errors() check so we return Some here. It's only nested one level deep so that's enough to get it to produce code:

fn make_struct_impl(
    input: &DeriveInput,
    s: &DataStruct,
    errors: ErrorSink<'_, syn::Error>,
) -> Option<TokenStream> {
    let Some(struct_config) =
        StructConfig::parse_from(&input.attrs, errors.new_child())
    else {
        // An error occurred parsing the struct configuration -- don't generate
        // anything.
        return None;
    };

Though this initial commit fails to add the duplicates back in in the event of a larger error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though this initial commit fails to add the duplicates back in in the event of a larger error.

Fixed in the second commit and I added a test exercising this mixed-error case.

The field code is nested much deeper than the struct error code making it non-trivial to apply there. One possible option is introducing some differentiation between the types of errors that are held by ErrorSink such as "critical_error" versus "warn_error." So we could have if errors.has_critical_error() { instead of if errors.has_errors() {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I see what you mean, and I really like the idea of having two categories of errors the way you've described them. How about updating ErrorSink to have "errors" and "warnings", knowing that we have to produce errors for warnings due to proc-macro API limitations.

@schneems schneems force-pushed the schneems/code-and-duplicate-attribute-errors branch from edfee73 to bcf735e Compare March 26, 2025 22:00
@schneems schneems marked this pull request as ready for review March 26, 2025 22:06
@andrewjstone
Copy link
Collaborator

@schneems Just a fly by to say that I really enjoyed and appreciated your blog post!

@schneems
Copy link
Contributor Author

schneems commented Apr 1, 2025

@sunshowers I implemented the suggestion to split up warnings and critical errors in #64 and I like how it turned out. The actual "emit code + errors" change once the infrastructure is in place was a one line edit which felt great. I'll close this one.

Also (might interest you) I have a PR open to syn to add docs to some of the lesser-known capabilities of syn::Error dtolnay/syn#1855. I'm open to feedback there or on mastodon for suggestions on how to improve the PR if you've got any.

appreciated your blog post!

Thanks for saying so @andrewjstone! I got very little feedback on it (no comments where it was posted), thanks for taking the time to let me know.

@schneems schneems closed this Apr 1, 2025
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 this pull request may close these issues.

3 participants