Repository: Aleph-Alpha/ts-rs
Pull request #287 (2024-03-28 14:03:04)

Fix `#[ts(rename_all_fields = "...")]` on enums containing tuple or unit variants
## Goal

Using `#[ts(rename_all_fields = "...")]` on an enum containing tuple or unit variants would cause the error "`rename_all` is not applicable to newtype/tuple/unit structs" 

## Changes

Only add `rename_all` from `rename_all_fields` if `variant.fields` is `Fields::Named(_)`

## 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.


NyxCode: To summarize my understanding:  
The problem is that when creating `VariantAttr`s in `VariantAttr::new`, we use the `rename_all_fields` of the enum as value for `rename_all`. 
These `VariantAttr`s are then converted into `StructAttr`s, and when they reach `newtype`, we get the error.

There are a couple of thinks that stood out to me.  
This basically duplicates `VariantAttr::new`, though we could fix that by re-writing `VariantAttr::new` to use `VariantAttr::from_attrs` internally.   
Also, the `match`, deciding between `VariantAttr::new` and `VariantAttr::from_attrs` really doesn't feel great.

An alternative way of getting rid of the error would be to remove the check from `newtype`. It probably wouldn't be a big deal to just ignore it if it's there, to be honest. 

If we wanted to keep the error, we could alternatively move these sanity checks, which we have in a lot of places, into the attribute parsing logic. But again, we'd then need to know what we're parsing (a unit struct?). So that'd probably be a larger refactor, and I'm not sure if it'd end up being nice.

So yeah, I'm not super satisfied with this solution, but I kinda don't see a better way of doing it right now.  
If we're going with this, I think we should definetely add a comment to the `match`, and re-write `VariantAttr::new` to use `VariantAttr::from_attrs` to not have that logic duplicated.
escritorio-gustavo: > This basically duplicates `VariantAttr::new`, though we could fix that by re-writing `VariantAttr::new` to use `VariantAttr::from_attrs` internally.

Agreed! I'll push a commit to do that

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5230b87..c58c69a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,9 +1,14 @@
 # master
+
 ### Breaking
+
 ### Features
+
 - Add support for `#[ts(type = "..")]` directly on structs and enums ([#286](https://github.com/Aleph-Alpha/ts-rs/pull/286))
+
 ### Fixes
 
+Fix `#[ts(rename_all_fields = "...")]` on enums containing tuple or unit variants ([#287](https://github.com/Aleph-Alpha/ts-rs/pull/287))
 
 # 8.1.0
 
diff --git a/macros/src/attr/variant.rs b/macros/src/attr/variant.rs
index c6db337..cca9532 100644
--- a/macros/src/attr/variant.rs
+++ b/macros/src/attr/variant.rs
@@ -21,9 +21,14 @@ pub struct SerdeVariantAttr(VariantAttr);
 
 impl VariantAttr {
     pub fn new(attrs: &[Attribute], enum_attr: &EnumAttr) -> Result<Self> {
+        let mut result = Self::from_attrs(attrs)?;
+        result.rename_all = result.rename_all.or(enum_attr.rename_all_fields);
+        Ok(result)
+    }
+
+    pub fn from_attrs(attrs: &[Attribute]) -> Result<Self> {
         let mut result = Self::default();
         parse_attrs(attrs)?.for_each(|a| result.merge(a));
-        result.rename_all = result.rename_all.or(enum_attr.rename_all_fields);
         #[cfg(feature = "serde-compat")]
         if !result.skip {
             crate::utils::parse_serde_attrs::<SerdeVariantAttr>(attrs)
diff --git a/macros/src/types/enum.rs b/macros/src/types/enum.rs
index a205ce2..afaa8a1 100644
--- a/macros/src/types/enum.rs
+++ b/macros/src/types/enum.rs
@@ -76,7 +76,14 @@ fn format_variant(
     variant: &Variant,
 ) -> syn::Result<()> {
     let crate_rename = enum_attr.crate_rename();
-    let variant_attr = VariantAttr::new(&variant.attrs, enum_attr)?;
+
+    // If `variant.fields` is not a `Fields::Named(_)` the `rename_all_fields`
+    // attribute must be ignored to prevent a `rename_all` from getting to
+    // the newtype, tuple or unit formatting, which would cause an error
+    let variant_attr = match variant.fields {
+        Fields::Unit | Fields::Unnamed(_) => VariantAttr::from_attrs(&variant.attrs)?,
+        Fields::Named(_) => VariantAttr::new(&variant.attrs, enum_attr)?,
+    };
 
     if variant_attr.skip {
         return Ok(());


Test patch:
diff --git a/ts-rs/tests/enum_struct_rename_all.rs b/ts-rs/tests/enum_struct_rename_all.rs
index bbc798f..b43edcb 100644
--- a/ts-rs/tests/enum_struct_rename_all.rs
+++ b/ts-rs/tests/enum_struct_rename_all.rs
@@ -44,12 +44,16 @@ pub enum TaskStatus2 {
         stdout: String,
         stderr: String,
     },
+
+    A(i32),
+    B(i32, i32),
+    C,
 }
 
 #[test]
 pub fn enum_struct_rename_all_fields() {
     assert_eq!(
         TaskStatus2::inline(),
-        r#"{ "Running": { "started-time": string, } } | { "Terminated": { status: number, stdout: string, stderr: string, } }"#
+        r#"{ "Running": { "started-time": string, } } | { "Terminated": { status: number, stdout: string, stderr: string, } } | { "A": number } | { "B": [number, number] } | "C""#
     )
 }