-
Notifications
You must be signed in to change notification settings - Fork 116
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
Introduce TypeList, always export Dependencies as well #221
Conversation
@escritorio-gustavo Here's a proof of concept of the TypeList. |
Just to illustrate what this would enable: fn export_dependencies<T: TS>() {
struct Exporter;
impl Visitor for Exporter {
fn visit<T: TS>(&mut self) {
T::export();
}
}
T::dependency_types().for_each(&mut Exporter);
} |
This looks awesome! Absolutely love it! |
I am very suprised that that's the case, but it seems that I ran that check over most of the test suite, and added a few contrived tests as well, but I cant manage to break it. |
ts-rs/src/lib.rs
Outdated
fn dependencies() -> Vec<Dependency> where Self: 'static { | ||
use crate::typelist::TypeVisitor; | ||
|
||
let mut deps: Vec<Dependency> = vec![]; | ||
struct Visit<'a>(&'a mut Vec<Dependency>); | ||
impl<'a> TypeVisitor for Visit<'a> { | ||
fn visit<T: TS + 'static + ?Sized>(&mut self) { | ||
if let Some(dep) = Dependency::from_ty::<T>() { | ||
self.0.push(dep); | ||
} | ||
} | ||
} | ||
Self::dependency_types().for_each(&mut Visit(&mut deps)); | ||
|
||
/// `true` if this is a transparent type, e.g tuples or a list. | ||
deps | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now completely removed the generation for TS::dependencies()
from the macro crate.
I kept the dependencies()
function here for now though, with a default implementation (that's not overwritten anywhere), which converts the new TypeList
into the old Vec<Dependency>
.
ts-rs/src/typelist.rs
Outdated
impl TypeList for () { | ||
fn contains<C: Sized>(self) -> bool { false } | ||
fn for_each(self, _: &mut impl TypeVisitor) {} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty TypeList
is just ()
. That's why TS::dependency_types() -> impl TypeList
can have a default implementation with an empty body, since that evaluates to ()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
ts-rs/src/typelist.rs
Outdated
impl<T> TypeList for (PhantomData<T>,) where T: TS + 'static + ?Sized { | ||
fn contains<C: Sized + 'static>(self) -> bool { | ||
TypeId::of::<C>() == TypeId::of::<T>() | ||
} | ||
|
||
fn for_each(self, v: &mut impl TypeVisitor) { | ||
v.visit::<T>(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A TypeList
with just one element is represented by the tuple (PhantomData<T>,)
.
ts-rs/src/typelist.rs
Outdated
impl<A, B> TypeList for (A, B) where A: TypeList, B: TypeList { | ||
fn contains<C: Sized + 'static>(self) -> bool { | ||
self.0.contains::<C>() || self.1.contains::<C>() | ||
} | ||
|
||
fn for_each(self, v: &mut impl TypeVisitor) { | ||
self.0.for_each(v); | ||
self.1.for_each(v); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, here is where the induction / recursion is happening:
For any two TypeLists
A
and B
, (A, B)
is a TypeList
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's honestly the most clever use of the type system I've seen! Especially given how Rust usually doesn't like recursive data structures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆 I think I've done much more insane things. For example, you can do real arithmetic in in the type system alone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow... just wow
ts-rs/src/export.rs
Outdated
/// Exports `T` to the file specified by the `#[ts(export_to = ..)]` attribute. | ||
/// Additionally, all dependencies of `T` will be exported as well. | ||
/// TODO: This might cause a race condition: | ||
/// If two types `A` and `B` are `#[ts(export)]` and depend on type `C`, | ||
/// then both tests for exporting `A` and `B` will try to write `C` to `C.ts`. | ||
/// Since rust, by default, executes tests in paralell, this might cause `C.ts` to be corrupted. | ||
pub(crate) fn export_type_with_dependencies<T: TS + ?Sized + 'static>() -> Result<(), ExportError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now added this function, which exports T
and recursively all of its dependencies.
The next step here should probably be to actually cause a race condition, and then figure out how to fix it (maybe by putting a static MUTEX: Mutex<()>
in TS
? Or run the tests with RUST_TEST_THREADS=1
to disable paralell execution?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah - that fixed both E2E tests ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah that's gonna be a hard one to handle! Especially since the compiler, which usually protects us from race conditions, can't do much for us here in test land
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly enough, I cant get it to fail, even with 16 types.
I think one easy solution might be to first write to a temp file, and then rename it. Gotta look into that, but I think renaming is atomic on all platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah - that fixed both E2E tests ^^
What fixed it? The mutex or the RUST_TEST_THREADS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing! I just tried to provoke the error, by exporting lots of types and setting RUST_TEST_THREADS=16
, but nothing broke. Maybe that's because the files are relatively small, and get written in just one syscall. I'll try exporting larger types, maybe that'll break it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, idk. I made the types now 17MB large with 2 million number
s in them. cargo test
still refuses to corrupt them..
I too refuse to believe my code is ever working first time xD |
It didn't fail CI! Is this somehow just thread safe? lol |
Really curious about this as well. My theory is the following
It's hard to find a definite resource on this. |
Yeah, I'm trying to read on |
An easy fix for the concurrent write issue would be to change pub(crate) fn export_type_to<T: TS + ?Sized + 'static, P: AsRef<Path>>(
path: P,
) -> Result<(), ExportError> {
static FILE_LOCK: Mutex<()> = Mutex::new(());
// ...
let lock = FILE_LOCK.lock().unwrap();
std::fs::write(path.as_ref(), buffer)?;
drop(lock);
Ok(())
} That lock would be shared by every file export, so no two files would be written concurrently ever. |
Though that issue is purely theoretical right now, so we could just do it & see if any1 encounters an issue - idk. |
Wait, this works? I always thought the mutex had to be shared between threads (hence the whole |
Or am I just missing something? |
No, I don't think that's right. Because it's
The reference can be static because the Now, I had to look that up, but that the function
|
You often see let x: &'static Mutex<()> = Box::leak(Box::new(Mutex::new(()))); Then, just like the |
Oh now I get it! And this is definetly correct, I set up a small test to check with a function that sleeps and 20 tests calling it with and without the Mutex and also switching With |
In this test I also created a file with 20 paragraphs of lorem ipsum. Even without the Mutex I couldn't cause it to corrupt the file either |
Okay, I've got the following: #[cfg(test)]
fn do_it() {
std::fs::write(PATH, CONTENT).unwrap();
}
#[test]
fn write_1() {
for i in 0..10 {std::thread::spawn(|| do_it());}
}
#[test]
fn write_2() {
for i in 0..10 {std::thread::spawn(|| do_it());}
}
#[test]
fn write_3() {
for i in 0..10 {std::thread::spawn(|| do_it());}
}
// ...
#[test]
fn write_20() {
for i in 0..10 {std::thread::spawn(|| do_it());}
}
const PATH: &str = "./foo.txt";
const CONTENT: &str = "VERY LONG STRING"; |
This is producing an empty file |
Alright, awesome! That makes me feel less bad putting a Mutex in there! |
Hold up, the way I did it there breaks, but when I join the threads, it's fine #[test]
fn write_1() {
let mut v = vec![];
for i in 0..10 {
v.push(std::thread::spawn(|| do_it()));
}
for i in v {
i.join();
}
} |
Sitll even if |
@@ -2,5 +2,5 @@ use ts_rs::TS; | |||
|
|||
#[derive(TS)] | |||
pub struct Crate1 { | |||
pub x: i32 | |||
pub x: [[[i32; 128]; 128]; 128], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how the fact that this generates stupidly large TS types ended up being useful lol
From my side this is ready for merge! Again, huge thanks for exploring the problem space with me. |
It really is! I love to have someone to about this kinda stuff with! |
I think it's ready for merging as well, though we have to remember to also merge #218 after this, as this is going to the |
Alright! A bit akward, but whatever ^^ |
Something which I completely forgot to pay attention to are self-referential structs!
So what failed before with a stack overflow now fails at compile time - That's pretty cool, i think!
Also, I realized that both 7.1.1 and master accept |
Found something interesting: #[derive(TS)]
struct X<T> {
a: T,
b: Vec<X<(i32, T)>>
} fails to compile with
Tested both on 7.1.1 and master, so this is not a regression we introduces (recently). Might be interesting to find out why it's happening though. In a perfect world, it should compile and product type X<T> = {
a: T,
b: Array<X<[number, T]>>,
} Will open an issue to keep track of this. |
Agreed, if something is not gonna work it's far better to know about it at compile time
Yeah, |
// Lock to make sure only one file will be written at a time. | ||
// In the future, it might make sense to replace this with something more clever to only prevent | ||
// two threads from writing the **same** file concurrently. | ||
static FILE_LOCK: Mutex<()> = Mutex::new(()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@escritorio-gustavo Regarding the lock we introduced here:
I've experimented with replacing this with a static EXPORTED: Mutex<Option<HashSet<PathBuf>>>
to track which files were written, and to skip an export if the file was already written to.
The case in our test suite where this is most helpfull is generics.rs
, where 30 distinct types are being exported while 37 unnecesarry exports are being skipped.
That being said, I don't think we actually need to do that. generics.rs
ran in 50ms before, and runs in 32ms after this (though with much higher variance). That seems plenty fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely seems fast enough as is, but if we can reduce the amount of fs writes without any negative impact, I think it's a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe push that into #295 as a part of the test performance improvements? That PR is more focused on improving build time, but improving runtime is also great!
No description provided.