From 86345ebcf5cdff0c24e110e4592be9d82b8a95b9 Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Mon, 2 Nov 2026 00:05:20 +0000 Subject: [PATCH] =?UTF-8?q?FU.4:=20craft-proof=20packs=20=E2=80=94=20remem?= =?UTF-8?q?bered=20hyphenation=20patterns=20keep=20to=20a=20256=20MB=20bud?= =?UTF-8?q?get=20of=20their=20own;=20a=20pack's=20spelling=20files=20are?= =?UTF-8?q?=20never=20candidates=20through=20a=20symbolic=20link?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups from craft-libs#485: up to 256 languages' pack patterns (64 MB files each) were kept by count only; `hunspell::ordered` checked pack files with `is_file`, which follows links, while the pack scan refuses them. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FypgCX8LsYYE8fsj4byBm2 --- crates/proof/src/hunspell.rs | 45 +++++++++++++++++++++----- crates/proof/src/packs.rs | 61 ++++++++++++++++++++++++++++++++---- 2 files changed, 93 insertions(+), 13 deletions(-) diff --git a/crates/proof/src/hunspell.rs b/crates/proof/src/hunspell.rs index 274af9eb..966ded72 100644 --- a/crates/proof/src/hunspell.rs +++ b/crates/proof/src/hunspell.rs @@ -196,19 +196,22 @@ pub(crate) fn ordered( let first = names.first().map(|n| norm(n)); let (region, rest): (Vec<&str>, Vec<&str>) = names.iter().copied().partition(|n| Some(norm(n)) == first); let pack: &[&str] = &[crate::packs::SPELLING]; + // (folder, names, a Language pack's: never read through a symbolic link, as the pack scan + // checks; a system folder's dictionaries often are links, `en_GB.dic` → `en_GB-ise.dic`). let tiers = dirs .iter() - .map(|d| (d, names)) - .chain(region_packs.iter().map(|d| (d, pack))) - .chain(system.iter().map(|d| (d, ®ion[..]))) - .chain(other_packs.iter().map(|d| (d, pack))) - .chain(system.iter().map(|d| (d, &rest[..]))); + .map(|d| (d, names, false)) + .chain(region_packs.iter().map(|d| (d, pack, true))) + .chain(system.iter().map(|d| (d, ®ion[..], false))) + .chain(other_packs.iter().map(|d| (d, pack, true))) + .chain(system.iter().map(|d| (d, &rest[..], false))); + let file = |p: &Path, pack: bool| if pack { std::fs::symlink_metadata(p).is_ok_and(|m| m.is_file()) } else { p.is_file() }; let mut out: Vec<(PathBuf, PathBuf)> = Vec::new(); - for (dir, names) in tiers { + for (dir, names, in_pack) in tiers { for n in names { let aff = dir.join(format!("{n}.aff")); let dic = dir.join(format!("{n}.dic")); - if aff.is_file() && dic.is_file() && !out.iter().any(|(_, d)| *d == dic) { + if file(&aff, in_pack) && file(&dic, in_pack) && !out.iter().any(|(_, d)| *d == dic) { out.push((aff, dic)); } } @@ -696,6 +699,34 @@ mod tests { } } + /// Review follow-up (craft-libs#485): a pack's `spelling.aff`/`.dic` that is a symbolic link + /// (swapped in after the pack scan) is never a candidate; one in a system folder still is. + #[cfg(unix)] + #[test] + fn pack_files_are_never_read_through_a_link() { + let root = std::env::temp_dir().join(format!("wc-proof-pack-links-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + let (pack, system, elsewhere) = (root.join("packs").join("nl"), root.join("system"), root.join("elsewhere")); + for d in [&pack, &system, &elsewhere] { + std::fs::create_dir_all(d).unwrap(); + } + std::fs::write(elsewhere.join("x.aff"), "SET UTF-8\n").unwrap(); + std::fs::write(elsewhere.join("x.dic"), "1\nx\n").unwrap(); + std::fs::write(pack.join("spelling.aff"), "SET UTF-8\n").unwrap(); + std::os::unix::fs::symlink(elsewhere.join("x.dic"), pack.join("spelling.dic")).unwrap(); + std::os::unix::fs::symlink(elsewhere.join("x.aff"), system.join("nl.aff")).unwrap(); + std::os::unix::fs::symlink(elsewhere.join("x.dic"), system.join("nl.dic")).unwrap(); + let got: Vec = + ordered(&["nl"], &[], std::slice::from_ref(&pack), std::slice::from_ref(&system), &[]).into_iter().map(|(_, d)| d).collect(); + assert_eq!(got, [system.join("nl.dic")], "the system's link is followed, the pack's never"); + std::fs::remove_file(pack.join("spelling.dic")).unwrap(); + std::fs::write(pack.join("spelling.dic"), "1\nx\n").unwrap(); + let got: Vec = + ordered(&["nl"], &[], std::slice::from_ref(&pack), std::slice::from_ref(&system), &[]).into_iter().map(|(_, d)| d).collect(); + assert_eq!(got, [pack.join("spelling.dic"), system.join("nl.dic")]); + let _ = std::fs::remove_dir_all(&root); + } + /// WordCraft's own dictionaries folder under the user's application data is searched on every /// desktop system, after the setting and before the shared system locations (#1063). #[test] diff --git a/crates/proof/src/packs.rs b/crates/proof/src/packs.rs index 7424cd11..0d6cfbc8 100644 --- a/crates/proof/src/packs.rs +++ b/crates/proof/src/packs.rs @@ -34,6 +34,13 @@ pub(crate) const SPELLING: &str = "spelling"; const HYPHENATION: &str = "hyphenation.pat"; /// Most languages whose pack patterns are remembered (a document may tag thousands). const MAX_HYPHENATION_LOOKUPS: usize = 256; +/// The memory budget of the remembered pack patterns, in estimated bytes ([`pattern_bytes`]): +/// a pack's patterns may be as large as a dictionary (64 MB of file), and up to +/// [`MAX_HYPHENATION_LOOKUPS`] of them are kept, so they get a budget of their own, as the +/// dictionaries do (`pack_lang::MAX_LOADED_BYTES`). +const MAX_HYPHENATION_BYTES: u64 = 256 << 20; +/// Estimated memory per pattern (its packed key, its values and the map's own room). +const BYTES_PER_PATTERN: u64 = 64; /// A pack's description. const PACK_TOML: &str = "pack.toml"; /// A pack's licence texts. @@ -80,15 +87,18 @@ struct State { dir: Option, scan: Option>, /// Each language's pack patterns once looked up (`None`: no pack has them), at most - /// [`MAX_HYPHENATION_LOOKUPS`] languages (cleared when full). + /// [`MAX_HYPHENATION_LOOKUPS`] languages and [`MAX_HYPHENATION_BYTES`] (cleared when full). hyphenation: BTreeMap>>, + /// The estimated memory of the patterns in [`State::hyphenation`]. + hyphenation_bytes: u64, /// (locale, reason) of pack hyphenation files that failed to load. hyphenation_errors: Vec<(String, String)>, /// Counts changes, so a scan or load that started before one is not kept. changes: u64, } -static STATE: RwLock = RwLock::new(State { dir: None, scan: None, hyphenation: BTreeMap::new(), hyphenation_errors: Vec::new(), changes: 0 }); +static STATE: RwLock = + RwLock::new(State { dir: None, scan: None, hyphenation: BTreeMap::new(), hyphenation_bytes: 0, hyphenation_errors: Vec::new(), changes: 0 }); fn read() -> RwLockReadGuard<'static, State> { STATE.read().unwrap_or_else(PoisonError::into_inner) @@ -121,6 +131,7 @@ pub fn reload() { fn forget(s: &mut State) { s.scan = None; s.hyphenation.clear(); + s.hyphenation_bytes = 0; s.hyphenation_errors.clear(); s.changes = s.changes.wrapping_add(1); } @@ -566,10 +577,8 @@ pub(crate) fn hyphenation(lang: ProofLang) -> Option> { let mut s = write(); if s.changes == changes && !s.hyphenation.contains_key(&lang) { // ponytail: cleared when full, an LRU if many languages each need their patterns. - if s.hyphenation.len() >= MAX_HYPHENATION_LOOKUPS { - s.hyphenation.clear(); - } - s.hyphenation.insert(lang, found.clone()); + let bytes = found.as_deref().map_or(0, pattern_bytes); + remember_patterns(&mut s, lang, found.clone(), bytes, MAX_HYPHENATION_BYTES); for e in errors { if !s.hyphenation_errors.contains(&e) { s.hyphenation_errors.push(e); @@ -579,6 +588,23 @@ pub(crate) fn hyphenation(lang: ProofLang) -> Option> { found } +/// Estimated memory of `p`. +fn pattern_bytes(p: &Patterns) -> u64 { + (p.len() as u64).saturating_add(p.exception_count() as u64).saturating_mul(BYTES_PER_PATTERN) +} + +/// Remember `lang`'s pack patterns (`bytes` of them): over [`MAX_HYPHENATION_LOOKUPS`] languages +/// or `budget` bytes, the ones remembered so far are forgotten first. Patterns larger than the +/// whole budget are still kept alone, or they would be read from disk for every word. +fn remember_patterns(s: &mut State, lang: ProofLang, found: Option>, bytes: u64, budget: u64) { + if s.hyphenation.len() >= MAX_HYPHENATION_LOOKUPS || s.hyphenation_bytes.saturating_add(bytes) > budget { + s.hyphenation.clear(); + s.hyphenation_bytes = 0; + } + s.hyphenation.insert(lang, found); + s.hyphenation_bytes = s.hyphenation_bytes.saturating_add(bytes); +} + /// How many languages' pack patterns are remembered now. #[cfg(test)] pub(crate) fn hyphenation_lookups() -> usize { @@ -600,6 +626,29 @@ fn load_patterns(path: &Path) -> Result { mod tests { use super::*; + /// Review follow-up (craft-libs#485): the remembered pack patterns have a memory budget, not + /// only a count, so 256 languages of large patterns can't each stay in memory. + #[test] + fn remembered_patterns_keep_to_a_memory_budget() { + let mut s = State { dir: None, scan: None, hyphenation: BTreeMap::new(), hyphenation_bytes: 0, hyphenation_errors: Vec::new(), changes: 0 }; + let p = Arc::new(Patterns::parse("hy3ph he2n hena4 hen5at 1na n2at 1tio 2io o2n")); + let bytes = pattern_bytes(&p); + assert_eq!(bytes, 9 * BYTES_PER_PATTERN); + let lang = |t: &str| ProofLang::from_tag(Some(t)); + let budget = bytes * 2; + remember_patterns(&mut s, lang("nl"), Some(p.clone()), bytes, budget); + remember_patterns(&mut s, lang("fr"), Some(p.clone()), bytes, budget); + remember_patterns(&mut s, lang("sv"), None, 0, budget); + assert_eq!((s.hyphenation.len(), s.hyphenation_bytes), (3, budget), "a language with no patterns takes no room"); + remember_patterns(&mut s, lang("da"), Some(p.clone()), bytes, budget); + assert_eq!((s.hyphenation.len(), s.hyphenation_bytes), (1, bytes), "over the budget: the others are forgotten"); + assert!(s.hyphenation.contains_key(&lang("da"))); + remember_patterns(&mut s, lang("fi"), Some(p.clone()), budget * 4, budget); + assert_eq!((s.hyphenation.len(), s.hyphenation_bytes), (1, budget * 4), "patterns over the whole budget are kept alone"); + forget(&mut s); + assert_eq!((s.hyphenation.len(), s.hyphenation_bytes), (0, 0)); + } + /// Review of #485: a pack's name loses bidirectional and other format characters. #[test] fn pack_names_are_plain_text() {