-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Refactor #[wasm_bindgen] attribute parsing #2880
Conversation
This commit moves to having a struct per `syn::Item` type that the macro operates on, then uses `darling` to handle validation of the attribute. This implicitly fixes rustwasm#2874, since `darling` automatically generates errors for unexpected properties. It also makes the macro code easier to follow by hiding the complexity associated with parsing out of sight in a well-tested crate. This also precludes a class of bug where someone might have attempted to read a property that was not accepted for that syntax item, since each syntax item now has its own options receiver. `darling` operates on `syn::Meta`, which only allows a literal to the right side of `=` signs in attributes. This is a problem for wasm_bindgen, which does not use them. To maintain API compatibility while using `darling`, this commit adds `macro_support::meta::Meta<T>`, a replica of `syn::Meta` where `Lit` is replaced by a type parameter. This may be useful enough beyond `wasm_bindgen` for it to move upstream to `darling`, which would mean this change would reduce the overall amount of code in `macro-support`.
@alexcrichton it's no longer a breaking change, and the size is coming down! (And may come down further if I do end up moving the new (Asterisk on the breaking change piece: In #2874, an invalid meta-item is silently ignored, while this change will cause that to be a compile error. So this could be "breaking" in that sense, but no valid usages will break from this) |
This isn't valid for top-level items, so relying on it for sub-items is fragile. This probably merits a dedicated compile-error.
Sorry but this is still a pretty big PR with a new crate that I've never personally used before so I don't have prior experience to help me with review. Additionally while the pain points mentioned here I think would be good to fix they don't seem like critical bugs and I'm not really in a position to review major changes to wasm-bindgen at the moment. |
Thanks for the reply. I don't know of a way to further shrink the PR, so I'll leave it as-is in case things change and time for review becomes available. |
I actually like Generally speaking I would avoid adding any further proc-macro crates on top of But if other maintainers speak in favor of adding this, I would be happy to review it. |
@daxpedda that build-time impact is gross. How did you make this chart? I currently don't know anything about reducing build times for Rust crates, but this is reason to start learning. -- Is build-time impact your primary reason for opposition? I'm totally fine with this PR being closed out, but darling's long-term goal is to be "serde for proc-macro parsing", where its feature set is compelling enough - and its barriers to entry, including performance impact - are low enough that there are only rare cases where it shouldn't be a good solution. Given that goal, any other concerns you have sound like they should be areas of exploration for darling's roadmap |
See the Cargo documentation on timings.
Yes, more reasoning to follow.
I think core crates in general should have as little dependencies as possible in Rust (probably grossly off-topic to discuss why here). So generally speaking, I think this is one of these rare cases. The only way I would re-consider is if the crate has very little overhead and provides a significant (quite subjective) improvement over just using |
I’d contend that good error handling for a project like this is very important - and is an example of the “undifferentiated heavy lifting” that makes sense to outsource to another crate. When I was learning Rust, I found the quality of the compiler errors to be hugely valuable, and I was disappointed by the comparative lack of quality in proc-macro errors.
The crate also has some quality-of-life features that I haven’t seen in most hand-rolled macros; key among those is the “did you mean” suggestions when encountering an unexpected field. I’m not sure if I’ll be able to get the build time down to a point where it’s good enough for a core crate such as this, but if I can I would contend that the benefits listed above are important for ease of adoption, particularly for people who might be trying Rust for the first time in a WASM context |
I think a significant increase in error message quality, which isn't hard considering it's currently quite low in So I would definitely welcome the improvement if |
@daxpedda I've filed TedDriggs/darling#237 to investigate improvements here - if you have any experience with reading the |
This commit moves to having a struct per
syn::Item
type that the macro operates on, then usesdarling
to handle validation of the attribute.This implicitly fixes #2874, since
darling
automatically generates errors for unexpected properties. It also makes the macro code easier to follow by hiding the complexity associated with parsing out of sight in a well-tested crate. This also precludes a class of bug where someone might have attempted to read a property that was not accepted for that syntax item, since each syntax item now has its own options receiver.darling
operates onsyn::Meta
, which only allows a literal to the right side of=
signs in attributes. This is a problem for wasm_bindgen, which does not use them.To maintain API compatibility while using
darling
, this commit addsmacro_support::meta::Meta<T>
, a replica ofsyn::Meta
whereLit
is replaced by a type parameter. This may be useful enough beyondwasm_bindgen
for it to move upstream todarling
, which would mean this change would reduce the overall amount of code inmacro-support
.