Skip to content

dedup dependencies#293

Merged
escritorio-gustavo merged 5 commits intoAleph-Alpha:mainfrom
kamadorueda:main
Apr 9, 2024
Merged

dedup dependencies#293
escritorio-gustavo merged 5 commits intoAleph-Alpha:mainfrom
kamadorueda:main

Conversation

@kamadorueda
Copy link
Copy Markdown
Contributor

@kamadorueda kamadorueda commented Apr 2, 2024

Goal

Generate TypeLists containing less duplicates.

Mentions #291 #289

Changes

This is an Internal API change, use a HashSet instead of a Vec, and delay formatting until just when it's needed.

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
Copy link
Copy Markdown
Collaborator

NyxCode commented Apr 2, 2024

Nice! Pretty much the same as #292, but I think I like this one more.

@kamadorueda kamadorueda force-pushed the main branch 2 times, most recently from 4f83cfe to 41c66e4 Compare April 2, 2024 22:01
@NyxCode
Copy link
Copy Markdown
Collaborator

NyxCode commented Apr 3, 2024

I explored if generating a custom TypeList impl (which would eliminate the recusion problem completely) would a sensible alternative.

It's definetely possible, but I think it'd require a breaking change to be nice. What I had in mind was

fn dependency_types() -> impl TypeList {
  #[derive(Copy, Clone)]
  struct Deps;
  impl TypeList for Deps {
    fn for_each(self, v: &mut impl TypeVisitor) {
      v.visit::<TypeOfField1>();
      v.visit::<TypeOfField2>();
      Something::dependency_types().for_each(v);
      SomethingElse::generics().for_each(v);
    }
  }
  Deps
}

This breaks with generic parameters though. The naive for_each impl would use them, but it can't. to make that work, Deps would need to be generic as well. Definetely something we could do right now, but it's kinda messy.

If we changed fn dependency_types() -> impl TypeList to fn dependency_types(v: &mut impl TypeVisitor), these problems should go away. That would remove one level of indirection.

So tl;dr: I think we should go ahead with this, and keep this change in mind if someone still runs into the issue or for a breaking 9.0 release some time in the future.

@NyxCode
Copy link
Copy Markdown
Collaborator

NyxCode commented Apr 8, 2024

Note to myself: Add two tests from other branch.

Comment thread ts-rs/tests/recursion_limit.rs Outdated
@NyxCode
Copy link
Copy Markdown
Collaborator

NyxCode commented Apr 9, 2024

So, I've added some reference counting (probably doesn't have a big performance impact, but it felt like the right thing to do), added the test from the other branch and used a HashSet for types as well.

From my side, this is ready to merge.

@escritorio-gustavo escritorio-gustavo merged commit 8cfc3d7 into Aleph-Alpha:main Apr 9, 2024
@NyxCode
Copy link
Copy Markdown
Collaborator

NyxCode commented Apr 9, 2024

Thanks @kamadorueda for getting this of the ground!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants