Bug: Type Parameters are not imported

[NyxCode]

No description provided.

[NyxCode]

While deriving TS for a struct, whenever we get to a field, we currently add that field to Dependencies.
However, we additionally need to add all generic parameters of that field to Dependencies as well.

To do that, we could just use the type we're given, and get the generic parameters out of it.
So in Dependencies::push, we'd do

pub fn push(&mut self, ty: &Type) {
    self.0.push(quote![.push::<#ty>()]);
    let generics: Vec<Type> = extract_generics(ty);
    generics.for_each(|g| self.push(g);
}

HOWEVER, that would immediately break if type aliases are used:

type MyAlias = Vec<TypeA>;
struct TypeB {
    a: MyAlias
}

Here, we only pass MyAlias to Dependencies::push, and there are no generics on that.

---

So, instead of that, what Dependencies::push should do is something like this:

pub fn push(&mut self, ty: &Type) {
    self.0.push(quote![.push::<#ty>()]);
    self.0.push(quote![
        .extend(<#ty as ts_rs::TS>::generics())
    ]);
}

For that, we'd need this method fn generics() -> impl TypeList on TS. This is what I referred to in the discussion as TS::name_dependency_types(). It would return a list of all used generics, so Vec<String> would return [String].

In other words, TS::generics() are the types needed for TS::name(), while TS::dependencies() are the types needed for TS::inline().

---

Entirely possible that i'm missing a simpler solution here, but I think that's the right approach.

[NyxCode]

@escritorio-gustavo I've implemented that fix here. The example still fails, but that issue might be orthogonal.

EDIT: I think that test still fails because I haven't yet updated the impls for Vec, Option, and - which causes the error in that case, Box

[escritorio-gustavo]

I've implemented that fix here

Nice! I spent the last 30 minutes trying to get this to work before realizing there was the TyoeList::push method lol

fn generate_generics_fn(generics: &Generics) -> TokenStream {
    let generic_types = generics
        .type_params()
        .map(|ty| ty.ident.clone())
        .collect::<Vec<_>>();

    if generic_types.is_empty() {
        return quote!{
            fn generics() -> impl ts_rs::typelist::TypeList {}
        };
    }

    let generic_types = generic_types
        .iter()
        .fold(
            quote!(),
            |acc, cur| quote!{
                (std::marker::PhantomData<#cur>, #acc)
            }
        ); 
        
    quote!{
        fn generics() -> impl ts_rs::typelist::TypeList {
            #generic_types
        }
    }
}

[NyxCode]

Alright, CI passes. (Meaning that our test coverage still isn't ideal ^^).

I haven't really touched the impls for Result, Option, etc., since they are, as you nicely outlined in the discussion, wonky.

[escritorio-gustavo]

Great! At least our underlying problem with generics is fixed! Now we just have to decide on how to even handle the impls I mentioned in the discussion

[escritorio-gustavo]

I think we have a new problem...

#[derive(TS)]
#[ts(export)]
struct Test {}

#[derive(TS)]
#[ts(export)]
enum MyOption<T> {
    Some(T),
    None
}

#[derive(TS)]
#[ts(export)]
struct Outer {
    a: MyOption<Test>
}

Outer.ts now generates correctly, but MyOption.ts

// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually.
import type { Test } from "./Test";

export type MyOption<T> = { "Some": T } | "None";

It now imports Test for no reason

[NyxCode]

I think we have a new problem...

#[derive(TS)]
#[ts(export)]
struct Test {}

#[derive(TS)]
#[ts(export)]
enum MyOption<T> {
    Some(T),
    None
}

#[derive(TS)]
#[ts(export)]
struct Outer {
    a: MyOption<Test>
}

Outer.ts now generates correctly, but MyOption.ts

// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually.
import type { Test } from "./Test";

export type MyOption<T> = { "Some": T } | "None";

It now imports Test for no reason

Ah :D
I think we had a similar issue before. Could you check if that issue is actually new?
As far as I can tell, this is caused by both Outer and MyOption wanting to write MyOption.ts. Here, it seems like Outer won, and got to export MyOption<Test>, which depends on Test.

Gotta look into that a bit deeper. Definetely something we want to fix (I even enabled errors for unused imports in the CI), but at least it's not as bad as a missing import.

[escritorio-gustavo]

I think I found out why. The expansion of Outer's #[derive(TS)] gives this:

#[allow(clippy::unused_unit)]
fn dependency_types() -> impl ts_rs::typelist::TypeList
where
    Self: 'static,
{
    {
        use ts_rs::typelist::TypeList;
        ().push::<MyOption<Test>>() // This should be MyOption<ts_rs::Dummy>
            .extend(<MyOption<Test> as ts_rs::TS>::generics())
    }
}

I have copied the expansion and replaced the type manually, which fixed MyOption.ts

[escritorio-gustavo]

As far as I can tell, this is caused by both Outer and MyOption wanting to write MyOption.ts. Here, it seems like Outer won, and got to export MyOption, which depends on Test.

Yup, seems that way to me as well

I think we had a similar issue before. Could you check if that issue is actually new?

Sure!

It hink I found out why. The expansion of Outer's #[derive(TS)] gives this:

#[allow(clippy::unused_unit)]
fn dependency_types() -> impl ts_rs::typelist::TypeList
where
    Self: 'static,
{
    {
        use ts_rs::typelist::TypeList;
        ().push::<MyOption<Test>>() // This should be MyOption<ts_rs::Dummy>
            .extend(<MyOption<Test> as ts_rs::TS>::generics())
    }
}

I have copied the expansion and replaced the type manually, which fixed MyOption.ts

If this really is the solution, I think type aliases are gonna be a problem again lol

[NyxCode]

If this really is the solution, I think type aliases are gonna be a problem again lol

True! I think we can do it a bit differently though:

Currently, we start at Outer, and recursively go through each dependency and export it. However, once we reach a type, MyOption<Test>, for example, we first generate the imports of MyOption<Test>, which includes Test.
Only then do we call MyOption<Test>::decl(), which converts it into a MyOption<SomeDummyType>.

If we had some way of first doing that conversion, and then doing the imports, we'd be fine

[NyxCode]

One approach would be to make the conversion an operation independant of TS::decl.

One way of doing this would be with an associated type. This is pretty ugly, especially since the dummy types would need to be in the public scope. With const &'static str, that'd be much much nicer

Without const &'static str, we could re-use the TypeVisitor trait. Then, decl() would look like this:

fn decl() -> String {
    struct Visit(String);
    impl TypeVisitor for Visit {
        fn visit<T: TS>(&mut self) {
            self.0 = T::decl_concrete();
        }
    }
 
    let mut visit = Visit(String::new());
    Self::with_generic(&mut visit);
    visit.0
}

Alternatively, we could also do the import generation inside decl.

[escritorio-gustavo]

Without const &'static str, we could re-use the TypeVisitor trait. Then, decl() would look like this:

fn decl() -> String {
    struct Visit(String);
    impl TypeVisitor for Visit {
        fn visit<T: TS>(&mut self) {
            self.0 = T::decl_concrete();
        }
    }
 
    let mut visit = Visit(String::new());
    Self::with_generic(&mut visit);
    visit.0
}

Okay... I'm gonna be honest, I have no idea what any of that means lol.

Alternatively, we could also do the import generation inside decl.

What would that look like?

[escritorio-gustavo]

One way of doing this would be with an associated type

So if I got this right, we'd add T::Generic to Dependencies rather than just T?

This is pretty ugly, especially since the dummy types would need to be in the public scope

ts_rs::Dummy is already public, can we just use that?

[escritorio-gustavo]

I think we had a similar issue before. Could you check if that issue is actually new?

I couldn't find this error anywhere. The closest is #168, which brings extra imports to the type itself, but not its dependencies

[NyxCode]

So if I got this right, we'd add T::Generic to Dependencies rather than just T?

No, sorry, I explained that very poorly. As far as I can tell, all dependencies and all TS impls are accurate! The problem is solely in export.rs. What's basically currently happening in export_to_string when exporting MyOption<Test> is the following:

let mut out = String::new();
generate_imports::<MyOption<Test>>(&mut out);
out.push_str(MyOption<Dummy>::decl());

So tl;dr: The type for which we generate the imports is not the type which we generate.
Why does this happen?

export_to_string generates the imports for MyOption<Test>, and then calls MyOption<Test>::decl(). But in MyOption<Test>::decl(), we swap the generic type Test for our Dummy!

Therefore, my suggestion was to swap the generic type for our Dummy type before generating the imports.

Does that make a bit more sense? Happy to expand on this a bit more if anything is still unclear.

[NyxCode]

Just confirmed that the same thing happens on main, so it's not actually related to the changes in this PR.
Pretty sure we introduced it with the last one, where we re-worked the generics handeling.

[escritorio-gustavo]

Does that make a bit more sense? Happy to expand on this a bit more if anything is still unclear.

Yeah, that makes more sense. So the T::Generics would be used on generate_imports then? Like generate_imports::<MyOption<Test>::Generics>, whcih would resolve to generate_imports::<MyOption<Dummy>>

[escritorio-gustavo]

Maybe we should rename the TS::Generics type though... I managed to get myself confused between the associated type and the method with the same name lol

[escritorio-gustavo]

@NyxCode this is my proposed fix for those types in the discussion. I have tested them against #168, even changing which types were #[ts(inline)]d and it seems to work as expected while keeping current behavior for inline

[escritorio-gustavo]

I think this also needs to be done for Range and RangeInclusive? Not sure since they don't have inline

[NyxCode]

Hey!
I see that you implemented TS::Generics using the Dummy struct - That works for that particular use-case, but it's not what I originally had in mind. I thought it'd be cool to be able to use the dummy types we currently generate in TS::decl() there, so that we could re-use that functionality.

That being said, it does work like you did here, because

• we're still using the on-the-fly generated dummy types for TS::decl and
• for imports, it doesn't matter if we're generating the imports for SomeType<Dummy, Dummy> or SomeType<A, B>.

So, if we keep it like this, I agree, we'd want to rename the associated type. Maybe TS::WithDummyGenerics?

I can also give the original idea (using the on-the-fly generated dummy types) a try to see how that'd look. Might be more complicated, but we'll see.

[NyxCode]

Now we actually see the real error since the files dont get overwritten! Seems like internally tagged enums don't like self-references.

Not sure we can do anything about that.

[escritorio-gustavo]

Looks like it doesn't like self references with type intersection

[escritorio-gustavo]

Not sure we can do anything about that.

Agreed. Self references are only allowed as property values

[NyxCode]

Oh, look at that! Someone left a comment about this xD

// NOTE: The generated type is actually not valid TS here, since the indirections rust enforces for recursive types
// gets lost during the translation to TypeScript (e.g "Box" => "T").

If that was me, then I dont remember ^^

I'll remove the #[ts(export)] then. I think it makes sense keeping the test itself though.

[escritorio-gustavo]

This either never worked (as in the TS generated was always invalid), or it is a regression caused by enum flattening support

[escritorio-gustavo]

Oh, look at that! Someone left a comment about this xD

// NOTE: The generated type is actually not valid TS here, since the indirections rust enforces for recursive types
// gets lost during the translation to TypeScript (e.g "Box" => "T").

Well that literally explains a lot xD

If that was me, then I dont remember ^^

Git blame says it was you 😆

[NyxCode]

This either never worked (as in the TS generated was always invalid), or it is a regression caused by enum flattening support

Okay, I did some thinking. This never worked in the first place, because it cant!

For one, the internally-tagged representation doesn't allow newtype/tuple variants. Serde just panics at runtime for them.
That restriction makes sense, since having an internal tag requires the variant to be an object.

"Internally tagged" means "put the tag inside the variant". But if the variant is not an object to put the tag in, this cant work.

Sensible types, like this one here work, since they don't use the type intersection:

#[derive(TS)]
#[ts(tag = "tag")]
enum X {
    A { inner: Box<Self> },
    B { inner: i32 }
}

export type X = { "tag": "A", inner: X, } | { "tag": "B", inner: number, };

[escritorio-gustavo]

That makes a lot of sense! Especially given that one of the variants was trying to do

{ "tag": "E" } & Array<I>

which is just nonsense

[escritorio-gustavo]

For one, the internally-tagged representation doesn't allow newtype/tuple variants. Serde just panics at runtime for them.
That restriction makes sense, since having an internal tag requires the variant to be an object.

Added a note on the wiki about that

[NyxCode]

Added a note on the wiki about that

Awesome!

I also managed to fix the lifetimes.rs and self_referential.rs tests by changing &T from impl_shadow to impl_wrapper

This made me curious, so I added tests for the 4 remaining types using impl_shadow! - All are broken.

[escritorio-gustavo]

I think it's because impl_shadow doesn't have TS::generics

[NyxCode]

Yep! Seems like I just skipped that (since it didnt break any tests 😆)

[escritorio-gustavo]

That's odd, in my computer TS complains that CustomKey(null) doesn't extend string | number | symbol, but CI still passes

[escritorio-gustavo]

I'll be out for a about 2 hours now, so I'll be unable to respond. See you later

[NyxCode]

That's odd, in my computer TS complains that CustomKey(null) doesn't extend string | number | symbol, but CI still passes

Interesting! Seems like that's only caught with --strict, while Record<{x: number}, Something> also fails without --strict.
I've enabled --strict for everything in CI, and fixed CustomKey. Everything works(TM) again!

---

I think it'd be alright if we merge this. It's getting annoying to scroll through this huge PR.
Is there anything you think still needs to be done here?

Before the next release, I'd like to

• see if we can remove TS::transparent()
• Maybe make the recusive exporting of dependencies we added in Introduce TypeList, always export Dependencies as well #221 configurable?
  • While I can't think of a use-case where this would be undesirable, I think it's the change most likely to break existing setups.
    • Are there any other features you can think of we might want to make configurable for that reason?

---

Thank you so much for your work, it has been invaluable! I'm super stoked to see where this library is going.
Stuff just works(tm) now, especially inlining, other enum representations and generics. And now we've also increased the test suite by 160 .ts files. Just awesome stuff 😆

[escritorio-gustavo]

I think it'd be alright if we merge this. It's getting annoying to scroll through this huge PR.

Yup, this PR is way too big already

Is there anything you think still needs to be done here?

While adding #[ts(export)] to everything, I noticed we don't have a #[ts(...)] equivalent to #[serde(tag = "...")] for structs, so I'd like to add that in a new PR.

• see if we can remove TS::transparent()

Definetly something I'd like to see!

• Maybe make the recusive exporting of dependencies we added in Introduce TypeList, always export Dependencies as well #221 configurable?

Cool! Would that be a feature flag or a new attribute?

• I think it's the change most likely to break existing setups.
  • Are there any other features you can think of we might want to make configurable for that reason?

I don't see much of a problem, as long as the next release is 8.0.0 to indicate there are breaking changes.

---

Thank you so much for your work, it has been invaluable! I'm super stoked to see where this library is going.

Thanks! It's awesome to be able to help out with a project that I use a lot!