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

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

Merged
merged 8 commits into from
Jan 30, 2024
Merged

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

merged 8 commits into from
Jan 30, 2024

Conversation

escritorio-gustavo
Copy link
Contributor

Fixes #137

@escritorio-gustavo
Copy link
Contributor Author

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

@escritorio-gustavo escritorio-gustavo added the enhancement New feature or request label Jan 26, 2024
@NyxCode
Copy link
Collaborator

NyxCode commented Jan 29, 2024

Cool! A couple of points though:

  • I feel like we'd definetely need a limit here. Just imagine
    struct RgbIcon {
        data: [u8; 256 * 256 * 3];
    }
  • Why was it necessary to alter the impl TS for Vec? Don't quite understand that one yet.
    Edit: Oh, seems like it was just re-ordered. My bad :D

@NyxCode
Copy link
Collaborator

NyxCode commented Jan 29, 2024

  • We don't implement TS for all tuples, so this would fail for large arrays, right?
  • Could we alternatively just alter the impl for [T; N]?
    (impl_shadow!(as Vec<T>: impl<T: TS, const N: usize> TS for [T; N]);)

@escritorio-gustavo
Copy link
Contributor Author

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

escritorio-gustavo commented Jan 30, 2024

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

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

@escritorio-gustavo
Copy link
Contributor Author

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

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

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

ts-rs/src/lib.rs Outdated Show resolved Hide resolved
@escritorio-gustavo
Copy link
Contributor Author

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

@@ -278,5 +278,6 @@ fn trait_bounds() {
t: [T; N],
}

assert_eq!(D::<&str, 41>::decl(), "type D<T> = { t: Array<T>, }")
let ty = format!("type D<T> = {{ t: [{}], }}", "T, ".repeat(41).trim_end_matches(", "));
Copy link
Collaborator

Choose a reason for hiding this comment

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

LOL 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I saw the 41 and I went "naaah" lol

@NyxCode
Copy link
Collaborator

NyxCode commented Jan 30, 2024

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

NyxCode commented Jan 30, 2024

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

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

NyxCode commented Jan 30, 2024

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

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

@escritorio-gustavo escritorio-gustavo merged commit 50f74b5 into Aleph-Alpha:main Jan 30, 2024
2 checks passed
@escritorio-gustavo escritorio-gustavo deleted the array_as_ts_tuple branch January 30, 2024 19:25
@NyxCode
Copy link
Collaborator

NyxCode commented Jan 30, 2024

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.

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 this pull request may close these issues.

Allow emitting arrays as TS tuples
2 participants