diff --git a/python/tach/extension.pyi b/python/tach/extension.pyi index b9ef64eb..60d3e00b 100644 --- a/python/tach/extension.pyi +++ b/python/tach/extension.pyi @@ -139,6 +139,7 @@ RuleSetting = Literal["error", "warn", "off"] class RulesConfig: unused_ignore_directives: RuleSetting + require_ignore_directive_reasons: RuleSetting class ProjectConfig: modules: list[ModuleConfig] diff --git a/src/check_int.rs b/src/check_int.rs index 2eb492cf..a86333b8 100644 --- a/src/check_int.rs +++ b/src/check_int.rs @@ -81,6 +81,9 @@ pub enum ImportCheckError { #[error("Import '{import_mod_path}' is unnecessarily ignored by a directive.")] UnusedIgnoreDirective { import_mod_path: String }, + #[error("Import '{import_mod_path}' is ignored without providing a reason.")] + MissingIgnoreDirectiveReason { import_mod_path: String }, + #[error("No checks enabled. At least one of dependencies or interfaces must be enabled.")] NoChecksEnabled(), } @@ -380,44 +383,76 @@ pub fn check( boundary_errors.push(boundary_error); } } - - if project_config.rules.unused_ignore_directives == RuleSetting::Off { + // Skip directive-related checks if both rules are off + if project_config.rules.unused_ignore_directives == RuleSetting::Off + && project_config.rules.require_ignore_directive_reasons == RuleSetting::Off + { continue; } + for directive_ignored_import in project_imports.directive_ignored_imports { - if check_import( - &directive_ignored_import.module_path, - &module_tree, - Arc::clone(&nearest_module), - project_config.root_module.clone(), - &interface_checker, - dependencies, - interfaces, - ) - .is_ok() + // Check for missing ignore directive reasons + if project_config.rules.require_ignore_directive_reasons != RuleSetting::Off + && directive_ignored_import.reason.is_empty() { - let message = format!( - "Import '{}' is unnecessarily ignored by a directive.", - directive_ignored_import.module_path - ); - match project_config.rules.unused_ignore_directives { - RuleSetting::Error => { + let error = BoundaryError { + file_path: file_path.clone(), + line_number: directive_ignored_import.import.line_no, + import_mod_path: directive_ignored_import.import.module_path.to_string(), + error_info: ImportCheckError::MissingIgnoreDirectiveReason { + import_mod_path: directive_ignored_import + .import + .module_path + .to_string(), + }, + }; + if project_config.rules.require_ignore_directive_reasons == RuleSetting::Error { + boundary_errors.push(error); + } else { + warnings.push(format!( + "Import '{}' is ignored without providing a reason", + directive_ignored_import.import.module_path + )); + } + } + + // Check for unnecessary ignore directives + if project_config.rules.unused_ignore_directives != RuleSetting::Off { + let is_unnecessary = check_import( + &directive_ignored_import.import.module_path, + &module_tree, + Arc::clone(&nearest_module), + project_config.root_module.clone(), + &interface_checker, + dependencies, + interfaces, + ) + .is_ok(); + + if is_unnecessary { + let message = format!( + "Import '{}' is unnecessarily ignored by a directive.", + directive_ignored_import.import.module_path + ); + + if project_config.rules.unused_ignore_directives == RuleSetting::Error { boundary_errors.push(BoundaryError { file_path: file_path.clone(), - line_number: directive_ignored_import.line_no, - import_mod_path: directive_ignored_import.module_path.to_string(), + line_number: directive_ignored_import.import.line_no, + import_mod_path: directive_ignored_import + .import + .module_path + .to_string(), error_info: ImportCheckError::UnusedIgnoreDirective { import_mod_path: directive_ignored_import + .import .module_path .to_string(), }, }); - } - RuleSetting::Warn => { + } else { warnings.push(message); } - // Should never be reached - RuleSetting::Off => {} } } } diff --git a/src/core/config.rs b/src/core/config.rs index b01093a5..ade1f2eb 100644 --- a/src/core/config.rs +++ b/src/core/config.rs @@ -291,12 +291,18 @@ pub struct RulesConfig { skip_serializing_if = "RuleSetting::is_warn" )] pub unused_ignore_directives: RuleSetting, + #[serde( + default = "RuleSetting::off", + skip_serializing_if = "RuleSetting::is_off" + )] + pub require_ignore_directive_reasons: RuleSetting, } impl Default for RulesConfig { fn default() -> Self { Self { unused_ignore_directives: RuleSetting::warn(), + require_ignore_directive_reasons: RuleSetting::off(), } } } diff --git a/src/imports.rs b/src/imports.rs index 63eb6a7a..fcbbfeb6 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -49,10 +49,16 @@ impl NormalizedImport { } } +#[derive(Debug)] +pub struct DirectiveIgnoredImport { + pub import: NormalizedImport, + pub reason: String, +} + #[derive(Debug, Default)] pub struct NormalizedImports { pub imports: Vec, - pub directive_ignored_imports: Vec, + pub directive_ignored_imports: Vec, } impl NormalizedImports { @@ -69,10 +75,21 @@ impl IntoPy for NormalizedImport { } } -pub type IgnoreDirectives = HashMap>; +#[derive(Debug)] +pub struct ProjectImports { + pub imports: Vec, + pub directive_ignored_imports: Vec, +} + +pub struct IgnoreDirective { + modules: Vec, + reason: String, +} + +pub type IgnoreDirectives = HashMap; static TACH_IGNORE_REGEX: Lazy = - Lazy::new(|| Regex::new(r"# *tach-ignore(?:\([^)]*\))?((?:\s+[\w.]+)*)\s*$").unwrap()); + Lazy::new(|| Regex::new(r"# *tach-ignore(?:\(([^)]*)\))?((?:\s+[\w.]+)*)\s*$").unwrap()); fn get_ignore_directives(file_content: &str) -> IgnoreDirectives { let mut ignores: IgnoreDirectives = HashMap::new(); @@ -80,7 +97,10 @@ fn get_ignore_directives(file_content: &str) -> IgnoreDirectives { for (lineno, line) in file_content.lines().enumerate() { let normal_lineno = lineno + 1; if let Some(captures) = TACH_IGNORE_REGEX.captures(line) { - let ignored_modules = captures.get(1).map_or("", |m| m.as_str()); + let reason = captures + .get(1) + .map_or("".to_string(), |m| m.as_str().to_string()); + let ignored_modules = captures.get(2).map_or("", |m| m.as_str()); let modules: Vec = if ignored_modules.is_empty() { Vec::new() } else { @@ -90,10 +110,12 @@ fn get_ignore_directives(file_content: &str) -> IgnoreDirectives { .collect() }; + let directive = IgnoreDirective { modules, reason }; + if line.starts_with('#') { - ignores.insert(normal_lineno + 1, modules); + ignores.insert(normal_lineno + 1, directive); } else { - ignores.insert(normal_lineno, modules); + ignores.insert(normal_lineno, directive); } } } @@ -134,21 +156,27 @@ impl<'a> ImportVisitor<'a> { .locator .compute_line_index(import_statement.range.start()) .get(); - let ignored_modules: Option<&Vec> = self.ignore_directives.get(&line_no); + let ignored_directive: Option<&IgnoreDirective> = self.ignore_directives.get(&line_no); - if let Some(ignored) = ignored_modules { - if ignored.is_empty() { + if let Some(ignored) = ignored_directive { + if ignored.modules.is_empty() { // Blanket ignore of current import - add all to directive_ignored_imports normalized_imports.directive_ignored_imports.extend( - import_statement.names.iter().map(|alias| NormalizedImport { - module_path: alias.name.to_string(), - line_no: self - .locator - .compute_line_index(alias.range.start()) - .get() - .try_into() - .unwrap(), - }), + import_statement + .names + .iter() + .map(|alias| DirectiveIgnoredImport { + import: NormalizedImport { + module_path: alias.name.to_string(), + line_no: self + .locator + .compute_line_index(alias.range.start()) + .get() + .try_into() + .unwrap(), + }, + reason: ignored.reason.clone(), + }), ); return normalized_imports; } @@ -165,9 +193,14 @@ impl<'a> ImportVisitor<'a> { .unwrap(), }; - if let Some(ignored) = ignored_modules { - if ignored.contains(alias.name.as_ref()) { - normalized_imports.directive_ignored_imports.push(import); + if let Some(ignored) = ignored_directive { + if ignored.modules.contains(alias.name.as_ref()) { + normalized_imports + .directive_ignored_imports + .push(DirectiveIgnoredImport { + import, + reason: ignored.reason.clone(), + }); } else { normalized_imports.imports.push(import); } @@ -235,23 +268,26 @@ impl<'a> ImportVisitor<'a> { .locator .compute_line_index(import_statement.range.start()) .get(); - let ignored_modules: Option<&Vec> = self.ignore_directives.get(&line_no); + let ignored_directive: Option<&IgnoreDirective> = self.ignore_directives.get(&line_no); - if let Some(ignored) = ignored_modules { - if ignored.is_empty() { + if let Some(ignored) = ignored_directive { + if ignored.modules.is_empty() { // Blanket ignore - add all imports to directive_ignored_imports for name in &import_statement.names { let global_mod_path = format!("{}.{}", base_mod_path, name.name.as_str()); normalized_imports .directive_ignored_imports - .push(NormalizedImport { - module_path: global_mod_path, - line_no: self - .locator - .compute_line_index(name.range.start()) - .get() - .try_into() - .unwrap(), + .push(DirectiveIgnoredImport { + import: NormalizedImport { + module_path: global_mod_path, + line_no: self + .locator + .compute_line_index(name.range.start()) + .get() + .try_into() + .unwrap(), + }, + reason: ignored.reason.clone(), }); } return normalized_imports; @@ -270,15 +306,20 @@ impl<'a> ImportVisitor<'a> { .unwrap(), }; - if let Some(ignored) = ignored_modules { - if ignored.contains( + if let Some(ignored) = ignored_directive { + if ignored.modules.contains( &name .asname .as_deref() .unwrap_or(name.name.as_ref()) .to_string(), ) { - normalized_imports.directive_ignored_imports.push(import); + normalized_imports + .directive_ignored_imports + .push(DirectiveIgnoredImport { + import, + reason: ignored.reason.clone(), + }); } else { normalized_imports.imports.push(import); } @@ -430,11 +471,6 @@ pub fn get_normalized_imports( } } -pub struct ProjectImports { - pub imports: Vec, - pub directive_ignored_imports: Vec, -} - pub fn get_project_imports( source_roots: &[PathBuf], file_path: &PathBuf, @@ -461,10 +497,10 @@ pub fn get_project_imports( directive_ignored_imports: normalized_imports .directive_ignored_imports .into_iter() - .filter_map(|normalized_import| { - is_project_import(source_roots, &normalized_import.module_path) + .filter_map(|directive_ignored_import| { + is_project_import(source_roots, &directive_ignored_import.import.module_path) .map_or(None, |is_project_import| { - is_project_import.then_some(normalized_import) + is_project_import.then_some(directive_ignored_import) }) }) .collect(), diff --git a/tach.toml b/tach.toml index 7f662085..34b416c7 100644 --- a/tach.toml +++ b/tach.toml @@ -333,3 +333,4 @@ exclude = [ [rules] unused_ignore_directives = "error" +require_ignore_directive_reasons = "error"