1

I'm trying to implement an abstraction that allows me to read from either a directory or a zip file. I start by implementing something of this sort:

pub trait FileOpener<'a> {
    type ReaderType: Read;

    fn open(&'a self, file_name: &str) -> Result<Self::ReaderType, Box<dyn Error>>;
}

pub struct DirectoryFileOpener<'a> {
    root: &'a Path
}

impl<'a> DirectoryFileOpener<'a> {
    pub fn new(root: &'a Path) -> Self {
        DirectoryFileOpener { root }
    }
}

impl<'a> FileOpener<'a> for DirectoryFileOpener<'a> {
    type ReaderType = File;

    fn open(&'a self, file_name: &str) -> Result<File, Box<dyn Error>> {
        Ok(File::open(self.root.join(file_name))?)
    }
}

But then I realize that the zip-rs package's zip::ZipFile is constructed from a mutable reference to the zip::ZipArchive which it is located in, so I end up with the following code:

use std::path::Path;
use std::error::Error;
use std::fs::File;
use std::io::prelude::*;
use zip::{ZipArchive, read::ZipFile};
use std::marker::PhantomData;

pub trait FileOpener<'a> {
    type ReaderType: Read;

    fn open(&'a mut self, file_name: &str) -> Result<Self::ReaderType, Box<dyn Error>>;
}

pub struct DirectoryFileOpener<'a> {
    root: &'a Path
}

impl<'a> DirectoryFileOpener<'a> {
    pub fn new(root: &'a Path) -> Self {
        DirectoryFileOpener { root }
    }
}

impl<'a> FileOpener<'a> for DirectoryFileOpener<'a> {
    type ReaderType = File;

    fn open(&'a mut self, file_name: &str) -> Result<File, Box<dyn Error>> {
        Ok(File::open(self.root.join(file_name))?)
    }
}

pub struct ZipFileOpener<'a, R: Read + Seek> {
    zip: ZipArchive<R>,
    phantom: PhantomData<&'a Self>
}

impl<'a, R: Read + Seek> ZipFileOpener<'a, R> {
    pub fn new(zip: ZipArchive<R>) -> Self {
        ZipFileOpener { zip, phantom: PhantomData }
    }
}

impl<'a, R: Read + Seek> FileOpener<'a> for ZipFileOpener<'a, R> {
    type ReaderType = ZipFile<'a>;

    fn open(&'a mut self, file_name: &str) -> Result<ZipFile<'a>, Box<dyn Error>> {
        Ok(self.zip.by_name(file_name)?)
    }
}

I'm not sure if that's the most optimal way to write that, but at least it compiles. Then I try to use it as such:

fn load(root: &Path) -> Result<...> {
        let mut opener = io::DirectoryFileOpener::new(root);
        let a = Self::parse_a(opener.open("a.txt")?)?;
        let b = Self::parse_b(opener.open("b.txt")?, a)?;
}

and I get cannot borrow 'opener' as mutable more than once at a time. This does not surprise me much, as I indeed use open(), which borrows opener as mutable, twice - although a is only a u64, and from my point of view it is unrelated to the lifetime of opener.open(), from the compiler's point of view it has to be in the same lifetime of the line below it, and thus we attempt to borrow opener as mutable twice.

However, I then look at the following code, which compiles and works well and which I started this whole thing by trying to improve:

fn load_zip(root: &Path) -> Result<...> {
    let file = File::open(root)?;
    let mut zip = ZipArchive::new(file)?;
    let a = Self::parse_a(zip.by_name("a.txt")?)?;
    let b = Self::parse_b(zip.by_name("b.txt")?, a)?;
}

This throws me off completely, because the function by_name() also borrows zip as mutable, and is also called twice! Why is it allowed to borrow zip as mutable twice here but not in the previous case?

leetrobot
  • 153
  • 1
  • 8
  • You don't show the declaration of `parse_a` or `parse_b`. Those may be important, too. – rodrigo Jul 25 '20 at 13:59
  • I didn't include them since they are very long, but they can essentially be replaced with something that consumes the reader and returns some output based on the file's data (e.g. return the 5th byte). Also, how could any possible implementation of those functions make the second use case compile and the first one to fail? They are essentially receiving the same argument (an object implementing the Read trait) – leetrobot Jul 25 '20 at 14:10
  • Does this answer your question? [Cannot borrow as mutable more than once at a time in one code - but can in another very similar](https://stackoverflow.com/questions/31067031/cannot-borrow-as-mutable-more-than-once-at-a-time-in-one-code-but-can-in-anoth) – trent Jul 25 '20 at 14:14
  • 1
    tl;dr the duplicate: `&'a self` is usually a mistake. `&'a mut self` is virtually always a mistake. Just remove those `'a`s; they overconstrain the lifetimes. – trent Jul 25 '20 at 14:16
  • I see. I understand what you're saying, and it does make sense to remove them. However, this means that I cannot declare e.g. `type ReaderType = ZipFile<'a>;` in the trait implementation because the lifetime of each ZipFile returned by open() is different! How can I tell the FileOpener trait that open() returns Self::ReaderType with different lifetimes? (also, do you wanna post your answer as an answer so that I can mark it as working?) – leetrobot Jul 25 '20 at 14:20
  • See also [Linking the lifetimes of self and a reference in method](/q/30273850/3650362) – trent Jul 25 '20 at 14:23
  • It depends on whether the `'a` in `ZipFile<'a>` is *logically* related to the lifetime parameter of `ZipFileOpener` or to the borrow of `self`. It looks like it's the second thing (just glancing over the API docs), which raises the question of why you have a lifetime parameter on `ZipFileOpener` *at all* because it's only used in the overconstrained `impl` . I'd *guess* you're looking for something like [this](https://gist.github.com/rust-play/5e0b767b777095065432b3c49f75c8d1) (untested). – trent Jul 25 '20 at 14:34
  • (Note the linked code does not fall foul of the "`&'a mut self` is virtually always a mistake" rule because `'a` is not a lifetime parameter of the struct, but only of the trait. `'a` is effectively a fresh lifetime that can be chosen differently at each invocation.) – trent Jul 25 '20 at 14:37
  • this last bit is what completed the puzzle for me. I was not aware that impl<'a> on a trait is not bound to the structure, and thus allows you to re-set 'a on trait types for each call to the trait method. I still think this is worth formatting into an independent answer to be accepted. – leetrobot Jul 25 '20 at 14:45
  • Would you agree that [How do I specify lifetime parameters in an associated type?](https://stackoverflow.com/questions/33734640/how-do-i-specify-lifetime-parameters-in-an-associated-type) essentially covers the problem here? I see you found your own solution; this is mostly about linking questions together to help future readers. – trent Sep 09 '20 at 15:12
  • It's a different problem but they are closely related since both revolve around specific cases in GAT, though it's probably relevant to have it linked here – leetrobot Sep 09 '20 at 15:25

1 Answers1

1

After researching the issue and Rust's semantics deeper, and building on top of the notes by trentcl, I came to realize that the problem essentially boils down to defining the FileOpener trait where the lifetime argument is bound to the associated type and not to the trait itself, e.g.

pub trait FileOpener {
    type ReaderType: Read;

    fn open(&'a mut self, file_name: &str) -> Result<Self::ReaderType, Box<dyn Error>>;
}


impl<'a, R: Read + Seek> FileOpener for ZipFileOpener<R> {
    type ReaderType = ZipFile<'a>;
    ...
}

However, this is known as generic associated types (GAT), and is not yet supported in Rust. The GAT RFC does however mention that in some cases the problem can be circumvented by binding the lifetime to the trait itself and using higher-rank trait bounds (HRTB) in the receiving function, which yields the following working solution to this question:

pub trait FileOpener<'a> {
    type ReaderType: Read;

    fn open(&'a self, file_name: &str) -> Result<Self::ReaderType, Box<dyn Error>>;
}

...

fn load<T: for<'a> FileOpener<'a>>(opener: T) -> ... {
    let a = parse_a(opener.open("a.txt")?)?;
    let b = parse_b(opener.open("b.txt")?, a)?;
}

This is because the HRTB allows us to bind T to a FileOpener without binding a specific lifetime to it, which enables the late binding of different lifetimes for each call to opener.open()

leetrobot
  • 153
  • 1
  • 8