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

Fix flatten generic by changing TS::name #235

Closed
wants to merge 4 commits into from

Conversation

escritorio-gustavo
Copy link
Contributor

@escritorio-gustavo escritorio-gustavo commented Feb 14, 2024

An attempt at fixing the problems caused by #215 by changing the name method to return the name of the generics

The reason I chose to do this is that there is no reasonable way to call name_with_type_args from the place where the problem showed up, so either inline or name would have to be used instead.

inline was being used, and causing problems
name, as showed by my attempt to fix it in #215, wouldn't brring T's generics

Out of sheer luck, this fixes #70

@NyxCode despite being a small change regarding how much code changed, this is a reasonably big change in how the library works, so I'd really like your feedback on this one

Comment on lines +67 to +72
match <#generic_ident>::name().as_str() {
"null" => #generic_ident_str.to_owned(),

// If name is not "null", a type has been provided, so we use its
// name instead
x => x.to_owned()
x => {
x.to_owned()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to re-add comments later

ts-rs/src/lib.rs Outdated
@@ -298,7 +298,7 @@ pub trait TS {

/// Name of this type in TypeScript, with type arguments.
fn name_with_type_args(args: Vec<String>) -> String {
format!("{}<{}>", Self::name(), args.join(", "))
format!("{}<{}>", Self::name().split_once('<').unwrap().0, args.join(", "))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hacky approach. Need to find a better way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed this to use a match so it won't panic!

Comment on lines +400 to +403
ts_name: match T::name() {
x if !x.contains('<') => x,
x => x.split('<').next().unwrap().to_owned()
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependency seems to need the name without any generics, so the same hacky approach was used here

@@ -256,7 +256,7 @@ fn default() {
// #[ts(inline)]
// xi2: X<i32>
}
assert_eq!(Y::decl(), "type Y = { a1: A, a2: A<number>, }")
assert_eq!(Y::decl(), "type Y = { a1: A<string>, a2: A<number>, }")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side effect: The generic is always emitted, even when using its default

@escritorio-gustavo
Copy link
Contributor Author

@NyxCode I also just realized that this accidentally fixes #70!

@escritorio-gustavo escritorio-gustavo changed the title Fix flatten generic by changing name Fix flatten generic by changing TS::name Feb 15, 2024
@NyxCode
Copy link
Collaborator

NyxCode commented Feb 15, 2024

Hey! Thanks for being on the ball on this issue, really appreciate it!
As you know, my time this month is very limited - however, I too did some experimentation and I think I ended up with something similar to this.

However, I went quite a bit further, and re-did the generics handeling in ts-rs completely. Here's how that went:

  • Like you did here, I changed TS::name to return the full name, including all generics.
    Here, I went further, and removed TS::name_with_type_args completely!
  • I completely removed the 100line format_type from macros/types/generics.rs. Everywhere where it was used, I now just use quote!(<ts as ts_rs::TS>::name()).

The biggest change here is how generic types are being exported. Right now, we're expecting users to export with all generic parameters equal to (), which required all that special handeling in generics.rs.

Instead, generic types are not handeled specially now. However, before they get exported, I switch out which generic parameters are actually used, and use "dummy" types there instead:

#[derive(TS)]
struct MyGenericType<A, B>{ .. }

// .. turns into this impl ..

impl<A: TS, B: TS> TS for MyGenericType<A, B> {
    // everything the same, except
    // - name() returns the full name, so `format!("MyGenericType<{}, {}>`, A::name(), B::name())`
    // - name_with_type_args() is gone 🥳
    // - decl() changed like this:

    fn decl() -> String {
        struct A;
        impl TS for A {
            fn name() -> String { "A".to_owned() }
            fn transparent() -> bool { false }
        }

        struct A;
        impl TS for A {
            fn name() -> String { "A".to_owned() }
            fn transparent() -> bool { false }
        }

        MyGenericType::<A, B>::old_decl()
    }
}

So tl;dr: I create "dummy" types "A" and "B" to represent the generics. They are then handeled like every other type.

The only failing test right now is related to default parameters. If a struct contains a field a1: A, where A is struct A<T = String>, the old implementation omits the parameter, resulting in a1: A.
After these changes, the output changes to a1: A<string>, so the default parameter is being made explicit.
I don't think that's a problem.

(This happens since A::name() is the same as A::<String>::name(), and that gives "A<string>" as output.)

Also, all dependencies are kinda fucked right now, but I think that'll be straight-forward to fix (That was done in the format_type function I removed ^^).


The only potential problem I see is that these "dummy" types for the generics need to fullfill all trait bounds of the struct. For that, I generate them with #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)], and manually implement ToString for them, which makes the test suite pass.


What do you think of this? Any problems you see with that approach, which might not be covered by the test suite?
I think I kinda like where this is going, though it's just something I hacked together and needs lots of cleanup. I mostly hope that it makes everything a bit less confusing & more maintainable.
I guess we're also not in a particular rush, so I think it makes sense to get this right before the next release.

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Feb 15, 2024

Like you did here, I changed TS::name to return the full name, including all generics.
Here, I went further, and removed TS::name_with_type_args completely!

That's awesome! I tried messing around with removing TS::name_with_type_args but couldn't get exports with the generics' names (The actual letter T for example) to work, instead I got null everywhere lol

I completely removed the 100line format_type from macros/types/generics.rs. Everywhere where it was used, I now just use quote!(::name()).

Nice! I remember in a previous PR you mentioned not liking this function very much,

The biggest change here is how generic types are being exported. Right now, we're expecting users to export with all generic parameters equal to (), which required all that special handeling in generics.rs.

I see. Never even thought of doing that but isolating the dummy types to TS::decl completely removes the whole "null" check. Nice!

The only failing test right now is related to default parameters. If a struct contains a field a1: A, where A is struct A<T = String>, the old implementation omits the parameter, resulting in a1: A.
After these changes, the output changes to a1: A, so the default parameter is being made explicit.
I don't think that's a problem.

Yup! I got that test failing for the same reason. I don't think it's a problem either

The only potential problem I see is that these "dummy" types for the generics need to fullfill all trait bounds of the struct. For that, I generate them with #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)], and manually implement ToString for them, which makes the test suite pass.

That's somewhat anoying, but as you mentioned in the #217 thread, shouldn't be that big a deal

What do you think of this? Any problems you see with that approach, which might not be covered by the test suite?
I think I kinda like where this is going, though it's just something I hacked together and needs lots of cleanup. I mostly hope that it makes everything a bit less confusing & more maintainable.
I guess we're also not in a particular rush, so I think it makes sense to get this right before the next release.

Overall I think this will be a great change. Generics are currently very confusing and anything done to reduce their complexity will make maintanence much easier.

As for problems not covered by the test suite, I think the only one we have is the dependencies, more specifically we currently have 2 issues about wrong import statements (#168 and #232)

@escritorio-gustavo
Copy link
Contributor Author

Btw, I'm sorry for absolutely blasting you with notifications 😅, I know you con't have much time to spare this month

@NyxCode
Copy link
Collaborator

NyxCode commented Feb 15, 2024

No problem, I just don't want you to think I'm uninterested in what's going on 😆

As for problems not covered by the test suite, I think the only one we have is the dependencies, more specifically we currently have 2 issues about wrong import statements (#168 and #232)

I feel like these will be easy to fix once the inlining is more predictable (not recursive) again.
I'll clean up my mess as well as I can, and push it to some branch so that you can take a look.
Then, we can concentrate on the original issue (wrong imports & the akward impl of Result).

@escritorio-gustavo
Copy link
Contributor Author

I think fixing Result's impl TS should be pretty strait forward, it just needs a few extra methods to be defined, such as export

@escritorio-gustavo
Copy link
Contributor Author

Closing in favor of #233

@escritorio-gustavo escritorio-gustavo deleted the fix-flatten-generic-by-changing-name branch February 16, 2024 12:37
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.

Incorrect TS types generated from types containing type aliases
2 participants