Aleph-Alpha / ts-rs

Generate TypeScript bindings from Rust types
MIT License
1.15k stars 115 forks source link

Convert fixed size Rust array to fixed size TS tuple #209

Closed escritorio-gustavo closed 9 months ago

escritorio-gustavo commented 9 months ago

Fixes #137

escritorio-gustavo commented 9 months ago

By the way, @NyxCode, if this feature is not something you want, feel free to close the PR and I'll mark the corresponding issue as not planned

NyxCode commented 9 months ago

Cool! A couple of points though:

NyxCode commented 9 months ago
escritorio-gustavo commented 9 months ago

I feel like we'd definetely need a limit here. Just imagine

 struct RgbIcon {
     data: [u8; 256 * 256 * 3];
 }

You're right, that would be a massive TS file! What do you think a sensible limit would be?

We don't implement TS for all tuples

I didn't realize that was the case, I saw they were converted into a struct with unnamed fields and just assumed they worked for every size, since I couldn't find a limit in macros/src/types/tuple.rs.

Could we alternatively just alter the impl for [T; N]?

Do you mean keep the changes in ts-rs/src/lib.rs and discard the ones in macros/src/types/generics.rs? That makes this change not affect struct fields unless they are #[ts(inline)]d

escritorio-gustavo commented 9 months ago

Do you mean keep the changes in ts-rs/src/lib.rs and discard the ones in macros/src/types/generics.rs? That makes this change not affect struct fields unless they are #[ts(inline)]d

This is due to a mistake I made: my impl<T: TS, const N: usize> TS for [T; N] just returns Array<T>...

escritorio-gustavo commented 9 months ago

One question: Can Vec::<T>::name return Array<T>? Or is there a specific reason for it to return Array?

escritorio-gustavo commented 9 months ago

Playing around with this further is causing conflicts with generic inlining, I'm getting nulls instead of T and when trait bounds like ToString are involved I get string instead of T

escritorio-gustavo commented 9 months ago

Or is there a specific reason for it to return Array?

I think I got it, there's no way to resolve T's possible generics, or if name or inline should be used

escritorio-gustavo commented 9 months ago

This is due to a mistake I made: my impl<T: TS, const N: usize> TS for [T; N] just returns Array

I fixed this, but if I remove the changes to macros/types/generics.rs the issue with inline generics shows up

escritorio-gustavo commented 9 months ago

I managed to remove the conversion to a tuple and resolve the generic inlining problem, still results in a separate case for the array though

NyxCode commented 9 months ago

This is due to a mistake I made: my impl<T: TS, const N: usize> TS for [T; N] just returns Array

I fixed this, but if I remove the changes to macros/types/generics.rs the issue with inline generics shows up

This generics code always gives me a headache. Do you have an example of what exactly breaks with just the impl TS for [T; N] alone?

NyxCode commented 9 months ago

I didn't realize that was the case, I saw they were converted into a struct with unnamed fields and just assumed they worked for every size, since I couldn't find a limit in macros/src/types/tuple.rs.

Yeah, so I'm not aware of any way to implement a trait for all homogeneous tuples of one type. If there was a spread syntax for tuples or something like that, i think it'd be possible to do it inductively/recursively, but there isnt.

escritorio-gustavo commented 9 months ago

This generics code always gives me a headache. Do you have an example of what exactly breaks with just the impl TS for [T; N] alone?

I also get confused a lot, proc macros are really hard to keep up with, but I get the following failing test:

test free ... ok
test newtype ... FAILED
test interface ... FAILED

failures:

---- newtype stdout ----
thread 'newtype' panicked at ts-rs\tests\arrays.rs:24:5:
assertion `left == right` failed
  left: "[]"
 right: "[number, number, number, number]"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- interface stdout ----
thread 'interface' panicked at ts-rs\tests\arrays.rs:16:5:
assertion `left == right` failed
  left: "{ a: [], }"
 right: "{ a: [number, number, number, number], }"

failures:
    interface
    newtype
NyxCode commented 9 months ago

Ah, okay, I think I (kinda) understand it now. Took me a bit to (again) understand how we hacked generics support into the lib ^^. I don't like how it currently works, but any attempt at really fixing it would be super involved i'm afraid.

That being said, this PR is strictly an improvement, so from my side, i'd be happy to merge this. Do you think it's ready, or is there something you'd still like to do?

Great work, btw! Given the technical debt, I like where this ended up - doing most of the work in the [T; N] impl, and only pulling out the T and formatting it to get generics working. Can't think of any better way to do this given the current constraints.

escritorio-gustavo commented 9 months ago

Do you think it's ready, or is there something you'd still like to do?

I think it's ready to merge, as far as I can tell there is nothing else to add for this feature

I don't like how it currently works, but any attempt at really fixing it would be super involved i'm afraid.

Agreed, any refactor in this area of the code would be absolutely massive It would be really helpful if the docs on proc macros were more in depth, this stuff is really hard to figure out at times

NyxCode commented 9 months ago

It would be really helpful if the docs on proc macros were more in depth, this stuff is really hard to figure out at times

I dont disagree with that, though we're doing pretty weird stuff here.
If I would re-write this library from scratch, I think I'd try to put more of the logic into the actual runtime, and less into the proc macros. Basically just have the TS trait to provide an AST for the type, and then doing the actual TypeScript codegen during the test. Not sure if that would work out, but if it did, I'm sure it'd be much nicer. This mixing of runtime and proc-macro-time we're doing ("This function returns a tokenstream that expands to an expression that returns a string ...") is just painfull to work with.