refactor: get rid of some allocations from WordSet

[hippietrail]

Issues

Inspired by the discussion at #3202 but I don't think there's an issue this fixes per se.

Description

WordSet had places it called collect(), which resulted in allocations.
I got rid of all the unnecessary ones by using methods on iterators instead.
This involved adding "lenient" versions of eq_ch() and eq_str() that do allow the right-hand side to not already be lowercase. Instead, the words in the WordSet are now guaranteed to be lowercase so that they can be on the right-hand side of comparisons against &[char] and avoid allocations and conversions.

Previously every word was allocated just to check if it was already in the WordSet. This is now avoided.

All tests pass but I would appreciate another pair of eyes well-versed in Harper's workings to make sure I didn't overlook anything as it's well past midnight.

How Has This Been Tested?

cargo test

AI Disclosure

• I am a human and didn't use any AI.
• I used LLM features of my editor, but not an agent.
• I consulted one or more coding AIs, but didn't use an agent.
• I used an AI agent interactively.
• I am an agent or I got an agent to do the work autonomously.

I'm pretty sure I rewrote by hand every single bit of code an AI came up with as it didn't always do what I wanted even though it did give me ideas of how to proceed.

Checklist

• I have performed a self-review of my own code
• I have added tests to cover my changes
• I have considered splitting this into smaller pull requests.

[mauropereiira]

Took a look, since you asked. I think there is one inconsistency worth fixing before this lands.

add and add_chars now normalize before storing:

word.chars().map(|c| c.normalized().to_ascii_lowercase()).collect()

but the dedup check and lookup go through eq_str_lenient / eq_ch_lenient, which only case-fold. Neither calls normalized(). So the write path and the read path disagree about what equality means.

Probe on your branch at 240111f3:

let mut set = WordSet::default();
set.add("They're");
set.add("They\u{2019}re");   // same word, typographic apostrophe

words.len()                = 2       // duplicate stored
contains("They're")        = true
contains("They\u{2019}re") = false   // present, but not findable
contains("THEY'RE")        = true

The duplicate is only a small waste, since matches_token normalizes at compare time and still matches both. The contains result is the part I would call a bug: the curly form is in the set, in normalized form, and the lookup cannot see it.

Making the lenient comparisons normalize as well as case-fold would line both paths up, and would also let matches_token drop its remaining b.normalized() call, since the stored side would already be normalized:

.all(|(a, b)| a.normalized().eq_ignore_ascii_case(&b.normalized()));

Worth noting contains also changed semantics for existing callers. It used to be an exact match on the collected chars; it is now case-insensitive, so contains("THEY'RE") returns true where it previously returned false. That is probably what you want given the words are stored lowercased now, but it is a public method, so it seemed worth naming.

[hippietrail]

but the dedup check and lookup go through eq_str_lenient / eq_ch_lenient, which only case-fold. Neither calls normalized(). So the write path and the read path disagree about what equality means.

Yeah this PR wasn't meant to tackle the apostrophe problem/bug directly, just to improve the underlying WordSet. That's probably the way to go though in a subsequent PR. WordSet only claims in its docs/comments to be case insensitive. The Unicode variant insensitivity is only documented in code. I just like to grok how the things work thoroughly lest I go too far.

The duplicate is only a small waste, since matches_token normalizes at compare time and still matches both. The contains result is the part I would call a bug: the curly form is in the set, in normalized form, and the lookup cannot see it.

Making the lenient comparisons normalize as well as case-fold would line both paths up, and would also let matches_token drop its remaining b.normalized() call, since the stored side would already be normalized:

It sounds like you have identified the right way to fix the apostrophe bug. The contract in the WordSet comments/docs should be updated to reflect this. When it's part of the published contract it'll be more comfortable to integrate right through the module instead of kind-of bolted-on as it is now.

.all(|(a, b)| a.normalized().eq_ignore_ascii_case(&b.normalized()));

Worth noting contains also changed semantics for existing callers. It used to be an exact match on the collected chars; it is now case-insensitive, so contains("THEY'RE") returns true where it previously returned false. That is probably what you want given the words are stored lowercased now, but it is a public method, so it seemed worth naming.

Are you sure? I only moved the calls to normalized() which were in the zip pipeline before to make it more obvious they were function calls on a single char and not allocating anything.

For the case matching part, yeah that was the reason I moved the normalized-to-lowercase into the inner collection. When one side is guaranteed to be lowercase we can use the slightly more efficient eq_ch/eq_str instead of the lenient versions.

[mauropereiira]

Yep, you're right about that one, and my fault for hanging the note off the wrong quote. The zip move is a pure no-op: both versions normalize both sides and then compare ASCII-case-insensitively, so nothing changed there. I was talking about contains, a few lines up.

I checked it against both commits to be sure. Same probe on the base and on your head:

                       base      head
contains("they're")    true      true
contains("THEY'RE")    false     true
contains("they’re")    false     false

The flip on the second line comes from two things stacking: contains used to be self.words.contains(&word.chars().collect()), which is exact Vec<char> equality, and the stored words weren't lowercased. Now the comparison case-folds and the stored side is lowercased, so an uppercase query matches. Nothing outside WordSet itself seems to call contains, so nothing in-tree breaks. It's pub on a published crate though, so it seemed worth writing down.

While I was in there I found something that makes the normalize-the-lenient-comparisons fix look more attractive. Adding both apostrophe spellings stores the same word twice:

base: [['t','h','e','y','\'','r','e'], ['t','h','e','y','’','r','e']]
head: [['t','h','e','y','\'','r','e'], ['t','h','e','y','\'','r','e']]

On the base the two entries were at least different strings. On the head they're byte-identical, because add normalizes on the way in but the dedup check doesn't. So the dedup guard can now let through an exact duplicate, which the old exact-equality version couldn't. Small waste either way, but it's a straightforward argument for lining the two paths up.

One thing on the last point: I don't think eq_ch/eq_str can stand in for the lenient versions in contains. Those two fold self and require the argument to already be lowercase, and there's a debug_assert! on it. In contains the guaranteed-lowercase side is self (the stored word) and the argument is whatever the caller passes, so it's the wrong way round and the assert would fire on any non-lowercase query. Where the guarantee does pay off is the b.normalized() in matches_token, which is now dead work.

Agreed on all the rest. Updating the WordSet docs to promise normalization as well as case-insensitivity, then doing the apostrophe fix on top, sounds like the right order.