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.

Sign up for GitHub

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

Jump to bottom

Remove TS::trasparent #243

Merged
merged 4 commits into from
Feb 29, 2024
Merged

Remove TS::trasparent #243

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
def7e64
Remove `TS::trasparent`
escritorio-gustavo Feb 29, 2024
59f7062
Update CHANGELOG.md
escritorio-gustavo Feb 29, 2024
38ec2f7
Use TypeList methods + impl_shadow
escritorio-gustavo Feb 29, 2024
5ebe368
Add missing method to impl_shadow
escritorio-gustavo Feb 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion macros/src/deps.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ impl Dependencies {
}

/// Adds the given type.
/// If the type is transparent, then we'll get resolve the child dependencies during runtime.
pub fn push(&mut self, ty: &Type) {
self.0.push(quote![.push::<#ty>()]);
self.0.push(quote![
Expand Down
5 changes: 0 additions & 5 deletions macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,6 @@ impl DerivedTS {
{
#dependencies
}

fn transparent() -> bool {
false
}
}

#export
Expand Down Expand Up @@ -145,7 +141,6 @@ impl DerivedTS {
impl TS for #generics {
type WithoutGenerics = #generics;
fn name() -> String { stringify!(#generics).to_owned() }
fn transparent() -> bool { false }
}
)*
}
Expand Down
7 changes: 0 additions & 7 deletions ts-rs/src/chrono.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ macro_rules! impl_dummy {
type WithoutGenerics = $t;
fn name() -> String { String::new() }
fn inline() -> String { String::new() }
fn transparent() -> bool { false }
}
)*};
}
Expand All @@ -33,9 +32,6 @@ impl<T: TimeZone + 'static> TS for DateTime<T> {
fn inline() -> String {
"string".to_owned()
}
fn transparent() -> bool {
false
}
}

impl<T: TimeZone + 'static> TS for Date<T> {
Expand All @@ -49,7 +45,4 @@ impl<T: TimeZone + 'static> TS for Date<T> {
fn inline() -> String {
"string".to_owned()
}
fn transparent() -> bool {
false
}
}
54 changes: 15 additions & 39 deletions ts-rs/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,12 +367,7 @@ pub trait TS {
struct Visit<'a>(&'a mut Vec<Dependency>);
impl<'a> TypeVisitor for Visit<'a> {
fn visit<T: TS + 'static + ?Sized>(&mut self) {
if T::transparent() {
// the dependency `T` is "transparent", meaning that our original type depends
// on the dependencies of `T` as well.
T::dependency_types().for_each(self);
} else if let Some(dep) = Dependency::from_ty::<T>() {
// the dependency `T` is not transparent, so we just add it to the output
if let Some(dep) = Dependency::from_ty::<T>() {
self.0.push(dep);
}
Comment on lines -370 to 372
Copy link
Collaborator

Choose a reason for hiding this comment

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

I love that this just works. Awesome!

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, since T::transparent is always false we just get rid of that check

}
Expand All @@ -382,10 +377,6 @@ pub trait TS {
deps
}

/// `true` if this is a transparent type, e.g tuples or a list.
/// This is used for resolving imports when using the `export!` macro.
fn transparent() -> bool;

/// Manually export this type to a file.
/// The output file can be specified by annotating the type with `#[ts(export_to = ".."]`.
/// By default, the filename will be derived from the types name.
Expand Down Expand Up @@ -453,7 +444,6 @@ macro_rules! impl_primitives {
type WithoutGenerics = Self;
fn name() -> String { $l.to_owned() }
fn inline() -> String { <Self as $crate::TS>::name() }
fn transparent() -> bool { false }
}
)*)* };
}
Expand All @@ -474,7 +464,6 @@ macro_rules! impl_tuples {
{
()$(.push::<$i>())*
}
fn transparent() -> bool { true }
}
};
( $i2:ident $(, $i:ident)* ) => {
Expand Down Expand Up @@ -504,7 +493,6 @@ macro_rules! impl_wrapper {
{
((std::marker::PhantomData::<T>,), T::generics())
}
fn transparent() -> bool { T::transparent() }
}
};
}
Expand All @@ -529,7 +517,6 @@ macro_rules! impl_shadow {
{
<$s>::generics()
}
fn transparent() -> bool { <$s>::transparent() }
}
};
}
Expand All @@ -554,9 +541,6 @@ impl<T: TS> TS for Option<T> {
{
((std::marker::PhantomData::<T>,), T::generics())
}
fn transparent() -> bool {
T::transparent()
}
}

impl<T: TS, E: TS> TS for Result<T, E> {
Expand All @@ -583,9 +567,6 @@ impl<T: TS, E: TS> TS for Result<T, E> {
((PhantomData::<E>,), E::generics()),
)
}
fn transparent() -> bool {
false
}
}

impl<T: TS> TS for Vec<T> {
Expand All @@ -611,9 +592,6 @@ impl<T: TS> TS for Vec<T> {
{
((std::marker::PhantomData::<T>,), T::generics())
}
fn transparent() -> bool {
false
}
}

// Arrays longer than this limit will be emitted as Array<T>
Expand Down Expand Up @@ -655,10 +633,6 @@ impl<T: TS, const N: usize> TS for [T; N] {
{
((std::marker::PhantomData::<T>,), T::generics())
}

fn transparent() -> bool {
false
}
}

impl<K: TS, V: TS, H> TS for HashMap<K, V, H> {
Expand Down Expand Up @@ -690,9 +664,6 @@ impl<K: TS, V: TS, H> TS for HashMap<K, V, H> {
((PhantomData::<V>,), V::generics()),
)
}
fn transparent() -> bool {
false
}
}

impl<I: TS> TS for Range<I> {
Expand All @@ -705,11 +676,15 @@ impl<I: TS> TS for Range<I> {
where
Self: 'static,
{
().push::<I>()
I::dependency_types()
}

fn transparent() -> bool {
true
fn generics() -> impl TypeList
where
Self: 'static,
{
use std::marker::PhantomData;
((PhantomData::<I>,), I::generics())
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this is equivalent to just returning I::generics().
I'm not sure that's correct, even though the tests pass.
Shouldn't this return ().push::<I>().extend(I::generics())? Right now, I itself would be missing from the TypeList, no?

Ahhh, I got confused by you constructing the TypeList manually here!
Is there a reason you're not doing ().push::<I>().extend(I::generics()) or, even better, I::generics().push::<I>, instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, I just kinda got confused calling methods on () so I did it this way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just kinda got confused calling methods on () so I did it this way

That tripped me up quite a bit in the macros, because the repetition syntax uses parentheses plus the unit type plus the calling of methods

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, yeah, I understand.
I do think it'd be a good idea to keep the implementation of TypeList private, and use the methods on it. That gives us the option to change TypeList is implemented however we like in the future.

In this particular case, I::generics().push::<I>() would probably be the nicest.
To make the code more readable (and keep the implementation detail that the empty TypeList is () hidden), we could add a const EMPTY_TYPELIST: () = () or pub fn new() -> impl TypeList {} to typelist.rs

For the macros, I try to use quote!{} or quote![] to disambiguate them, but yeah, there's still the #()* syntax.

In the end, this is not a big deal, if you've got strong opinions here. Personally, I prefer using the methods on the type.

Edit: I see that you already changed it, thanks! 😆

Being able to call methods on TypeList was the motivation to make dependency_types a function in the first place. It could have been an associated type (e.g type Dependencies = ((A), B), C::Dependencies)), but I liked the idea of being able to pretend that TypeList is just a Vec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of that makes a lot of sense! I'll open another PR to apply this change to the impls I changed in #241, all of which manually build a TypeList


Expand All @@ -723,11 +698,15 @@ impl<I: TS> TS for RangeInclusive<I> {
where
Self: 'static,
{
().push::<I>()
I::dependency_types()
}

fn transparent() -> bool {
true
fn generics() -> impl TypeList
where
Self: 'static,
{
use std::marker::PhantomData;
((PhantomData::<I>,), I::generics())
}
}

Expand Down Expand Up @@ -817,7 +796,4 @@ impl TS for Dummy {
fn name() -> String {
"Dummy".to_owned()
}
fn transparent() -> bool {
false
}
}
Loading