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

Implement generic inlining with trait bounds #217

Closed
wants to merge 2 commits into from
Closed

Implement generic inlining with trait bounds #217

wants to merge 2 commits into from

Conversation

escritorio-gustavo
Copy link
Contributor

@escritorio-gustavo escritorio-gustavo commented Jan 30, 2024

This should be the last PR related to inlining generics (unless a new bug shows up)

Now, when dealing with trait bounds, instead of the unit type (), a unit struct defined by the user that implements all the given traits must be passed into #[ts(unit_type = "path::to::Struct")]

Example

#[derive(TS)]
struct Unit;
impl ToString for Unit {
    fn to_string(&self) -> String {
        unimplemented!(
            "This type only exists to satisfy trait bounds for TS, it should never have methods called on it"
        )
    }
}

#[derive(TS)]
#[ts(unit_type = "Unit", export)] // exporting now works!
struct Generic<T: ToString> {
    foo: T
}

#[derive(TS)]
struct Container {
    g: Generic<String>,

    #[ts(inline)]
    gi: Generic<String>,

    #[ts(flatten)]
    t: Generic<u32>,
}

Fixes #214

TODO:

  • Make sure everything works for generics with trait bounds
  • Find good name for the new attribute
  • Find a way to remove hard coded null (maybe this should be a separate PR?)

@escritorio-gustavo escritorio-gustavo added bug Something isn't working P-HIGH This will be treated with high priority labels Jan 30, 2024
@escritorio-gustavo
Copy link
Contributor Author

@NyxCode what do you think of this approach?

@NyxCode
Copy link
Collaborator

NyxCode commented Jan 31, 2024

I like that this fixed the issue, but to be honest, I kinda dislike that we have to fix it.
If it's okay with you, I'd like to hold off on merging this for a bit - Specifically, I'm hoping that maybe improving the way we export types could have a trickle-down effect, maybe making this change unnecessary. Really not sure about that though, and if it doesn't, I'd be happy to merge this.

@escritorio-gustavo
Copy link
Contributor Author

Don't worry about it, this one can wait

@escritorio-gustavo
Copy link
Contributor Author

In that case I'm gonna convert this to a draft to signify that it's on hold

@escritorio-gustavo escritorio-gustavo marked this pull request as draft January 31, 2024 20:21
@escritorio-gustavo
Copy link
Contributor Author

Reopening this PR as discussed in #219

@escritorio-gustavo escritorio-gustavo marked this pull request as ready for review February 1, 2024 20:01
@@ -34,7 +35,7 @@ impl DerivedTS {
.params
.iter()
.filter(|param| matches!(param, GenericParam::Type(_)))
.map(|_| quote! { () });
.map(|_| self.unit_type.as_ref().map(|x| x.to_token_stream()).unwrap_or(quote!{ () }));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This here looks interesting!
What happens with something like this, when there are multiple generic parameters with different bounds?

#[ts(export)]
struct X<A: ToString, B: IntoIterator<Item = u32>> {
    a: A,
    b: B
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type given to #[ts(unit_type = "...")] will be used for all generics, same as (), so it must implement all trait bounds from all of the generic parameters

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, makes sense! Just an idea here: What if the attribute didnt only parse one type, but a list of types? That should just work, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if the attribute didnt only parse one type, but a list of types?

What do you mean? One type per generic?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah!

Copy link
Contributor Author

@escritorio-gustavo escritorio-gustavo Feb 2, 2024

Choose a reason for hiding this comment

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

Then, we'd get rid of the hardcoded "null", and a user could specify any "default generic type", without the need to declare his own "unit" type.

#[derive(TS)]
#[ts(default_generics = "String, Box<dyn Error>")]
struct Something<S: AsRef<str>, E: Error>();

But: TypeId::of requires 'static, and idk if it's even possible to work around that in this case.

Yeah, I tried that when I was making the first PR about inlining generics and couldn't get past this, but the bigger problem is, if the user actually wanted at some point to use the struct with the default generic (String in this example), inlining the generic would fail, emiiting S instead of string

Copy link
Contributor Author

@escritorio-gustavo escritorio-gustavo Feb 2, 2024

Choose a reason for hiding this comment

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

if the user actually wanted at some point to use the struct with the default generic (String in this example), inlining the generic would fail, emiiting S instead of String

What I mean here is:

#[derive(TS)]
#[ts(default_generics = "String")]
pub struct Generic<T: ToString> {
    pub x: T,
}

#[derive(TS)]
pub struct Foo {
    #[ts(inline)]
    pub a: Generic<String>, // This will fail, will emit `T` instead of `string` because String == String

   #[ts(inline)]
    pub b: Generic<i32>, // This will not fail because i32 != String

   #[ts(inline)]
    pub b: Generic<&'static str>, // This will not fail because &str != String
}

This generates

type Foo = { a: { x: T, }, b: { x: number, }, c: { x: string, }, }
//                   ^ should be `string`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another alternative is, when deciding whether to use T or string, we need to know if the generic passed to the field is the identifier of one of the struct definition generics or an actual type.
Don't know how we could get this information though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EDIT: I think I made it work. Only hacked something together, but it does pass the tests. It's not as robust as the TypeId solution before, but it might be preferable to the "null" solution - i dont know yet. Here's how it looks like. What do you think?

return quote!(
    if std::any::type_name::<#generic_ident>() == std::any::type_name::<#unit_type>() {
        #generic_ident_str.to_owned()
    } else {
        <#generic_ident>::inline()
    }
);

If that's something you'd like to look in further, I'd be happy to clean my hacky solution up a bit & push it to a branch for you to check out.

This looks interesting, let's take a look!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I tried that when I was making the first PR about inlining generics and couldn't get past this, but the bigger problem is, if the user actually wanted at some point to use the struct with the default generic (String in this example), inlining the generic would fail, emiiting S instead of string

Yeah, I think you're right. Let's scratch that idea.

Another alternative is, when deciding whether to use T or string, we need to know if the generic passed to the field is the identifier of one of the struct definition generics or an actual type.
Don't know how we could get this information though

This might be worth exploring. I'll play with that idea if I find some time later. I do have the feeling though that we'd have to make a somewhat big change to the TS trait, so I'm not sure yet if it'd be worth it.

@NyxCode
Copy link
Collaborator

NyxCode commented Feb 1, 2024

Rest looks good!
I'm still trying to come up with a better, more descriptive name, but I havent yet.

@escritorio-gustavo
Copy link
Contributor Author

I'm still trying to come up with a better, more descriptive name, but I havent yet.

Yeah, I also struggled with that, still have no idea what would be a good name for this attribute

@NyxCode
Copy link
Collaborator

NyxCode commented Feb 1, 2024

Also, I just realized what would probably fix this, if that feature ever comes to stable - !, the never type! It's the perfect default, since it's a subtype of every trait!

#![feature(never_type)]

struct X<T: ToString>(T);

type XX = X<!>;

@NyxCode
Copy link
Collaborator

NyxCode commented Feb 1, 2024

(there's no point in waiting for it, the Rust PR is from 2016)

@escritorio-gustavo
Copy link
Contributor Author

(there's no point in waiting for it, the Rust PR is from 2016)

That's a huge shame, I'm sure ! would help with some other stuff too

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Feb 1, 2024

Maybe std::convert::Infallible works?

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Feb 1, 2024

Maybe std::convert::Infallible works?

Nope, just tried it

@NyxCode
Copy link
Collaborator

NyxCode commented Feb 1, 2024

WOW, wait a minute!

@NyxCode
Copy link
Collaborator

NyxCode commented Feb 1, 2024

Okay, never mind 😅
I just realized that ! is already kind of a thing, but only in specific places as far as i can tell - for example in function return types.
So I tried to use some traits to "extract" the "!" from there. But unfortunately, it seems like it currently does not have the property that it's a subtype of everything: playground

@escritorio-gustavo
Copy link
Contributor Author

Yeah, I think there is currently no way to do this properly

@NyxCode
Copy link
Collaborator

NyxCode commented Feb 3, 2024

So I've spent some time experimenting, but I haven't been able to come up with a nicer way to handle generics.

If you believe there's a need for this feature1, I think we can merge this. I'll leave it up to you to decide if adding this feature is worth the added complexity.

There's no docs so far - maybe a sentence at the very bottom of the docs for the TS trait might be usefull.
Or maybe this is something which would be better placed in a wiki, not sure.

Footnotes

  1. In general, I believe having trait bounds on structs is a mistake in almost all cases. Even std::collections::HashMap doesn't have any trait bounds on it. This StackOverflow answer describes that nicely.

@escritorio-gustavo
Copy link
Contributor Author

In general, I believe having trait bounds on structs is a mistake in almost all cases. Even std::collections::HashMap doesn't have any trait bounds on it. This StackOverflow answer describes that nicely.

Interesting! I haven't been writing Rust for very long (just under a year now) so I never realized trait bounds on structs were such a bad idea.

@escritorio-gustavo
Copy link
Contributor Author

I'm fine with closing this PR in light of that! It really adds a lot of complexity with the totally useless unit type that must implement all traits required by the struct and the attribute name is not helpful at all.
If people really end up needing it, someone will open an issue

@escritorio-gustavo
Copy link
Contributor Author

Or maybe this is something which would be better placed in a wiki, not sure.

I love the idea of creating a wiki! It's usually dificult to find docs for derive macro helper attributes through LSP definitions (which don't autocomplete, only telling you the mispelled word isn't accepted) or docs.rs, which only allows you to document the trait itself, making something like #[ts(...)] hard to find (although documented under the TS trait, docs.rs doesn't generate a separate page or links for it, we have to manually do that, which is easy to forget)

@escritorio-gustavo escritorio-gustavo deleted the fix_inline_generics_trait_bound branch February 5, 2024 20:44
@escritorio-gustavo
Copy link
Contributor Author

@NyxCode I have started writing the wiki! Check it out :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P-HIGH This will be treated with high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generics don't work with inline and flatten when they have a default type or a trait bound
2 participants