read_zipfile_from_stream takes reference to R: Read, instead of R: Read

[wfehrnstrom]

I'm currently encountering an issue in design of a program that uses the zip crate centered around the function zip::read::read_zipfile_from_stream. I'm using it to create a zipfile struct, however, because read_zipfile_from_stream takes a reference to something implementing the read trait, instead of something taking the read trait itself, I'm finding it impossible to use this function in this instance. That's because I actually pass the ZipFile struct up out of the body where the Vec, and it's corresponding slice, are created that initialize the ZipFile, meaning that I end up invalidating the reference, because the reference refers to the Vec, however, the vec is dropped at the end of the function, whereas the ZipFile is supposed to live beyond that. This issue is intractable because I cannot move where I read the bytes from the stream a level up, and I have designed my own interfaces to take not a reference to something implementing Read, but to borrow something implementing Read. For the reasons above, I need a function exactly like read_zipfile_from_stream, but that does not take a reference. Is this possible to add to the zip crate? Its presence would make possible a much wider range of use cases for read_zipfile_from_stream than are currently possible.

In practice, here's how this occurs:

let buf: Vec<u8> = Vec::new();
// ...Omitted for brevity, but read into buf all of the bytes from some file ...
// Now internally use get_zipfile_from_stream to create a ZipFile from a reference to a buffer
let zipfile: ZipFile = getzipfilefrombufref(&buf[..]);
// whoops, we're about to return something that contains a reference to a buf that is about to go out of // scope
return zipfile;

In practice, it looks like this may be difficult because you've implemented everything in terms of taking references: for instance, your interfaces in podio that are relied upon take mutable references to read. I will think about how this might be overcome.

[mvdnes]

It does not really make sense to take ownership of the reader for most use-cases.
When doing that you can call the iterator function only once, for a single zip file.

Why not just return the Vec and parse the zipfile at a later time? Or use the normal zip parsing methods?

[Plecra]

An owned generic parameter wouldn't need to take ownership of the reader - &mut impl Read: Read. I see no reason not to make this fix - The code changes should be fairly mechanical, but will be easier once we've cleaned up the open PRs.

One little thing is that this will have to wait for the next (pre-)release since it would technically be a breaking change for some edge cases.

[sergiimk]

I also found this API particularly troublesome for composition:

Imagine you have a Reader over a File and you want to wrap it with some sort of ZipDecoder which gives you a Read interface for decompressed file (assuming single-file archive) and then you do some post-processing e.g. Transcode which can also be abstracted with the Read trait...

Having &mut API that returns ZipFile<'a> with lifetime bound to the reader makes composition impossible. You can't have something like:

struct ZipDecoder<R: Read> {
  reader: R,
  zip_file: ZipFile<?> // references reader somehow???
}

because as far as I understand one member holding a &mut to another member of the same struct is impossible in rust.

[BenjaminRi]

I also found this API particularly troublesome for composition:

Yep, I also had this problem. I wanted to create a reader that concatenates multiple files inside a zip file to one contiguous reader. I ended up using unsafe and transmute to achieve what I wanted. The API is certainly somewhat hostile, but I don't know how you could do it better.

[mvdnes]

@Plecra's argument makes sense, this indeed makes more sense.

[Plecra]

We have a slight issue in the implementation: A File that borrows from an Archive is different to one that owns its reader.

I doubt it'll be possible to make them the same type, which means we aren't going to be able to have a Vec<ZipFile> that could store files from both a file and stream. Will this be a problem for anyone?