diff --git a/harper-core/src/linting/expr_linter.rs b/harper-core/src/linting/expr_linter.rs index 675231e3f8..d0fb0eeba4 100644 --- a/harper-core/src/linting/expr_linter.rs +++ b/harper-core/src/linting/expr_linter.rs @@ -113,6 +113,10 @@ where fn description(&self) -> &str { self.description() } + + fn merged_linter_child_names(&self) -> Vec<&'static str> { + vec![] + } } pub fn run_on_chunk<'a>( diff --git a/harper-core/src/linting/lint_group/mod.rs b/harper-core/src/linting/lint_group/mod.rs index 7f2deaf085..9e07e67dc1 100644 --- a/harper-core/src/linting/lint_group/mod.rs +++ b/harper-core/src/linting/lint_group/mod.rs @@ -332,21 +332,61 @@ impl LintGroup { || self.sentence_expr_linters.contains_key(name.as_ref()) } + /// Check for clashes between a new linter and existing linters + /// Returns detailed clash information with counts for debugging + fn check_clashes(&self, name: &str, child_names: &[&str]) -> Option> { + use hashbrown::HashMap; + let mut clash_counts = HashMap::new(); + + // Check direct name clash with existing `Linter`s or `ExprLinter`s + if self.contains_key(name) { + *clash_counts.entry(name.to_string()).or_insert(0) += 1; + } + + // Check if new linter's name clashes with child names of existing linters + for existing_linter in self.linters.values() { + if existing_linter.merged_linter_child_names().contains(&name) { + *clash_counts.entry(name.to_string()).or_insert(0) += 1; + } + } + + // Check if child names clash with existing linters + for &child_name in child_names { + if self.contains_key(child_name) { + *clash_counts.entry(child_name.to_string()).or_insert(0) += 1; + } + } + + if clash_counts.is_empty() { + None + } else { + Some( + clash_counts + .into_iter() + .map(|(name, count)| format!("{name} (used {} times)", count + 1)) + .collect(), + ) + } + } + /// Add a [`Linter`] to the group, returning whether the operation was successful. /// If it returns `false`, it is because a linter with that key already existed in the group. pub fn add(&mut self, name: impl AsRef, linter: impl Linter + 'static) -> bool { - if self.contains_key(&name) { + let name_str = name.as_ref(); + let child_names = linter.merged_linter_child_names(); + + if let Some(clashes) = self.check_clashes(name_str, &child_names) { if self.clashing_linter_names.is_none() { - self.clashing_linter_names = Some(vec![name.as_ref().to_string()]); + self.clashing_linter_names = Some(clashes); } else if let Some(clashing_names) = &mut self.clashing_linter_names { - clashing_names.push(name.as_ref().to_string()); + clashing_names.extend(clashes); } - false - } else { - self.linters - .insert(name.as_ref().to_string(), Box::new(linter)); - true + return false; } + + // No clashes found, add the linter + self.linters.insert(name_str.to_string(), Box::new(linter)); + true } /// Add a chunk-based [`ExprLinter`] to the group, returning whether the operation was successful. @@ -359,18 +399,22 @@ impl LintGroup { name: impl AsRef, linter: impl ExprLinter + 'static, ) -> bool { - if self.contains_key(&name) { + let name_str = name.as_ref(); + let child_names = linter.merged_linter_child_names(); + + if let Some(clashes) = self.check_clashes(name_str, &child_names) { if self.clashing_linter_names.is_none() { - self.clashing_linter_names = Some(vec![name.as_ref().to_string()]); + self.clashing_linter_names = Some(clashes); } else if let Some(clashing_names) = &mut self.clashing_linter_names { - clashing_names.push(name.as_ref().to_string()); + clashing_names.extend(clashes); } - false - } else { - self.chunk_expr_linters - .insert(name.as_ref().to_string(), Box::new(linter) as _); - true + return false; } + + // No clashes found, add the linter + self.chunk_expr_linters + .insert(name_str.to_string(), Box::new(linter) as _); + true } /// Add a sentence-based [`ExprLinter`] to the group, returning whether the operation was successful. @@ -1180,8 +1224,9 @@ mod tests { if let Some(names) = &group.clashing_linter_names { if !names.is_empty() { panic!( - "⚠️ Found {} clashing linter names: {}", + "⚠️ Found {} linter name clash{}: {}", names.len(), + if names.len() == 1 { "" } else { "es" }, names.join(", ") ); } diff --git a/harper-core/src/linting/lint_group/structured_config/mod.rs b/harper-core/src/linting/lint_group/structured_config/mod.rs index 5ebfbf7562..f3acf26c49 100644 --- a/harper-core/src/linting/lint_group/structured_config/mod.rs +++ b/harper-core/src/linting/lint_group/structured_config/mod.rs @@ -331,7 +331,7 @@ mod tests { assert!( missing_from_default_config.is_empty() && extra_in_default_config.is_empty(), - "default_config.json drifted from the registered rule set\nmissing from default_config.json: {:?}\nextra in default_config.json: {:?}\nDid you forget to add the missing rule(s) to default_config.json?", + "⚠️ default_config.json drifted from the registered rule set\nmissing from default_config.json: {:?}\nextra in default_config.json: {:?}\nDid you forget to add the missing rule(s) to default_config.json?\nOr are there any clashing linter names?", missing_from_default_config, extra_in_default_config, ); diff --git a/harper-core/src/linting/merge_linters.rs b/harper-core/src/linting/merge_linters.rs index c34a47e797..6517a246de 100644 --- a/harper-core/src/linting/merge_linters.rs +++ b/harper-core/src/linting/merge_linters.rs @@ -34,6 +34,17 @@ macro_rules! merge_linters { fn description(&self) -> &'static str { $desc } + + fn merged_linter_child_names(&self) -> Vec<&'static str> { + let mut names = Vec::new(); + + $( + names.push(stringify!($linter)); + names.extend(self.[< $linter:snake >].merged_linter_child_names()); + )* + + names + } } } } diff --git a/harper-core/src/linting/mod.rs b/harper-core/src/linting/mod.rs index 229fb2afb5..343e2ce0ef 100644 --- a/harper-core/src/linting/mod.rs +++ b/harper-core/src/linting/mod.rs @@ -306,6 +306,14 @@ pub trait Linter: LSend { /// A user-facing description of what kinds of grammatical errors this rule looks for. /// It is usually shown in settings menus. fn description(&self) -> &str; + + /// Get the names of all component linters. + /// For merged linters, this includes children, grandchildren, etc. + /// Non-merged linters should return an empty array. + /// This is used by LintGroup to detect name conflicts. + fn merged_linter_child_names(&self) -> Vec<&'static str> { + vec![] + } } /// A blanket-implemented trait that renders the Markdown description field of a linter to HTML.