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

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

Merged
merged 30 commits into from
Mar 13, 2024
Merged

Conversation

NyxCode
Copy link
Collaborator

@NyxCode NyxCode commented Mar 11, 2024

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

  • Cleanup
  • Figure out how exactly TS::decl and TS::decl_concrete should behave
    • decl is clear, and decl_concrete still uses the actual type parameters, even if #[ts(concrete)] is present.
  • Write more tests
  • How does this intersect with dependency tracking?
    • not at all. We end up with less types in TS::generics though.

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 11, 2024

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

  • decl is pretty clear, I think. For all remaining generic parameters, we generate dummys like we currently do. For disabled/concrete generic parameters, we use the type the user specified
  • For decl_concrete, we should use all remaining generic parameters like they are and not swap them out for dummys (it's what we currently do).
    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?

@escritorio-gustavo

This comment was marked as resolved.

@escritorio-gustavo

This comment was marked as resolved.

@escritorio-gustavo

This comment was marked as resolved.

@NyxCode NyxCode changed the title #[ts(discrete)]: Allow generic parameter to be "disabled" #[ts(concrete)]: Allow generic parameter to be "disabled" Mar 11, 2024
@escritorio-gustavo

This comment was marked as outdated.

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 11, 2024

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<AssociatedType = MyType> 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

This comment was marked as resolved.

macros/src/lib.rs Outdated Show resolved Hide resolved
macros/src/lib.rs Outdated Show resolved Hide resolved
@escritorio-gustavo escritorio-gustavo linked an issue Mar 11, 2024 that may be closed by this pull request
@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 11, 2024

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

This comment was marked as resolved.

@escritorio-gustavo

This comment was marked as resolved.

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 11, 2024

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

This comment was marked as resolved.

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 11, 2024

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

This comment was marked as outdated.

@escritorio-gustavo

This comment was marked as outdated.

@escritorio-gustavo

This comment was marked as outdated.

@escritorio-gustavo

This comment was marked as outdated.

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 12, 2024

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
Copy link
Contributor

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

NyxCode commented Mar 12, 2024

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

NyxCode commented Mar 12, 2024

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

This comment was marked as resolved.

CHANGELOG.md Outdated Show resolved Hide resolved
@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 13, 2024

Is there anything you think still needs to be done here? I'm pretty happy with where this ended up, especially with the new trait bounds 😊

@escritorio-gustavo
Copy link
Contributor

Is there anything you think still needs to be done here?

There is nothing I can think of, from my side this is ready for merge. @WilsonGramer in its current state does this PR fully resolve your issue?

I'm pretty happy with where this ended up, especially with the new trait bounds 😊

Me too! And creating that filter function turned out to be a lot of fun :D

@@ -4,7 +4,7 @@ mod issue_261 {
use ts_rs::TS;

trait Driver {
type Info: TS;
type Info;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to show : TS is no longer needed here

@escritorio-gustavo
Copy link
Contributor

@WilsonGramer in its current state does this PR fully resolve your issue?

I think it may be a while until he responds, maybe we should merge this and if needed @WilsonGramer can open a new issue. This PR is already getting way too long anyway

@WilsonGramer
Copy link
Contributor

@escritorio-gustavo @NyxCode Sorry for the delay, I will test it now!

@WilsonGramer
Copy link
Contributor

@escritorio-gustavo It looks like this example with nested structs fails to compile:

use ts_rs::TS;

trait Driver {
    type Info;
}

struct TsDriver;

#[derive(TS)]
struct TsInfo;

impl Driver for TsDriver {
    type Info = TsInfo;
}

#[derive(TS)]
#[ts(export, concrete(D = TsDriver))]
struct Inner<D: Driver> {
    info: D::Info,
}

#[derive(TS)]
#[ts(export, concrete(D = TsDriver))]
struct Outer<D: Driver> {
    inner: Inner<D>,
}
error[E0277]: the trait bound `<D as Driver>::Info: ts_rs::TS` is not satisfied
  --> src/lib.rs:25:12
   |
25 |     inner: Inner<D>,
   |            ^^^^^^^^ the trait `ts_rs::TS` is not implemented for `<D as Driver>::Info`
   |
note: required for `Inner<D>` to implement `ts_rs::TS`
  --> src/lib.rs:16:10
   |
16 | #[derive(TS)]
   |          ^^ unsatisfied trait bound introduced in this `derive` macro
   = note: this error originates in the derive macro `TS` (in Nightly builds, run with -Z macro-backtrace for more info)

I think it's adding the bound to D because it thinks D is being used in Inner, when it's actually only the associated type that's being used. I ran into this issue in my compiler because I have, for example, an Expression<D> that contains a Type<D> — both types only use D::Info.

@WilsonGramer
Copy link
Contributor

If I add a Info: TS bound in the trait, I get another error:

use ts_rs::TS;

trait Driver {
    type Info: TS;
}

struct TsDriver;

#[derive(TS)]
struct TsInfo;

impl Driver for TsDriver {
    type Info = TsInfo;
}

#[derive(TS)]
#[ts(export, concrete(D = TsDriver))]
struct Inner<D: Driver> {
    info: D::Info,
}

#[derive(TS)]
#[ts(export, concrete(D = TsDriver))]
struct Outer<D: Driver> {
    inner: Inner<D>,
}
error[E0277]: the trait bound `TsDriver: ts_rs::TS` is not satisfied
   --> src/lib.rs:22:10
    |
22  | #[derive(TS)]
    |          ^^ the trait `ts_rs::TS` is not implemented for `TsDriver`
    |
    = help: the following other types implement trait `ts_rs::TS`:
              bool
              char
              isize
              i8
              i16
              i32
              i64
              i128
            and 66 others
note: required for `Outer<TsDriver>` to implement `ts_rs::TS`
   --> src/lib.rs:22:10
    |
22  | #[derive(TS)]
    |          ^^ unsatisfied trait bound introduced in this `derive` macro
note: required by a bound in `ts_rs::TS::WithoutGenerics`
   --> /Users/wilson/.cargo/git/checkouts/ts-rs-092fe627e6f08b9c/95e6dac/ts-rs/src/lib.rs:332:27
    |
332 |     type WithoutGenerics: TS + ?Sized;
    |                           ^^ required by this bound in `TS::WithoutGenerics`
    = note: this error originates in the derive macro `TS` (in Nightly builds, run with -Z macro-backtrace for more info)

I believe this is the same issue — it thinks D is used directly because it's passed as a generic parameter to another type.

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Mar 13, 2024

I am not sure there's anything we can do to automatically fix this, since that would require struct Outer<D: Driver> where D::Info: TS even though Outer doesn't directly contain D::Info, but at least the following code compiles:

use ts_rs::TS;

trait Driver {
    type Info;
}

#[derive(TS)]
struct TsDriver;

#[derive(TS)]
struct TsInfo;

impl Driver for TsDriver {
    type Info = TsInfo;
}

#[derive(TS)]
#[ts(export, concrete(D = TsDriver))]
struct Inner<D: Driver> {
    info: D::Info,
}

#[derive(TS)]
#[ts(export, concrete(D = TsDriver))]
struct Outer<D: Driver>
where
    D::Info: TS,
{
    inner: Inner<D>,
}

You'd need to derive TS for TsDriver and add the where clause I mentioned manually

@escritorio-gustavo
Copy link
Contributor

Alternatively, this also compiles:

use ts_rs::TS;

trait Driver {
    type Info: TS;
}

#[derive(TS)]
struct TsDriver;

#[derive(TS)]
struct TsInfo;

impl Driver for TsDriver {
    type Info = TsInfo;
}

#[derive(TS)]
#[ts(export, concrete(D = TsDriver))]
struct Inner<D: Driver> {
    info: D::Info,
}

#[derive(TS)]
#[ts(export, concrete(D = TsDriver))]
struct Outer<D: Driver> {
    inner: Inner<D>,
}

@escritorio-gustavo
Copy link
Contributor

Either way, you need TsDriver to implement TS because D is directly used in Outer

@WilsonGramer
Copy link
Contributor

WilsonGramer commented Mar 13, 2024

@escritorio-gustavo What do you think about adding a #[ts(bound)] attribute to suppress the bounds if needed?

#[derive(TS)]
#[ts(export, concrete(D = TsDriver), bound = "")]
struct Outer<D: Driver> {
    inner: Inner<D>,
}

That way the D::Info: TS bound is only added in the TS impl for Outer, and isn't required for all users of Outer.

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 13, 2024

Yeah, I don't think there's a better trait bound we can generate here.
You'd run into the same issue with serde (literally the exact same error if you replace TS with Serialize).
There, there's a #[serde(bound(..))] workaround to make the trait bounds more specific, though I have never used it.

@escritorio-gustavo
Copy link
Contributor

There, there's a #[serde(bound(..))] workaround to make the trait bounds more specific, though I have never used it.

I've never used it either, I think this should be a brand new issue & PR showing the desired macro expansion, this PR is getting hard to follow

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 13, 2024

@WilsonGramer's snippet

#[derive(TS)]
#[ts(export, concrete(D = TsDriver), bound = "")]
struct Outer<D: Driver> {
    inner: Inner<D>,
}

Here, the bound would still need to be D::Info: TS, because Inner is using impl<D: Driver> TS for Inner<D> where D::Inner: TS. Just leaving it blank would not compile.

My suggestion would be to - unless there's something missing in this PR - merge this first, and tackle a potential #[ts(bound)] separately.

The easiest approach there would be to have #[ts(bound(...)] completely replace our trait bounds. Serde does that more granually on a per-field basis (which I think we could also pretty easily do), but we'll have to see which one makes more sense here.

@WilsonGramer
Copy link
Contributor

I think opening a new PR for the bound attribute makes sense! I can draft an issue later today.

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 13, 2024

Awesome, thanks!

Then we're all set & can merge this?

@NyxCode NyxCode marked this pull request as ready for review March 13, 2024 18:01
@NyxCode NyxCode merged commit 02031e9 into main Mar 13, 2024
14 checks passed
@escritorio-gustavo escritorio-gustavo deleted the concrete branch March 13, 2024 18:04
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.

#[derive(TS)] and associated types
3 participants