Stop auto-fix from joining words or typing guesses

crates/cc-app/src/fixgate.rs

@@ -4,6 +4,8 @@
 use std::collections::HashMap;
 use std::time::{Duration, Instant};
 
+use cc_correct::Severity;
+
 /// Two undone fixes this close together in one window trip the breaker.
 const UNDO_WINDOW: Duration = Duration::from_secs(15);
 const UNDO_LIMIT: usize = 2;
@@ -17,10 +19,56 @@ pub fn closes_word(c: char) -> bool {
     c.is_whitespace() || matches!(c, '.' | ',' | ';' | ':' | '!' | '?' | ')' | ']' | '}' | '"')
 }
 
-/// Why `original` -> `replacement` must not be typed, if it must not. `whole_word`: the
-/// shadow buffer saw where `original` starts, so the deletion cannot reach into an unseen
-/// part of the word. `is_word` says whether a token is spelled right.
-pub fn reject(original: &str, replacement: &str, whole_word: bool, is_word: impl Fn(&str) -> bool) -> Option<&'static str> {
+/// Whether a candidate may be typed without asking at auto-fix `level`. Bold (S2) adds only
+/// doubled-letter spelling fixes, which S0 refuses on capitalised words because those look
+/// like names (`Outllook` -> `Outlook`).
+pub fn eligible(severity: &Severity, rule: &str, original: &str, replacement: &str, level: &Severity) -> bool {
+    match severity {
+        Severity::S0 | Severity::S1 => severity <= level,
+        Severity::S2 => {
+            *level >= Severity::S2 && rule == "spell" && cc_correct::is_doubled_letter_typo(original, replacement)
+        }
+        Severity::S3 => false,
+    }
+}
+
+/// `s` without the punctuation around it: `(teh` is `teh`, `reviews,` is `reviews`.
+pub fn word_core(s: &str) -> &str {
+    s.trim_matches(|c: char| !c.is_alphanumeric())
+}
+
+/// The characters before and after the letters and digits of `s`.
+fn outer_punctuation(s: &str) -> (&str, &str) {
+    let not_alnum = |c: char| !c.is_alphanumeric();
+    let lead = &s[..s.len() - s.trim_start_matches(not_alnum).len()];
+    let trail = &s[s.trim_end_matches(not_alnum).len()..];
+    (lead, trail)
+}
+
+/// Why `original` -> `replacement` must not be typed, if it must not. A fix replaces one
+/// word with one word and keeps the punctuation around it; only an autocorrect entry
+/// (`expands`) may stand for several words (`alot` -> `a lot`). `whole_word`: the shadow
+/// buffer saw where `original` starts, so the deletion cannot reach into an unseen part of
+/// the word. `is_word` says whether a token is spelled right.
+pub fn reject(
+    original: &str,
+    replacement: &str,
+    whole_word: bool,
+    expands: bool,
+    is_word: impl Fn(&str) -> bool,
+) -> Option<&'static str> {
+    if original.trim().is_empty() || original.chars().any(char::is_whitespace) {
+        return Some("changes more than one word");
+    }
+    if replacement.trim().is_empty() {
+        return Some("would delete the word");
+    }
+    if replacement.chars().any(char::is_whitespace) && !expands {
+        return Some("would split the word");
+    }
+    if outer_punctuation(original) != outer_punctuation(replacement) {
+        return Some("would change the punctuation around the word");
+    }
     if !whole_word {
         return Some("start of the word not seen");
     }
@@ -112,13 +160,126 @@ mod tests {
 
     #[test]
     fn reject_explains_unsafe_fixes() {
-        assert_eq!(reject("teh", "the", true, known), None);
-        assert_eq!(reject("alot", "a lot", true, known), None);
-        assert_eq!(reject("erge", "edge", false, known), Some("start of the word not seen"));
-        assert_eq!(reject("doesn", "does't", true, known), Some("result is not a known word"));
-        assert_eq!(reject("ne", "NE", true, |_| true), Some("would turn a word into capitals"));
-        assert_eq!(reject("i", "I", true, known), None);
-        assert_eq!(reject("Teh", "The", true, known), None);
+        assert_eq!(reject("teh", "the", true, false, known), None);
+        assert_eq!(reject("alot", "a lot", true, true, known), None);
+        assert_eq!(reject("alot", "a lot", true, false, known), Some("would split the word"));
+        assert_eq!(reject("Every one", "Everyone", true, false, |_| true), Some("changes more than one word"));
+        assert_eq!(reject("the the", "the", true, true, known), Some("changes more than one word"));
+        assert_eq!(reject("teh", "", true, true, known), Some("would delete the word"));
+        assert_eq!(
+            reject("reeviews,", "reviews", true, true, |_| true),
+            Some("would change the punctuation around the word")
+        );
+        assert_eq!(reject("(teh", "(the", true, false, known), None);
+        assert_eq!(reject("erge", "edge", false, false, known), Some("start of the word not seen"));
+        assert_eq!(reject("doesn", "does't", true, false, known), Some("result is not a known word"));
+        assert_eq!(reject("ne", "NE", true, false, |_| true), Some("would turn a word into capitals"));
+        assert_eq!(reject("i", "I", true, false, known), None);
+        assert_eq!(reject("Teh", "The", true, false, known), None);
+    }
+
+    #[test]
+    fn bold_adds_only_doubled_letter_spelling_fixes() {
+        use Severity::*;
+        assert!(eligible(&S0, "spell", "thhe", "the", &S0));
+        assert!(!eligible(&S1, "sentence_capitalization", "this", "This", &S0));
+        assert!(eligible(&S1, "sentence_capitalization", "this", "This", &S1));
+        assert!(eligible(&S2, "spell", "Outllook", "Outlook", &S2));
+        assert!(!eligible(&S2, "spell", "Outllook", "Outlook", &S1));
+        assert!(!eligible(&S2, "spell", "pc", "pd", &S2));
+        assert!(!eligible(&S2, "compound_nouns", "quick start", "quickstart", &S2));
+        assert!(!eligible(&S3, "repeated_words", "the the", "the", &S2));
+    }
+
+    #[test]
+    fn word_core_drops_surrounding_punctuation_only() {
+        assert_eq!(word_core("(teh"), "teh");
+        assert_eq!(word_core("reeviews,"), "reeviews");
+        assert_eq!(word_core("don't"), "don't");
+        assert_eq!(word_core("..."), "");
+    }
+
+    fn checker() -> &'static cc_correct::Checker {
+        static CHECKER: std::sync::OnceLock<cc_correct::Checker> = std::sync::OnceLock::new();
+        CHECKER.get_or_init(cc_correct::Checker::new)
+    }
+
+    /// What `on_word_boundary` would type after each word of `sentence` at `level`, as
+    /// `(text so far, original, replacement)`.
+    fn auto_fixes(sentence: &str, level: &Severity) -> Vec<(String, String, String)> {
+        let dict = cc_correct::PersonalDictionary::default();
+        let mut typed = String::new();
+        let mut out = Vec::new();
+        for word in sentence.split(' ') {
+            if !typed.is_empty() {
+                typed.push(' ');
+            }
+            typed.push_str(word);
+            let n = typed.chars().count();
+            for c in checker().check(&typed, &dict) {
+                let at_caret = c.end == n && typed.ends_with(&c.original);
+                if at_caret
+                    && eligible(&c.severity, &c.rule, &c.original, &c.replacement, level)
+                    && reject(&c.original, &c.replacement, true, false, |t| checker().is_word(t, &dict)).is_none()
+                {
+                    out.push((typed.clone(), c.original, c.replacement));
+                }
+            }
+        }
+        out
+    }
+
+    /// Sentences for which Harper offers to join, split or delete words.
+    const JOIN_BAIT: &[&str] = &[
+        "Every one of them came to the quick start meeting",
+        "We should check out the new web site some time",
+        "They had had enough and I know that that is true",
+        "It is a good idea and also a cheap one",
+        "The more friendly option is more better",
+        "He should of known better alot of times",
+        "This is a test of the the system",
+        "Please make sure to double check the work",
+        "The data base is on line now",
+        "The kids played out side all day long",
+        "Any one of these will do and some one who knows can help",
+        "Some time ago some times I would go every where",
+        "Every thing is fine with out a doubt",
+        "Where as the old one failed there fore we left",
+        "How ever you do it what ever happens is fine",
+        "I need to up date the over load setting",
+        "Our team is under way with the work",
+        "It was a long while ago and we waited a while",
+        "She is in deed right and in fact over joyed",
+        "The book shelf and the note book are on the table",
+        "I sent an e mail to my class mate yesterday",
+        "We had a work shop on Monday and a lunch break",
+    ];
+
+    #[test]
+    fn no_auto_fix_joins_splits_or_deletes_words() {
+        for level in [Severity::S0, Severity::S1, Severity::S2] {
+            for sentence in JOIN_BAIT {
+                for (typed, original, replacement) in auto_fixes(sentence, &level) {
+                    assert!(
+                        original.split_whitespace().count() == 1 && replacement.split_whitespace().count() == 1,
+                        "{level:?} after {typed:?}: {original:?} -> {replacement:?}"
+                    );
+                }
+            }
+        }
+    }
+
+    #[test]
+    fn typos_are_still_fixed_at_their_level() {
+        let fixes = |s: &str, level: Severity| -> Vec<(String, String)> {
+            auto_fixes(s, &level).into_iter().map(|(_, o, r)| (o, r)).collect()
+        };
+        let pair = |o: &str, r: &str| (o.to_string(), r.to_string());
+        assert_eq!(fixes("I want thhe cat", Severity::S0), vec![pair("thhe", "the")]);
+        assert_eq!(fixes("I said soo", Severity::S0), vec![pair("soo", "so")]);
+        assert!(fixes("We use Outllook", Severity::S1).is_empty());
+        assert_eq!(fixes("We use Outllook", Severity::S2), vec![pair("Outllook", "Outlook")]);
+        assert!(fixes("My name is Dimitri", Severity::S2).is_empty());
     }
 
     #[test]

crates/cc-app/src/main.rs

@@ -565,9 +565,10 @@ impl App {
         }
     }
 
-    /// Runs after the word before `boundary` is closed. Auto-applies one S0/S1 candidate that
-    /// spans exactly that word; anything else is left alone. Never runs after Enter, because
-    /// in chat fields the text has already been sent.
+    /// Runs after the word before `boundary` is closed. Auto-applies one candidate that the
+    /// auto-fix level allows (`fixgate::eligible`) and that replaces exactly that word;
+    /// anything else is left alone. Never runs after Enter, because in chat fields the text
+    /// has already been sent.
     fn on_word_boundary(&mut self, boundary: char) {
         let Some(level) = self.auto_level.clone() else { return };
         if self.breaker.paused(self.last_hwnd, Instant::now()) {
@@ -591,11 +592,15 @@ impl App {
         }
         let t0 = Instant::now();
         let n_chars = sentence.chars().count();
-        let candidate = match self.auto.lookup(&word) {
+        // The autocorrect list holds bare words: `(teh` looks up `teh`. A word with
+        // punctuation after it was already looked up when that punctuation was typed.
+        let core = word.trim_start_matches(|c: char| !c.is_alphanumeric());
+        let listed = if core.ends_with(char::is_alphanumeric) { self.auto.lookup(core) } else { None };
+        let candidate = match listed {
             Some(fix) => Some(cc_correct::Candidate {
-                start: n_chars - word.chars().count(),
+                start: n_chars - core.chars().count(),
                 end: n_chars,
-                original: word.clone(),
+                original: core.to_string(),
                 replacement: fix,
                 rule: "autocorrect".into(),
                 severity: Severity::S0,
@@ -607,7 +612,7 @@ impl App {
                 .into_iter()
                 .find(|c| {
                     c.end == n_chars
-                        && c.severity <= level
+                        && fixgate::eligible(&c.severity, &c.rule, &c.original, &c.replacement, &level)
                         && sentence.ends_with(&c.original)
                         && !self.demoted.contains(&c.rule)
                 }),
@@ -619,9 +624,10 @@ impl App {
         // The fix deletes `original` back from the caret. Unless the shadow saw what precedes
         // it, that may be the tail of a longer word, and the result would be a mangled word.
         let whole_word = body_len > c.original.len() || self.shadow.anchored();
-        let taught = c.rule == "autocorrect" && self.auto.is_user_entry(&word);
+        let listed = c.rule == "autocorrect";
+        let taught = listed && self.auto.is_user_entry(&c.original);
         let is_word = |token: &str| taught || self.checker.is_word(token, &self.dict);
-        if let Some(why) = fixgate::reject(&c.original, &c.replacement, whole_word, is_word) {
+        if let Some(why) = fixgate::reject(&c.original, &c.replacement, whole_word, listed, is_word) {
             log::info!("fix {:?} -> {:?} [{}] not applied: {why}", c.original, c.replacement, c.rule);
             self.prev_boundary = Some((word, self.shadow.text().chars().count()));
             return;
@@ -681,21 +687,23 @@ impl App {
     /// pair becomes an autocorrect entry: that is how the list learns your own typos.
     fn learn_self_correction(&mut self, word: &str) {
         let Some((prev, prev_len)) = self.prev_boundary.take() else { return };
+        // Punctuation typed around a word is not part of the typo.
+        let (prev, word) = (fixgate::word_core(&prev), fixgate::word_core(word));
         let now_len = self.shadow.text().chars().count();
         if now_len > prev_len || prev == word || prev.chars().count() < 3 || word.chars().count() < 2 {
             return;
         }
-        if cc_correct::is_code_like(&prev) || cc_correct::is_code_like(word) || !self.checker.is_word(word, &self.dict) {
+        if cc_correct::is_code_like(prev) || cc_correct::is_code_like(word) || !self.checker.is_word(word, &self.dict) {
             return;
         }
         if cc_correct::edit_distance(&prev.to_lowercase(), &word.to_lowercase()) > 2 {
             return;
         }
-        let prev_is_unknown = self.checker.check(&prev, &self.dict).iter().any(|c| c.rule == "spell");
+        let prev_is_unknown = self.checker.check(prev, &self.dict).iter().any(|c| c.rule == "spell");
         if !prev_is_unknown {
             return;
         }
-        self.auto.add(&prev, word);
+        self.auto.add(prev, word);
         if let Err(e) = self.auto.save(&self.auto_path) {
             log::warn!("autocorrect save failed: {e}");
         }
@@ -1410,6 +1418,12 @@ fn start() -> anyhow::Result<()> {
     let ui = UiServer::start(&profile.join("events.db"), &profile, ui_state.clone(), ui_tx).context("ui server")?;
     let windows = Windows::new().context("ui windows")?;
     log::info!("dashboard: {}", ui.url("dashboard"));
+    let auto = AutocorrectList::load(&auto_path);
+    if auto.file_is_stale() {
+        if let Err(e) = auto.save(&auto_path) {
+            log::warn!("autocorrect save failed: {e}");
+        }
+    }
     let long_ago = Instant::now() - Duration::from_secs(24 * 3600);
     let mut app = App {
         store,
@@ -1446,7 +1460,7 @@ fn start() -> anyhow::Result<()> {
         last_status: long_ago,
         checker: Checker::new(),
         dict: PersonalDictionary::load(&dict_path),
-        auto: AutocorrectList::load(&auto_path),
+        auto,
         auto_path,
         prev_boundary: None,
         dict_path,