Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement generic inlining with trait bounds #217

Closed
wants to merge 2 commits into from
Closed

Implement generic inlining with trait bounds #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ad556a
Implement generic inlining with trait bounds
escritorio-gustavo Jan 30, 2024
b4027dc
Merge branch 'main' into fix_inline_generics_trait_bound
escritorio-gustavo Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion macros/src/attr/enum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use syn::{Attribute, Ident, Result};
use syn::{Attribute, Ident, Result, Path};

use crate::{
attr::{parse_assign_inflection, parse_assign_str, Inflection},
Expand All @@ -11,6 +11,7 @@ pub struct EnumAttr {
pub rename: Option<String>,
pub export_to: Option<String>,
pub export: bool,
pub unit_type: Option<Path>,
tag: Option<String>,
untagged: bool,
content: Option<String>,
Expand Down Expand Up @@ -59,6 +60,7 @@ impl EnumAttr {
untagged,
export_to,
export,
unit_type,
}: EnumAttr,
) {
self.rename = self.rename.take().or(rename);
Expand All @@ -68,13 +70,17 @@ impl EnumAttr {
self.content = self.content.take().or(content);
self.export = self.export || export;
self.export_to = self.export_to.take().or(export_to);
self.unit_type = self.unit_type.take().or(unit_type);
}
}

impl_parse! {
EnumAttr(input, out) {
"rename" => out.rename = Some(parse_assign_str(input)?),
"rename_all" => out.rename_all = Some(parse_assign_inflection(input)?),
"unit_type" => out.unit_type = Some(
parse_assign_str(input).and_then(|x| syn::parse_str::<Path>(&x))?
),
"export_to" => out.export_to = Some(parse_assign_str(input)?),
"export" => out.export = true
}
Expand Down
8 changes: 7 additions & 1 deletion macros/src/attr/struct.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::convert::TryFrom;

use syn::{Attribute, Ident, Result};
use syn::{Attribute, Ident, Result, Path};

use crate::{
attr::{parse_assign_str, Inflection, VariantAttr},
Expand All @@ -14,6 +14,7 @@ pub struct StructAttr {
pub export_to: Option<String>,
pub export: bool,
pub tag: Option<String>,
pub unit_type: Option<Path>,
}

#[cfg(feature = "serde-compat")]
Expand All @@ -37,13 +38,15 @@ impl StructAttr {
export,
export_to,
tag,
unit_type,
}: StructAttr,
) {
self.rename = self.rename.take().or(rename);
self.rename_all = self.rename_all.take().or(rename_all);
self.export_to = self.export_to.take().or(export_to);
self.export = self.export || export;
self.tag = self.tag.take().or(tag);
self.unit_type = self.unit_type.take().or(unit_type);
}
}

Expand All @@ -66,6 +69,9 @@ impl_parse! {
StructAttr(input, out) {
"rename" => out.rename = Some(parse_assign_str(input)?),
"rename_all" => out.rename_all = Some(parse_assign_str(input).and_then(Inflection::try_from)?),
"unit_type" => out.unit_type = Some(
parse_assign_str(input).and_then(|x| syn::parse_str::<Path>(&x))?
),
"export" => out.export = true,
"export_to" => out.export_to = Some(parse_assign_str(input)?)
}
Expand Down
7 changes: 4 additions & 3 deletions macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
#![deny(unused)]

use proc_macro2::{Ident, TokenStream};
use quote::{format_ident, quote};
use quote::{format_ident, quote, ToTokens};
use syn::{
parse_quote, spanned::Spanned, ConstParam, GenericParam, Generics, Item, LifetimeParam, Result,
TypeParam, WhereClause,
TypeParam, WhereClause, Path,
};

use crate::deps::Dependencies;
Expand All @@ -22,6 +22,7 @@ struct DerivedTS {
decl: TokenStream,
inline_flattened: Option<TokenStream>,
dependencies: Dependencies,
unit_type: Option<Path>,

export: bool,
export_to: Option<String>,
Expand All @@ -34,7 +35,7 @@ impl DerivedTS {
.params
.iter()
.filter(|param| matches!(param, GenericParam::Type(_)))
.map(|_| quote! { () });
.map(|_| self.unit_type.as_ref().map(|x| x.to_token_stream()).unwrap_or(quote!{ () }));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This here looks interesting!
What happens with something like this, when there are multiple generic parameters with different bounds?

#[ts(export)]
struct X<A: ToString, B: IntoIterator<Item = u32>> {
    a: A,
    b: B
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type given to #[ts(unit_type = "...")] will be used for all generics, same as (), so it must implement all trait bounds from all of the generic parameters

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, makes sense! Just an idea here: What if the attribute didnt only parse one type, but a list of types? That should just work, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the attribute didnt only parse one type, but a list of types?

What do you mean? One type per generic?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah!

Copy link
Contributor Author

@escritorio-gustavo escritorio-gustavo Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, we'd get rid of the hardcoded "null", and a user could specify any "default generic type", without the need to declare his own "unit" type.

#[derive(TS)]
#[ts(default_generics = "String, Box<dyn Error>")]
struct Something<S: AsRef<str>, E: Error>();

But: TypeId::of requires 'static, and idk if it's even possible to work around that in this case.

Yeah, I tried that when I was making the first PR about inlining generics and couldn't get past this, but the bigger problem is, if the user actually wanted at some point to use the struct with the default generic (String in this example), inlining the generic would fail, emiiting S instead of string

Copy link
Contributor Author

@escritorio-gustavo escritorio-gustavo Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the user actually wanted at some point to use the struct with the default generic (String in this example), inlining the generic would fail, emiiting S instead of String

What I mean here is:

#[derive(TS)]
#[ts(default_generics = "String")]
pub struct Generic<T: ToString> {
    pub x: T,
}

#[derive(TS)]
pub struct Foo {
    #[ts(inline)]
    pub a: Generic<String>, // This will fail, will emit `T` instead of `string` because String == String

   #[ts(inline)]
    pub b: Generic<i32>, // This will not fail because i32 != String

   #[ts(inline)]
    pub b: Generic<&'static str>, // This will not fail because &str != String
}

This generates

type Foo = { a: { x: T, }, b: { x: number, }, c: { x: string, }, }
//                   ^ should be `string`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative is, when deciding whether to use T or string, we need to know if the generic passed to the field is the identifier of one of the struct definition generics or an actual type.
Don't know how we could get this information though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: I think I made it work. Only hacked something together, but it does pass the tests. It's not as robust as the TypeId solution before, but it might be preferable to the "null" solution - i dont know yet. Here's how it looks like. What do you think?

return quote!(
    if std::any::type_name::<#generic_ident>() == std::any::type_name::<#unit_type>() {
        #generic_ident_str.to_owned()
    } else {
        <#generic_ident>::inline()
    }
);

If that's something you'd like to look in further, I'd be happy to clean my hacky solution up a bit & push it to a branch for you to check out.

This looks interesting, let's take a look!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I tried that when I was making the first PR about inlining generics and couldn't get past this, but the bigger problem is, if the user actually wanted at some point to use the struct with the default generic (String in this example), inlining the generic would fail, emiiting S instead of string

Yeah, I think you're right. Let's scratch that idea.

Another alternative is, when deciding whether to use T or string, we need to know if the generic passed to the field is the identifier of one of the struct definition generics or an actual type.
Don't know how we could get this information though

This might be worth exploring. I'll play with that idea if I find some time later. I do have the feeling though that we'd have to make a somewhat big change to the TS trait, so I'm not sure yet if it'd be worth it.

let ty = quote!(<#rust_ty<#(#generic_params),*> as ts_rs::TS>);

Some(quote! {
Expand Down
3 changes: 3 additions & 0 deletions macros/src/types/enum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub(crate) fn r#enum_def(s: &ItemEnum) -> syn::Result<DerivedTS> {
decl: quote!("type {} = never;"),
inline_flattened: None,
dependencies: Dependencies::default(),
unit_type: enum_attr.unit_type,
export: enum_attr.export,
export_to: enum_attr.export_to,
});
Expand All @@ -55,6 +56,7 @@ pub(crate) fn r#enum_def(s: &ItemEnum) -> syn::Result<DerivedTS> {
)),
dependencies,
name,
unit_type: enum_attr.unit_type,
export: enum_attr.export,
export_to: enum_attr.export_to,
})
Expand Down Expand Up @@ -195,6 +197,7 @@ fn empty_enum(name: impl Into<String>, enum_attr: EnumAttr) -> DerivedTS {
name,
inline_flattened: None,
dependencies: Dependencies::default(),
unit_type: enum_attr.unit_type,
export: enum_attr.export,
export_to: enum_attr.export_to,
}
Expand Down
4 changes: 0 additions & 4 deletions macros/src/types/generics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,6 @@ pub fn format_type(ty: &Type, dependencies: &mut Dependencies, generics: &Generi
let generic_ident = generic.ident.clone();
let generic_ident_str = generic_ident.to_string();

if !generic.bounds.is_empty() {
return quote!(#generic_ident_str.to_owned());
}

return quote!(
match <#generic_ident>::inline().as_str() {
// When exporting a generic, the default type used is `()`,
Expand Down
1 change: 1 addition & 0 deletions macros/src/types/named.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ pub(crate) fn named(
inline_flattened: Some(quote!(format!("{{ {} }}", #fields))),
name: name.to_owned(),
dependencies,
unit_type: attr.unit_type.clone(),
export: attr.export,
export_to: attr.export_to.clone(),
})
Expand Down
1 change: 1 addition & 0 deletions macros/src/types/newtype.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ pub(crate) fn newtype(
inline_flattened: None,
name: name.to_owned(),
dependencies,
unit_type: attr.unit_type.clone(),
export: attr.export,
export_to: attr.export_to.clone(),
})
Expand Down
1 change: 1 addition & 0 deletions macros/src/types/tuple.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ pub(crate) fn tuple(
inline_flattened: None,
name: name.to_owned(),
dependencies,
unit_type: attr.unit_type.clone(),
export: attr.export,
export_to: attr.export_to.clone(),
})
Expand Down
3 changes: 3 additions & 0 deletions macros/src/types/unit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ pub(crate) fn empty_object(attr: &StructAttr, name: &str) -> Result<DerivedTS> {
inline_flattened: None,
name: name.to_owned(),
dependencies: Dependencies::default(),
unit_type: attr.unit_type.clone(),
export: attr.export,
export_to: attr.export_to.clone(),
})
Expand All @@ -26,6 +27,7 @@ pub(crate) fn empty_array(attr: &StructAttr, name: &str) -> Result<DerivedTS> {
inline_flattened: None,
name: name.to_owned(),
dependencies: Dependencies::default(),
unit_type: attr.unit_type.clone(),
export: attr.export,
export_to: attr.export_to.clone(),
})
Expand All @@ -40,6 +42,7 @@ pub(crate) fn null(attr: &StructAttr, name: &str) -> Result<DerivedTS> {
inline_flattened: None,
name: name.to_owned(),
dependencies: Dependencies::default(),
unit_type: attr.unit_type.clone(),
export: attr.export,
export_to: attr.export_to.clone(),
})
Expand Down
40 changes: 21 additions & 19 deletions ts-rs/tests/generics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ use std::{

use ts_rs::TS;

#[derive(Debug, Clone, Copy, PartialEq, PartialOrd, TS)]
struct Unit;
impl ToString for Unit {
fn to_string(&self) -> String {
"".to_owned()
}
}

#[derive(TS)]
struct Generic<T>
where
Expand Down Expand Up @@ -170,12 +178,9 @@ fn inline() {
}

#[test]
#[ignore = "We haven't figured out how to inline generics with bounds yet"]
#[allow(unreachable_code)]
fn inline_with_bounds() {
todo!("FIX ME: https://github.com/Aleph-Alpha/ts-rs/issues/214");

#[derive(TS)]
#[ts(unit_type = "Unit")]
struct Generic<T: ToString> {
t: T,
}
Expand All @@ -190,15 +195,11 @@ fn inline_with_bounds() {
#[ts(flatten)]
t: Generic<u32>,
}

assert_eq!(
Generic::<&'static str>::decl(),
"type Generic<T> = { t: T, }"
);
// ^^^^^^^^^^^^ Replace with something else

assert_eq!(Generic::<Unit>::decl(), "type Generic<T> = { t: T, }");
assert_eq!(
Container::decl(),
"type Container = { g: Generic<string>, gi: { t: string, }, t: number, }" // Actual output: { g: Generic<string>, gi: { t: T, }, t: T, }
"type Container = { g: Generic<string>, gi: { t: string, }, t: number, }"
);
}

Expand Down Expand Up @@ -262,35 +263,36 @@ fn default() {
#[test]
fn trait_bounds() {
#[derive(TS)]
#[ts(unit_type = "Unit")]
struct A<T: ToString = i32> {
t: T,
}
assert_eq!(A::<i32>::decl(), "type A<T = number> = { t: T, }");
assert_eq!(A::<Unit>::decl(), "type A<T = number> = { t: T, }");

#[derive(TS)]
#[ts(unit_type = "Unit")]
struct B<T: ToString + Debug + Clone + 'static>(T);
assert_eq!(B::<&'static str>::decl(), "type B<T> = T;");
assert_eq!(B::<Unit>::decl(), "type B<T> = T;");

#[derive(TS)]
#[ts(unit_type = "Unit")]
enum C<T: Copy + Clone + PartialEq, K: Copy + PartialOrd = i32> {
A { t: T },
B(T),
C,
D(T, K),
}
assert_eq!(
C::<&'static str, i32>::decl(),
C::<Unit, Unit>::decl(),
r#"type C<T, K = number> = { "A": { t: T, } } | { "B": T } | "C" | { "D": [T, K] };"#
);

#[derive(TS)]
#[ts(unit_type = "Unit")]
struct D<T: ToString, const N: usize> {
t: [T; N],
}

let ty = format!(
"type D<T> = {{ t: [{}], }}",
"T, ".repeat(41).trim_end_matches(", ")
);
assert_eq!(D::<&str, 41>::decl(), ty)
let ty = format!("type D<T> = {{ t: [{}], }}", "T, ".repeat(41).trim_end_matches(", "));
assert_eq!(D::<Unit, 41>::decl(), ty)
}
Loading