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

Infered types (_) in #[ts(as = "...")] #299

Merged
merged 11 commits into from
Apr 10, 2024
Merged

Infered types (_) in #[ts(as = "...")] #299

merged 11 commits into from
Apr 10, 2024

Conversation

escritorio-gustavo
Copy link
Contributor

@escritorio-gustavo escritorio-gustavo commented Apr 8, 2024

Goal

This PR aims to allow the use of infered types (_) inside the #[ts(as = "...")] attribute. The _ type will be interpreted as the field's original type. E.g.:

#[derive(TS)]
struct Foo {
    #[ts(as = "Option<_>"]
    bar: i32,
}

is the same as:

#[derive(TS)]
struct Foo {
    #[ts(as = "Option<i32>"]
    bar: i32,
}

but without having to repeat the original type.

This allows us to somewhat support using #[ts(optional)] with any type, by using #[ts(as = "Option<_>", optional)] without requiring us to change the semantics of #[ts(optional)]

Changes

Added a recursive function to traverse the provided type and replace Type::Infer(_) with the field's original type

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

NyxCode commented Apr 8, 2024

Interesting! So _ refers to the original type, right?

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 8, 2024

I do think this is pretty neat, though I think it might be a bit unintuitive for anyone who doesn't know what _ does in that context.
Have you considered a different syntax, e.g #[ts(as = "Option<type>")]. I feel like we might be able to come up with something more intuitive, but I'm not sure if the extra effort/complexity involved in parsing this would be worth it.

If the syntax was #[ts(as = "Option<$type>")], we might be able to just string-replace $type with the actual type before parsing it into a syn::Type. Pretty hacky, though.

@gustavo-shigueo
Copy link
Collaborator

Interesting! So _ refers to the original type, right?

Yes, this is mostly a way to resolve the discussion in #175

@gustavo-shigueo
Copy link
Collaborator

gustavo-shigueo commented Apr 9, 2024

I do think this is pretty neat, though I think it might be a bit unintuitive for anyone who doesn't know what _ does in that context. Have you considered a different syntax, e.g #[ts(as = "Option<type>")]. I feel like we might be able to come up with something more intuitive, but I'm not sure if the extra effort/complexity involved in parsing this would be worth it.

Both of these would require some heavy refactoring, because as expects valid syntax for a type. Option<type> isn't a valid type because type is a keyword and any syntax we come up with (like Option<$type>) is not gonna be valid either.

Type::Infer(_) is the only thing I could think of that both:

  • Would previously be a compiler error before this PR (type annotations required ... can't infer type)
  • Is a valid type that syn can parse

If the syntax was #[ts(as = "Option<$type>")], we might be able to just string-replace $type with the actual type before parsing it into a syn::Type. Pretty hacky, though.

We could make a parse_type function like:

fn parse_type(input: ParseStream) -> Result<Type> {
    let str = parse_assign_str(input)?;
    syn::parse_str(&str.replace("$type", "_"))
}

but since this function wouldn't have access to the original_type the parser implemented in this PR would have to stay, and this function's job would be to replace our custom syntax with _

@abhay-agarwal
Copy link

I think the understore is super intuitive, as it's already widely used throughout rust syntax. So the code is instantly readable. The other nice thing is that you could, for example, turn a type into a vector, such as as = Vec<_>, or as = Result<_,Something>, etc.

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Apr 10, 2024

There is also precedent for this behavior in serde_with https://github.com/jonasbb/serde_with?tab=readme-ov-file#examples

Annotate your struct or enum to enable the custom de/serializer. The #[serde_as] attribute must be placed before the #[derive].

The as is analogous to the with attribute of serde. You mirror the type structure of the field you want to de/serialize. You can specify converters for the inner types of a field, e.g., Vec<DisplayFromStr>. The default de/serialization behavior can be restored by using _ as a placeholder, e.g., BTreeMap<_, DisplayFromStr>.

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 10, 2024

Oh, alright! Maybe it's just me then.
I feel like the semantics of _ are very different here, but I think you two convinced me!

@@ -46,3 +51,122 @@ fn type_def(attr: &StructAttr, ident: &Ident, fields: &Fields) -> Result<Derived
Fields::Unit => unit::null(attr, &name),
}
}

pub(super) fn type_as_infer(type_as: &Type, original_type: &Type) -> Type {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is perfectly fine, but just an idea: Would changing the signature to type_as_infer(type_as: &Type, original: &mut Type) -> () make the impl cleaner? I think that might get rid of the parse_quote! calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't mutate original_type because deeper recursive calls into type_as_infer require the original, unaltered type

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've put something together in #305 to highlight what I had in mind. I've put the &mut on the wrong argument in the snippet above, i think that caused some confusion. Sorry about that.

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've put something together in #305 to highlight what I had in mind.

Oh, that makes a lot more sense! I'll merge that in real quick

macros/src/types/newtype.rs Outdated Show resolved Hide resolved
Comment on lines +141 to +144
let ty = match field_attr.type_override {
Some(type_override) => quote!(#type_override),
None => {
let ty = field_attr.type_as(&field.ty);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I love how this got a lot simpler. Awesome.

Copy link
Collaborator

@NyxCode NyxCode left a comment

Choose a reason for hiding this comment

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

Awesome stuff, great work!

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 10, 2024

Also, you were spot on with your take in #175. This solution keeps #[ts(optional)] as it was. Specifically, #[ts(optional)] still doesn't alter the type, just how it's represented. This seems super clean to me!

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 10, 2024

Interestingly, this kinda makes #[ts(optional = nullable)] obsolete. It'd be equivalent to

#[ts(as = "Option<_>", optional)]
field: Option<i32>,

@escritorio-gustavo
Copy link
Contributor Author

Interestingly, this kinda makes #[ts(optional = nullable)] obsolete. It'd be equivalent to

#[ts(as = "Option<_>", optional)]
field: Option<i32>,

I just hope no one is crazy enough to do that 😆

@escritorio-gustavo escritorio-gustavo merged commit e671a7a into main Apr 10, 2024
14 checks passed
@escritorio-gustavo escritorio-gustavo deleted the infer_as branch April 10, 2024 17:07
@escritorio-gustavo escritorio-gustavo mentioned this pull request Apr 18, 2024
3 tasks
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.

4 participants