Repository: Aleph-Alpha/ts-rs (rust)

Issue:
Feature request: Handle unsupported serde attributes more gracefully
When an a `#[serde(..)]` attribute contains an unsupported setting (e.g `skip_serializing_if`), the whole attribute is ignored.

We should explore if this could be handled more gracefully. A plan to take this on might be to
- adjust parsing logic to not short-circuit on an unsupported attribute
- produce warnings with better spans to indicate *which* attributes are unsupported


Pull request description:
Handle serde fail gracefully
## Goal

Prevent `serde` attribute from being ignored when it contains unsupported settings
Closes #388

## Changes

- Split `impl_parse` into two branches to handle `Serde<T>` separately
- Move error reporting from `parse_serde_attrs` to `impl_parse`
- When an unsupported setting is found, skip until the next comma in the attribute

## Checklist

- [x] I have followed the steps listed in the [Contributing guide](https://github.com/Aleph-Alpha/ts-rs/blob/main/CONTRIBUTING.md).
- [x] If necessary, I have added documentation related to the changes made.
- [x] I have added or updated the tests related to the changes made.


Patch:
diff --git a/macros/src/attr/mod.rs b/macros/src/attr/mod.rs
index 61debed..681c834 100644
--- a/macros/src/attr/mod.rs
+++ b/macros/src/attr/mod.rs
@@ -1,8 +1,10 @@
 use std::collections::HashMap;
 
 pub use field::*;
+use proc_macro2::TokenTree;
 pub use r#enum::*;
 pub use r#struct::*;
+use quote::quote;
 use syn::{
     parse::{Parse, ParseStream},
     punctuated::Punctuated,
@@ -126,6 +128,27 @@ impl Inflection {
     }
 }
 
+fn skip_until_next_comma(input: ParseStream) -> proc_macro2::TokenStream {
+    input
+        .step(|cursor| {
+            let mut stuff = quote!();
+            let mut rest = *cursor;
+            while let Some((tt, next)) = rest.token_tree() {
+                if let Some((TokenTree::Punct(punct), _)) = next.token_tree() {
+                    if punct.as_char() == ',' {
+                        return Ok((stuff, next));
+                    }
+                }
+
+                rest = next;
+                stuff = quote!(#stuff #tt)
+            }
+
+            Ok((stuff, rest))
+        })
+        .unwrap()
+}
+
 fn parse_assign_str(input: ParseStream) -> Result<String> {
     input.parse::<Token![=]>()?;
     match Lit::parse(input)? {
diff --git a/macros/src/utils.rs b/macros/src/utils.rs
index 445487f..ceffdf8 100644
--- a/macros/src/utils.rs
+++ b/macros/src/utils.rs
@@ -26,14 +26,14 @@ macro_rules! syn_err_spanned {
 }
 
 macro_rules! impl_parse {
-    ($i:ident $(<$inner: ident>)? ($input:ident, $out:ident) { $($k:pat => $e:expr),* $(,)? }) => {
-        impl std::convert::TryFrom<&syn::Attribute> for $i $(<$inner>)? {
+    ($i:ident ($input:ident, $out:ident) { $($k:pat => $e:expr),* $(,)? }) => {
+        impl std::convert::TryFrom<&syn::Attribute> for $i {
             type Error = syn::Error;
 
             fn try_from(attr: &syn::Attribute) -> syn::Result<Self> { attr.parse_args() }
         }
 
-        impl syn::parse::Parse for $i $(<$inner>)? {
+        impl syn::parse::Parse for $i {
             fn parse($input: syn::parse::ParseStream) -> syn::Result<Self> {
                 let mut $out = Self::default();
                 loop {
@@ -47,14 +47,56 @@ macro_rules! impl_parse {
                             "Unknown attribute \"{x}\". Allowed attributes are: {}",
                             [$(stringify!($k),)*].join(", ")
                         )
+
+                    }
+
+                    if $input.is_empty() {
+                        break;
                     }
 
-                    match $input.is_empty() {
-                        true => break,
-                        false => {
-                            $input.parse::<syn::Token![,]>()?;
+                    $input.parse::<syn::Token![,]>()?;
+                }
+
+                Ok($out)
+            }
+        }
+    };
+    ($i:ident<$inner: ident> ($input:ident, $out:ident) { $($k:pat => $e:expr),* $(,)? }) => {
+        impl std::convert::TryFrom<&syn::Attribute> for $i<$inner> {
+            type Error = syn::Error;
+
+            fn try_from(attr: &syn::Attribute) -> syn::Result<Self> { attr.parse_args() }
+        }
+
+        impl syn::parse::Parse for $i<$inner> {
+            fn parse($input: syn::parse::ParseStream) -> syn::Result<Self> {
+                let mut $out = Self::default();
+                loop {
+                    let key: syn::Ident = $input.call(syn::ext::IdentExt::parse_any)?;
+                    match &*key.to_string() {
+                        $($k => $e,)*
+                        #[allow(unreachable_patterns)]
+                        x => {
+                            if cfg!(not(feature = "no-serde-warnings")) {
+                                let tokens = crate::attr::skip_until_next_comma($input);
+
+                                crate::utils::warning::print_warning(
+                                    "failed to parse serde attribute",
+                                    format!("{x} {tokens}"),
+                                    "ts-rs failed to parse this attribute. It will be ignored.",
+                                )
+                                .unwrap();
+                            } else {
+                                crate::attr::skip_until_next_comma($input);
+                            }
                         }
                     }
+
+                    if $input.is_empty() {
+                        break;
+                    }
+
+                    $input.parse::<syn::Token![,]>()?;
                 }
 
                 Ok($out)
@@ -119,22 +161,7 @@ where
     attrs
         .iter()
         .filter(|a| a.path().is_ident("serde"))
-        .flat_map(|attr| match Serde::<A>::try_from(attr) {
-            Ok(attr) => Some(attr),
-            Err(_) => {
-                #[cfg(not(feature = "no-serde-warnings"))]
-                use quote::ToTokens;
-
-                #[cfg(not(feature = "no-serde-warnings"))]
-                warning::print_warning(
-                    "failed to parse serde attribute",
-                    format!("{}", attr.to_token_stream()),
-                    "ts-rs failed to parse this attribute. It will be ignored.",
-                )
-                .unwrap();
-                None
-            }
-        })
+        .flat_map(|attr| Serde::<A>::try_from(attr).ok())
         .fold(Serde::<A>::default(), |acc, cur| acc.merge(cur))
 }
 
@@ -180,7 +207,7 @@ pub fn parse_docs(attrs: &[Attribute]) -> Result<String> {
 }
 
 #[cfg(feature = "serde-compat")]
-mod warning {
+pub(crate) mod warning {
     use std::{fmt::Display, io::Write};
 
     use termcolor::{BufferWriter, Color, ColorChoice, ColorSpec, WriteColor};
@@ -232,7 +259,7 @@ mod warning {
     }
 }
 #[cfg(not(feature = "serde-compat"))]
-mod warning {
+pub(crate) mod warning {
     use std::fmt::Display;
 
     // Just a stub!


Test patch:
diff --git a/ts-rs/tests/integration/field_rename.rs b/ts-rs/tests/integration/field_rename.rs
index 007850f..4abeab6 100644
--- a/ts-rs/tests/integration/field_rename.rs
+++ b/ts-rs/tests/integration/field_rename.rs
@@ -3,13 +3,22 @@
 use ts_rs::TS;
 
 #[derive(TS)]
+#[cfg_attr(feature = "serde-compat", derive(serde::Serialize, serde::Deserialize))]
 struct Rename {
-    a: i32,
+    #[cfg_attr(
+        feature = "serde-compat",
+        serde(rename = "c", skip_serializing_if = "String::is_empty")
+    )]
+    a: String,
     #[ts(rename = "bb")]
     b: i32,
 }
 
 #[test]
 fn test() {
-    assert_eq!(Rename::inline(), "{ a: number, bb: number, }")
+    if (cfg!(feature = "serde-compat")) {
+        assert_eq!(Rename::inline(), "{ c: string, bb: number, }")
+    } else {
+        assert_eq!(Rename::inline(), "{ a: string, bb: number, }")
+    }
 }