Analysis paralysis around mutability

help
1

I'm creating a program that lets you edit the metadata of of audio files (e.g., "artist", "title", etc.). A Track has a metadata HashMap<String, Ediit> where an Edit contains the original and current value of the metadata (this allows decoupling metadata edits and file saves).

I'm very new to Rust, and the issue I'm running into is finding a compiler-compliant and clean way for my app to maintain a mutable reference to an underlying Edit while the user is editing.

The original way I tried solving this problem was by using a chain of &mut self methods to return a &mut Edit from Track, but (1) this always results in multiple mutable references existing at once and (2) it seems odd using &mut self methods within Track for what should be immutable methods:

use std::collections::HashMap;

pub struct Edit {
    original: Option<String>,
    current: Option<String>,
}

impl Edit {
    pub fn delete(&mut self) {}
    pub fn update(&mut self) {}
    pub fn value(&self) -> &str {
        return match &self.current {
            Some(v) => &v[..],
            None => "",
        };
    }
}

pub struct Track {
    metadata: HashMap<String, Edit>,
}

impl Track {
    pub fn get_artist(&mut self) -> Option<&mut Edit> {
        return self.get("Artist");
    }

    pub fn get_title(&mut self) -> Option<&mut Edit> {
        return self.get("Title");
    }

    // A private get() method to keep DRY.
    fn get(&mut self, k: &str) -> Option<&mut Edit> {
        return self.metadata.get_mut(k);
    }

    pub fn remove(&mut self, k: &str) {
        self.metadata.remove(k);
    }
}

fn main() {
    let mut track = Track {
        metadata: HashMap::new(),
    };

    // Assume these `Option`s don't come back as None.
    // In this example, it will compile if I update `Edit.value()` to return a `String`,
    // But my actual app state needs to maintain mutable references to multiple `Edit`s in a `Track`.
    let title: &str = track.get_title().unwrap().value(); // fist mutable borrow
    let artist: &str = track.get_artist().unwrap().value(); // ERROR: cannot borrow `track` as mutable more than once at a time

    println!("{} by {}", title, artist);
}

Some alternatives I've thought of:

Option 1: Using RefCell... somewhere.

I'm unsure if my use case meets one of three situations the Rust docs recommend using inherited mutability.

I can't have Track.get() return RefCell<Option<&mut Edit>> or Option<RefCell<&mut Edit>> because the presence of &mut Edit presents the same problem as before; this type also looks a little bit silly.

I could skip having to chain &mut self methods by changing the type of Edit.current to RefCell<Option<String>>, allowing me to have Edit.update() and Edit.delete() now use &self instead of &mut self. However, this feels disingenuous to the caller as these methods are clearly mutating the struct, unless these methods qualify as "logically immutable"?

I could change the type of Track.metadata toHashMap<String, RefCell<Edit>>. This feels the most congruent because it allows me to stop using &mut self within Track methods while also keeping &mut self within Edit. I'm not sure if this is a good use case of "introducing mutability inside of something immutable" or if I'm just using it to circumvent the language?

Option 2: Refactoring my approach.

Perhaps I'm not thinking in Rust well enough? Is there something I'm missing about the way I could be structuring my program? Is my approach to the Edit and Track data structures wrong? Is my philosophy on data flow wrong?

As a not-so-great (broken) example, I could change the contract so that:

  1. Edit implements the Clone trait.
  2. Track.lookup now returns Option<Edit> (where the wrapped Edit is a clone of the original Edit).
  3. Add a method like Track.set(&mut self, k: String, v: Option<String>).

(This is broken because it is possible for the clone to still exist even if I were to call Track.remove(). References would prevent this!)

2

how about duplicating all getters, with one non-mut version and one mut version:

    pub fn get_artist(&self) -> Option<&Edit> {
        return self.get("Artist");
    }

    ...

    // A private get() method to keep DRY.
    fn get(&self, k: &str) -> Option<&Edit> {
        return self.metadata.get(k);
    }

    pub fn get_artist_mut(&mut self) -> Option<&mut Edit> {
        return self.get_mut("Artist");
    }

    ...

    // A private get() method to keep DRY.
    fn get_mut(&mut self, k: &str) -> Option<&mut Edit> {
        return self.metadata.get_mut(k);
    }

if you're trying to modify multiple edits at once this will still have issues, but at least you can read multiple at once

3

The term is interior mutability (AKA shared mutability). If you go this route, you want shared mutability for something owned -- Edit or Option<String> as you explored -- not for something already borrowed (&mut _).

In case it helps, think of &_ as shared borrowing, &mut _ as exclusive borrowing, and the following signature...

impl Track {
    pub fn get_artist(&mut self) -> Option<&mut Edit> {
    // Desugared:
    // pub fn get_artist<'s>(&'s mut self) -> Option<&'s mut Edit> {

...means that so long as the return value is in use, the Track (*self) remains exclusively borrowed.[1] Exclusive borrows enable UB-free mutability without runtime checks or other potential performance hits (like atomics). Shared mutability allow mutability without requiring exclusive access (via runtime checks, atomics, enforcing single-threadedness, and the like).

Between those latter two options, I think the relevant question is if you need or want to support something like this pattern or not:

let artist = track.get_artist().unwrap();
let original: &str = artist.original();
artist.update(...);
println!("Can still use {original}");

If you do, you'd need the shared mutability to be in Edit (e.g. current: RefCell<_>). If you don't, the shared mutability can be outside of Edit (RefCell<Edit>). Personally I don't find the above pattern very compelling -- original is read-only, right? -- because the caller can just call original() again after the update if they need to.

But either way, the fact that shared mutability is involved in Track will be apparent due to this general pattern working:

let title = track.get_title().unwrap();
let artist = track.get_artist().unwrap();
artist.mutate_a_value_in_track_somehow();
println!("Can still use {title} (which references things in track) at all");

Because if you can mutate something track owns through artist, but calling track.get_artist didn't invalidate title, get_artist must not have required an exclusive reference to track and there's some form of shared mutability involved at some level.

And depending on how you want your library to work, that can be perfectly fine.

I guess what I'm trying to say is, "feels disingenuous" sounds like a distraction to me when considering those two cases. Both expose shared mutability in the API; the considerations should be things like "what usage patterns are useful" and "will this hinder me in the future".

On the other hand, maybe you don't need the above pattern either, if something like this is sufficient...

// These take `&self` and return something immutable now
let title = track.get_title().unwrap();
let artist = track.get_artist().unwrap();
use_both_at_once(&title, &artist);

// This one takes `&mut self` and invalidates the `title` and `artist` above
let artist = track.get_artist_mut().unwrap();
artist.mutate_a_value_in_track_somehow();

// But you can re-get the title if you need it
let title = track.get_title().unwrap();
println!("{title}");

...which is how things may look with @miro's suggestion.

If you had a more type-driven model of the track metadata...

#[non_exhaustive]
pub struct TrackMetaData {
    pub title: Edit,
    pub artist: Edit,
}

...then it would be possible to obtain a borrow of all the metadata in a single call. There's also an API on HashMap that would allow something similar to that without changing your data structures:

// We'll use this instead of having separate methods.
// (I get the impression you don't want to look things up by `&str` in the API.)
#[non_exhaustive]
pub enum MetaData {
    Artist,
    Title,
}

impl Track {
    pub fn get_meta<const N: usize>(
        &mut self,
        meta: [MetaData; N]
    ) -> [Option<&mut Edit>; N] {
        // If you change `meta` to a `HashMap<MetaData, Edit>`, this
        // step isn't needed.
        let keys = meta.map(|m| match m {
            // Else this is probably a method; look into crates like `strum`
            MetaData::Artist => "Artist",
            MetaData::Title => "Title",
        });
        self.metadata.get_disjoint_mut(keys)
    }
}

// Caller
let [title, artist] = track.get_meta([MetaData::Title, MetaData::Artist]);

Or you could take a sort of half-way approach; keep your HashMap, but have something like

#[non_exaustive]
pub struct LoanedMetaData<'a> {
    pub artist: Option<&'a mut Edit>,
    pub title: Option<&'a mut Edit>,
}

impl Track {
    fn get_meta(&mut self) -> LoanedMetaData<'_> {
        // (The `HashMap` could potentially still be keyed by an
        //  `enum`, perhaps private now.)
        let [ .. ] = self.metadata.get_disjoint_mut(..);
        LoanedMetaData { .. }
    }
}

However, all these approaches need the kinds of metadata to be known at compile time, or at least for the "first class" APIs. Given your existing get_artist-like API, though,[2] this is probably fine.

Another option would be something like take_artist(&mut self) -> Option<String> and set_artist(&mut self, String) which doesn't use cloning, but just takes the contents out.

  1. Even if the values are None! Borrow checking is a compile-time, type-and-control-flow level check, not a value-level check. Which variant gets returned is a runtime property. ↩︎

  2. i.e. your API is not "stringily typed" even though your HashMap is ↩︎

4

If you would like to explore this direction, maybe you can simplify the whole problem a lot:

Citing this code fragment:

I conclude, you would like to handle non existing fields as empty strings, which is completely fine in my opinion.

As other mentioned, when you have methods like get_title() and get_artist() etc., drop the HashMap and use named struct fields. So you get this simple struct:

#[derive(Debug, Default, Clone, PartialEq, Eq)]
pub struct Track {
    pub title: String,
    pub artist: String,
}

Please note, that the struct itself and its fields are all public. This struct is all you need. Your example becomes:

fn main() {
    let original = Track {
        title: "Original Title".to_string(),
        artist: "Mister X".to_string(),
    };

    let mut edit = original.clone();

    edit.title = "New title".to_string();

    println!("{} by {}", &edit.title, &edit.artist);

    if edit != original {
        // Save the new track data
    }
}

The general idea is, just work with plain data objects, if you have plain data. You don't need to introduce OO mechanism for no benefit.

There is no reason, that Track needs to know (and handle) the fact, that there is an original and an edited version of the track data.

You can elaborate this further:

#[derive(Debug, Default, Clone, PartialEq, Eq)]
pub struct Album {
    pub title: String,
    pub tracks: Vec<Track>,
}

fn main() {
    let original = Album {
        ...
    };

    let mut edit = original.clone();

    ...
}