Use conventional meta-item syntax for attributes #2875

TedDriggs · 2022-04-28T01:59:12Z

Disclaimers

I apologize for the diff size. The meaningful changes are in crates/macro-support - the other files are changed to avoid any commits that won't build.
This introduces a potentially-divisive change, using quotation marks for most key-value meta items. This has some advantages (such as working with rustfmt), but I know there are those in the community who really dislike the extra quotation marks. If wasm-bindgen has already decided not to use them, I'll close out this PR.
This is a major breaking change; almost all key-value meta items need updates. If people are receptive to this direction but concerned about a huge breaking change, I am happy to explore ways to temporarily support both formats.
I'm the author of darling, the attribute-parsing library this uses. There's probably a way to do this without darling, but it's surprisingly difficult to maintain the level of error quality without it.

Commit message

Using the conventional form for attributes, which is compatible with rustfmt and syn's meta-item parsing. This change is breaking and substantial, but the necessary edits are largely mechanical.

With meta-items, it becomes possible to offload attribute parsing and validation to darling. This offload fixes #2874, as darling will automatically generate errors for unexpected meta-item names. This also makes it easier for people who are new to the codebase to understand how to add new options.

crates/backend/src/error.rs

TedDriggs · 2022-04-28T02:03:03Z

crates/macro-support/src/d.rs

@@ -0,0 +1,383 @@
+//! Structs to receive attribute options for each supported syntax type.


This module needs a different name.

TedDriggs · 2022-04-28T02:04:23Z

crates/macro-support/src/d.rs

+pub mod fields {
+    use darling::util::Flag;
+
+    macro_rules! field_trait {
+        ($name:ident, $return:ty, $field:ident) => {
+            pub trait $name {
+                fn $field(&self) -> $return;
+            }
+        };
+    }
+
+    field_trait!(JsName, Option<&syn::LitStr>, js_name);
+    field_trait!(SkipTypescript, Flag, skip_typescript);
+}


I tried - and failed - to find a good way to make these driven by a single macro. It'd be very doable with a proc-macro, but having a proc-macro crate that supports macro-support ...which is a proc-macro... felt too recursive.

TedDriggs · 2022-04-28T02:05:44Z

crates/macro-support/src/d.rs

+impl<'a> From<&'a ImplItemMethod> for ast::OperationKind {
+    fn from(opts: &'a ImplItemMethod) -> Self {
+        let mut operation_kind = ast::OperationKind::Regular;
+        if let Some(g) = &opts.getter {
+            operation_kind = ast::OperationKind::Getter(g.deref().clone().explicit());
+        }
+        if let Some(s) = &opts.setter {
+            operation_kind = ast::OperationKind::Setter(s.deref().clone().explicit());
+        }
+        if opts.indexing_getter.is_present() {
+            operation_kind = ast::OperationKind::IndexingGetter;
+        }
+        if opts.indexing_setter.is_present() {
+            operation_kind = ast::OperationKind::IndexingSetter;
+        }
+        if opts.indexing_deleter.is_present() {
+            operation_kind = ast::OperationKind::IndexingDeleter;
+        }
+
+        operation_kind
+    }
+}
+
+impl<'a> From<&'a ItemFn> for ast::OperationKind {
+    fn from(opts: &'a ItemFn) -> Self {
+        let mut operation_kind = ast::OperationKind::Regular;
+        if let Some(g) = &opts.getter {
+            operation_kind = ast::OperationKind::Getter(g.deref().clone().explicit());
+        }
+        if let Some(s) = &opts.setter {
+            operation_kind = ast::OperationKind::Setter(s.deref().clone().explicit());
+        }
+        if opts.indexing_getter.is_present() {
+            operation_kind = ast::OperationKind::IndexingGetter;
+        }
+        if opts.indexing_setter.is_present() {
+            operation_kind = ast::OperationKind::IndexingSetter;
+        }
+        if opts.indexing_deleter.is_present() {
+            operation_kind = ast::OperationKind::IndexingDeleter;
+        }
+
+        operation_kind
+    }
+}
+
+impl<'a> From<&'a ForeignItemFn> for ast::OperationKind {
+    fn from(opts: &'a ForeignItemFn) -> Self {
+        let mut operation_kind = ast::OperationKind::Regular;
+        if let Some(g) = &opts.getter {
+            operation_kind = ast::OperationKind::Getter(g.deref().clone().explicit());
+        }
+        if let Some(s) = &opts.setter {
+            operation_kind = ast::OperationKind::Setter(s.deref().clone().explicit());
+        }
+        if opts.indexing_getter.is_present() {
+            operation_kind = ast::OperationKind::IndexingGetter;
+        }
+        if opts.indexing_setter.is_present() {
+            operation_kind = ast::OperationKind::IndexingSetter;
+        }
+        if opts.indexing_deleter.is_present() {
+            operation_kind = ast::OperationKind::IndexingDeleter;
+        }
+
+        operation_kind
+    }
+}


These shouldn't be copy-paste duplicated. A macro or intermediate trait would be better.

TedDriggs · 2022-04-28T02:06:30Z

crates/macro/src/lib.rs

+    let attr_args = syn::parse_macro_input!(attr as syn::AttributeArgs);
+    match wasm_bindgen_macro_support::expand(attr_args, input.into()) {


There may be a way to construct an AttributeArgs from a proc_macro2::TokenStream but I couldn't get it to work. If that does exist, the syn dependency can be removed from this crate.

TedDriggs · 2022-04-28T02:07:35Z

crates/web-sys/src/features/gen_AbortController.rs

+    #[wasm_bindgen(
+        extends = "::js_sys::Object",
+        js_name = "AbortController",
+        typescript_type = "AbortController"
+    )]


For anyone wondering how this diff is +55k, here's the answer; rustfmt now moves these to new lines.

TedDriggs · 2022-04-28T14:31:24Z

crates/macro-support/src/d.rs

+
+use self::fields::SkipTypescript;
+
+/// A JS namespace, which can be represented as `"value"` or `r#"["first", "second"]#"`.


Having to write #[wasm_bindgen(js_namespace = r#"["window", "document"]"#)] is probably the single most unfortunate consequence of this change, and I think it can be avoided in most cases by accepting one of three alternate syntaxes:

// Approach 1 #[wasm_bindgen(js_namespace(window, document))] // Approach 2 #[wasm_bindgen(js_namespace(window::document))] // Approach 3 #[wasm_bindgen(js_namespace = "window.document")]

It would be straightforward to add any of these to impl FromMeta for JsNamespace.

alexcrichton · 2022-04-28T16:19:30Z

Thanks for the PR here, but unfortunately this project isn't really in a place to manage or merge large breaking changes. I don't think that there's going to be a viable way to land this, even if the repo is fully updated, any time soon.

TedDriggs · 2022-04-28T16:28:00Z

Makes sense: Getting that feedback was part of why I posted after 5 hours of work so I wouldn't be too deeply invested. A couple follow-up questions:

Is the change to having dedicated options structs per input-item-type interesting, if it can be done without changing the outward attribute API?
Is a non-breaking version of the API change - adding support for the quoted format alongside the current format interesting, or would you prefer to stick to the generally-unquoted syntax long-term?

alexcrichton · 2022-04-29T14:11:22Z

Sorry this is a big change I don't have the time to review, but overall making the proc-macro more readable or easier to understand/modify seems reasonable to me. Whether that could be achieved with the struct-per-input-item-type I'm not sure.

Personally I would prefer just one syntax. I'm not wed to with or without quotes, but I'd rather not have both.

Using the conventional form for attributes, which is compatible with rustfmt and syn's meta-item parsing. This change is breaking and substantial, but the necessary edits are largely mechanical. With meta-items, it becomes possible to offload attribute parsing and validation to `darling`. This offload fixes rustwasm#2874, as `darling` will automatically generate errors for unexpected meta-item names. This also makes it easier for people who are new to the codebase to understand how to add new options. This commit does not build: It changes the macro parser, but does not update the usage of the macro.

TedDriggs · 2022-04-29T15:13:58Z

Sorry this is a big change I don't have the time to review

If I take out the diff parts caused by the breaking change, the new size is +554/-578; is that more tractable?

Whether that could be achieved with the struct-per-input-item-type I'm not sure.

I have two ideas for this. Both of them start by adding macro_support::Meta, which is equivalent to syn::Meta except that it doesn't insist on the things to the right of = being valid literals.

Option 1

Do low-level parsing using syn::Parse to turn the attr TokenStream into a macro_support::Meta.
(Somehow) convert each macro_support::Meta into a syn::Meta by wrapping values right of = in LitStr, preserving spans while doing so.
Pass these newly-created syn::Meta items to the derived FromMeta impls.

Option 2

Create a proc-macro crate macro-support-gen, which exposes a derive-macro that's analogous to darling::FromMeta but operates on the macro_support::Meta type above. Then recreate some amount of darling's meta-parsing loop in the macro-support-gen crate.

Option 1 is much easier and more maintainable, at the expense of traversing the attribute twice and sometimes doing more allocations.

TedDriggs · 2022-04-29T17:11:40Z

Option 1 actually works! I'm going to update the review shortly.

TedDriggs · 2022-04-29T17:54:35Z

I'm closing this in favor of #2880.

TedDriggs commented Apr 28, 2022

View reviewed changes

TedDriggs force-pushed the darling branch from 77dd217 to 3078ea5 Compare April 28, 2022 14:25

TedDriggs commented Apr 28, 2022

View reviewed changes

TedDriggs force-pushed the darling branch from 3078ea5 to 45030a1 Compare April 28, 2022 15:36

Ted Driggs added 2 commits April 29, 2022 08:10

Update macro usage and docs

84dd6f9

TedDriggs force-pushed the darling branch from 45030a1 to 84dd6f9 Compare April 29, 2022 15:10

TedDriggs closed this Apr 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use conventional meta-item syntax for attributes #2875

Use conventional meta-item syntax for attributes #2875

TedDriggs commented Apr 28, 2022

TedDriggs Apr 28, 2022

TedDriggs Apr 28, 2022

TedDriggs Apr 28, 2022

TedDriggs Apr 28, 2022

TedDriggs Apr 28, 2022

TedDriggs Apr 28, 2022

alexcrichton commented Apr 28, 2022

TedDriggs commented Apr 28, 2022

alexcrichton commented Apr 29, 2022

TedDriggs commented Apr 29, 2022

TedDriggs commented Apr 29, 2022

TedDriggs commented Apr 29, 2022

		@@ -0,0 +1,383 @@
		//! Structs to receive attribute options for each supported syntax type.

		let attr_args = syn::parse_macro_input!(attr as syn::AttributeArgs);
		match wasm_bindgen_macro_support::expand(attr_args, input.into()) {


		use self::fields::SkipTypescript;

		/// A JS namespace, which can be represented as `"value"` or `r#"["first", "second"]#"`.

Use conventional meta-item syntax for attributes #2875

Use conventional meta-item syntax for attributes #2875

Conversation

TedDriggs commented Apr 28, 2022

Disclaimers

Commit message

TedDriggs Apr 28, 2022

Choose a reason for hiding this comment

TedDriggs Apr 28, 2022

Choose a reason for hiding this comment

TedDriggs Apr 28, 2022

Choose a reason for hiding this comment

TedDriggs Apr 28, 2022

Choose a reason for hiding this comment

TedDriggs Apr 28, 2022

Choose a reason for hiding this comment

TedDriggs Apr 28, 2022

Choose a reason for hiding this comment

alexcrichton commented Apr 28, 2022

TedDriggs commented Apr 28, 2022

alexcrichton commented Apr 29, 2022

TedDriggs commented Apr 29, 2022

Option 1

Option 2

TedDriggs commented Apr 29, 2022

TedDriggs commented Apr 29, 2022