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

Feature request: #[ts(bound)] attribute #268

Closed
WilsonGramer opened this issue Mar 13, 2024 · 13 comments · Fixed by #269
Closed

Feature request: #[ts(bound)] attribute #268

WilsonGramer opened this issue Mar 13, 2024 · 13 comments · Fixed by #269
Labels
enhancement New feature or request

Comments

@WilsonGramer
Copy link
Contributor

WilsonGramer commented Mar 13, 2024

Is your feature request related to a problem? Please describe.
#264 introduces the #[ts(concrete)] attribute, but it can generate unnecessary bounds on the type parameter when it's used "directly" in a type:

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)

ts-rs thinks that a value of type D is used in Outer because it's passed as a parameter to Inner, when in reality Inner only uses D::Info. The generated code looks like this:

impl<D: Driver> ts_rs::TS for Outer<D>
where
    D: ts_rs::TS, // <- unnecessary bound
{
    ...
}

Describe the solution you'd like
Add a #[ts(bound)] attribute to let people specify the bounds on the TS impl manually:

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

Which would replace the auto-generated bounds with the provided string:

impl<D: Driver> ts_rs::TS for Outer<D>
where D::Info: TS,
{
    ...
}

Describe alternatives you've considered
See #261 and #264.

@WilsonGramer WilsonGramer added the enhancement New feature or request label Mar 13, 2024
@NyxCode
Copy link
Collaborator

NyxCode commented Mar 13, 2024

I'm a bit confused why this doesn't need a where D::Info: TS bound.
Is that implied by the fact that the struct contains an Inner<D>, which has the D::Info: TS, and the bound is transferred to Outer?

@NyxCode
Copy link
Collaborator

NyxCode commented Mar 13, 2024

Ah, never mind - thought i was losing my mind here for a second.
That D::Info: TS bound is indeed still required. I've updated the Issue.

@WilsonGramer
Copy link
Contributor Author

@NyxCode You're right, I meant to write this:

impl ts_rs::TS for Outer<TsDriver>
where /* no bounds */
{
    ...
}

...because D is substituted for TsDriver with #[ts(concrete(D = TsDriver), bound = "/* no bounds */"]. You're right that without concrete, the bound would be needed. I might be missing something though.

@WilsonGramer
Copy link
Contributor Author

Actually it looks like #264 preserves the type parameter in the impl block — is that the correct behavior?

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

Generates this:

impl<D: Driver> ts_rs::TS for Outer<D>
where
    D: ts_rs::TS,
{
    type WithoutGenerics = Outer<TsDriver>;

    ...
}

But shouldn't it generate this?

impl ts_rs::TS for Outer<TsDriver>
{
    ...
}

@NyxCode
Copy link
Collaborator

NyxCode commented Mar 13, 2024

Ah, alright, now I see where the confusion came from.

So the original concrete branch started out with impl<D: Driver> TS for MyStruct<D>.
That worked, but required this annoying type Info: TS bound in the definition of Driver.

Then, we tried actually generating impl TS for MyStruct<TsDriver>. However, we hit a hard wall going down that road. If you're interested in the details, see #264. The gist of it is that we generated <TsDriver::Info as TS>::name() inside the impl, which the trait solver refuses to accept. We'd have had to generate <<TsDriver as Driver>::Info as TS>::name() instead, but that's not possible inside the proc macro (as soon as there's more than one trait bound on D)1.

Therefore, we settled on generating impl<D: Driver> TS for MyStruct<D> where D::Info: TS. That gives us all the benifits we'd have gotten with the other approach (namely, get rid of the type Info: TS bound).

Everything works like it was an impl TS for MyStruct<TsDriver>. However, MyStruct<OtherDriver> still implements TS if OtherDriver::Info: TS. If you'd call MyStruct<OtherDriver>::decl() though, it'll behave as-if it was an MyStruct<TsDriver>.

So the tl;dr why it didnt work is: If you access an associated type of a concrete type (e.g TsDriver::Info), Rust doesn't infer which trait it should use to get Info. That makes sense, since it's possible that TsDriver implemented two traits with an associated type Info.

Footnotes

  1. https://github.com/Aleph-Alpha/ts-rs/pull/264#issuecomment-1991687523

@WilsonGramer
Copy link
Contributor Author

@NyxCode What do you think about requiring that the associated types be spelled out completely (ie. <X as T>::Y)?

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

Then I believe Rust would raise an error whenever it can't determine which associated type to use. That would also solve the issue of multiple trait bounds.

@NyxCode
Copy link
Collaborator

NyxCode commented Mar 13, 2024

Absolutely, that'd have worked! With the codegen as-is, which does <{field_type} as TS>::name(), that'd have generated <<D as Driver>::Info as TS>::name() - exactly what we tried to do "by hand".

We didn't see any downside with generating impl<D: Driver> TS for MyStruct<D> where D::Info, and it doesn't require that you touch the type definition at all.

@NyxCode
Copy link
Collaborator

NyxCode commented Mar 13, 2024

Well, there is one downside, which is that you don't get a compiler error if you do MyStruct::<OtherDriver>::export() if it's #[ts(concrete(D = TsDriver))]. Instead, you just get the definition of MyStruct<TsDriver> according to the #[ts(concrete(..)].

Since (we hope) most users just use #[ts(export)], this seemed like a minor issue.

@WilsonGramer
Copy link
Contributor Author

@NyxCode Got it! I just tested the latest changes on main, but I'm still getting the extra bound when I #[derive(TS)] on Outer:

use ts_rs::TS;

trait Driver {
    type Info: TS;
}

struct MyDriver;

#[derive(TS)]
struct MyInfo;

impl Driver for MyDriver {
    type Info = MyInfo;
}

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

#[derive(TS)]
#[ts(export, concrete(D = MyDriver))]
struct Outer<D: Driver> {
    inner: Inner<D>,
}
  impl<D: Driver> ::ts_rs::TS for Outer<D>
  where
+     D: ::ts_rs::TS,
  {
      ...
  }

Basically, the current implementation requires MyDriver itself implements TS, not just its associated types, since it sees that D is used "directly" in Outer — the goal with #[ts(bound = "")] is to remove this bound (since it's not actually necessary that D itself implements TS, only its associated types).

@NyxCode
Copy link
Collaborator

NyxCode commented Mar 13, 2024

Yup, we're on the same page here!

The tradeoff between the two solutions of generating trait bounds we explored turned out to be:

  • Generate perfect bounds, but fail on self-referential types
  • Overestimate bounds, but allow self-referential types

So you're absolutely right here - The generated bound D: TS is wrong, and it needs to be D::Info: TS instead.
And it needs to be D::Info: TS, since the impl is still generic over <D: Driver>, even with #[ts(concrete)].
And I agree, #[ts(bound = "...")] seems perfect for that.

@WilsonGramer
Copy link
Contributor Author

OK great! I am putting together a bound attribute now and will open a PR soon.

@NyxCode
Copy link
Collaborator

NyxCode commented Mar 14, 2024

Awesome, thanks! I think the best place to add it is here. If there's a #[ts(bound)], then we can just skip the bound-generation we do in generate_where_clause.

@WilsonGramer
Copy link
Contributor Author

I just implemented the attribute here: main...WilsonGramer:ts-rs:bound-attribute

Let me know what you think!

@escritorio-gustavo escritorio-gustavo linked a pull request Mar 14, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants