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.

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

Recusion Limit - Round 2 #306

Merged
merged 14 commits into from
May 17, 2024
Merged

Recusion Limit - Round 2 #306

merged 14 commits into from
May 17, 2024

Conversation

NyxCode
Copy link
Collaborator

@NyxCode NyxCode commented Apr 11, 2024

Goal

Determine when recusion limit errors still happen, reproduce and possibly fix them

Changes

  • Remove TypeList, move TypeVisitor to lib.rs
    • dependency_types() -> TypeList got changed to visit_dependencies(v: &mut impl TypeVisitor)
    • generics() -> TypeList got changed to visit_generics(v: &mut impl TypeVisitor)
    • codegen was adjusted accordingly

This is again a technically breaking change for anyone interacting with the TS trait directly.

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
Collaborator Author

NyxCode commented Apr 11, 2024

The "chain" of types struct T000(T001), struct T001(T002), ..., struct T082(T083), struct T083 manages to still run into the error.

If there are only 83 instead of 84 types, then everything works, so that seems to be the limit currently.

error: reached the recursion limit while instantiating `<((), (PhantomData<T083>,)) as TypeList>::for_each::<...>`                                                                                                                                     
   --> C:\Users\morit\Coding\ts-rs\ts-rs\src\typelist.rs:102:9
    |
102 |         self.0.for_each(v);
    |         ^^^^^^^^^^^^^^^^^^
    |
note: `<(A, B) as TypeList>::for_each` defined here
   --> C:\Users\morit\Coding\ts-rs\ts-rs\src\typelist.rs:101:5
    |
101 |     fn for_each(self, v: &mut impl TypeVisitor) {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: the full type name has been written to 'C:\Users\morit\Coding\ts-rs\target\debug\deps\recursion_limit-57ec16690c4b1d02.long-type.txt'

error: could not compile `ts-rs` (test "recursion_limit") due to 1 previous error

@NyxCode
Copy link
Collaborator Author

NyxCode commented Apr 11, 2024

New codegen for

#[derive(TS)]
#[ts(export, export_to = "generics/")]
struct Generic<T>
where
    T: TS,
{
    value: T,
    values: Vec<T>,
}

looks like this:

fn visit_generics(v: &mut impl TypeVisitor) where Self: 'static, {
    v.visit::<T>();
    <T as TS>::visit_generics(v);
}
fn visit_dependencies(v: &mut impl TypeVisitor) where Self: 'static, {
    <T as TS>::visit_generics(v);
    <Vec<T> as TS>::visit_generics(v);
    v.visit::<T>();
    v.visit::<Vec<T>>();
}

What previously was a .push::<T>() is now v.visit::<T>().
.extend(T::dependency_types()) turned into T::visit_dependencies(v), and
.extend(T::generics()) turned into T::visit_generics(v)

This completely sidesteps the recusion limit problem: There's no complicated type like (((((), PhantomData<T>, ......))) for which the compiler has to figure out that it implements TypeList.
Instead, you just pass a TypeVisitor into visit_dependencies, and its visit method gets called for every dependency. Done!

@NyxCode
Copy link
Collaborator Author

NyxCode commented Apr 11, 2024

Originally, I thought that having a TypeList would be nice. But it turned out that the only thing we ever do with it is to call .for_each.
These changes basically put the for_each method directly into TS. T::visit_dependencies(v) is the same as T::dependency_types().for_each(v), and T::visit_generics(v) is the same as T::generics().for_each(v).

macros/src/deps.rs Show resolved Hide resolved
ts-rs/src/export.rs Show resolved Hide resolved
ts-rs/src/lib.rs Show resolved Hide resolved
@NyxCode
Copy link
Collaborator Author

NyxCode commented Apr 11, 2024

The Generic struct above nicely illustrates why it was easier to just get rid of TypeList.
With the old generics, I wanted to generate

fn generics() -> impl TypeList where Self: 'static {
    struct List;
    impl TypeList for List {
        fn for_each(&self, v: &mut impl TypeVisitor) {
            v.visit::<T>();
            T::generics().for_each(v);
        }
    }
    List
}

Here, the v.visit::<T>() fails to compile, since the type parameter T can't be used within the impl TypeList. The workaround for this would have been kinda complicated.

ts-rs/src/lib.rs Outdated Show resolved Hide resolved
@escritorio-gustavo escritorio-gustavo added the breaking change This PR will change the public API in some way label Apr 17, 2024
@escritorio-gustavo
Copy link
Contributor

Due to this PR removing trait functions that return impl Trait, if we make parse_attrs and parse_serde_attrs pub(crate) instead of just pub, it can lower the MSRV all the way down to 1.63.0!

@NyxCode NyxCode marked this pull request as ready for review May 17, 2024 20:10
@NyxCode
Copy link
Collaborator Author

NyxCode commented May 17, 2024

Merging this now. Not yet sure if we should publish a 9.0, or wait for the CLI.
I guess publishing more often doesn't hurt, right?

@NyxCode NyxCode merged commit 7d28c1a into main May 17, 2024
14 checks passed
@escritorio-gustavo escritorio-gustavo deleted the better-dependencies branch May 20, 2024 11:37
@escritorio-gustavo
Copy link
Contributor

Merging this now. Not yet sure if we should publish a 9.0, or wait for the CLI. I guess publishing more often doesn't hurt, right?

I don't mind publishing a 9.0, especially with the bug mentioned in #317 being fixed in this PR. The CLI can wait untill version 10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This PR will change the public API in some way
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants