Prevent empty Dependencies from being added to dependency_types

[escritorio-gustavo]

Goal

A type with 128+ variants/fields will cause a massive TypeList to be generated, even if all of its items are (). This can cause an overflow before the type actually has 128+ dependencies
Closes #289

Changes

I added a check to Dependencies::append so that it only appends if other.dependencies is not empty

Checklist

• I have followed the steps listed in the Contributing guide.
• If necessary, I have added documentation related to the changes made.
• I have added or updated the tests related to the changes made.

[NyxCode]

Oh, that's great! Didn't think that was enough to get #289 fixed!

Do you see any limitations with this as-is? What would we need to provoke the error now?

[escritorio-gustavo]

Do you see any limitations with this as-is? What would we need to provoke the error now?

I think we actually need 128+ dependencies on the same type, but I'm not sure about that yet

Scratch that. Just make all the variants Variant(String) lol

[NyxCode]

I think we should then merge this fix, and think about more complicated solutions once someone has a usecase for such an abomination 😅

[escritorio-gustavo]

Scratch that. Just make all the variants Variant(String) lol

That'll generate this in dependency_types

{
    use ::ts_rs::typelist::TypeList;
    ().extend({
        use ::ts_rs::typelist::TypeList;
        ().push::<String>()
            .extend(<String as ::ts_rs::TS>::generics())
    })
    .extend({
        use ::ts_rs::typelist::TypeList;
        ().push::<String>()
            .extend(<String as ::ts_rs::TS>::generics())
    })
    // Repeat this a bunch more times...
    .extend({
        use ::ts_rs::typelist::TypeList;
        ().push::<String>()
            .extend(<String as ::ts_rs::TS>::generics())
    })
}

[NyxCode]

Ah, I see! I'll prepare a PR to de-duplicate the dependencies based on their path. So std::string::String and String will still show up as separate items in the TypeList, but that's okay, I think.

This is somewhat orthogonal to generating a separate type implementing TypeList - If we do that at some point, the de-duplication will still be usefull, saving time when generating imports.

[kamadorueda]

Something like this would work to dedup dependencies

diff --git a/macros/src/deps.rs b/macros/src/deps.rs
index b99c49f..ea31bfd 100644
--- a/macros/src/deps.rs
+++ b/macros/src/deps.rs
@@ -1,3 +1,5 @@
+use std::collections::HashSet;
+
 use proc_macro2::TokenStream;
 use quote::{quote, ToTokens};
 use syn::{Path, Type};
@@ -5,6 +7,8 @@ use syn::{Path, Type};
 pub struct Dependencies {
     dependencies: Vec<TokenStream>,
     crate_rename: Path,
+    seen_append: HashSet<Type>,
+    seen_push: HashSet<Type>,
     pub types: Vec<Type>,
 }
 
@@ -12,12 +16,19 @@ impl Dependencies {
     pub fn new(crate_rename: Path) -> Self {
         Self {
             dependencies: Vec::default(),
+            seen_append: HashSet::default(),
+            seen_push: HashSet::default(),
             crate_rename,
             types: Vec::default(),
         }
     }
     /// Adds all dependencies from the given type
     pub fn append_from(&mut self, ty: &Type) {
+        if self.seen_append.contains(&ty) {
+            return;
+        }
+        self.seen_append.insert(ty.clone());
+
         let crate_rename = &self.crate_rename;
         self. dependencies
             .push(quote![.extend(<#ty as #crate_rename::TS>::dependency_types())]);
@@ -26,6 +37,11 @@ impl Dependencies {
 
     /// Adds the given type.
     pub fn push(&mut self, ty: &Type) {
+        if self.seen_push.contains(&ty) {
+            return;
+        }
+        self.seen_push.insert(ty.clone());
+
         let crate_rename = &self.crate_rename;
         self.dependencies.push(quote![.push::<#ty>()]);
         self.dependencies.push(quote![
@@ -35,8 +51,19 @@ impl Dependencies {
     }
 
     pub fn append(&mut self, mut other: Dependencies) {
-        self.dependencies.push(quote![.extend(#other)]);
-        self.types.append(&mut other.types);
+        if self.seen_append == other.seen_append && self.seen_push == other.seen_push {
+            return;
+        }
+
+        self.seen_append.extend(other.seen_append.clone());
+        self.seen_push.extend(other.seen_push.clone());
+
+        if !other.dependencies.is_empty() {
+            self.dependencies.push(quote![.extend(#other)]);
+        }
+        if !other.types.is_empty() {
+            self.types.append(&mut other.types);
+        }
     }
 }
 
@@ -44,6 +71,7 @@ impl ToTokens for Dependencies {
     fn to_tokens(&self, tokens: &mut TokenStream) {
         let crate_rename = &self.crate_rename;
         let lines = &self.dependencies;
+
         tokens.extend(quote![{
             use #crate_rename::typelist::TypeList;
             ()#(#lines)*

it passes the tests and works with big enums like enum test { A(X), B(X), ... } where X repeats a lot

it honestly feels ugly though, I think changing the type signature of Dependencies to use a HashSet instead of a Vec would be way cleaner. This is not possible today as it uses TokenStream, but since in the end everything that goes into dependencies is "extend by dependency types", "push type", and "extend by generics of type", I think we could make dependencies be a HashSet of an enum with 3 variants, and delay the actual conversion to TokenStream just before it's needed by ToTokens

[NyxCode]

@kamadorueda I had the same idea and implemented something similar in #292. It needs some cleanup, but it seems to work nicely.

[kamadorueda]

@NyxCode #293