search

colin-kiegel / rust-derive-builder

derive builder implementation for rust structs
https://colin-kiegel.github.io/rust-derive-builder/
Apache License 2.0
1.28k stars 82 forks source link

Reimplement attribute parsing without `darling` #310

Closed dtolnay closed 6 months ago

dtolnay commented 7 months ago

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

TedDriggs commented 7 months ago

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 commented 7 months ago

Rebased on #311 to fix nightly CI.

TedDriggs commented 6 months ago

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 commented 6 months ago

Makes sense; I appreciate the counterpoints.