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

Reimplement attribute parsing without darling #310

Closed
wants to merge 1 commit into from

Conversation

dtolnay
Copy link
Contributor

@dtolnay dtolnay commented Feb 15, 2024

This PR reimplements parsing using Syn's attribute parsing library, which lifts a few limitations previously imposed on derive_builder's implementation by darling.

  • Keywords in attributes: darling doesn't make it possible to use keywords in a nested attribute, such as field(type = "..."). I have added back support for that syntax in this PR (but also kept field(ty = "...") as an alias for the same thing for backward compatibility).
    Attributes containing keywords can't be parsed with 0.20 TedDriggs/darling#238

  • Factoring commonality from different data structures: for this PR I have not gone overboard with refactoring, but I did move the trio of public + private + vis = "..." to a single central place, as it was repeated across 5 different structs. This PR unlocks additional possibilities for factoring out common behavior, and there are pre-existing comments to the effect that this would be desirable.
    Add support for flatten TedDriggs/darling#146

  • Validation during parse: with darling, structs are created in a potentially invalid state and then code needs to crawl them after the fact to perform validation. In this PR, structs do not get constructed in an invalid state; validation is integrated with the parsing. This also means various "DO NOT USE" fields go away in this PR.

  • Passing state from outer contexts to inner: derive_builder has workarounds that exist because darling's FromMeta needs to be able to fully construct the output with no context other than the input Meta. In this PR, parsing is handled top to bottom by free-form function calls; additional arguments can be passed through as it becomes useful. For example, one can parse attributes at the struct level and then pass data from those to the code that parses attributes at the field level.

  • Compile time: this PR improves compile time of derive_builder by 35% on my machine (5.9 seconds to 3.8 seconds).

--> tests/compile-fail/build_fn_error.rs:4:52
|
4 | #[builder(build_fn(error(validation_error = false, path = "hello")))]
| ^^^^

error: Cannot set `error(validation_error = false)` when using `validate`
--> tests/compile-fail/build_fn_error.rs:10:45
error: `error(validation_error = false)` cannot be set when using `validate`
Copy link

Choose a reason for hiding this comment

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

if validate = "hello" is highlighted below then shouldn't this say something like "validate cannot be set if error(validation_error = false) is used"? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I updated the PR rewording it to "error(validation_error = false) and validate cannot be used together" which is consistent with wording that is already used prior to this PR when dealing with the conflict between builder(default) and builder(field(build = "...")).

@TedDriggs
Copy link
Collaborator

Acknowledging receipt of PR. darling was created because the original maintainers and I were struggling to use syn to deliver the experience we wanted, so I expect I'll need some time to review this in full. I likely won't have that time this week, but will get to this as soon as I can.

@dtolnay
Copy link
Contributor Author

dtolnay commented Feb 21, 2024

Rebased on #311 to fix nightly CI.

@TedDriggs
Copy link
Collaborator

Factoring commonality from different data structures: for this PR I have not gone overboard with refactoring, but I did move the trio of public + private + vis = "..." to a single central place, as it was repeated across 5 different structs. This PR unlocks additional possibilities for factoring out common behavior, and there are pre-existing comments to the effect that this would be desirable.

This finally gave me the push needed to get flatten implemented in darling, and it does make a noticeable difference in the readability of this code. I've put up #312 to adopt that in derive_builder.

Keywords in attributes: darling doesn't make it possible to use keywords in a nested attribute, such as field(type = "..."). I have added back support for that syntax in this PR (but also kept field(ty = "...") as an alias for the same thing for backward compatibility).

We'd previously discussed this in dtolnay/syn#1458, and my impression was that not accepting type was deemed to be the correct behavior by syn. If it's better to accept this, I'd rather make that change at the darling layer so that other crates don't need to do similar parsing themselves.

Validation during parse: with darling, structs are created in a potentially invalid state and then code needs to crawl them after the fact to perform validation. In this PR, structs do not get constructed in an invalid state; validation is integrated with the parsing. This also means various "DO NOT USE" fields go away in this PR.

With the introduction of #[darling(flatten)], the highlighted example has disappeared. The remaining validation items are all, AFAIK, pretty distant cross-field checks like this one in Field::resolve.

Since those checks are part of the generated darling impl via the use of and_then, I view the few remaining such checks as very ergonomic and reasonable.

derive_builder has workarounds that exist because darling's FromMeta needs to be able to fully construct the output with no context other than the input Meta.

I view these as a benefit, not a drawback. This may be personal preference, but I find it easier to reason over a set of structs that represent what was extracted from the DeriveInput and then to have usage-time pullthrough/fallback logic that makes explicit which things do - or do not - depend on the parent context.

Compile time: this PR improves compile time of derive_builder by 35% on my machine (5.9 seconds to 3.8 seconds).

I would very much like to give users of this library and users of darling build-time performance improvements.

I'm not sold on the idea that abandoning darling is necessary for those build-time improvements, or that it's the best long-term decision to do so. This PR introduces a lot more parsing machinery code that @colin-kiegel and I deliberately removed years ago, and since then derive_builder has enjoyed a number of new features with no bugs in attribute handling that I can recall.

I view darling the same way I view serde_derive or structopt - a tradeoff of performance vs maintainability.

@dtolnay
Copy link
Contributor Author

dtolnay commented Feb 23, 2024

Makes sense; I appreciate the counterpoints.

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