What are the best practices for multiple mutable and immutable borrows?

Question

I'm developing a function that returns the content of a particular file in a Zip archive. Since I know the location of the file, I try to access it with the ZipArchive.by_name method. However, it is possible that the name of the file is written in a different case. If this happens (FileNotFound), I need to iterate over all files in the archive and perform a case-insensitive comparison with the template. However, in this case I get two errors connected with the borrowing.

Here is the minimal example (I use BarcodeScanner Android app (../test_data/simple_apks/BarcodeScanner4.0.apk) but you can use any apk file, just substitute path to it. You can download one, e.g., on ApkMirror):

use std::{error::Error, fs::File, path::Path};

const MANIFEST_MF_PATH: &str = "META-INF/MANIFEST.MF";

fn main() {
    let apk_path = Path::new("../test_data/simple_apks/BarcodeScanner4.0.apk");
    let _manifest_data = get_manifest_data(apk_path);
}

fn get_manifest_data(apk_path: &Path) -> Result<String, Box<dyn Error>> {
    let f = File::open(apk_path)?;
    let mut apk_as_archive = zip::ZipArchive::new(f)?;

    let _manifest_entry = match apk_as_archive.by_name(MANIFEST_MF_PATH) {
        Ok(manifest) => manifest,
        Err(zip::result::ZipError::FileNotFound) => {
            let manifest_entry = apk_as_archive
                .file_names()
                .find(|f| f.eq_ignore_ascii_case(MANIFEST_MF_PATH));

            match manifest_entry {
                Some(entry) => apk_as_archive.by_name(entry).unwrap(),
                None => {
                    return Err(Box::new(zip::result::ZipError::FileNotFound));
                }
            }
        }
        Err(err) => {
            return Err(Box::new(err));
        }
    };

    Ok(String::new()) //stub
}

Cargo.toml:

[package]
name = "multiple_borrows"
version = "0.1.0"
authors = ["Yury"]
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
zip = "^0.5.8"

Here are the errors:

error[E0502]: cannot borrow `apk_as_archive` as immutable because it is also borrowed as mutable
  --> src/main.rs:17:34
   |
14 |     let _manifest_entry = match apk_as_archive.by_name(MANIFEST_MF_PATH) {
   |                                 ----------------------------------------
   |                                 |
   |                                 mutable borrow occurs here
   |                                 a temporary with access to the mutable borrow is created here ...
...
17 |             let manifest_entry = apk_as_archive
   |                                  ^^^^^^^^^^^^^^ immutable borrow occurs here
...
31 |     };
   |      - ... and the mutable borrow might be used here, when that temporary is dropped and runs the destructor for type `std::result::Result<ZipFile<'_>, ZipError>`

error[E0499]: cannot borrow `apk_as_archive` as mutable more than once at a time
  --> src/main.rs:22:32
   |
14 |     let _manifest_entry = match apk_as_archive.by_name(MANIFEST_MF_PATH) {
   |                                 ----------------------------------------
   |                                 |
   |                                 first mutable borrow occurs here
   |                                 a temporary with access to the first borrow is created here ...
...
22 |                 Some(entry) => apk_as_archive.by_name(entry).unwrap(),
   |                                ^^^^^^^^^^^^^^ second mutable borrow occurs here
...
31 |     };
   |      - ... and the first borrow might be used here, when that temporary is dropped and runs the destructor for type `std::result::Result<ZipFile<'_>, ZipError>`

I understand that these errors are connected with poor architectural decisions. What are the best practices in this case?

Answer 1

by_name() returns data that points inside the object that represents the archive. At the same time, it takes a &mut reference to the archive, presumably because it needs to update some internal data structures while reading. The unfortunate consequence is that you cannot call by_name() while holding on to data returned by a previous call to by_name(). In your case the arm of the match that goes on to access the archive doesn't contain references into the archive, but the borrow checker is not yet smart enough to detect that.

The usual workaround is to do it in two steps: first, determine whether the manifest file is present, and then either retrieve it by name or search for it among file_names(). In the latter case you will need to do another split, this time by cloning the name of the file before calling by_name() again. This is for the same reason: by_name() requires a mutable reference to the archive which cannot be obtained while you're holding on to the file name that refers to the data inside it. Cloning the name creates a fresh copy at a (usually negligible) run-time cost, allowing the second call to by_name().

Finally, you can combine the ok_or_else() combinator with the ? operator to simplify error propagation. With all these applied, the function could look like this:

fn get_manifest_data(apk_path: &Path) -> Result<String, Box<dyn Error>> {
    let f = File::open(apk_path)?;
    let mut apk_as_archive = zip::ZipArchive::new(f)?;

    let not_found = matches!(
        apk_as_archive.by_name(MANIFEST_MF_PATH),
        Err(zip::result::ZipError::FileNotFound)
    );
    let _manifest_entry = if not_found {
        let entry_name = apk_as_archive
            .file_names()
            .find(|f| f.eq_ignore_ascii_case(MANIFEST_MF_PATH))
            .ok_or_else(|| zip::result::ZipError::FileNotFound)?
            .to_owned();
        apk_as_archive.by_name(&entry_name).unwrap()
    } else {
        apk_as_archive.by_name(MANIFEST_MF_PATH)?
    };

    Ok(String::new()) //stub
}