From e4be836b99b7c559c646bb4e739c8340433ba1fb Mon Sep 17 00:00:00 2001 From: Bo Lopker Date: Mon, 6 Jul 2026 11:38:18 -0700 Subject: [PATCH 1/3] Overhaul error handling: remove panic paths, standardize on thiserror - Fix panics on user input/IO: spell_check_file returns Result, tree-sitter parse failures fall back to plain-text word splitting instead of poisoning the global parser cache, downloader no longer panics creating the cache dir (message also had an uninterpolated placeholder), dictionary manager handles non-UTF-8 paths and missing repo URLs - Add ConfigError (thiserror) to codebook-config; TOML parse/serialize errors are no longer shoehorned into io::Error and carry the file path - WatchedFile: loader errors are generic instead of String; a pathless watched file is a no-op, making reload_if_changed and reload infallible - Drop vestigial Results: Codebook::new/with_dictionary_dir return Self, CodebookConfig add_* methods return bool (persisting stays in save()) - HunspellDictionary::new returns a concrete HunspellError - Keep hot-path logs quiet: failed grammars are cached and logged once, invalid regex patterns log once per process, broken-config reload warns only when the file changes; LSP config poll interval 1s -> 5s and save failures are now logged --- Cargo.lock | 2 + Cargo.toml | 1 + crates/codebook-config/Cargo.toml | 1 + crates/codebook-config/src/helpers.rs | 16 +- crates/codebook-config/src/lib.rs | 237 +++++++++--------- crates/codebook-config/src/watched_file.rs | 76 +++--- crates/codebook-lsp/src/lint.rs | 11 +- crates/codebook-lsp/src/lsp.rs | 72 ++---- crates/codebook-lsp/src/main.rs | 8 +- crates/codebook/Cargo.toml | 1 + crates/codebook/src/checker.rs | 2 +- .../codebook/src/dictionaries/dictionary.rs | 34 ++- crates/codebook/src/dictionaries/manager.rs | 32 ++- crates/codebook/src/lib.rs | 14 +- crates/codebook/src/main.rs | 10 +- crates/codebook/src/parser.rs | 51 +++- .../codebook/tests/languages/test_config.rs | 2 +- crates/codebook/tests/languages/test_files.rs | 4 +- crates/codebook/tests/languages/utils.rs | 16 +- crates/downloader/src/lib.rs | 12 +- 20 files changed, 346 insertions(+), 256 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 778f2e7..6fecc80 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -361,6 +361,7 @@ dependencies = [ "spellbook", "streaming-iterator", "tempfile", + "thiserror", "tree-sitter", "tree-sitter-bash", "tree-sitter-c", @@ -445,6 +446,7 @@ dependencies = [ "regex", "serde", "tempfile", + "thiserror", "toml", "unicase", ] diff --git a/Cargo.toml b/Cargo.toml index a0cb046..6a92f15 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -47,6 +47,7 @@ spellbook = "<0.5.0" streaming-iterator = "0.1.9" string-offsets = "0.2.0" tempfile = "3" +thiserror = "2" tokio = { version = "1", features = ["full"] } toml = "<2" tower-lsp = "0.20.0" diff --git a/crates/codebook-config/Cargo.toml b/crates/codebook-config/Cargo.toml index 1196f44..08bedc9 100644 --- a/crates/codebook-config/Cargo.toml +++ b/crates/codebook-config/Cargo.toml @@ -18,6 +18,7 @@ glob.workspace = true log.workspace = true regex.workspace = true serde.workspace = true +thiserror.workspace = true toml.workspace = true unicase.workspace = true diff --git a/crates/codebook-config/src/helpers.rs b/crates/codebook-config/src/helpers.rs index 995a074..854000e 100644 --- a/crates/codebook-config/src/helpers.rs +++ b/crates/codebook-config/src/helpers.rs @@ -1,7 +1,9 @@ use log::error; use regex::{Regex, RegexBuilder}; +use std::collections::HashSet; use std::env; use std::path::{Path, PathBuf}; +use std::sync::{LazyLock, Mutex}; pub(crate) fn default_cache_dir() -> PathBuf { #[cfg(windows)] @@ -55,6 +57,12 @@ pub(crate) fn unix_cache_dir() -> PathBuf { env::temp_dir().join("codebook").join("cache") } +/// Invalid patterns already reported. This function runs on every spell check +/// when a file matches a config override — every keystroke in the LSP — so a +/// bad pattern is logged once per process, not once per check. +static REPORTED_INVALID_PATTERNS: LazyLock>> = + LazyLock::new(|| Mutex::new(HashSet::new())); + /// Compile user-provided ignore regex patterns, dropping invalid entries. /// Patterns are compiled with multiline mode so `^` and `$` match line boundaries. pub fn build_ignore_regexes(patterns: &[String]) -> Vec { @@ -64,7 +72,13 @@ pub fn build_ignore_regexes(patterns: &[String]) -> Vec { |pattern| match RegexBuilder::new(pattern).multi_line(true).build() { Ok(regex) => Some(regex), Err(e) => { - error!("Ignoring invalid regex pattern '{pattern}': {e}"); + if REPORTED_INVALID_PATTERNS + .lock() + .unwrap() + .insert(pattern.clone()) + { + error!("Ignoring invalid regex pattern '{pattern}': {e}"); + } None } }, diff --git a/crates/codebook-config/src/lib.rs b/crates/codebook-config/src/lib.rs index f571c8e..5c84428 100644 --- a/crates/codebook-config/src/lib.rs +++ b/crates/codebook-config/src/lib.rs @@ -11,7 +11,6 @@ use std::env; use std::fmt::Debug; use std::fs; use std::io; -use std::io::ErrorKind; use std::path::{Path, PathBuf}; use std::sync::{Arc, RwLock}; @@ -19,12 +18,34 @@ static CACHE_DIR: &str = "codebook"; static GLOBAL_CONFIG_FILE: &str = "codebook.toml"; static USER_CONFIG_FILES: [&str; 2] = ["codebook.toml", ".codebook.toml"]; +/// Errors from loading or saving Codebook configuration. +#[derive(Debug, thiserror::Error)] +pub enum ConfigError { + #[error("failed to read config file {path}: {source}")] + Read { path: PathBuf, source: io::Error }, + #[error("failed to parse config file {path}: {source}")] + Parse { + path: PathBuf, + source: toml::de::Error, + }, + #[error("failed to write config file {path}: {source}")] + Write { path: PathBuf, source: io::Error }, + #[error("failed to serialize config: {0}")] + Serialize(#[from] toml::ser::Error), + #[error(transparent)] + Io(#[from] io::Error), +} + /// The main trait for Codebook configuration. +/// +/// The `add_*` methods only mutate in-memory settings and report whether the +/// entry was newly added; persisting is a separate, fallible step (see +/// `CodebookConfigFile::save`). pub trait CodebookConfig: Sync + Send + Debug { - fn add_word(&self, word: &str) -> Result; - fn add_word_global(&self, word: &str) -> Result; - fn add_ignore(&self, file: &str) -> Result; - fn add_include(&self, file: &str) -> Result; + fn add_word(&self, word: &str) -> bool; + fn add_word_global(&self, word: &str) -> bool; + fn add_ignore(&self, file: &str) -> bool; + fn add_include(&self, file: &str) -> bool; fn get_dictionary_ids(&self) -> Vec; fn should_ignore_path(&self, path: &Path) -> bool; fn should_include_path(&self, path: &Path) -> bool; @@ -81,7 +102,7 @@ impl Default for CodebookConfigFile { impl CodebookConfigFile { /// Load configuration by searching for both global and project-specific configs - pub fn load(current_dir: Option<&Path>) -> Result { + pub fn load(current_dir: Option<&Path>) -> Result { Self::load_with_overrides(current_dir, None, None) } @@ -93,7 +114,7 @@ impl CodebookConfigFile { current_dir: Option<&Path>, global_config_path: Option, project_config_path: Option, - ) -> Result { + ) -> Result { debug!("Initializing CodebookConfig"); let current_dir = match current_dir { @@ -109,7 +130,7 @@ impl CodebookConfigFile { start_dir: &Path, global_config_override: Option, project_config_override: Option, - ) -> Result { + ) -> Result { // Canonicalize so a relative start_dir (like the CLI's default ".") can // walk up past the current directory: Path::new(".").parent() is "" // and then None, so the search would never reach real parent folders. @@ -137,12 +158,8 @@ impl CodebookConfigFile { // error the user should hear about, not a silent fallback to // defaults. Mid-session reload stays lenient (keeps the last // good config) since there's state to fall back to there. - inner.global_config = global_config - .load(|path| { - Self::load_settings_from_file(path) - .map_err(|e| format!("Failed to load global config: {}", e)) - }) - .map_err(|e| io::Error::new(ErrorKind::InvalidData, e))?; + inner.global_config = + global_config.load(|path| Self::load_settings_from_file(path))?; debug!("Loaded global config from {}", global_path.display()); } else { info!("No global config found, using default"); @@ -166,19 +183,15 @@ impl CodebookConfigFile { Some(override_path) } } - None => Self::find_project_config(start_dir)?, + None => Self::find_project_config(start_dir), }; if let Some(project_path) = project_path { let project_config = WatchedFile::new(Some(project_path.clone())); if project_path.exists() { - inner.project_config = project_config - .load(|path| { - Self::load_settings_from_file(path) - .map_err(|e| format!("Failed to load project config: {}", e)) - }) - .map_err(|e| io::Error::new(ErrorKind::InvalidData, e))?; + inner.project_config = + project_config.load(|path| Self::load_settings_from_file(path))?; debug!("Loaded project config from {}", project_path.display()); } else { inner.project_config = project_config; @@ -229,7 +242,7 @@ impl CodebookConfigFile { } /// Find project configuration by searching up from the current directory - fn find_project_config(start_dir: &Path) -> Result, io::Error> { + fn find_project_config(start_dir: &Path) -> Option { let config_files = USER_CONFIG_FILES; // Start from the given directory and walk up to root @@ -240,7 +253,7 @@ impl CodebookConfigFile { for config_name in &config_files { let config_path = dir.join(config_name); if config_path.is_file() { - return Ok(Some(config_path)); + return Some(config_path); } } @@ -248,24 +261,21 @@ impl CodebookConfigFile { current_dir = dir.parent().map(PathBuf::from); } - Ok(None) + None } /// Load settings from a file - fn load_settings_from_file>(path: P) -> Result { + fn load_settings_from_file>(path: P) -> Result { let path = path.as_ref(); - let content = fs::read_to_string(path)?; - - match toml::from_str(&content) { - Ok(settings) => Ok(settings), - Err(e) => { - let err = io::Error::new( - ErrorKind::InvalidData, - format!("Failed to parse config file {}: {e}", path.display()), - ); - Err(err) - } - } + let content = fs::read_to_string(path).map_err(|source| ConfigError::Read { + path: path.to_path_buf(), + source, + })?; + + toml::from_str(&content).map_err(|source| ConfigError::Parse { + path: path.to_path_buf(), + source, + }) } /// Calculate the effective settings based on global and project settings @@ -296,8 +306,11 @@ impl CodebookConfigFile { self.inner.read().unwrap().snapshot.clone() } - /// Reload both global and project configurations, only reading files if they've changed - pub fn reload(&self) -> Result { + /// Reload both global and project configurations, only reading files if + /// they've changed. Returns true when the effective settings changed. + /// Never fails: a config that can no longer be loaded keeps its last good + /// content (see WatchedFile::reload_if_changed). + pub fn reload(&self) -> bool { let mut inner = self.inner.write().unwrap(); let mut changed = false; @@ -305,13 +318,7 @@ impl CodebookConfigFile { let (new_global, global_changed) = inner .global_config .clone() - .reload_if_changed(|path| { - Self::load_settings_from_file(path).map_err(|e| e.to_string()) - }) - .unwrap_or_else(|e| { - debug!("Failed to reload global config: {}", e); - (inner.global_config.clone(), false) - }); + .reload_if_changed(|path| Self::load_settings_from_file(path)); if global_changed { debug!("Global config reloaded"); @@ -323,13 +330,7 @@ impl CodebookConfigFile { let (new_project, project_changed) = inner .project_config .clone() - .reload_if_changed(|path| { - Self::load_settings_from_file(path).map_err(|e| e.to_string()) - }) - .unwrap_or_else(|e| { - debug!("Failed to reload project config: {}", e); - (inner.project_config.clone(), false) - }); + .reload_if_changed(|path| Self::load_settings_from_file(path)); if project_changed { debug!("Project config reloaded"); @@ -342,11 +343,11 @@ impl CodebookConfigFile { Self::rebuild_snapshot(&mut inner); } - Ok(changed) + changed } /// Save the project configuration to its file - pub fn save(&self) -> Result<(), io::Error> { + pub fn save(&self) -> Result<(), ConfigError> { let mut inner = self.inner.write().unwrap(); let watched = &inner.project_config; Self::save_watched(watched, "project")?; @@ -355,7 +356,7 @@ impl CodebookConfigFile { } /// Save the global configuration to its file - pub fn save_global(&self) -> Result<(), io::Error> { + pub fn save_global(&self) -> Result<(), ConfigError> { let mut inner = self.inner.write().unwrap(); let watched = &inner.global_config; Self::save_watched(watched, "global")?; @@ -365,7 +366,7 @@ impl CodebookConfigFile { /// Write a watched config's content to its file. The caller must restamp /// the watched file afterwards so the write isn't seen as an external change. - fn save_watched(watched: &WatchedFile, label: &str) -> Result<(), io::Error> { + fn save_watched(watched: &WatchedFile, label: &str) -> Result<(), ConfigError> { let Some(path) = watched.path() else { return Ok(()); }; @@ -373,12 +374,16 @@ impl CodebookConfigFile { return Ok(()); }; - let content = toml::to_string_pretty(settings).map_err(io::Error::other)?; + let content = toml::to_string_pretty(settings)?; info!("Saving {label} configuration to {}", path.display()); + let write_err = |source| ConfigError::Write { + path: path.to_path_buf(), + source, + }; if let Some(parent) = path.parent() { - fs::create_dir_all(parent)?; + fs::create_dir_all(parent).map_err(write_err)?; } - fs::write(path, content) + fs::write(path, content).map_err(write_err) } /// Clean the cache directory pub fn clean_cache(&self) { @@ -483,22 +488,22 @@ impl CodebookConfigFile { impl CodebookConfig for CodebookConfigFile { /// Add a word to the project configs allowlist - fn add_word(&self, word: &str) -> Result { - Ok(self.update_project_settings(|settings| settings.insert_word(word))) + fn add_word(&self, word: &str) -> bool { + self.update_project_settings(|settings| settings.insert_word(word)) } /// Add a word to the global configs allowlist - fn add_word_global(&self, word: &str) -> Result { - Ok(self.update_global_settings(|settings| settings.insert_word(word))) + fn add_word_global(&self, word: &str) -> bool { + self.update_global_settings(|settings| settings.insert_word(word)) } /// Add a file to the ignore list - fn add_ignore(&self, file: &str) -> Result { - Ok(self.update_project_settings(|settings| settings.insert_ignore(file))) + fn add_ignore(&self, file: &str) -> bool { + self.update_project_settings(|settings| settings.insert_ignore(file)) } /// Add a file to the include list - fn add_include(&self, file: &str) -> Result { - Ok(self.update_project_settings(|settings| settings.insert_include(file))) + fn add_include(&self, file: &str) -> bool { + self.update_project_settings(|settings| settings.insert_include(file)) } /// Get dictionary IDs from effective configuration @@ -599,23 +604,23 @@ impl CodebookConfigMemory { } impl CodebookConfig for CodebookConfigMemory { - fn add_word(&self, word: &str) -> Result { + fn add_word(&self, word: &str) -> bool { let mut settings = self.settings.write().unwrap(); - Ok(settings.insert_word(word)) + settings.insert_word(word) } - fn add_word_global(&self, word: &str) -> Result { + fn add_word_global(&self, word: &str) -> bool { self.add_word(word) } - fn add_ignore(&self, file: &str) -> Result { + fn add_ignore(&self, file: &str) -> bool { let mut settings = self.settings.write().unwrap(); - Ok(settings.insert_ignore(file)) + settings.insert_ignore(file) } - fn add_include(&self, file: &str) -> Result { + fn add_include(&self, file: &str) -> bool { let mut settings = self.settings.write().unwrap(); - Ok(settings.insert_include(file)) + settings.insert_include(file) } fn get_dictionary_ids(&self) -> Vec { @@ -696,7 +701,7 @@ mod tests { } #[test] - fn test_save_global_creates_directories() -> Result<(), io::Error> { + fn test_save_global_creates_directories() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let global_dir = temp_dir.path().join("deep").join("nested").join("dir"); let config_path = global_dir.join("codebook.toml"); @@ -725,7 +730,7 @@ mod tests { } #[test] - fn test_add_word() -> Result<(), io::Error> { + fn test_add_word() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); @@ -739,7 +744,7 @@ mod tests { config.save()?; // Add a word - config.add_word("testword")?; + config.add_word("testword"); config.save()?; // Reload config and verify @@ -750,7 +755,7 @@ mod tests { } #[test] - fn test_add_word_global() -> Result<(), io::Error> { + fn test_add_word_global() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); @@ -765,7 +770,7 @@ mod tests { config.save_global()?; // Add a word - config.add_word_global("testword")?; + config.add_word_global("testword"); config.save_global()?; // Reload config and verify @@ -776,7 +781,7 @@ mod tests { } #[test] - fn test_ignore_patterns() -> Result<(), io::Error> { + fn test_ignore_patterns() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); let mut file = File::create(&config_path)?; @@ -798,7 +803,7 @@ mod tests { } #[test] - fn test_reload_ignore_patterns() -> Result<(), io::Error> { + fn test_reload_ignore_patterns() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); @@ -827,7 +832,7 @@ mod tests { file.write_all(a.as_bytes())?; // Reload and verify both patterns work - config.reload()?; + config.reload(); assert!(config.get_ignore_patterns().len() == 2); // Update config to remove all patterns @@ -840,14 +845,14 @@ mod tests { )?; // Reload and verify no patterns match - config.reload()?; + config.reload(); assert!(config.get_ignore_patterns().is_empty()); Ok(()) } #[test] - fn test_config_recursive_search() -> Result<(), io::Error> { + fn test_config_recursive_search() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let sub_dir = temp_dir.path().join("sub"); let sub_sub_dir = sub_dir.join("subsub"); @@ -878,7 +883,7 @@ mod tests { } #[test] - fn test_config_recursive_search_from_relative_dir() -> Result<(), io::Error> { + fn test_config_recursive_search_from_relative_dir() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let sub_dir = temp_dir.path().join("sub"); fs::create_dir_all(&sub_dir)?; @@ -902,7 +907,7 @@ mod tests { } #[test] - fn test_global_config_override_is_used() -> Result<(), io::Error> { + fn test_global_config_override_is_used() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let workspace_dir = temp_dir.path().join("workspace"); fs::create_dir_all(&workspace_dir)?; @@ -929,7 +934,7 @@ mod tests { } #[test] - fn test_project_config_override_is_used() -> Result<(), io::Error> { + fn test_project_config_override_is_used() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let workspace_dir = temp_dir.path().join("workspace"); fs::create_dir_all(&workspace_dir)?; @@ -958,7 +963,7 @@ mod tests { } #[test] - fn test_project_config_override_missing_file_used_on_save() -> Result<(), io::Error> { + fn test_project_config_override_missing_file_used_on_save() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let workspace_dir = temp_dir.path().join("workspace"); fs::create_dir_all(&workspace_dir)?; @@ -986,7 +991,7 @@ mod tests { // Adding a word and saving should create both the directory and the file // at the override location. - assert!(config.add_word("newword")?); + assert!(config.add_word("newword")); config.save()?; assert!(override_path.exists()); @@ -1053,7 +1058,7 @@ mod tests { } #[test] - fn test_reload() -> Result<(), io::Error> { + fn test_reload() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); @@ -1076,14 +1081,14 @@ mod tests { )?; // Reload config and verify - config.reload()?; + config.reload(); assert!(config.is_allowed_word("testword")); Ok(()) } #[test] - fn test_reload_when_deleted() -> Result<(), io::Error> { + fn test_reload_when_deleted() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); @@ -1106,21 +1111,21 @@ mod tests { )?; // Reload config and verify - config.reload()?; + config.reload(); assert!(config.is_allowed_word("testword")); // Delete the config file fs::remove_file(&config_path)?; // Reload config and verify - config.reload()?; + config.reload(); assert!(!config.is_allowed_word("testword")); Ok(()) } #[test] - fn test_add_word_case() -> Result<(), io::Error> { + fn test_add_word_case() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); @@ -1134,7 +1139,7 @@ mod tests { config.save()?; // Add a word with mixed case - config.add_word("TestWord")?; + config.add_word("TestWord"); config.save()?; // Reload config and verify with different cases @@ -1147,7 +1152,7 @@ mod tests { } #[test] - fn test_add_word_global_case() -> Result<(), io::Error> { + fn test_add_word_global_case() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); @@ -1162,7 +1167,7 @@ mod tests { config.save_global()?; // Add a word with mixed case - config.add_word_global("TestWord")?; + config.add_word_global("TestWord"); config.save_global()?; // Reload config and verify with different cases @@ -1175,7 +1180,7 @@ mod tests { } #[test] - fn test_global_and_project_config() -> Result<(), io::Error> { + fn test_global_and_project_config() -> Result<(), ConfigError> { // Create temporary directories for global and project configs let global_temp = TempDir::new().unwrap(); let project_temp = TempDir::new().unwrap(); @@ -1269,7 +1274,7 @@ mod tests { )?; // Reload - config.reload()?; + config.reload(); // Now should only see project words assert!(config.is_allowed_word("projectword")); // From project @@ -1300,7 +1305,7 @@ mod tests { } #[test] - fn test_resolve_for_file_with_matching_override() -> Result<(), io::Error> { + fn test_resolve_for_file_with_matching_override() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); let mut file = File::create(&config_path)?; @@ -1331,7 +1336,7 @@ mod tests { } #[test] - fn test_resolve_for_file_global_and_project_overrides() -> Result<(), io::Error> { + fn test_resolve_for_file_global_and_project_overrides() -> Result<(), ConfigError> { let global_temp = TempDir::new().unwrap(); let project_temp = TempDir::new().unwrap(); @@ -1397,7 +1402,7 @@ mod tests { } #[test] - fn test_resolve_for_file_use_global_false_ignores_global_overrides() -> Result<(), io::Error> { + fn test_resolve_for_file_use_global_false_ignores_global_overrides() -> Result<(), ConfigError> { let global_temp = TempDir::new().unwrap(); let project_temp = TempDir::new().unwrap(); @@ -1462,7 +1467,7 @@ mod tests { } #[test] - fn test_save_preserves_overrides() -> Result<(), io::Error> { + fn test_save_preserves_overrides() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); fs::write( @@ -1479,7 +1484,7 @@ mod tests { let config = load_from_file(ConfigType::Project, &config_path)?; // Add a word and save - config.add_word("newword")?; + config.add_word("newword"); config.save()?; // Reload and verify overrides are preserved @@ -1496,7 +1501,7 @@ mod tests { } #[test] - fn test_reload_picks_up_override_changes() -> Result<(), io::Error> { + fn test_reload_picks_up_override_changes() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); fs::write( @@ -1523,7 +1528,7 @@ mod tests { "#, )?; - config.reload()?; + config.reload(); // Now overrides should apply let resolved = config.resolve_for_file(Path::new("README.md")); @@ -1535,7 +1540,7 @@ mod tests { #[cfg(not(windows))] #[test] - fn test_tilde_in_global_override_is_expanded_at_load() -> Result<(), io::Error> { + fn test_tilde_in_global_override_is_expanded_at_load() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config = CodebookConfigFile::load_with_overrides( @@ -1571,12 +1576,12 @@ mod tests { None, ); let err = result.expect_err("invalid config should fail to load"); - assert_eq!(err.kind(), ErrorKind::InvalidData); + assert!(matches!(err, ConfigError::Parse { .. })); assert!(err.to_string().contains("codebook.toml")); } #[test] - fn test_reload_keeps_config_on_parse_error() -> Result<(), io::Error> { + fn test_reload_keeps_config_on_parse_error() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); fs::write(&config_path, r#"words = ["frobnicate"]"#)?; @@ -1593,7 +1598,7 @@ mod tests { // Simulate a mid-edit save of broken TOML fs::write(&config_path, r#"words = [invalid !!! toml"#)?; assert!( - !config.reload()?, + !config.reload(), "parse error should not count as a change" ); assert!( @@ -1603,13 +1608,13 @@ mod tests { // Once the file is valid again, the reload picks it up fs::write(&config_path, r#"words = ["frobnicate", "fixed"]"#)?; - assert!(config.reload()?); + assert!(config.reload()); assert!(config.is_allowed_word("fixed")); Ok(()) } #[test] - fn test_save_does_not_trigger_spurious_reload() -> Result<(), io::Error> { + fn test_save_does_not_trigger_spurious_reload() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("codebook.toml"); fs::write(&config_path, r#"words = ["zebra"]"#)?; @@ -1620,11 +1625,11 @@ mod tests { None, )?; - config.add_word("frobnicate").unwrap(); + config.add_word("frobnicate"); config.save()?; // Our own save must not read back as an external change - assert!(!config.reload()?); + assert!(!config.reload()); assert!(config.is_allowed_word("frobnicate")); assert!(config.is_allowed_word("zebra")); Ok(()) diff --git a/crates/codebook-config/src/watched_file.rs b/crates/codebook-config/src/watched_file.rs index d126be2..31dabca 100644 --- a/crates/codebook-config/src/watched_file.rs +++ b/crates/codebook-config/src/watched_file.rs @@ -16,6 +16,11 @@ pub struct WatchedFile { content: Option, last_modified: Option, last_size: Option, + /// Disk meta of the last load attempt that failed. Reload retries a + /// failing file on every poll (the stamp stays stale on purpose); this + /// gates the warn log to once per broken state of the file, so it fires + /// again only when the file actually changes. + last_failed_meta: Option<(Option, u64)>, } impl WatchedFile { @@ -26,6 +31,7 @@ impl WatchedFile { content: None, last_modified: None, last_size: None, + last_failed_meta: None, } } @@ -61,56 +67,69 @@ impl WatchedFile { } } - /// Load the file content, returning a new instance with updated content - pub fn load(self, loader: F) -> Result + /// Load the file content, returning a new instance with updated content. + /// A watched file without a path has nothing to load and is returned + /// unchanged. + pub fn load(self, loader: F) -> Result where - F: FnOnce(&Path) -> Result, + F: FnOnce(&Path) -> Result, { - let path = self - .path - .as_ref() - .ok_or_else(|| "No path configured for watched file".to_string())?; + let Some(path) = self.path.clone() else { + return Ok(self); + }; // Stat before reading: if a write races the read, the stamp stays older // than the file and the next check reloads, instead of missing the write. - let meta = disk_meta(path); + let meta = disk_meta(&path); if self.content.is_none() || self.meta_differs(&meta) { - let content = loader(path)?; + let content = loader(&path)?; Ok(self.with_content_meta(content, meta)) } else { Ok(self) } } - /// Load the file content if it has changed - /// Returns (new_instance, was_changed) - pub fn reload_if_changed(self, loader: F) -> Result<(Self, bool), String> + /// Load the file content if it has changed. + /// Returns (new_instance, was_changed). Loader failures never propagate: + /// if the file was deleted the content is cleared; if it exists but is + /// unreadable or invalid the last good content is kept. + pub fn reload_if_changed(self, loader: F) -> (Self, bool) where - F: FnOnce(&Path) -> Result, + F: FnOnce(&Path) -> Result, + E: std::fmt::Display, { - let path = self - .path - .as_ref() - .ok_or_else(|| "No path configured for watched file".to_string())?; + let Some(path) = self.path.clone() else { + // A watched file without a path never changes. + return (self, false); + }; // Stat before reading (see load() for why) - let meta = disk_meta(path); + let meta = disk_meta(&path); if !self.meta_differs(&meta) { - return Ok((self, false)); + return (self, false); } - match loader(path) { - Ok(content) => Ok((self.with_content_meta(content, meta), true)), + match loader(&path) { + Ok(content) => (self.with_content_meta(content, meta), true), Err(_) if meta.is_none() => { // File was deleted, clear the content - Ok((self.cleared(), true)) + (self.cleared(), true) } Err(e) => { // File exists but is unreadable or invalid (e.g. mid-edit TOML). // Keep the last good content; the stale stamp means we retry on - // the next reload. - log::warn!("Keeping previous config for {}: {e}", path.display()); - Ok((self, false)) + // the next reload. Warn only when the failing file changed, so + // a config left broken on disk logs once, not on every poll. + if self.last_failed_meta != meta { + log::warn!("Keeping previous config for {}: {e}", path.display()); + } + ( + Self { + last_failed_meta: meta, + ..self + }, + false, + ) } } } @@ -152,6 +171,7 @@ impl WatchedFile { content: Some(content), last_modified, last_size, + last_failed_meta: None, } } @@ -162,6 +182,7 @@ impl WatchedFile { content: None, last_modified: None, last_size: None, + last_failed_meta: None, } } } @@ -233,9 +254,8 @@ mod tests { assert!(watched.has_changed()); // Reload should clear the content - let (watched, changed) = watched - .reload_if_changed(|path| fs::read_to_string(path).map_err(|e| e.to_string())) - .unwrap(); + let (watched, changed) = + watched.reload_if_changed(|path| fs::read_to_string(path).map_err(|e| e.to_string())); assert!(changed); // Content should now be None diff --git a/crates/codebook-lsp/src/lint.rs b/crates/codebook-lsp/src/lint.rs index 94374b3..b94915f 100644 --- a/crates/codebook-lsp/src/lint.rs +++ b/crates/codebook-lsp/src/lint.rs @@ -102,13 +102,7 @@ pub fn run_lint(files: &[String], root: &Path, unique: bool, suggest: bool) -> L print_config_source(&config); eprintln!(); - let codebook = match Codebook::new(config.clone()) { - Ok(c) => c, - Err(e) => { - err!("failed to initialize: {e}"); - return LintResult::Failure; - } - }; + let codebook = Codebook::new(config.clone()); // Canonicalize the root once here rather than once per file. let root_canonical = root.canonicalize().ok(); @@ -428,8 +422,7 @@ mod tests { let cb = Codebook::with_dictionary_dir( Arc::new(CodebookConfigMemory::default()), Some(fixtures), - ) - .unwrap(); + ); let mut seen = HashSet::new(); // Test basic flagging and multi-occurrence counting diff --git a/crates/codebook-lsp/src/lsp.rs b/crates/codebook-lsp/src/lsp.rs index 0c0564c..1f37ab2 100644 --- a/crates/codebook-lsp/src/lsp.rs +++ b/crates/codebook-lsp/src/lsp.rs @@ -31,7 +31,7 @@ const SOURCE_NAME: &str = "Codebook"; /// on every keystroke with checkWhileTyping, so polling is debounced rather /// than done per call; changes made via code actions bypass this by calling /// recheck_all directly. -const CONFIG_POLL_INTERVAL: Duration = Duration::from_secs(1); +const CONFIG_POLL_INTERVAL: Duration = Duration::from_secs(5); /// Computes the relative path of a file from a workspace directory. /// Returns the relative path if the file is within the workspace, otherwise returns the absolute path. @@ -342,7 +342,9 @@ impl LanguageServer for Backend { ); let updated = self.add_words(config.as_ref(), words); if updated { - let _ = config.save(); + if let Err(e) = config.save() { + error!("Failed to save config: {e}"); + } self.recheck_all().await; } Ok(None) @@ -355,7 +357,9 @@ impl LanguageServer for Backend { .filter_map(|arg| arg.as_str().map(|s| s.to_string())); let updated = self.add_words_global(config.as_ref(), words); if updated { - let _ = config.save_global(); + if let Err(e) = config.save_global() { + error!("Failed to save global config: {e}"); + } self.recheck_all().await; } Ok(None) @@ -368,7 +372,9 @@ impl LanguageServer for Backend { let config = self.config_handle(); let updated = self.add_ignore_file(config.as_ref(), file_uri); if updated { - let _ = config.save(); + if let Err(e) = config.save() { + error!("Failed to save config: {e}"); + } self.recheck_all().await; } Ok(None) @@ -432,12 +438,7 @@ impl Backend { fn codebook_handle(&self) -> Arc { self.codebook - .get_or_init(|| { - Arc::new( - Codebook::new(self.config_handle()) - .unwrap_or_else(|e| panic!("Unable to initialize codebook: {e}")), - ) - }) + .get_or_init(|| Arc::new(Codebook::new(self.config_handle()))) .clone() } @@ -448,16 +449,10 @@ impl Backend { fn add_words(&self, config: &CodebookConfigFile, words: impl Iterator) -> bool { let mut should_save = false; for word in words { - match config.add_word(&word) { - Ok(true) => { - should_save = true; - } - Ok(false) => { - info!("Word '{word}' already exists in dictionary."); - } - Err(e) => { - error!("Failed to add word: {e}"); - } + if config.add_word(&word) { + should_save = true; + } else { + info!("Word '{word}' already exists in dictionary."); } } should_save @@ -470,16 +465,10 @@ impl Backend { ) -> bool { let mut should_save = false; for word in words { - match config.add_word_global(&word) { - Ok(true) => { - should_save = true; - } - Ok(false) => { - info!("Word '{word}' already exists in global dictionary."); - } - Err(e) => { - error!("Failed to add word: {e}"); - } + if config.add_word_global(&word) { + should_save = true; + } else { + info!("Word '{word}' already exists in global dictionary."); } } should_save @@ -505,16 +494,11 @@ impl Backend { let Some(relative_path) = self.get_relative_path(file_uri) else { return false; }; - match config.add_ignore(&relative_path) { - Ok(true) => true, - Ok(false) => { - info!("File {file_uri} already exists in the ignored files."); - false - } - Err(e) => { - error!("Failed to add ignore file: {e}"); - false - } + if config.add_ignore(&relative_path) { + true + } else { + info!("File {file_uri} already exists in the ignored files."); + false } } @@ -564,13 +548,7 @@ impl Backend { *last_poll = Some(Instant::now()); } - match self.config_handle().reload() { - Ok(did_reload) => did_reload, - Err(e) => { - error!("Failed to reload config: {e}"); - false - } - } + self.config_handle().reload() } async fn spell_check(&self, uri: &Url) { diff --git a/crates/codebook-lsp/src/main.rs b/crates/codebook-lsp/src/main.rs index bae0495..331df2f 100644 --- a/crates/codebook-lsp/src/main.rs +++ b/crates/codebook-lsp/src/main.rs @@ -5,7 +5,7 @@ mod lsp; mod lsp_logger; use clap::{Parser, Subcommand}; -use codebook_config::{CodebookConfig, CodebookConfigFile}; +use codebook_config::{CodebookConfig, CodebookConfigFile, ConfigError}; use log::{LevelFilter, debug, info}; use lsp::Backend; use lsp_logger::LspLogger; @@ -108,14 +108,14 @@ async fn main() { /// Adds words to the project (or global) config's allowlist and saves the file, /// creating it if it doesn't exist yet. -fn add_words(root: &Path, words: &[String], global: bool) -> Result<(), std::io::Error> { +fn add_words(root: &Path, words: &[String], global: bool) -> Result<(), ConfigError> { let config = CodebookConfigFile::load(Some(root))?; let mut added = 0; for word in words { let inserted = if global { - config.add_word_global(word)? + config.add_word_global(word) } else { - config.add_word(word)? + config.add_word(word) }; if inserted { added += 1; diff --git a/crates/codebook/Cargo.toml b/crates/codebook/Cargo.toml index 26ab79c..93a35bd 100644 --- a/crates/codebook/Cargo.toml +++ b/crates/codebook/Cargo.toml @@ -25,6 +25,7 @@ lru.workspace = true regex.workspace = true spellbook.workspace = true streaming-iterator.workspace = true +thiserror.workspace = true tree-sitter-bash.workspace = true tree-sitter-c.workspace = true tree-sitter-cpp.workspace = true diff --git a/crates/codebook/src/checker.rs b/crates/codebook/src/checker.rs index 5685919..f89f7a4 100644 --- a/crates/codebook/src/checker.rs +++ b/crates/codebook/src/checker.rs @@ -145,7 +145,7 @@ mod tests { fn test_check_words_respects_allowed_words() { let dict = Arc::new(TextDictionary::new("")); let config = Arc::new(codebook_config::CodebookConfigMemory::default()); - config.add_word("codebook").unwrap(); + config.add_word("codebook"); let candidates = make_candidates(&[("codebook", 0, 8)]); let results = check_words(&candidates, &[dict], config.as_ref(), None); assert!(results.is_empty(), "Allowed words should not be flagged"); diff --git a/crates/codebook/src/dictionaries/dictionary.rs b/crates/codebook/src/dictionaries/dictionary.rs index d21a5ab..efd5361 100644 --- a/crates/codebook/src/dictionaries/dictionary.rs +++ b/crates/codebook/src/dictionaries/dictionary.rs @@ -13,6 +13,22 @@ pub trait Dictionary: Send + Sync { fn suggest(&self, word: &str) -> Vec; } +/// Failure to load a Hunspell dictionary from its `.aff`/`.dic` file pair. +#[derive(Debug, thiserror::Error)] +pub enum HunspellError { + #[error("failed to read dictionary file {path}: {source}")] + Read { + path: PathBuf, + source: std::io::Error, + }, + #[error("failed to parse dictionary [aff: {aff}, dic: {dic}]: {message}")] + Parse { + aff: String, + dic: String, + message: String, + }, +} + enum WordCase { AllCaps, AllLower, @@ -28,11 +44,19 @@ pub struct HunspellDictionary { } impl HunspellDictionary { - pub fn new(aff_path: &str, dic_path: &str) -> Result> { - let aff = std::fs::read_to_string(aff_path)?; - let dic = std::fs::read_to_string(dic_path)?; - let dict = spellbook::Dictionary::new(&aff, &dic).map_err(|e| { - format!("Dictionary [aff: {aff_path}, dic: {dic_path}] parse error: {e}") + pub fn new(aff_path: &str, dic_path: &str) -> Result { + let read = |path: &str| { + std::fs::read_to_string(path).map_err(|source| HunspellError::Read { + path: path.into(), + source, + }) + }; + let aff = read(aff_path)?; + let dic = read(dic_path)?; + let dict = spellbook::Dictionary::new(&aff, &dic).map_err(|e| HunspellError::Parse { + aff: aff_path.to_string(), + dic: dic_path.to_string(), + message: e.to_string(), })?; Ok(HunspellDictionary { diff --git a/crates/codebook/src/dictionaries/manager.rs b/crates/codebook/src/dictionaries/manager.rs index 84a7400..1a2f682 100644 --- a/crates/codebook/src/dictionaries/manager.rs +++ b/crates/codebook/src/dictionaries/manager.rs @@ -131,7 +131,7 @@ impl DictionaryManager { if aff.is_file() && dic.is_file() { match HunspellDictionary::new(aff.to_str()?, dic.to_str()?) { Ok(dict) => return Some(Arc::new(dict)), - Err(e) => error!("Failed to load local dictionary '{id}': {e:?}"), + Err(e) => error!("Failed to load local dictionary '{id}': {e}"), } } None @@ -139,7 +139,7 @@ impl DictionaryManager { fn download(&self, url: &str) -> Result { self.downloader.get(url).map_err(|e| { - error!("Error: {e:?}"); + error!("Failed to download dictionary file {url}: {e:?}"); LoadError { permanent: e.downcast_ref::().is_some(), } @@ -152,14 +152,17 @@ impl DictionaryManager { ) -> Result, LoadError> { let aff_path = self.download(&repo.aff_url)?; let dic_path = self.download(&repo.dict_url)?; - let dict = - match HunspellDictionary::new(aff_path.to_str().unwrap(), dic_path.to_str().unwrap()) { - Ok(dict) => dict, - Err(e) => { - error!("Error: {e:?}"); - return Err(LoadError { permanent: false }); - } - }; + let (Some(aff), Some(dic)) = (aff_path.to_str(), dic_path.to_str()) else { + error!("Dictionary cache path is not valid UTF-8: {aff_path:?}"); + return Err(LoadError { permanent: true }); + }; + let dict = match HunspellDictionary::new(aff, dic) { + Ok(dict) => dict, + Err(e) => { + error!("Failed to load Hunspell dictionary: {e}"); + return Err(LoadError { permanent: false }); + } + }; let base: Arc = Arc::new(dict); Ok(match repo.transliteration { Some(t) => Arc::new(TransliteratingDictionary::new(base, t.variants_fn())), @@ -171,7 +174,14 @@ impl DictionaryManager { if let Some(text) = repo.text { return Ok(Arc::new(TextDictionary::new(text))); } - let text_path = self.download(&repo.url.unwrap())?; + let Some(url) = repo.url else { + error!( + "Text dictionary repo '{}' has neither embedded text nor a URL", + repo.name + ); + return Err(LoadError { permanent: true }); + }; + let text_path = self.download(&url)?; let dict = TextDictionary::new_from_path(&text_path); Ok(Arc::new(dict)) } diff --git a/crates/codebook/src/lib.rs b/crates/codebook/src/lib.rs index dd409b0..d1977dd 100644 --- a/crates/codebook/src/lib.rs +++ b/crates/codebook/src/lib.rs @@ -27,7 +27,7 @@ pub struct Codebook { pub static DEFAULT_DICTIONARIES: &[&str; 3] = &["codebook", "software_terms", "computing_acronyms"]; impl Codebook { - pub fn new(config: Arc) -> Result> { + pub fn new(config: Arc) -> Self { Self::with_dictionary_dir(config, None) } @@ -38,10 +38,10 @@ impl Codebook { pub fn with_dictionary_dir( config: Arc, dictionary_dir: Option, - ) -> Result> { + ) -> Self { let manager = DictionaryManager::with_local_dir(&config.cache_dir().to_path_buf(), dictionary_dir); - Ok(Self { config, manager }) + Self { config, manager } } /// Get WordLocations for a block of text. @@ -141,10 +141,12 @@ impl Codebook { dictionaries } - pub fn spell_check_file(&self, path: &str) -> Vec { + /// Spell check a file on disk, detecting the language from its path. + /// Errors if the file can't be read (including non-UTF-8 content). + pub fn spell_check_file(&self, path: &str) -> Result, std::io::Error> { let lang_type = queries::get_language_name_from_filename(path); - let file_text = std::fs::read_to_string(path).unwrap(); - self.spell_check(&file_text, Some(lang_type), Some(path)) + let file_text = std::fs::read_to_string(path)?; + Ok(self.spell_check(&file_text, Some(lang_type), Some(path))) } /// Get suggestions for a misspelled word. Returns None when the word is diff --git a/crates/codebook/src/main.rs b/crates/codebook/src/main.rs index be427e6..47f472b 100644 --- a/crates/codebook/src/main.rs +++ b/crates/codebook/src/main.rs @@ -53,7 +53,7 @@ fn main() { } let config = Arc::new(codebook_config::CodebookConfigFile::load(None).unwrap()); - let processor = Codebook::new(config).unwrap(); + let processor = Codebook::new(config); // Check for benchmark flag if args.contains(&"--benchmark".to_string()) { @@ -80,7 +80,13 @@ fn main() { eprintln!("Can't find file {path:?}"); return; } - let results = processor.spell_check_file(path.to_str().unwrap()); + let results = match processor.spell_check_file(path.to_str().unwrap()) { + Ok(results) => results, + Err(e) => { + eprintln!("Can't read file {path:?}: {e}"); + return; + } + }; println!("Misspelled words: {results:?}"); println!("Done"); } diff --git a/crates/codebook/src/parser.rs b/crates/codebook/src/parser.rs index 3f4ea2e..2c4d509 100644 --- a/crates/codebook/src/parser.rs +++ b/crates/codebook/src/parser.rs @@ -1,7 +1,9 @@ use crate::checker::WordCandidate; -use crate::queries::{LANGUAGE_SETTINGS, LanguageType, get_language_setting}; +use crate::queries::{LANGUAGE_SETTINGS, LanguageSetting, LanguageType, get_language_setting}; use crate::splitter; +use log::{debug, error}; use regex::Regex; +use std::collections::hash_map::Entry; use std::collections::{HashMap, HashSet}; use std::str::FromStr; use std::sync::{LazyLock, Mutex}; @@ -13,7 +15,9 @@ use unicode_segmentation::UnicodeSegmentation; /// Global parser cache protected by a mutex. Serializes all tree-sitter /// operations (create, parse, destroy) to protect external scanners that /// use global mutable C state (e.g. tree-sitter-vhdl's static TokenTree). -static PARSER_CACHE: LazyLock>> = +/// A cached `None` records a grammar that failed to load, so the failure is +/// logged once instead of retried (and re-logged) on every spell check. +static PARSER_CACHE: LazyLock>>> = LazyLock::new(|| Mutex::new(HashMap::new())); /// One `(#not-has-ancestor? @capture "kind" ...)` rule, pre-parsed so the @@ -232,6 +236,23 @@ struct ExtractionResult<'a> { languages: HashSet, } +/// Create a parser for a language's grammar, or None when the grammar can't +/// be loaded (e.g. a tree-sitter ABI version mismatch). +fn new_parser(setting: &LanguageSetting) -> Option { + let lang = setting.language()?; + let mut parser = Parser::new(); + match parser.set_language(&lang) { + Ok(()) => Some(parser), + Err(e) => { + error!( + "Failed to load tree-sitter grammar for {:?}: {e}", + setting.type_ + ); + None + } + } +} + /// Recursively extract words from a byte range of the document. /// /// For languages with a tree-sitter grammar and .scm query: @@ -262,16 +283,26 @@ fn extract_recursive<'a>( let region_text = &document_text[start_byte..end_byte]; - // Parse under global lock + // Parse under global lock. This block must not panic: a panic while + // holding PARSER_CACHE poisons the mutex and fails every spell check + // for the rest of the process. let tree = { let mut cache = PARSER_CACHE.lock().unwrap(); - let parser = cache.entry(language).or_insert_with(|| { - let mut parser = Parser::new(); - let lang = language_setting.language().unwrap(); - parser.set_language(&lang).unwrap(); - parser - }); - parser.parse(region_text, None).unwrap() + let parser = match cache.entry(language) { + Entry::Occupied(entry) => entry.into_mut(), + Entry::Vacant(entry) => entry.insert(new_parser(language_setting)), + }; + parser.as_mut().and_then(|parser| parser.parse(region_text, None)) + }; + + let Some(tree) = tree else { + // Grammar unavailable or parse failed: degrade to word-splitting the + // region as plain text so it is still checked. debug-level because + // this runs per check; the grammar failure itself was already logged + // once by new_parser. + debug!("Parsing {language:?} unavailable; checking region as plain text"); + extract_words_from_text(region_text, start_byte, skip_ranges, &mut result.candidates); + return; }; let root_node = tree.root_node(); diff --git a/crates/codebook/tests/languages/test_config.rs b/crates/codebook/tests/languages/test_config.rs index c127be4..72672c1 100644 --- a/crates/codebook/tests/languages/test_config.rs +++ b/crates/codebook/tests/languages/test_config.rs @@ -5,7 +5,7 @@ use std::sync::Arc; fn get_processor_with_words(words: &[&str]) -> Codebook { let config = Arc::new(CodebookConfigMemory::default()); for w in words { - let _ = config.add_word(w); + config.add_word(w); } super::utils::make_codebook(config) } diff --git a/crates/codebook/tests/languages/test_files.rs b/crates/codebook/tests/languages/test_files.rs index 0951c7a..c394881 100644 --- a/crates/codebook/tests/languages/test_files.rs +++ b/crates/codebook/tests/languages/test_files.rs @@ -190,7 +190,9 @@ fn test_example_files() { ]; let processor = super::utils::get_processor(); for (file, expected) in files { - let results = processor.spell_check_file(&example_file_path(file)); + let results = processor + .spell_check_file(&example_file_path(file)) + .unwrap(); let mut misspelled: Vec<&str> = results.iter().map(|r| r.word.as_str()).collect(); misspelled.sort_unstable(); assert_eq!(&misspelled, expected, "flagged words in {file}"); diff --git a/crates/codebook/tests/languages/utils.rs b/crates/codebook/tests/languages/utils.rs index 16df3a1..57fe823 100644 --- a/crates/codebook/tests/languages/utils.rs +++ b/crates/codebook/tests/languages/utils.rs @@ -13,31 +13,25 @@ use codebook_config::{CodebookConfig, CodebookConfigMemory}; pub fn make_codebook(config: Arc) -> Codebook { init_logging(); let fixtures = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/dictionaries"); - Codebook::with_dictionary_dir(config, Some(fixtures)).unwrap() + Codebook::with_dictionary_dir(config, Some(fixtures)) } pub fn get_processor() -> Codebook { let config = Arc::new(CodebookConfigMemory::default()); - config - .add_ignore("**/ignore.txt") - .expect("Should ignore file"); + config.add_ignore("**/ignore.txt"); make_codebook(config) } pub fn get_processor_with_include(include: &str) -> Codebook { let config = Arc::new(CodebookConfigMemory::default()); - config - .add_include(include) - .expect("Should add include path"); + config.add_include(include); make_codebook(config) } pub fn get_processor_with_include_and_ignore(include: &str, ignore: &str) -> Codebook { let config = Arc::new(CodebookConfigMemory::default()); - config - .add_include(include) - .expect("Should add include path"); - config.add_ignore(ignore).expect("Should add ignore path"); + config.add_include(include); + config.add_ignore(ignore); make_codebook(config) } diff --git a/crates/downloader/src/lib.rs b/crates/downloader/src/lib.rs index 029b22c..f9537bc 100644 --- a/crates/downloader/src/lib.rs +++ b/crates/downloader/src/lib.rs @@ -1,7 +1,7 @@ use anyhow::Result; use base16ct::lower; use chrono::{DateTime, Utc}; -use log::info; +use log::{error, info}; use reqwest::blocking::Client; use reqwest::header::{IF_MODIFIED_SINCE, LAST_MODIFIED}; use rustls::ClientConfig; @@ -212,8 +212,14 @@ impl Downloader { let metadata_path = self.metadata_path.clone(); let cache_dir = self.cache_dir.clone(); self.metadata.get_or_init(move || { - fs::create_dir_all(&cache_dir) - .expect("Failed to create cache directory: {cache_dir:?}"); + // Best-effort: if the directory can't be created, downloads fail + // later with a real error instead of panicking here. + if let Err(e) = fs::create_dir_all(&cache_dir) { + error!( + "Failed to create cache directory {}: {e}", + cache_dir.display() + ); + } let metadata = Self::load_metadata(&metadata_path); RwLock::new(metadata) }) From 5d9e9b6932c5c3283b2a1e6d1fd47084501a3f54 Mon Sep 17 00:00:00 2001 From: Bo Lopker Date: Mon, 6 Jul 2026 12:28:23 -0700 Subject: [PATCH 2/3] Compile config patterns at parse time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ignore_patterns are now Vec and override paths are Vec, compiled during deserialization via field-level serde helpers. An invalid regex or glob is a config parse error naming the offending pattern (strict at startup, keeps last good config on mid-session reload) instead of a silently disabled pattern. The spell-check hot path no longer compiles or validates anything — it only clones pre-compiled handles. That deletes the machinery that existed to work around compile-per-use: build_ignore_regexes and its log-dedup/memo cache, and the snapshot-level ignore_regexes field. Also: ignore_patterns keep their written order (no more sort/dedup, so save() stops alphabetizing them), and ConfigSettings/OverrideBlock drop their PartialEq derives (Regex doesn't compare; the two whole-struct test asserts compare serialized forms instead). --- README.md | 3 +- crates/codebook-config/src/helpers.rs | 69 ------ crates/codebook-config/src/lib.rs | 30 ++- crates/codebook-config/src/settings.rs | 292 +++++++++++++++++++------ crates/codebook/src/lib.rs | 3 +- 5 files changed, 250 insertions(+), 147 deletions(-) diff --git a/README.md b/README.md index 804e2b5..c1b598a 100644 --- a/README.md +++ b/README.md @@ -401,6 +401,7 @@ The `ignore_patterns` configuration allows you to define custom regex patterns t - **Multiline mode is enabled**: `^` and `$` match line boundaries, not just start/end of file - Example: `'^vim\..*'` skips all words on lines starting with `vim.` - Example: `'vim\.opt\.[a-z]+'` matches `vim.opt.showmode`, so `showmode` is skipped +- Patterns are validated when the config is loaded: an invalid regex is a configuration error, reported with the offending pattern **TOML Literal Strings**: Use single quotes for regex patterns to avoid escaping backslashes: @@ -472,7 +473,7 @@ dictionaries = ["de"] | Field | Behavior | | ----------------------- | -------- | -| `paths` | Required. Glob patterns matched against the file path relative to the project root. A file matches the block if it matches *any* pattern. | +| `paths` | Required. Glob patterns matched against the file path relative to the project root. A file matches the block if it matches *any* pattern. An invalid glob is a configuration error. | | `dictionaries` | Replaces the resolved `dictionaries` list. | | `words` | Replaces the resolved `words` list. | | `flag_words` | Replaces the resolved `flag_words` list. | diff --git a/crates/codebook-config/src/helpers.rs b/crates/codebook-config/src/helpers.rs index 854000e..59abf52 100644 --- a/crates/codebook-config/src/helpers.rs +++ b/crates/codebook-config/src/helpers.rs @@ -1,9 +1,5 @@ -use log::error; -use regex::{Regex, RegexBuilder}; -use std::collections::HashSet; use std::env; use std::path::{Path, PathBuf}; -use std::sync::{LazyLock, Mutex}; pub(crate) fn default_cache_dir() -> PathBuf { #[cfg(windows)] @@ -57,35 +53,6 @@ pub(crate) fn unix_cache_dir() -> PathBuf { env::temp_dir().join("codebook").join("cache") } -/// Invalid patterns already reported. This function runs on every spell check -/// when a file matches a config override — every keystroke in the LSP — so a -/// bad pattern is logged once per process, not once per check. -static REPORTED_INVALID_PATTERNS: LazyLock>> = - LazyLock::new(|| Mutex::new(HashSet::new())); - -/// Compile user-provided ignore regex patterns, dropping invalid entries. -/// Patterns are compiled with multiline mode so `^` and `$` match line boundaries. -pub fn build_ignore_regexes(patterns: &[String]) -> Vec { - patterns - .iter() - .filter_map( - |pattern| match RegexBuilder::new(pattern).multi_line(true).build() { - Ok(regex) => Some(regex), - Err(e) => { - if REPORTED_INVALID_PATTERNS - .lock() - .unwrap() - .insert(pattern.clone()) - { - error!("Ignoring invalid regex pattern '{pattern}': {e}"); - } - None - } - }, - ) - .collect() -} - /// Expand `~` and `~/` prefixes to the current user's home directory on all /// platforms (`~\` is also accepted on Windows, where it is a separator). /// Other paths, including `~user/...`, are returned unchanged. @@ -242,40 +209,4 @@ mod tests { assert_eq!(expand_tilde(&path), Some(path)); } - #[test] - fn test_build_ignore_regexes_valid_patterns() { - let patterns = vec![r"\b[A-Z]{2,}\b".to_string(), r"TODO:.*".to_string()]; - - let compiled = build_ignore_regexes(&patterns); - assert_eq!(compiled.len(), 2); - assert!(compiled[0].is_match("HTML")); - assert!(compiled[1].is_match("TODO: fix this")); - } - - #[test] - fn test_build_ignore_regexes_invalid_pattern_skipped() { - let patterns = vec![ - r"valid.*".to_string(), - r"[invalid".to_string(), // Missing closing bracket - r"also_valid".to_string(), - ]; - - let compiled = build_ignore_regexes(&patterns); - // Invalid pattern should be skipped, not crash - assert_eq!(compiled.len(), 2); - } - - #[test] - fn test_build_ignore_regexes_multiline_mode() { - let patterns = vec![r"^vim\..*".to_string()]; - let compiled = build_ignore_regexes(&patterns); - - let text = "let x = 1\nvim.opt.showmode = false\nlet y = 2"; - - // Should match line starting with vim. (multiline mode) - assert!(compiled[0].is_match(text)); - - let m = compiled[0].find(text).unwrap(); - assert_eq!(m.as_str(), "vim.opt.showmode = false"); - } } diff --git a/crates/codebook-config/src/lib.rs b/crates/codebook-config/src/lib.rs index 5c84428..0864906 100644 --- a/crates/codebook-config/src/lib.rs +++ b/crates/codebook-config/src/lib.rs @@ -72,8 +72,6 @@ struct ConfigInner { global_config: WatchedFile, /// Current snapshot snapshot: Arc, - /// Ignore regexes compiled from the snapshot, rebuilt whenever it changes - ignore_regexes: Vec, } #[derive(Debug)] @@ -90,7 +88,6 @@ impl Default for CodebookConfigFile { project_config: WatchedFile::new(None), global_config: WatchedFile::new(None), snapshot: Arc::new(ConfigSettings::default()), - ignore_regexes: Vec::new(), }; Self { @@ -441,7 +438,6 @@ impl CodebookConfigFile { fn rebuild_snapshot(inner: &mut ConfigInner) { let effective = Self::calculate_effective_settings(&inner.project_config, &inner.global_config); - inner.ignore_regexes = helpers::build_ignore_regexes(&effective.ignore_patterns); inner.snapshot = Arc::new(effective); } @@ -537,9 +533,9 @@ impl CodebookConfig for CodebookConfigFile { } /// Get the list of user-defined ignore patterns, compiled when the - /// snapshot was last rebuilt. Regex clones are cheap (internally Arc'd). + /// config was parsed. Regex clones are cheap (internally Arc'd). fn get_ignore_patterns(&self) -> Vec { - self.inner.read().unwrap().ignore_regexes.clone() + self.snapshot().ignore_patterns.clone() } /// Get the minimum word length which should be checked @@ -649,7 +645,7 @@ impl CodebookConfig for CodebookConfigMemory { } fn get_ignore_patterns(&self) -> Vec { - helpers::build_ignore_regexes(&self.settings.read().unwrap().ignore_patterns) + self.settings.read().unwrap().ignore_patterns.clone() } fn get_min_word_length(&self) -> usize { @@ -795,8 +791,8 @@ mod tests { let config = load_from_file(ConfigType::Project, &config_path)?; let patterns = config.snapshot().ignore_patterns.clone(); - assert!(patterns.contains(&String::from("^[ATCG]+$"))); - assert!(patterns.contains(&String::from("\\d{3}-\\d{2}-\\d{4}"))); + assert!(patterns.iter().any(|p| p.as_str() == "^[ATCG]+$")); + assert!(patterns.iter().any(|p| p.as_str() == "\\d{3}-\\d{2}-\\d{4}")); let patterns = config.get_ignore_patterns(); assert!(patterns.len() == 2); Ok(()) @@ -1580,6 +1576,22 @@ mod tests { assert!(err.to_string().contains("codebook.toml")); } + #[test] + fn test_load_fails_on_invalid_ignore_pattern() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("codebook.toml"); + fs::write(&config_path, r#"ignore_patterns = ["[invalid"]"#).unwrap(); + + let result = CodebookConfigFile::load_with_overrides( + Some(temp_dir.path()), + Some(temp_dir.path().join("global.toml")), + None, + ); + let err = result.expect_err("invalid regex should fail to load"); + assert!(matches!(err, ConfigError::Parse { .. })); + assert!(err.to_string().contains("invalid regex pattern '[invalid'")); + } + #[test] fn test_reload_keeps_config_on_parse_error() -> Result<(), ConfigError> { let temp_dir = TempDir::new().unwrap(); diff --git a/crates/codebook-config/src/settings.rs b/crates/codebook-config/src/settings.rs index 47ed5be..71ae642 100644 --- a/crates/codebook-config/src/settings.rs +++ b/crates/codebook-config/src/settings.rs @@ -1,19 +1,109 @@ use glob::Pattern; use log::warn; +use regex::{Regex, RegexBuilder}; use serde::{Deserialize, Serialize}; use std::path::Path; +/// Compile a user-supplied ignore pattern. Multiline mode is enabled so `^` +/// and `$` match line boundaries. +fn compile_pattern(pattern: &str) -> Result { + RegexBuilder::new(pattern).multi_line(true).build() +} + +/// Deserialize regex strings, compiled at parse time: an invalid pattern is +/// a config parse error, and the spell-check hot path never compiles or +/// validates patterns. +fn regex_vec<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + let v = Vec::::deserialize(deserializer)?; + v.into_iter() + .map(|p| { + compile_pattern(&p).map_err(|e| { + serde::de::Error::custom(format!("invalid regex pattern '{p}': {e}")) + }) + }) + .collect() +} + +fn regex_opt_vec<'de, D>(deserializer: D) -> Result>, D::Error> +where + D: serde::Deserializer<'de>, +{ + let v = Option::>::deserialize(deserializer)?; + v.map(|v| { + v.into_iter() + .map(|p| { + compile_pattern(&p).map_err(|e| { + serde::de::Error::custom(format!("invalid regex pattern '{p}': {e}")) + }) + }) + .collect() + }) + .transpose() +} + +/// Serialize compiled regexes back to their original pattern strings. +fn regexes_as_strings(patterns: &[Regex], serializer: S) -> Result +where + S: serde::Serializer, +{ + serializer.collect_seq(patterns.iter().map(Regex::as_str)) +} + +fn opt_regexes_as_strings( + patterns: &Option>, + serializer: S, +) -> Result +where + S: serde::Serializer, +{ + match patterns { + Some(patterns) => regexes_as_strings(patterns, serializer), + None => serializer.serialize_none(), + } +} + +/// Deserialize glob strings, compiled at parse time: an invalid glob is a +/// config parse error, and path matching never re-compiles patterns. +fn glob_vec<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + let v = Vec::::deserialize(deserializer)?; + v.into_iter() + .map(|p| { + Pattern::new(&p) + .map_err(|e| serde::de::Error::custom(format!("invalid glob pattern '{p}': {e}"))) + }) + .collect() +} + +/// Serialize compiled globs back to their original pattern strings. +fn globs_as_strings(patterns: &[Pattern], serializer: S) -> Result +where + S: serde::Serializer, +{ + serializer.collect_seq(patterns.iter().map(Pattern::as_str)) +} + /// A single `[[overrides]]` block in the config file. /// /// Word-related fields deserialize through `lowercase_*` so lookups are /// case-insensitive; paths and regex patterns keep their original casing. -#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] +#[derive(Debug, Serialize, Deserialize, Clone)] pub struct OverrideBlock { - /// Required: glob patterns matched against file path relative to project root. - /// Defaulted rather than required so a block missing `paths` is filtered out - /// with a warning instead of failing the whole config parse. - #[serde(default)] - pub paths: Vec, + /// Required: glob patterns matched against file path relative to project + /// root, compiled at parse time. Defaulted rather than required so a + /// block missing `paths` is filtered out with a warning instead of + /// failing the whole config parse (an invalid glob does fail the parse). + #[serde( + default, + deserialize_with = "glob_vec", + serialize_with = "globs_as_strings" + )] + pub paths: Vec, // --- Replace fields (replace the base list entirely) --- #[serde( @@ -37,8 +127,13 @@ pub struct OverrideBlock { )] pub flag_words: Option>, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub ignore_patterns: Option>, + #[serde( + default, + deserialize_with = "regex_opt_vec", + serialize_with = "opt_regexes_as_strings", + skip_serializing_if = "Option::is_none" + )] + pub ignore_patterns: Option>, // --- Append fields (append to the resolved list) --- #[serde( @@ -62,8 +157,13 @@ pub struct OverrideBlock { )] pub extra_flag_words: Option>, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub extra_ignore_patterns: Option>, + #[serde( + default, + deserialize_with = "regex_opt_vec", + serialize_with = "opt_regexes_as_strings", + skip_serializing_if = "Option::is_none" + )] + pub extra_ignore_patterns: Option>, } fn lowercase_vec<'de, D>(deserializer: D) -> Result, D::Error> @@ -92,7 +192,7 @@ where .into_iter() .filter(|o| { if !o.is_valid() { - warn!("Skipping invalid override block (empty or invalid paths)"); + warn!("Skipping invalid override block (no paths)"); return false; } if !o.has_effect() { @@ -105,22 +205,16 @@ where } impl OverrideBlock { - /// Returns true if this override block is valid (has non-empty paths with at least one valid glob). + /// Returns true if this override block is valid (has at least one path + /// glob — globs are always valid here, having been compiled at parse time). pub fn is_valid(&self) -> bool { - if self.paths.is_empty() { - return false; - } - self.paths.iter().any(|p| Pattern::new(p).is_ok()) + !self.paths.is_empty() } /// Check if this override applies to the given relative file path. pub fn matches_path(&self, relative_path: &Path) -> bool { let path_str = normalize_separators(&relative_path.to_string_lossy()); - self.paths.iter().any(|pattern| { - Pattern::new(pattern) - .map(|p| p.matches(&path_str)) - .unwrap_or(false) - }) + self.paths.iter().any(|pattern| pattern.matches(&path_str)) } /// Returns true if any field besides `paths` is set (the override has an effect). @@ -136,7 +230,7 @@ impl OverrideBlock { } } -#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] +#[derive(Debug, Serialize, Deserialize, Clone)] pub struct ConfigSettings { /// List of dictionaries to use for spell checking. /// Dictionary IDs are language codes (e.g. "en_US") — normalized to @@ -165,9 +259,14 @@ pub struct ConfigSettings { #[serde(default, skip_serializing_if = "Vec::is_empty")] pub ignore_paths: Vec, - /// Regex patterns for text to ignore - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub ignore_patterns: Vec, + /// Regex patterns for text to ignore, compiled at parse time + #[serde( + default, + deserialize_with = "regex_vec", + serialize_with = "regexes_as_strings", + skip_serializing_if = "Vec::is_empty" + )] + pub ignore_patterns: Vec, /// Whether to use global configuration #[serde( @@ -267,10 +366,11 @@ impl ConfigSettings { sort_and_dedup_unicase(&mut self.flag_words); sort_and_dedup(&mut self.include_paths); sort_and_dedup(&mut self.ignore_paths); - sort_and_dedup(&mut self.ignore_patterns); sort_and_dedup(&mut self.include_tags); sort_and_dedup(&mut self.exclude_tags); - // Note: overrides are NOT sorted — order matters for resolution + // Note: overrides are NOT sorted — order matters for resolution. + // ignore_patterns keep their written order too: they're never added + // programmatically, and a duplicate pattern is harmless. } /// Apply a single override block to this settings (mutates in place). @@ -470,6 +570,64 @@ fn sort_and_dedup_unicase(vec: &mut Vec) { mod tests { use super::*; + fn pat(pattern: &str) -> Regex { + compile_pattern(pattern).unwrap() + } + + fn glob(pattern: &str) -> Pattern { + Pattern::new(pattern).unwrap() + } + + /// Regexes don't implement PartialEq; compare them by pattern string. + fn pattern_strings(patterns: &[Regex]) -> Vec<&str> { + patterns.iter().map(Regex::as_str).collect() + } + + #[test] + fn test_ignore_pattern_is_multiline() { + let pattern = pat(r"^vim\..*"); + let text = "let x = 1\nvim.opt.showmode = false\nlet y = 2"; + + let m = pattern.find(text).unwrap(); + assert_eq!(m.as_str(), "vim.opt.showmode = false"); + } + + #[test] + fn test_invalid_ignore_pattern_fails_deserialization() { + let toml_str = r#" + ignore_patterns = ["valid.*", "[invalid"] + "#; + + let err = toml::from_str::(toml_str).unwrap_err(); + assert!(err.to_string().contains("invalid regex pattern '[invalid'")); + } + + #[test] + fn test_invalid_override_glob_fails_deserialization() { + let toml_str = r#" + [[overrides]] + paths = ["docs/[invalid"] + extra_words = ["test"] + "#; + + let err = toml::from_str::(toml_str).unwrap_err(); + assert!( + err.to_string() + .contains("invalid glob pattern 'docs/[invalid'") + ); + } + + #[test] + fn test_ignore_pattern_serializes_as_string() { + let settings = ConfigSettings { + ignore_patterns: vec![pat(r"^```.*$")], + ..Default::default() + }; + + let toml_str = toml::to_string(&settings).unwrap(); + assert!(toml_str.contains(r#"ignore_patterns = ["^```.*$"]"#)); + } + #[test] fn test_default() { let config = ConfigSettings::default(); @@ -478,7 +636,7 @@ mod tests { assert_eq!(config.flag_words, Vec::::new()); assert_eq!(config.include_paths, Vec::::new()); assert_eq!(config.ignore_paths, Vec::::new()); - assert_eq!(config.ignore_patterns, Vec::::new()); + assert!(config.ignore_patterns.is_empty()); assert!(config.use_global); assert_eq!(config.min_word_length, None); assert_eq!(config.min_word_length(), 3); @@ -505,10 +663,7 @@ mod tests { assert_eq!(config.include_paths, vec!["src/**/*.rs", "lib/"]); assert_eq!(config.ignore_paths, vec!["**/*.md", "target/"]); - // Don't test the exact order, just check that both elements are present - assert_eq!(config.ignore_patterns.len(), 2); - assert!(config.ignore_patterns.contains(&"^```.*$".to_string())); - assert!(config.ignore_patterns.contains(&"^//.*$".to_string())); + assert_eq!(pattern_strings(&config.ignore_patterns), ["^```.*$", "^//.*$"]); assert!(!config.use_global); } @@ -570,7 +725,7 @@ mod tests { flag_words: vec!["todo".to_string()], include_paths: vec!["src/".to_string()], ignore_paths: vec!["**/*.md".to_string()], - ignore_patterns: vec!["^```.*$".to_string()], + ignore_patterns: vec![pat("^```.*$")], use_global: true, min_word_length: Some(3), ..Default::default() @@ -582,7 +737,7 @@ mod tests { flag_words: vec!["fixme".to_string()], include_paths: vec!["lib/".to_string(), "src/".to_string()], ignore_paths: vec!["target/".to_string()], - ignore_patterns: vec!["^//.*$".to_string()], + ignore_patterns: vec![pat("^//.*$")], use_global: false, min_word_length: Some(2), ..Default::default() @@ -597,10 +752,11 @@ mod tests { assert_eq!(base.include_paths, vec!["lib/", "src/"]); assert_eq!(base.ignore_paths, vec!["**/*.md", "target/"]); - // Don't test the exact order, just check that both elements are present - assert_eq!(base.ignore_patterns.len(), 2); - assert!(base.ignore_patterns.contains(&"^```.*$".to_string())); - assert!(base.ignore_patterns.contains(&"^//.*$".to_string())); + // Patterns keep their order: base's first, then other's + assert_eq!( + pattern_strings(&base.ignore_patterns), + ["^```.*$", "^//.*$"] + ); // use_global from the base should be preserved assert!(base.use_global); @@ -666,11 +822,7 @@ mod tests { "**/*.md".to_string(), "target/".to_string(), ], - ignore_patterns: vec![ - "^//.*$".to_string(), - "^```.*$".to_string(), - "^//.*$".to_string(), - ], + ignore_patterns: vec![pat("^//.*$"), pat("^```.*$"), pat("^//.*$")], use_global: true, min_word_length: Some(3), ..Default::default() @@ -683,10 +835,11 @@ mod tests { assert_eq!(config.flag_words, vec!["fixme", "todo"]); assert_eq!(config.ignore_paths, vec!["**/*.md", "target/"]); - // Don't test the exact order, just check that both elements are present and duplicates removed - assert_eq!(config.ignore_patterns.len(), 2); - assert!(config.ignore_patterns.contains(&"^```.*$".to_string())); - assert!(config.ignore_patterns.contains(&"^//.*$".to_string())); + // Patterns are not sorted or deduplicated: they keep their written order + assert_eq!( + pattern_strings(&config.ignore_patterns), + ["^//.*$", "^```.*$", "^//.*$"] + ); } #[test] @@ -704,7 +857,12 @@ mod tests { let toml_str = ""; let config: ConfigSettings = toml::from_str(toml_str).unwrap(); - assert_eq!(config, ConfigSettings::default()); + // ConfigSettings has no PartialEq (Regex doesn't compare); an empty + // config and the default both serialize to nothing. + assert_eq!( + toml::to_string(&config).unwrap(), + toml::to_string(&ConfigSettings::default()).unwrap() + ); } #[test] @@ -866,7 +1024,7 @@ mod tests { assert_eq!(config.words, vec!["CodeBook"]); assert_eq!(config.flag_words, Vec::::new()); assert_eq!(config.ignore_paths, Vec::::new()); - assert_eq!(config.ignore_patterns, Vec::::new()); + assert!(config.ignore_patterns.is_empty()); assert!(config.use_global); } @@ -886,11 +1044,11 @@ mod tests { let config: ConfigSettings = toml::from_str(toml_str).unwrap(); assert_eq!(config.overrides.len(), 1); let ovr = &config.overrides[0]; - assert_eq!(ovr.paths, vec!["**/*.md"]); + assert_eq!(ovr.paths, vec![glob("**/*.md")]); assert_eq!(ovr.extra_words, Some(vec!["markdown".to_string()])); // lowercased assert_eq!(ovr.dictionaries, Some(vec!["en_gb".to_string()])); // lowercased assert_eq!(ovr.words, None); - assert_eq!(ovr.ignore_patterns, None); + assert!(ovr.ignore_patterns.is_none()); } #[test] @@ -919,7 +1077,7 @@ mod tests { #[test] fn test_override_matches_path() { let ovr = OverrideBlock { - paths: vec!["**/*.md".to_string(), "docs/**/*".to_string()], + paths: vec![glob("**/*.md"), glob("docs/**/*")], extra_words: Some(vec!["test".to_string()]), ..OverrideBlock::default_for_test() }; @@ -934,7 +1092,7 @@ mod tests { #[test] fn test_override_matches_backslash_path_on_windows() { let ovr = OverrideBlock { - paths: vec!["docs/**/*".to_string()], + paths: vec![glob("docs/**/*")], extra_words: Some(vec!["test".to_string()]), ..OverrideBlock::default_for_test() }; @@ -985,7 +1143,7 @@ mod tests { }; let over = OverrideBlock { - paths: vec!["**/*.md".to_string()], + paths: vec![glob("**/*.md")], words: Some(vec!["gamma".to_string()]), ..OverrideBlock::default_for_test() }; @@ -1002,7 +1160,7 @@ mod tests { }; let ovr = OverrideBlock { - paths: vec!["**/*.md".to_string()], + paths: vec![glob("**/*.md")], extra_words: Some(vec!["gamma".to_string()]), ..OverrideBlock::default_for_test() }; @@ -1019,7 +1177,7 @@ mod tests { }; let ovr = OverrideBlock { - paths: vec!["**/*.md".to_string()], + paths: vec![glob("**/*.md")], words: Some(vec!["gamma".to_string()]), extra_words: Some(vec!["delta".to_string()]), ..OverrideBlock::default_for_test() @@ -1038,7 +1196,7 @@ mod tests { }; let over = OverrideBlock { - paths: vec!["**/*.md".to_string()], + paths: vec![glob("**/*.md")], extra_flag_words: Some(vec!["hack".to_string()]), ..OverrideBlock::default_for_test() }; @@ -1056,7 +1214,7 @@ mod tests { let settings = ConfigSettings { words: vec!["base".to_string()], overrides: vec![OverrideBlock { - paths: vec!["**/*.md".to_string()], + paths: vec![glob("**/*.md")], extra_words: Some(vec!["markdown".to_string()]), ..OverrideBlock::default_for_test() }], @@ -1073,7 +1231,7 @@ mod tests { let settings = ConfigSettings { words: vec!["base".to_string()], overrides: vec![OverrideBlock { - paths: vec!["**/*.md".to_string()], + paths: vec![glob("**/*.md")], extra_words: Some(vec!["markdown".to_string()]), ..OverrideBlock::default_for_test() }], @@ -1091,12 +1249,12 @@ mod tests { words: vec!["base".to_string()], overrides: vec![ OverrideBlock { - paths: vec!["**/*.md".to_string()], + paths: vec![glob("**/*.md")], extra_words: Some(vec!["markdown".to_string()]), ..OverrideBlock::default_for_test() }, OverrideBlock { - paths: vec!["docs/**/*".to_string()], + paths: vec![glob("docs/**/*")], extra_words: Some(vec!["documentation".to_string()]), ..OverrideBlock::default_for_test() }, @@ -1113,7 +1271,7 @@ mod tests { let settings = ConfigSettings { dictionaries: vec!["en_us".to_string()], overrides: vec![OverrideBlock { - paths: vec!["docs/de/**/*".to_string()], + paths: vec![glob("docs/de/**/*")], dictionaries: Some(vec!["de".to_string()]), extra_words: Some(vec!["codebook".to_string()]), ..OverrideBlock::default_for_test() @@ -1131,7 +1289,7 @@ mod tests { let mut global = ConfigSettings { words: vec!["global".to_string()], overrides: vec![OverrideBlock { - paths: vec!["**/*.md".to_string()], + paths: vec![glob("**/*.md")], extra_words: Some(vec!["from_global".to_string()]), ..OverrideBlock::default_for_test() }], @@ -1141,7 +1299,7 @@ mod tests { let project = ConfigSettings { words: vec!["project".to_string()], overrides: vec![OverrideBlock { - paths: vec!["**/*.md".to_string()], + paths: vec![glob("**/*.md")], extra_words: Some(vec!["from_project".to_string()]), ..OverrideBlock::default_for_test() }], @@ -1167,7 +1325,7 @@ mod tests { let config = ConfigSettings { words: vec!["base".to_string()], overrides: vec![OverrideBlock { - paths: vec!["**/*.md".to_string()], + paths: vec![glob("**/*.md")], extra_words: Some(vec!["markdown".to_string()]), ..OverrideBlock::default_for_test() }], @@ -1177,7 +1335,9 @@ mod tests { let serialized = toml::to_string_pretty(&config).unwrap(); let deserialized: ConfigSettings = toml::from_str(&serialized).unwrap(); - assert_eq!(config, deserialized); + // ConfigSettings has no PartialEq (Regex doesn't compare); a stable + // re-serialization proves the round-trip lost nothing. + assert_eq!(serialized, toml::to_string_pretty(&deserialized).unwrap()); } #[test] diff --git a/crates/codebook/src/lib.rs b/crates/codebook/src/lib.rs index d1977dd..6018d30 100644 --- a/crates/codebook/src/lib.rs +++ b/crates/codebook/src/lib.rs @@ -11,7 +11,6 @@ use std::collections::HashSet; use std::path::Path; use std::sync::Arc; -use codebook_config::helpers::build_ignore_regexes; use codebook_config::{CodebookConfig, ConfigSettings}; use dictionaries::{dictionary, manager::DictionaryManager}; use dictionary::Dictionary; @@ -70,7 +69,7 @@ impl Codebook { // Combine default and user skip patterns let mut all_patterns = get_default_skip_patterns().clone(); if let Some(ref settings) = resolved { - all_patterns.extend(build_ignore_regexes(&settings.ignore_patterns)); + all_patterns.extend(settings.ignore_patterns.iter().cloned()); } else { all_patterns.extend(self.config.get_ignore_patterns()); } From 02ead551d5ab437ba60a3d35cb089247486f6ea5 Mon Sep 17 00:00:00 2001 From: Bo Lopker Date: Mon, 6 Jul 2026 12:40:20 -0700 Subject: [PATCH 3/3] Cover regex fields in the overrides round-trip test --- crates/codebook-config/src/settings.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/crates/codebook-config/src/settings.rs b/crates/codebook-config/src/settings.rs index 71ae642..ccdbbb7 100644 --- a/crates/codebook-config/src/settings.rs +++ b/crates/codebook-config/src/settings.rs @@ -1324,9 +1324,12 @@ mod tests { fn test_serialization_with_overrides() { let config = ConfigSettings { words: vec!["base".to_string()], + ignore_patterns: vec![pat(r"^```.*$")], overrides: vec![OverrideBlock { paths: vec![glob("**/*.md")], extra_words: Some(vec!["markdown".to_string()]), + ignore_patterns: Some(vec![pat(r"\bhttps?://\S+")]), + extra_ignore_patterns: Some(vec![pat(r"^\s*//.*")]), ..OverrideBlock::default_for_test() }], ..Default::default() @@ -1338,6 +1341,17 @@ mod tests { // ConfigSettings has no PartialEq (Regex doesn't compare); a stable // re-serialization proves the round-trip lost nothing. assert_eq!(serialized, toml::to_string_pretty(&deserialized).unwrap()); + + let ovr = &deserialized.overrides[0]; + assert_eq!(pattern_strings(&deserialized.ignore_patterns), [r"^```.*$"]); + assert_eq!( + pattern_strings(ovr.ignore_patterns.as_deref().unwrap()), + [r"\bhttps?://\S+"] + ); + assert_eq!( + pattern_strings(ovr.extra_ignore_patterns.as_deref().unwrap()), + [r"^\s*//.*"] + ); } #[test]