[Beginner] Improvements to anagram searching function (exercism)

This is an easy problem from exercism; it's about finding out which candidate words are anagrams of a "reference word".

It passes all tests. The idea is to convert the words into HashMaps, to take advantage of the comparison allowed between them.

One current issue is that &cand.to_lowercase() is repeated.

One possible issue is that filter is used several times for readability, which may be wrong.

use std::collections::{HashMap, HashSet};

/// Puts all true anagrams of `word` into a HashSet.
pub fn anagrams_for<'a>(word: &'a str, possible_anagrams: &[&'a str]) -> HashSet<&'a str> {
    let lw_word = word.to_lowercase();
    let word_hm = letter_count(&lw_word);
    let mut out = HashSet::new();

    possible_anagrams
        .iter()
        .filter(|cand| cand.len() == word.len())
        .filter(|cand| lw_word.ne(&cand.to_lowercase()))
        .for_each(|cand| {
            let candidate_hm = letter_count(&cand.to_lowercase());
            if word_hm.eq(&candidate_hm) {
                out.insert(*cand);
            }
        });

    out
}

/// Convert word into a HashMap of character-count.
/// We don't need i32 because HashMaps can be compared!
fn letter_count(word: &str) -> HashMap<char, u32> {
    let mut count = HashMap::new();
    word.chars().for_each(|c| {
        let v = count.entry(c).or_insert(0);
        *v += 1
    });
    count
}

Yeah, I'd get rid of that too. Perhaps:

    possible_anagrams
        .iter()
        .map(|cand| (cand, cand.to_lowercase()))
        // ...

It's generally fine unless you end up doing too many unnecessary things that could be avoided within a single closure. You could filter a third time instead of for_each, even. It's a matter of style to some extent.

    // `out` is no longer a variable and the function ends with
    possible_anagrams
        .iter()
        .copied() // changes `Item` from `&&str` to `&str`
        // ...
        .filter(|cand| {
            let candidate_hm = letter_count(&cand.to_lowercase());
            word_hm.eq(&candidate_hm)
        })
        .collect()

word isn't put in the returned HashSet, so you can make this change.

-pub fn anagrams_for<'a>(word: &'a str, possible_anagrams: &[&'a str]) -> HashSet<&'a str> {
+pub fn anagrams_for<'a>(word: &str, possible_anagrams: &[&'a str]) -> HashSet<&'a str> {
+//          this part changed ^^^^

Which allows this to compile.

fn example() -> HashSet<&'static str> {
    let word = String::new();
    anagrams_for(&word, &[])
}

fn example_2<'a>(list: &[&'a str]) -> HashSet<&'a str> {
    let word = String::new();
    anagrams_for(&word, list)
}

Everything else seems fine to me, except some Unicode nits, which don't really matter in the context of this learning exercise. The bottom of my reply is about those.

If we ignore those and keep your approach the same, there are various potential optimizations. You could use the same HashMap for all candidates, and you could even avoid allocating new Strings for the lowercase words if you jump through enough hoops (create iterators of lowercased chars and compare those). But I don't think they're worth it unless you think you'd learn something significant from trying them out.

So, this next part that I'm about to point out is probably fine given that this is a learning exercise. But there are some parts that wouldn't work quite as intended with certain inputs. For example, you have

        .filter(|cand| cand.len() == word.len())
        .filter(|cand| lw_word.ne(&cand.to_lowercase()))

But without normalizing inputs, a word may have multiple UTF8 representations (a different sequence of code points), including different lengths. The lowercase version might be a different length than the non-lowercase version. And your HashSet of chars approach doesn't properly account for graphemes that consist of multiple chars (Unicode scalar values), either. Two words with an accent char which applied to different base letters could compare the same, for example.

Normalization and splitting on Unicode graphemes aren't supported by std; you need a third party library to address these problems. That is a reason why learning exercises tend not to require doing the right thing. The weird part about this exercise is that they specifically said they were expanding the problem to Unicode, but then don't actually require an approach that handles all the Unicode issues.[1]

It's not a problem for you in the context of learning exercise, but is something to be aware of should you end up working with Unicode comparisons in a more production context.


  1. If any of them? Maybe they test some 1-to-1 non-ASCII lowercase letters or something. (I don't have an account to play around and see.) ↩︎

That's awesome.

I tried to address all the issues except the allocation for new Strings which I think is even worse.

Their tests are here.

New code is here, I don't think it handles HashMaps well yet (mutating them as I did seems a bad idea).

I could not find a simple way to reuse the HashMap while using graphemes.

The code does not compile since I can't figure out the references.
I would like to keep it somewhat efficient.

use std::collections::{HashMap, HashSet};
use unicode_segmentation::UnicodeSegmentation;

/// This function takes a reference word, and checks which possible anagrams are actual anagrams.
pub fn anagrams_for<'a>(word: &str, possible_anagrams: &[&'a str]) -> HashSet<&'a str> {
    let mut out = HashSet::new();

    // for `word`
    let mut word_hash_map: HashMap<&str, u32> = HashMap::new();
    let word_lw = word.to_lowercase();
    letter_count(&mut word_hash_map, &word_lw);

    // for `cand`, cleared by function.
    let mut cand_hash_map: HashMap<&str, u32> = HashMap::new();

    possible_anagrams
        .iter()
        .copied() // remove extra reference to item
        // cand = candidate word.
        // carry over the lowercase version (used twice).
        .map(|cand| (cand, cand.to_lowercase()))
        // quick filter _after_ lowercasing.
        .filter(|(_, c_lw)| c_lw.len() == word_lw.len())
        .filter(|(_, c_lw)| word_lw.ne(c_lw))
        .for_each(|(cand, c_lw)| {
            letter_count(&mut cand_hash_map, &c_lw);
            if word_hash_map == cand_hash_map {
                out.insert(cand);
            }
        });

    out
}

/// Convert word into a HashMap of character-count.
/// We don't need i32 because HashMaps can be compared!
fn letter_count<'a>(count: &mut HashMap<&'a str, u32>, word: &'a str) {
    // count.is_empty().not().then(|| count.clear());
    if !count.is_empty() {
        count.clear()
    }
    word.graphemes(true).for_each(|c| {
        *count.entry(c).or_insert(0) += 1;
    });
}

PS: I think I do see what the problem is, or the main one. The cand_hash_map (updated in the for_each) clashes with references which live only for the span of the closure (I think).

PS2: So now it copies them upfront but have to iterate twice. (Seems a bad idea).

Yes, using graphemes (borrows of the lowercased strings) makes re-using the HashMap much more complicated. I'd probably skip it for find another approach. But perhaps talking about it will be useful from a learning perspective...

Think of Rust lifetimes ('a things) as the duration of some borrow. Or perhaps in a more refined sense, as points in the control flow of your program where something (or things) are borrowed. So long as some place (a variable, a field, ...) is borrowed, there are restrictions on what can happen to that place. For example, you can't move or destruct a place that is borrowed. It's the borrow checker's job to enforce those restrictions.

The problem in this case is easier to see if we unroll the loop.

    // Rewritten to make the loop more conventionally obvious
    let mut cand_hash_map: HashMap<&str, u32> = HashMap::new();
    let mut iter = possible_anagrams
        // ...
        .filter(|(_, c_lw)| word_lw.ne(c_lw));

    // The loop unrolled a bit
    if let Some((cand, c_lw_1)) = iter.next() {
        letter_count(&mut cand_hash_map, &c_lw_1); // L10
        if word_hash_map == cand_hash_map {
            out.insert(cand);
        } 
        // L18
    } // L20
    if let Some((cand, c_lw_2)) = iter.next() {
        letter_count(&mut cand_hash_map, &c_lw_2); // L30
        if word_hash_map == cand_hash_map {
            out.insert(cand);
        }
    } // L40

cand_hash_map is a HashMap<&'g str, u32>, so for any s: &str you insert as a key, *s has to be borrowed for (at least) the same duration 'g. Clearly that duration has to include L10 and L30, where you're borrowing c_lw_1 and c_lw_2, and might insert &str from those borrows into the HashMap. In turn that means the borrow from L10 must continue from L10 to at least L30, including L20. But c_lw_1 goes out of scope and gets destructed on L20, which conflicts with being borrowed.

In simpler terms, a borrow of c_lw_1 may still be in the HashMap when you call letter_count on L30, but c_lw_1 has been destructed by that point, so this can't be allowed to compile.

However, note that adding some unconditional .clear()s (like on L18) so that this won't actually happen doesn't fix the borrow checker error. The borrow checker doesn't understand the implementation details of the HashMap; there's no logic like "one cleared, the lifetimes in a HashMap don't matter" it can exploit. You would need some other trick on L18 that the borrow checker accepts as proof that c_lw_1 is no longer borrowed; something rooted in the type system (where the borrow checker operates).

For example, you could have something like this:

// This could be more generic but that would be a distraction.
fn reuse_hashmap(mut map: HashMap<&str, u32>) -> HashMap<&'static str, u32> {
    map.clear();
    // SAFETY: We are only changing a lifetime which does not effect
    // layout and represents borrows held by the key entries in the
    // map. We know there are no keys in our map and thus no borrows.
    unsafe { std::mem::transmute(map) }
}

And use it like so:

    for (cand, c_lw) in iter {
        // Due to variance, `this_map` may have an arbitrarily
        // short lifetime in its type.
        let mut this_map = cand_hash_map;
        letter_count(&mut this_map, &c_lw);
        if word_hash_map == this_map {
            out.insert(cand);
        }
        // The borrow represented in the type of `this_map` has
        // to be active in this call.  But after this call,
        // `this_map` has been moved and there's nothing that keeps
        // that borrow active any more.
        cand_hash_map = reuse_hashmap(this_map);
        // So here, where `c_lw` is about to drop, the borrow is no
        // longer active and there's no conflict with `c_lw` getting
        // destructed.
    }

Whether it's worth it is another question.

This comes up with Vec<_> sometimes too, where there's a safe sorta-workaround:

// Often optimizes to reuse the existing `vec`, preserving its allocation
vec.into_iter().map(|_| unreachable!()).collect()

I say "sorta" because the optimization is not guaranteed. (I tried the analogous code for HashMap, but it didn't look like it optimized.)

Thanks for the detailed explanation; I understand now that the 'g requires that the s: &'a str is living for longer than it actually does.

As written, it is requiring the place to be borrowed for 'g "time" and getting some other 'z which is shorter.

I ended up with the approach in the last line above, which is less efficient, but maybe good enough until I learn more (I am avoiding unsafe).