Refactor derive(Freeze)

Summary:
Use the same parser for `#[freeze]` on struct and on field. Errors with better spans.

For future changes. What we need is something like

```
#[freeze(frozen = ...)]
```

to `derive(Freeze)` when some fields are generic but freeze is identity. Currently it is not possible, because even if bounds is specified, generated code still uses `T::Frozen` for frozen type parameter.

Reviewed By: JakobDegen

Differential Revision: D64337850

fbshipit-source-id: 970c3ade11061b9f3bd4f55baabf78cc076a9f6d

starlark-rust/starlark_derive/src/freeze.rs

@@ -24,7 +24,6 @@ use syn::parse_macro_input;
 use syn::spanned::Spanned;
 use syn::Attribute;
 use syn::DeriveInput;
-use syn::Error;
 use syn::GenericParam;
 use syn::LitStr;
 use syn::Token;
@@ -81,7 +80,10 @@ impl<'a> Input<'a> {
                     output_params.push(quote_spanned! { span=> 'static });
                 }
                 GenericParam::Const(_) => {
-                    return Err(Error::new_spanned(param, "const generics not supported"));
+                    return Err(syn::Error::new_spanned(
+                        param,
+                        "const generics not supported",
+                    ));
                 }
             }
         }
@@ -99,19 +101,31 @@ fn derive_freeze_impl(input: DeriveInput) -> syn::Result<syn::ItemImpl> {
 
     let name = &input.input.ident;
 
-    let opts = extract_options(&input.input.attrs)?;
+    let FreezeDeriveOptions {
+        validator,
+        bounds,
+        identity,
+    } = extract_options(&input.input.attrs)?;
+
+    if let Some(identity) = identity {
+        return Err(syn::Error::new_spanned(
+            identity,
+            "`identity` can only be used on fields",
+        ));
+    }
+
     let (impl_params, input_params, output_params) =
-        input.format_impl_generics(opts.bounds.is_some())?;
+        input.format_impl_generics(bounds.is_some())?;
 
-    let validate_body = match opts.validator {
+    let validate_body = match validator {
         Some(validator) => quote_spanned! {
             span=>
             #validator(&frozen)?;
         },
         None => quote_spanned! { span=> },
     };
 
-    let bounds_body = match opts.bounds {
+    let bounds_body = match bounds {
         Some(bounds) => quote_spanned! { span=> where #bounds },
         None => quote_spanned! { span=> },
     };
@@ -134,10 +148,16 @@ fn derive_freeze_impl(input: DeriveInput) -> syn::Result<syn::ItemImpl> {
     Ok(gen)
 }
 
+syn::custom_keyword!(identity);
+
 #[derive(Default)]
 struct FreezeDeriveOptions {
+    /// `#[freeze(validator = function)]`.
     validator: Option<Ident>,
+    /// `#[freeze(bounds = ...)]`.
     bounds: Option<WherePredicate>,
+    /// `#[freeze(identity)]`.
+    identity: Option<identity>,
 }
 
 /// Parse a #[freeze(validator = function)] annotation.
@@ -154,19 +174,30 @@ fn extract_options(attrs: &[Attribute]) -> syn::Result<FreezeDeriveOptions> {
 
         attr.parse_args_with(|input: ParseStream| {
             loop {
-                if input.parse::<validator>().is_ok() {
+                if let Some(validator) = input.parse::<Option<validator>>()? {
                     if opts.validator.is_some() {
-                        return Err(input.error("`validator` was set twice"));
+                        return Err(syn::Error::new_spanned(
+                            validator,
+                            "`validator` was set twice",
+                        ));
                     }
                     input.parse::<Token![=]>()?;
                     opts.validator = Some(input.parse()?);
-                } else if input.parse::<bounds>().is_ok() {
+                } else if let Some(bounds) = input.parse::<Option<bounds>>()? {
                     if opts.bounds.is_some() {
-                        return Err(input.error("`bounds` was set twice"));
+                        return Err(syn::Error::new_spanned(bounds, "`bounds` was set twice"));
                     }
                     input.parse::<Token![=]>()?;
                     let bounds_input = input.parse::<LitStr>()?;
                     opts.bounds = Some(bounds_input.parse()?);
+                } else if let Some(identity) = input.parse::<Option<identity>>()? {
+                    if opts.identity.is_some() {
+                        return Err(syn::Error::new_spanned(
+                            identity,
+                            "`identity` was set twice",
+                        ));
+                    }
+                    opts.identity = Some(identity);
                 } else {
                     return Err(input.lookahead1().error());
                 }
@@ -183,40 +214,33 @@ fn extract_options(attrs: &[Attribute]) -> syn::Result<FreezeDeriveOptions> {
     Ok(opts)
 }
 
-/// Parse attribute `#[freeze(identity)]`.
-///
-/// Currently it fails on any attribute argument other than `id`.
-fn is_identity(attrs: &[Attribute]) -> syn::Result<bool> {
-    syn::custom_keyword!(identity);
-
-    for attr in attrs.iter() {
-        if !attr.path().is_ident("freeze") {
-            continue;
-        }
-
-        let ignore = attr.parse_args_with(|input: ParseStream| {
-            let ignore = input.parse::<Option<identity>>()?.is_some();
-            Ok(ignore)
-        })?;
-
-        if !ignore {
-            continue;
-        }
-
-        return Ok(true);
-    }
-
-    Ok(false)
-}
-
 fn freeze_impl(derive_input: &DeriveInput) -> syn::Result<syn::Expr> {
     let derive_input = DeriveInputUtil::new(derive_input)?;
     derive_input.match_self(|struct_or_enum_variant, fields| {
         let fields: Vec<syn::Expr> = fields
             .iter()
             .map(|(ident, f)| {
                 let span = ident.span();
-                if is_identity(&f.attrs)? {
+
+                let FreezeDeriveOptions {
+                    validator,
+                    bounds,
+                    identity,
+                } = extract_options(&f.attrs)?;
+                if let Some(validator) = validator {
+                    return Err(syn::Error::new_spanned(
+                        validator,
+                        "Cannot use `validator` on field",
+                    ));
+                }
+                if let Some(bounds) = bounds {
+                    return Err(syn::Error::new_spanned(
+                        bounds,
+                        "Cannot use `bounds` on field",
+                    ));
+                }
+
+                if identity.is_some() {
                     Ok(syn::parse_quote_spanned! { span=>
                         #ident
                     })