Bug: Flattening generic type also flattens all generic parameters #233
Conversation
|
Same thing happens if |
NyxCode
changed the title
|
ts-rs/macros/src/types/generics.rs Lines 66 to 77 in ed9abd3 Seems to be the issue. Changing |
|
Every other test seems to be fine though |
|
Ah, I see, thanks! |
|
This is a very annoying bug. It's enterely caused by what I did in #215 because |
|
Hey @NyxCode, I'm at home right now, so I don't have access to my work account (@escritorio-gustavo). return quote!(
match <#generic_ident>::name().as_str() {
"null" => #generic_ident_str.to_owned(),
x if !std::any::type_name::<#generic_ident>().contains('<') => {
x.to_owned()
}
_ => <#generic_ident>::inline()
}
);I am not sure if it is a good idea or if it breaks other stuff though |
Just confirming, this was actually me |
|
If the macro could somehow read the value of |
This somewhat improves the situation, but still generates more inlining than expected. That is: #[derive(TS)]
struct OtherType<T>(T);
#[derive(TS)]
struct SomeType<T>(OtherType<T>);
#[derive(TS)]
enum MyEnum<A, B> {
VariantA(A),
VariantB(B)
}
#[derive(TS)]
struct Parent {
e: MyEnum<i32, i32>,
#[ts(inline)]
e1: MyEnum<i32, SomeType<String>>
}
// This fails!
// The #[ts(inline)] seems to inline recursively, so not only the definition of `MyEnum`, but
// also the definition of `SomeType`.
assert_eq!(
Parent::decl(),
"type Parent = { \
e: MyEnum<number, number>, \
e1: { \"VariantA\": number } | { \"VariantB\": SomeType<string> }, \
}"
);Will fail, giving |
|
I think it is no longer recursively inlining, but instead, it inlines one level deeper than it should. I'm really not sure though, especially because, the failure I described is the same as current behavior, even though the new test somehow passes |
import type { LibraryType2<LibraryType2<ConsumerType>> } from "./LibraryType2";
import type { LibraryType2<LibraryType2<LibraryType1>> } from "./LibraryType2";whoops 😆 |
|
Not only are the import names broken, but our original logic of de-duping them doesn't work anymore (did it ever work with generics?). Lines 191 to 195 in 5daeed2 |
|
So there's something left to fix (besides adding tests) after all. |
|
Don't worry about it! I've already left work for today, so I'll take a good look at it tomorrow |
I remember implementing some kind of hack to try and deal with that in #235, I'll check what I was and make it less hacky |
I've seen that! Seemed like a reasonable solution, though we could also add a |
|
Ohh, I'm blind! I thought that was a |
| /// | ||
| /// If this type is generic, then all provided generic parameters will be swapped for | ||
| /// placeholders, resulting in a generic typescript definition. | ||
| /// Both `SomeType::<i32>::decl()` and `SomeType::<String>::decl()` will therefore result in | ||
| /// the same TypeScript declaration `type SomeType<A> = ...`. | ||
| fn decl() -> String { | ||
| panic!("{} cannot be declared", Self::name()); | ||
| } | ||
|
|
||
| /// Declaration of this type using the supplied generic arguments. | ||
| /// The resulting TypeScript definition will not be generic. For that, see `TS::decl()`. | ||
| /// If this type is not generic, then this function is equivalent to `TS::decl()`. | ||
| fn decl_concrete() -> String { | ||
| panic!("{} cannot be declared", Self::name()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Here's the user-visible effect of this change.
For
struct Generic<A, B> { a: A, b: B }Generic::<i32, String>::decl_concrete() returns type Generic = { a: i32, b: String };.
Generic::<i32, String>::decl() swaps out the generic parameters i32 and String out for on-the-fly generated dummy types A and B, resulting in type Generic<A, B> = { a: A, b: B };
macros/src/lib.rs
Outdated
| /// Generate a dummy unit struct for every generic type parameter of this type. | ||
| /// Example: | ||
| /// ```ignore | ||
| /// struct Generic<A, B, const C: usize> { /* ... */ } | ||
| /// ``` | ||
| /// has two generic type parameters, `A` and `B`. This function will therefor generate | ||
| /// ```ignore | ||
| /// struct A; | ||
| /// impl ts_rs::TS for A { /* .. */ } | ||
| /// | ||
| /// struct B; | ||
| /// impl ts_rs::TS for B { /* .. */ } | ||
| /// ``` | ||
| fn generate_generic_types(&self) -> TokenStream { | ||
| let generics = self.generics.params.iter().filter_map(|g| match g { | ||
| GenericParam::Lifetime(_) => None, | ||
| GenericParam::Type(t) => Some(t.ident.clone()), | ||
| GenericParam::Const(_) => None, | ||
| }); | ||
|
|
||
| quote! { | ||
| #( | ||
| #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)] | ||
| struct #generics; | ||
| impl std::fmt::Display for #generics { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| write!(f, "{:?}", self) | ||
| } | ||
| } | ||
| impl TS for #generics { | ||
| fn name() -> String { stringify!(#generics).to_owned() } | ||
| fn transparent() -> bool { false } | ||
| } | ||
| )* | ||
| } | ||
| } |
There was a problem hiding this comment.
Here, these dummy types are generated, named the same as the generic parameters.
I thought about only defining one dummy type, so e.g #[docs(hidden)] pub struct Dummy<const NAME: &'static str>.
However, const-generics don't work with strings yet. Maybe something for the future to improve compile times.
macros/src/lib.rs
Outdated
| /// Generates the `decl()` and `decl_concrete()` methods. | ||
| /// `decl_concrete()` is simple, and simply defers to `inline()`. | ||
| /// For `decl()`, however, we need to change out the generic parameters of the type, replacing | ||
| /// them with the dummy types generated by `generate_generic_types()`. | ||
| fn generate_decl_fn(&mut self, rust_ty: &Ident) -> TokenStream { | ||
| let name = &self.ts_name; | ||
| let generic_types = self.generate_generic_types(); | ||
| let ts_generics = format_generics(&mut self.dependencies, &self.generics); | ||
| // These are the generic parameters we'll be using. | ||
| let generic_idents = self.generics.params.iter().filter_map(|p| match p { | ||
| GenericParam::Lifetime(_) => None, | ||
| // Since we named our dummy types the same as the generic parameters, we can just keep | ||
| // the identifier of the generic parameter - its name is shadowed by the dummy struct. | ||
| GenericParam::Type(TypeParam { ident, .. }) => Some(quote!(#ident)), | ||
| // We keep const parameters as they are, since there's no sensible default value we can | ||
| // use instead. This might be something to change in the future. | ||
| GenericParam::Const(ConstParam { ident, .. }) => Some(quote!(#ident)), | ||
| }); | ||
| quote! { | ||
| fn decl_concrete() -> String { | ||
| format!("type {} = {};", #name, Self::inline()) | ||
| } | ||
| fn decl() -> String { | ||
| #generic_types | ||
| let inline = <#rust_ty<#(#generic_idents,)*> as ts_rs::TS>::inline(); | ||
| let generics = #ts_generics; | ||
| format!("type {}{generics} = {inline};", #name) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
And finally, decl and decl_concrete are implemented here.
decl first creates the dummy types, shadowing the names of the generics (#generic_types). Then, I just call inline on that type.
| format_type(&syn::parse_str::<Type>(&type_as)?, dependencies, generics) | ||
| let ty = syn::parse_str::<Type>(&type_as)?; | ||
| quote!(<#ty as ts_rs::TS>::name()) |
There was a problem hiding this comment.
This is just one example of how every use of generics::format_type got replaced with TS::name.
|
Hey @NyxCode! Is there anything else you'd like to change with this PR before merging? |
|
Hey @escritorio-gustavo ! |
Agreed. I'm thinking we could have a separate PR that
This should hopefully increase our certainty about dependencies (I'm pretty sure #168 still needs to be solved though)
Besides dependencies, what else do you think needs more thorough testing? |
|
It seems like This idea seems ridiculous, but think about it: But notice that The problem is instead of stating this to be illegal (or just the same as calling |
|
The same is true for In fact, if |
|
TL;DR I think we should change the
|
|
Alternatively, to keep current behavior, we have to recursively search for a type that isn't of ot those I mentioned (or their |
|
@NyxCode I'll merge this PR and open a discussion for us to try and figure out how to best deal with all of these other issues |
|
The discussion link: #239 |
Hey @escritorio-gustavo!
While trying to understand #232, I encountered this issue here. I'm not yet sure if it's the underlying cause, but maybe we should start with this.
Flattening an generic enum currently seems to not only flatten the enum, but also everything inside.
I've added a test to highlight this behaviour.
Any ideas what's going on here?