Stebalien / tempfile

Temporary file library for rust
http://stebalien.com/projects/tempfile-rs
Apache License 2.0
1.15k stars 113 forks source link

The generic version of NamedTempFile can break compilation #224

Closed Erk- closed 1 year ago

Erk- commented 1 year ago

The more generic version of NamedTempFile introduced in #177 by @jasonwhite can break compilation in some cases without even being involved itself. It seems to happen in some cases with generic where it will turn into a infinitely recursive type.

Example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=671872049ef51f20f8bcc976177c6b62

if you comment the line use tempfile; the code will compile as expected.

edit: Made the MRE smaller.

jasonwhite commented 1 year ago

Sorry about that! I had been using the generic version of NamedTempFile for months without any issues in a huge code base.

I suspect it is triggered by this impl: https://github.com/Stebalien/tempfile/blob/7c77c198f1d3d28b86ee759342cc1d61dffaf521/src/file/mod.rs#L940-L951

Then, the for<'a> &'a T: Write bound has some weird interaction with it. I'll take a closer look today.

jasonwhite commented 1 year ago

Here's another reproducer (without tempfile): https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7682acbe9f55d9c8195877f86c4a3301

Ultimately, it seems that this is an issue with the compiler not supporting cycles in trait bounds: https://github.com/rust-lang/rust/issues/96634

I think the easiest way to fix this is to change this

impl<'a, F> Write for &'a NamedTempFile<F> 
where 
   &'a F: Write, 
{
   // ...
}

to this:

impl Write for &NamedTempFile<File> {
   // ...
}

This makes it more specialized, but keeps the same requirements the same as before NamedTempFile was given a generic parameter. So, I doubt this specialization will break any code.

jasonwhite commented 1 year ago

@Erk- Also, I am curious why T: Write + Read is not sufficient for your needs. It works perfectly fine in the example you linked.

Erk- commented 1 year ago

@Erk- Also, I am curious why T: Write + Read is not sufficient for your needs. It works perfectly fine in the example you linked.

It was from a larger piece of code where T is wrapped in a Arc so if we want to be able to read and write through it we need that bound.

And thanks for the quick fix