bug: Deriving `TS` should not fail for empty structs in the presence of a `#[serde(rename_all)]` attribute
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
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
To Fix
Just delete the following offending lines from
fn check_attributes(…):if attr.rename_all.is_some() { syn_err!("
rename_allis 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
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.