Aleph-Alpha / ts-rs

Generate TypeScript bindings from Rust types
MIT License
1.08k stars 107 forks source link

`#[ts(concrete)]`: Allow generic parameter to be "disabled" #264

Closed NyxCode closed 6 months ago

NyxCode commented 6 months ago

This PR aims to allow users to "disable" a generic parameter:

#[ts(concrete(Y = i32)]
struct X<Y> {
  y: Y
}

should behave as if the struct was declared without the generic parameter Y:

struct X {
  y: i32
}

Motivation

The main motivation behind this are associated types, which have no equivalent in TypeScript. Therefore, given a type like this, there's no generic TypeScript type we could generate:

trait MyTrait {
  type AssociatedType;
}

struct X<Y: MyTrait> {
  y: Y::AssociatedType
}

The solution here is to "disable" the generic parameter "Y" by making it "concrete".

#[ts(concrete(Y = MyType))]
struct X<Y: MyTrait> {
  y: Y::AssociatedType
}

TODOs

NyxCode commented 6 months ago

How exactly should TS::decl and TS::decl_concrete behave?

escritorio-gustavo commented 6 months ago

Hey @NyxCode! I was quite confused when I saw #262 and had no idea what #[ts(concrete)] was lol

I'm still kinda confused on something:

I think I get it, but shouldn't Y be constrained to Y: MyTrait<AssociatedType = MyType> to avoid users doing weird stuff with their types like:

struct MyType;

impl MyTrait for MyType {
    type AssociatedType = i32;
}

struct OtherType;

impl OtherTrait for MyType {
    type AssociatedType = String;
}

#[ts(concrete(Y = MyType))]
struct X<Y: MyTrait> {
  y: Y::AssociatedType
}

This would generate valid TS and Rust would be totally cool with it, but the user can shoot themselves in the foot by serializing X<OtherType>, causing their TS type to be wrong (y will be typed as number but contain a string)


Edit: remove resolved points that were just typos

escritorio-gustavo commented 6 months ago

We also don't really support user defined traits as generic constraints for our derive macro anyway, so wouldn't this be a very limited usecase? Or does the fact that this removes the generic altogether eliminate that problem?

escritorio-gustavo commented 6 months ago

Last thing, does this add the concrete type to the dependencies?

Edit: nevermind, just saw the todo lol

escritorio-gustavo commented 6 months ago

But what do we do for the concrete/disabled generic parameters? Should be keep them as they are as well, or should we replace them with the user-provided one as well?

I think we should use the type passed into #[ts(concrete)], since the idea is to pretend there is no generic at all

NyxCode commented 6 months ago

Is the PR title a typo or are you planning a #[ts(discrete)] attribute as well?

😆 A typo, was studying maths before, I think that's where that came from ^^

In this second definition, should Y still be present? The description sounds like it's not.

Correct, sorry about that. I think the 2nd snippet wouldn't even compile as-is. Fixed!

I think I get it, but shouldn't Y be constrained to Y: MyTrait to avoid users doing weird stuff with their types

I think allowing that is intended, see #261 for @WilsonGramer's usecase.
Besides, that's just the effect "disabling" a generic parameter has - you're only getting bindings for one specific type, and you have to make sure not to mix them. It's the same case for the struct X<Y> in the description of this PR - There, you're also only getting bindings for one specific type, X<i32>

We also don't really support user defined traits as generic constraints for our derive macro anyway, so wouldn't this be a very limited usecase? Or does the fact that this removes the generic altogether eliminate that problem?

Good question! So this prototype here kind of works, but I'll have to go back and see why exactly 😆

But in general: We couldn't deal with trait bounds before because we're generating dummy types and plugging them into the generic parameters. If there's a trait bound we don't know about, we can't do that. But with #[ts(concrete)], the user is giving us the type to use, so we don't use a dummy type for that parameter.

escritorio-gustavo commented 6 months ago

😆 A typo, was studying maths before, I think that's where that came from ^^

I see! You must be neck deep in set theory 😆

I think allowing that is intended, see #261 for @WilsonGramer's usecase. Besides, that's just the effect "disabling" a generic parameter has - you're only getting bindings for one specific type, and you have to make sure not to mix them. It's the same case for the struct X<Y> in the description of this PR - There, you're also only getting bindings for one specific type, X<i32>

I hadn't even seen #261 lol, I think I just didn't see a linked issue, so I didn't look for one, my bad!

But with #[ts(concrete)], the user is giving us the type to use, so we don't use a dummy type for that parameter.

That makes sense, we use MyType as a replacement for the dummies, so if MyType: MyTrait it just works

So this prototype here kind of works, but I'll have to go back and see why exactly 😆

Always fun when our code works and we don't know why 🤣

NyxCode commented 6 months ago

I'm definetely still open to alternative solutions to the "associated types" problem!
The thought process with this here was

"There are associated types in the struct" => "We can't make that a generic TS def, since TS doesn't have that feature" => "The generated TS type can't be generic" => "To make it that, we disable the generic parameter by specifying a concrete type instead"

escritorio-gustavo commented 6 months ago

does this add the concrete type to the dependencies?

So, I experimented with this

for ty in attr.concrete.values() {
    dependencies.push(ty);
}

And this doesn't quite work, because in the concrete_generic test for example, this would add TsDriver to dependencies, when we want to add TsDriver::Info, though, this would be needed too if T is used in another struct field

escritorio-gustavo commented 6 months ago

Edit, I just realized that this works with dependencies out of the box with the associated types, I can push a test for that

NyxCode commented 6 months ago

Edit, I just realized that this works with dependencies out of the box with the associated types, I can push a test for that

Was just about to write that I think it just works.

We're doing a SomeType<A, B, C> -> SomeType<DummyA, DummyB, Concrete> conversion in decl, and from then on, dependencies are resolved as they normally are, so everything should be fine.

There are a few interesting edge-cases though, so tests would be great!

escritorio-gustavo commented 6 months ago

The case that won't work is this:

#[derive(TS)]
#[ts(export, export_to = "concrete_generic/", concrete(T = OtherDriver))]
struct OtherStruct<T: Driver> {
    u: T::Info,
    x: T,
}

For that, T needs a constraint on TS:

#[derive(TS)]
#[ts(export, export_to = "concrete_generic/", concrete(T = OtherDriver))]
struct OtherStruct<T: Driver + TS> {
    u: T::Info,
    x: T,
}
NyxCode commented 6 months ago

Right! So the case which would be nice if it worked is:

#[derive(TS)]
struct OtherDriver;
impl Driver for OtherDriver {
    type Info = Foo;
}

#[derive(TS)]
#[ts(concrete(T = OtherDriver))]
struct OtherStruct<T: Driver> {
    u: T::Info,
    x: T,
}

But:

The impl TS for OtherStruct is still generic, and must work for every T we pass in.
It's only at the step that we call decl() that we go from OtherStruct<Something> to OtherStruct<OtherDriver>.

Maybe we could generate an impl TS for OtherStruct<OtherDriver> instead (it's currently a impl<D: Driver> TS for OtherStruct<D>)? That'd be pretty cool!

escritorio-gustavo commented 6 months ago

Maybe we could generate an impl TS for OtherStruct<OtherDriver> instead (it's currently a impl<D: Driver> TS for OtherStruct<D>)? That'd be pretty cool!

I've started experimenting with this and managed to convert the impl header, I'm clocking out for today, but tomorrow I'll work on generate_decl_fn

escritorio-gustavo commented 6 months ago

I managed to replace the type in the remaining functions, but I get ambiguous associated type: use fully-qualified syntax: <TsDriver as Driver>::Info

escritorio-gustavo commented 6 months ago

This is what the macro is expanding to:

impl ts_rs::TS for MyStruct<TsDriver> {
    type WithoutGenerics = MyStruct<TsDriver>;
    fn ident() -> String {
        "MyStruct".to_owned()
    }
    fn name() -> String {
        "MyStruct".to_owned()
    }
    fn decl_concrete() -> String {
        format!("type {} = {};", "MyStruct", Self::inline())
    }
    fn decl() -> String {
        let inline = <MyStruct<TsDriver> as ts_rs::TS>::inline();
        let generics = "";
        format!("type {}{generics} = {inline};", "MyStruct")
    }
    fn inline() -> String {
        format!(
            "{{ {} }}",
            <[String]>::join(
                &[format!(
                    "{}{}{}: {},",
                    "",
                    "u",
                    "",
                    <TsDriver::Info as ts_rs::TS>::name() // Ambiguous associated type
                )],
                " "
            )
        )
        .replace(" } & { ", " ")
    }
    fn inline_flattened() -> String {
        format!(
            "{{ {} }}",
            <[String]>::join(
                &[format!(
                    "{}{}{}: {},",
                    "",
                    "u",
                    "",
                    <TsDriver::Info as ts_rs::TS>::name() // Ambiguous associated type
                )],
                " "
            )
        )
    }
    #[allow(clippy::unused_unit)]
    fn generics() -> impl ts_rs::typelist::TypeList
    where
        Self: 'static,
    {
        use ts_rs::typelist::TypeList;
        ()
    }
    fn output_path() -> Option<std::path::PathBuf> {
        let path = std::env::var("TS_RS_EXPORT_DIR");
        let path = path.as_deref().unwrap_or("./bindings");
        Some(std::path::Path::new(path).join("concrete_generic/MyStruct.ts"))
    }
    #[allow(clippy::unused_unit)]
    fn dependency_types() -> impl ts_rs::typelist::TypeList
    where
        Self: 'static,
    {
        {
            use ts_rs::typelist::TypeList;
            ().push::<TsDriver::Info>() // Ambiguous associated type
                .extend(<TsDriver::Info as ts_rs::TS>::generics()) // Ambiguous associated type
        }
    }
}
#[cfg(test)]
#[test]
fn export_bindings_mystruct() {
    <MyStruct<TsDriver> as ts_rs::TS>::export().expect("could not export type");
}
escritorio-gustavo commented 6 months ago

I think we could store the bounds in DerivedTS::concrete and use them to allow this syntax

NyxCode commented 6 months ago

Ah, that's very annoying!
Rust is refusing TsDriver::Info, since Info is not on TsDriver itself, but on the Driver trait.
What would work is <<TsDriver as Driver>::Info as ts_rs::TS>, or putting it inside a function to help the type resolution:

fn get_info_name<T: Driver>() -> String {
    <T::Info as TS>::name()
}

Seems like a tough problem. If you'd like, i'd be happy to take a look if you push it to a new branch. Getting the feeling that it might be difficult without doing too much special-casing though.

escritorio-gustavo commented 6 months ago

Seems like a tough problem. If you'd like, i'd be happy to take a look if you push it to a new branch. Getting the feeling that it might be difficult without doing too much special-casing though.

Agreed. What could make this even more complicated is the fact that we'd have to handle both

struct MyStruct<T: Driver>

and

struct MyStruct<T> where T: Driver
NyxCode commented 6 months ago

It might even be impossible - what if there are two trait bounds?

struct MyStruct<D: Driver + Context> {
    info: D::Info,
    ctx: D::Ctx,
}

(Info is from Driver, Ctx is from Context)

Then, we'd have to figure out where the associated type came from to use that syntax, which we cant.

NyxCode commented 6 months ago

There, only the 2nd approach using a helper function seems to work:

fn helper<T: Driver + Context>() {
    let x = <T::Ctx as TS>::name(); // works
    let y = <T::Info as TS>::name(); // works
}
escritorio-gustavo commented 6 months ago

Yeah, I think we should just keep the <T: TS> requirement, it's not that big a deal

NyxCode commented 6 months ago

After playing with this for a bit, i'm afraid I agree with you.

Getting it to generate <<TsDriver as Driver>::Info as TS> was pretty hacky [^1], but it immediately breaks if there are multiple trait bounds. There, we could just take the first one and hope for the best, but that's not great.

The alternative - using this "helper" function - would be super intrusive and complicated. We'd have to do that everywhere throughout the codebase, and even then, it's complicated.
Whenever we want to call <T as TS>::name(), we'd have to first generate a helper function with the right trait bounds, e.g fn helper<T: Driver>. And even that doesn't buy us much, since to be actually usefull, it'd need to be fn helper<T: Driver> where T::Info: TS. So yeah, this get's out of hand really fast.

[^1]: Before doing anything else, right at the macro entry point, I went through the ItemStruct, iterated through all fields, found the ones whose types included the concrete generic parameter, and swapped it out for <{concrete replacement} as {first trait bound}>

NyxCode commented 6 months ago

I think we should use the type passed into #[ts(concrete)], since the idea is to pretend there is no generic at all

I'm still a bit unsure about this, though I find your argument convincing.
However, here's the other argument i'm struggeling with:

NyxCode commented 6 months ago

Next problem: Omitting concrete generics from the trait bounds did "solve" the scenario with associated types. However, it was just a workaround. For example, this here now fails to compile:

#[derive(TS)]
#[ts(concrete(A = i32))]
struct Generic<A> {
    a: A
}

That's because in inline, we somewhere call <A as TS>::name(), but our impl no longer contains the A: TS bound.

Maybe we could track this information throughout the proc-macro, like we do with dependencies. Whenever we encounter a type on which we call <T as TS>::name(), we keep in mind that we have to require that we need a T: TS bound.

For associated types, we'd at some point call <T::Type as TS>::name(), storing T::Type somewhere, and finally generating a where T::Type: TS bound. That'd be great, since it'd not only fix the issue above, but also not require this anymore:

trait Driver {
    type Info: TS; // <-- this ": TS" bound
}

In some sense, impl<D: Driver> TS for SomeType<D> where D::Info: TS is the next-best thing we can do other than impl TS for SomeType<TsDriver>

NyxCode commented 6 months ago

Update: That seems to work, but results in "overflow evaluating the requirement T<'_>: TS" for everything in self_referential.rs.
Nothing is ever easy, is it? ._.

NyxCode commented 6 months ago

For

#[derive(TS)]
struct Simple {
    t: &'static Simple
}

this naive approach generates

impl TS for Simple where &'static Simple: TS { ... }

which sends the trait solver into an infinite loop.

escritorio-gustavo commented 6 months ago

Maybe there is no need to make every type : TS. Everything works already except when using the generic with concrete type. Maybe just check if any field uses the type T and if so add T: TS

escritorio-gustavo commented 6 months ago

I'll see if I can come up with something for that

escritorio-gustavo commented 6 months ago

I got it to work, but the code is kinda disgusting, even getting a clippy warning of shame 😅

image

escritorio-gustavo commented 6 months ago

Idk why I said enum in the commit message. I meant struct but this should work with enums too

escritorio-gustavo commented 6 months ago

I got it to work, but the code is kinda disgusting, even getting a clippy warning of shame 😅

image

Looking at this... Why do we need _generics? It's not used within the function right?

NyxCode commented 6 months ago

I think we don't need it since removing the special generics handeling and replacing it with our dummy types.

NyxCode commented 6 months ago
#[derive(TS)]
#[ts(export, export_to = "concrete_generic/simple")]
#[ts(concrete(T = i32))]
struct Simple<T> {
    t: T,
}

works, but

#[derive(TS)]
#[ts(export, export_to = "concrete_generic/simple")]
#[ts(concrete(T = i32))]
struct WithOption<T> {
    opt: Option<T>,
}

doesn't, as soon as the #[ts(concrete)] is added. Not sure yet why, though.

NyxCode commented 6 months ago

Yeah - That's a case where we'd need a Option<T>: TS or T: TS bound

NyxCode commented 6 months ago

And for

#[derive(TS)]
#[ts(export, export_to = "concrete_generic/issue_261", concrete(T = TsDriver))]
struct Consumer1<T: Driver> {
    info: T::Info,
}

we don't have any bounds (we'd need a where T::Info: TS bound), so that still requires that : TS bound in the definition of Driver:

trait Driver {
    type Info: TS;
}
escritorio-gustavo commented 6 months ago

we don't have any bounds (we'd need a where T::Info: TS bound), so that still requires that : TS bound in the definition of Driver:

trait Driver {
    type Info: TS;
}

Hm I thought that having Info: TS in Driver was the desired behavior

NyxCode commented 6 months ago

Hm I thought that having Info: TS in Driver was the desired behavior

It's maybe acceptable, but it'd be much nicer if that wasn't required.
If that bound was on the impl instead, then it'd be okay to have Drivers whose Info was not TS.
Our TS impl would then be more specific - Before, it covered <D: Driver> SomeType<D>, and then, it'd only cover <D: Driver> SomeType<D> where D::Info: TS.

Imagine

struct MyStruct<I: Iterator> {
    item: I::Item
}
NyxCode commented 6 months ago

Maybe we could give the "For every field, add its type to the where clause" approach an other try.
Maybe it works as a starting point, and we're only left trying to not add bounds for self-referential fields, e.g where Option<Self>: TS.

escritorio-gustavo commented 6 months ago

The export_to on the new tests was missing the trailing /

escritorio-gustavo commented 6 months ago

Maybe we could give the "For every field, add its type to the where clause" approach an other try. Maybe it works as a starting point, and we're only left trying to not add bounds for self-referential fields, e.g where Option<Self>: TS.

I have reverted the commit where I did the extra_ts_bounds stuff to (hopefully) make things easier

escritorio-gustavo commented 6 months ago

For

#[derive(TS)]
struct Simple {
    t: &'static Simple
}

this naive approach generates

impl TS for Simple where &'static Simple: TS { ... }

which sends the trait solver into an infinite loop.

I think the problem here is that the bounds should only be generated for generic types and their associated types, not the type of every field

escritorio-gustavo commented 6 months ago

Maybe the where clause needs to be generated inside type_def for this to be possible

NyxCode commented 6 months ago

I think that's true! Lemme quickly generate the bounds like I did originally, and then we can see which bounds we can omit to prevent that.

escritorio-gustavo commented 6 months ago

I think that's true! Lemme quickly generate the bounds like I did originally, and then we can see which bounds we can omit to prevent that.

Maybe filter on types whose path first segment is one of the generic identifiers (e.g.: T::AssocType, T) or types that contain those (e.g.: Option<T::AssocType>)

NyxCode commented 6 months ago

Alright, now everything works but self_referential.rs.
I add bounds for

Maybe filter on types whose path first segment is one of the generic identifiers (e.g.: T::AssocType, T) or types that contain those (e.g.: Option)

That might work! You wanna give that a try, or should I?

escritorio-gustavo commented 6 months ago

Maybe filter on types whose path first segment is one of the generic identifiers (e.g.: T::AssocType, T) or types that contain those (e.g.: Option<T::AssocType>)

I've been tinkering with a possible filter function and have come up with this:

fn filter_ty(ty: &Type, generic_idents: &[Ident]) -> bool {
    use syn::{PathArguments as P, GenericArgument as G};
    match ty {
        Type::Array(TypeArray { elem, .. }) |
        Type::Paren(TypeParen { elem, .. }) |
        Type::Reference(TypeReference { elem, .. }) |
        Type::Slice(TypeSlice { elem, .. }) => filter_ty(elem, generic_idents),
        Type::Tuple(TypeTuple { elems, .. }) => elems.iter().any(|x| filter_ty(x, generic_idents)),
        Type::Path(TypePath { qself: None, path }) => {
            let first_segment = path
                .segments
                .first()
                .expect("All paths have at least one segment");

            if generic_idents.contains(&first_segment.ident) {
                return true;
            }

            let last_segment = path
                .segments
                .last()
                .expect("All paths have at least one segment");

            return match last_segment.arguments {
                P::None => false,
                P::AngleBracketed(AngleBracketedGenericArguments {
                    ref args, ..
                }) => args
                        .iter()
                        .filter_map(|x| match x {
                            G::Type(ty) => Some(filter_ty(ty, generic_idents)),
                            _ => None,
                        })
                        .any(std::convert::identity),
                P::Parenthesized(_) => todo!(),
            }
        },
        _ => false,
    }
}
NyxCode commented 6 months ago

Amazing, that works! I'll write a few more tests to see if there are edge-cases where I think they are.

Very nice, thanks!

escritorio-gustavo commented 6 months ago

Amazing, that works! I'll write a few more tests to see if there are edge-cases where I think they are.

Very nice, thanks!

There are a few I'm working on!

Option<T> will generate where Option<T>: TS, T: TS (T, U) will generate (T, U): TS

I am fixing these, but I'm still getting some duplicates image

escritorio-gustavo commented 6 months ago

Turns out the duplication is from having both our versions still there xD