ts-rs icon indicating copy to clipboard operation
ts-rs copied to clipboard

bug: Deriving `TS` should not fail for empty structs in the presence of a `#[serde(rename_all)]` attribute

Open regexident opened this issue 8 months ago • 3 comments

Describe the bug

The implementation of fn check_attributes(…) is overly strict, resulting in undesirable compilation failures for totally valid Rust code.

https://github.com/Aleph-Alpha/ts-rs/blob/40b82771c8b63c986d6fcf8a71540d45816418c2/macros/src/types/unit.rs#L64-L74

If at all, then the TS derive macro should emit a warning for empty structs with rename_all attributes.

Preferably neither an error, nor a warning should be emitted, since there are many valid reasons for adding serde attributes to types, even if they might not have an actual effect in certain scenarios. As such the type might become empty on certain platforms or only be non-empty given a certain feature. The current overzealous behavior of ts_rs::TS prohibits those valid use-cases.

To Reproduce

Given the following code, which compiles just fine …

use serde::Serialize;

#[derive(Serialize)]
#[serde(rename_all = "camelCase")]
pub struct EmptyDto {}

Add a #[derive(ts_rs::TS)] to EmptyDto and the code (despite still being 100% valid) will suddenly fail to compile with the following error:

error: `rename_all` is not applicable to unit structs

Expected behavior

… the code should still compile fine, since it is still valid.

To Fix

Just delete the following offending lines from fn check_attributes(…):

if attr.rename_all.is_some() { 
    syn_err!("`rename_all` is not applicable to unit structs"); 
}

Screenshots

n/a

Version

10.1

Additional context

n/a

regexident avatar May 11 '25 19:05 regexident

Hey @regexident, thanks for pointing this out!

I think it still makes sense to forbid rename_all for the cases of struct Foo();, struct Foo(T, U, ...) and struct Foo;, but struct Foo {} should be valid, because, as you said, it could just be a feature flag that makes the type empty, I'll make a PR for that

gustavo-shigueo avatar May 11 '25 22:05 gustavo-shigueo

To Fix

Just delete the following offending lines from fn check_attributes(…):

if attr.rename_all.is_some() { syn_err!("rename_all is not applicable to unit structs"); }

We can actually go further, StructAttr::assert_validity already contains these checks and already allows struct Foo {} to use rename_all, we can just delete check_attributes all together

gustavo-shigueo avatar May 11 '25 22:05 gustavo-shigueo

I had to deal with this issue a few weeks ago, I found a workaround, but it was annoying, since I generate the structs with macros. So, thanks to post this issue, and thanks for the fix.

eythaann avatar May 17 '25 22:05 eythaann