diff --git a/Cargo.lock b/Cargo.lock index 6399cbd0..599b2f6f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -328,6 +328,20 @@ dependencies = [ "syn", ] +[[package]] +name = "dashmap" +version = "6.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5041cc499144891f3790297212f32a74fb938e5136a14943f338ef9e0ae276cf" +dependencies = [ + "cfg-if", + "crossbeam-utils", + "hashbrown 0.14.5", + "lock_api", + "once_cell", + "parking_lot_core 0.9.10", +] + [[package]] name = "directories" version = "5.0.1" @@ -2153,6 +2167,7 @@ dependencies = [ "console", "crossbeam-channel", "ctrlc", + "dashmap", "glob", "globset", "itertools 0.14.0", diff --git a/Cargo.toml b/Cargo.toml index 4e20572c..60e208bd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,6 +34,7 @@ parking_lot = "0.12.3" itertools = "0.14.0" toml_edit = "0.22.23" console = "0.15.10" +dashmap = { version = "6.1.0", features = ["inline"] } [features] extension-module = ["pyo3/extension-module"] diff --git a/python/tach/extension.pyi b/python/tach/extension.pyi index 0c896c7b..70824a53 100644 --- a/python/tach/extension.pyi +++ b/python/tach/extension.pyi @@ -12,12 +12,6 @@ def get_external_imports( file_path: str, ignore_type_checking_imports: bool, ) -> list[tuple[str, int]]: ... -def get_normalized_imports( - source_roots: list[str], - file_path: str, - ignore_type_checking_imports: bool, - include_string_imports: bool, -) -> list[tuple[str, int]]: ... def set_excluded_paths( project_root: str, exclude_paths: list[str], use_regex_matching: bool ) -> None: ... diff --git a/python/tests/test_check.py b/python/tests/test_check.py index 60d325ac..df806e5d 100644 --- a/python/tests/test_check.py +++ b/python/tests/test_check.py @@ -8,7 +8,7 @@ from tach.cli import tach_check from tach.errors import TachCircularDependencyError, TachVisibilityError from tach.extension import Diagnostic -from tach.icons import SUCCESS, WARNING +from tach.icons import FAIL, SUCCESS, WARNING from tach.parsing.config import parse_project_config @@ -188,3 +188,26 @@ def test_check_visibility_error_json(example_dir, capfd, mocker): result = json.loads(captured.out) assert result["error"] == "Visibility error" assert result["visibility_errors"] == [["mod1", "mod2", ["public"]]] + + +def test_distributed_config_example_dir(example_dir, capfd): + project_root = example_dir / "distributed_config" + project_config = parse_project_config(root=project_root) + assert project_config is not None + + with pytest.raises(SystemExit) as exc_info: + tach_check( + project_root=project_root, + project_config=project_config, + exclude_paths=project_config.exclude, + ) + assert exc_info.value.code == 1 + + captured = capfd.readouterr() + assert FAIL in captured.err or "FAIL" in captured.err + assert "Cannot import 'project.module_one.module_one'" in captured.err + assert ( + "Module 'project.top_level' cannot depend on 'project.module_one'" + in captured.err + ) + assert "project/top_level.py" in captured.err diff --git a/src/checks/external_dependency.rs b/src/checks/external_dependency.rs new file mode 100644 index 00000000..e7261aae --- /dev/null +++ b/src/checks/external_dependency.rs @@ -0,0 +1,83 @@ +use std::collections::{HashMap, HashSet}; + +use crate::diagnostics::{CodeDiagnostic, Diagnostic, DiagnosticDetails}; +use crate::diagnostics::{FileChecker, Result as DiagnosticResult}; +use crate::external::parsing::ProjectInfo; +use crate::processors::file_module::FileModuleExternal; +use crate::processors::imports::ExternalImportWithDistributionNames; + +pub struct ExternalDependencyChecker<'a> { + project_info: &'a ProjectInfo, + module_mappings: &'a HashMap>, + stdlib_modules: &'a HashSet, + excluded_external_modules: &'a HashSet, +} + +impl<'a> ExternalDependencyChecker<'a> { + pub fn new( + project_info: &'a ProjectInfo, + module_mappings: &'a HashMap>, + stdlib_modules: &'a HashSet, + excluded_external_modules: &'a HashSet, + ) -> Self { + Self { + project_info, + module_mappings, + stdlib_modules, + excluded_external_modules, + } + } + + fn check_import( + &'a self, + import: ExternalImportWithDistributionNames<'a>, + processed_file: &FileModuleExternal<'a>, + ) -> Option { + if import + .distribution_names + .iter() + .any(|dist_name| self.excluded_external_modules.contains(dist_name)) + || self + .stdlib_modules + .contains(&import.import.top_level_module_name().to_string()) + { + return None; + } + + let is_declared = import + .distribution_names + .iter() + .any(|dist_name| self.project_info.dependencies.contains(dist_name)); + + if !is_declared { + Some(Diagnostic::new_located_error( + processed_file.relative_file_path().to_path_buf(), + import.import.import_line_no, + DiagnosticDetails::Code(CodeDiagnostic::UndeclaredExternalDependency { + import_mod_path: import.import.top_level_module_name().to_string(), + }), + )) + } else { + None + } + } +} + +impl<'a> FileChecker<'a> for ExternalDependencyChecker<'a> { + type ProcessedFile = FileModuleExternal<'a>; + type Output = Vec; + + fn check(&'a self, processed_file: &Self::ProcessedFile) -> DiagnosticResult { + let mut diagnostics = Vec::new(); + for import in processed_file + .imports + .all_imports_with_distribution_names(self.module_mappings) + { + if let Some(diagnostic) = self.check_import(import, processed_file) { + diagnostics.push(diagnostic); + } + } + + Ok(diagnostics) + } +} diff --git a/src/checks/ignore_directive.rs b/src/checks/ignore_directive.rs new file mode 100644 index 00000000..47168416 --- /dev/null +++ b/src/checks/ignore_directive.rs @@ -0,0 +1,138 @@ +use std::path::Path; + +use crate::config::{ProjectConfig, RuleSetting}; +use crate::diagnostics::{CodeDiagnostic, Diagnostic, DiagnosticDetails}; +use crate::processors::imports::{IgnoreDirective, IgnoreDirectives}; + +pub struct IgnoreDirectivePostProcessor<'a> { + project_config: &'a ProjectConfig, +} + +impl<'a> IgnoreDirectivePostProcessor<'a> { + pub fn new(project_config: &'a ProjectConfig) -> Self { + Self { project_config } + } + + fn get_unused_ignore_directive_diagnostic( + &self, + ignore_directive: &IgnoreDirective, + relative_file_path: &Path, + ) -> Diagnostic { + Diagnostic::new_located( + (&self.project_config.rules.unused_ignore_directives) + .try_into() + .unwrap(), + DiagnosticDetails::Code(CodeDiagnostic::UnusedIgnoreDirective()), + relative_file_path.to_path_buf(), + ignore_directive.line_no, + ) + } + + fn check_unused_ignore_directive( + &self, + ignore_directive: &IgnoreDirective, + diagnostics: &Vec, + relative_file_path: &Path, + ) -> Option { + if self.project_config.rules.unused_ignore_directives == RuleSetting::Off { + return None; + } + + if !diagnostics + .iter() + .any(|diagnostic| ignore_directive.matches_diagnostic(diagnostic)) + { + Some(self.get_unused_ignore_directive_diagnostic(ignore_directive, relative_file_path)) + } else { + None + } + } + + fn check_missing_ignore_directive_reason( + &self, + ignore_directive: &IgnoreDirective, + relative_file_path: &Path, + ) -> Option { + if self.project_config.rules.require_ignore_directive_reasons == RuleSetting::Off { + return None; + } + + if ignore_directive.reason.is_empty() { + Some(Diagnostic::new_located( + (&self.project_config.rules.require_ignore_directive_reasons) + .try_into() + .unwrap(), + DiagnosticDetails::Code(CodeDiagnostic::MissingIgnoreDirectiveReason()), + relative_file_path.to_path_buf(), + ignore_directive.line_no, + )) + } else { + None + } + } + + fn check_ignore_directive( + &self, + ignore_directive: &IgnoreDirective, + diagnostics: &Vec, + relative_file_path: &Path, + ) -> Vec { + vec![ + self.check_unused_ignore_directive(ignore_directive, diagnostics, relative_file_path), + self.check_missing_ignore_directive_reason(ignore_directive, relative_file_path), + ] + .into_iter() + .flatten() + .collect() + } + + fn check_ignore_directives( + &self, + ignore_directives: &IgnoreDirectives, + existing_diagnostics: &Vec, + relative_file_path: &Path, + ) -> Vec { + let mut diagnostics = Vec::new(); + for ignore_directive in ignore_directives.active_directives() { + diagnostics.extend(self.check_ignore_directive( + ignore_directive, + existing_diagnostics, + relative_file_path, + )); + } + for ignore_directive in ignore_directives.redundant_directives() { + diagnostics.push( + self.get_unused_ignore_directive_diagnostic(ignore_directive, relative_file_path), + ); + } + + diagnostics + } + + fn remove_ignored_diagnostics( + &self, + ignore_directives: &IgnoreDirectives, + diagnostics: &mut Vec, + ) { + for ignore_directive in ignore_directives.active_directives() { + diagnostics.retain(|diagnostic| !ignore_directive.matches_diagnostic(diagnostic)); + } + } + + pub fn process_diagnostics( + &self, + ignore_directives: &IgnoreDirectives, + diagnostics: &mut Vec, + relative_file_path: &Path, + ) { + // Check for diagnostics related to ignore directives + let ignore_directive_diagnostics = + self.check_ignore_directives(ignore_directives, diagnostics, relative_file_path); + + // Remove ignored diagnostics + self.remove_ignored_diagnostics(ignore_directives, diagnostics); + + // Add the new diagnostics to the list + diagnostics.extend(ignore_directive_diagnostics); + } +} diff --git a/src/checks/interface.rs b/src/checks/interface.rs new file mode 100644 index 00000000..b22b87de --- /dev/null +++ b/src/checks/interface.rs @@ -0,0 +1,164 @@ +use std::path::PathBuf; + +use crate::config::root_module::RootModuleTreatment; +use crate::config::{ModuleConfig, ProjectConfig}; +use crate::diagnostics::{ + CodeDiagnostic, ConfigurationDiagnostic, Diagnostic, DiagnosticDetails, FileChecker, + Result as DiagnosticResult, +}; +use crate::interfaces::compiled::CompiledInterfaces; +use crate::interfaces::data_types::{TypeCheckCache, TypeCheckResult}; +use crate::interfaces::error::InterfaceError; +use crate::modules::ModuleTree; +use crate::processors::file_module::FileModuleInternal; +use crate::processors::imports::NormalizedImport; + +#[derive(Debug)] +pub enum InterfaceCheckResult { + Exposed { type_check_result: TypeCheckResult }, + NotExposed, + NoInterfaces, + TopLevelModule, +} + +pub struct InterfaceChecker<'a> { + project_config: &'a ProjectConfig, + module_tree: &'a ModuleTree, + interfaces: CompiledInterfaces, + type_check_cache: Option, +} + +impl<'a> InterfaceChecker<'a> { + pub fn new(project_config: &'a ProjectConfig, module_tree: &'a ModuleTree) -> Self { + let compiled = CompiledInterfaces::build(project_config.all_interfaces()); + + Self { + project_config, + module_tree, + interfaces: compiled, + type_check_cache: None, + } + } + + pub fn with_type_check_cache( + mut self, + modules: &[ModuleConfig], + source_roots: &[PathBuf], + ) -> Result { + let type_check_cache = TypeCheckCache::build(&self.interfaces, modules, source_roots)?; + self.type_check_cache = Some(type_check_cache); + Ok(self) + } + + fn check_member(&self, member: &str, module_path: &str) -> InterfaceCheckResult { + if member.is_empty() { + return InterfaceCheckResult::TopLevelModule; + } + + let matching_interfaces = self.interfaces.get_interfaces(module_path); + + if matching_interfaces.is_empty() { + return InterfaceCheckResult::NoInterfaces; + } + + let mut is_exposed = false; + for interface in matching_interfaces { + if interface.expose.iter().any(|re| re.is_match(member)) { + is_exposed = true; + } + } + + if !is_exposed { + return InterfaceCheckResult::NotExposed; + } + + InterfaceCheckResult::Exposed { + type_check_result: self + .type_check_cache + .as_ref() + .map(|cache| cache.get_result(member)) + .unwrap_or(TypeCheckResult::Unknown), + } + } + + fn check_interfaces( + &self, + import: &NormalizedImport, + internal_file: &FileModuleInternal, + ) -> DiagnosticResult> { + if let Some(import_module_config) = self + .module_tree + .find_nearest(&import.module_path) + .as_ref() + .and_then(|module| module.config.as_ref()) + { + if import_module_config == internal_file.module_config() { + return Ok(vec![]); + } + + if import_module_config.is_root() + && self.project_config.root_module == RootModuleTreatment::Ignore + { + return Ok(vec![]); + } + + let import_member = import + .module_path + .strip_prefix(&import_module_config.path) + .and_then(|s| s.strip_prefix('.')) + .unwrap_or(""); + let check_result = self.check_member(import_member, &import_module_config.path); + match check_result { + InterfaceCheckResult::NotExposed => Ok(vec![Diagnostic::new_located_error( + internal_file.relative_file_path().to_path_buf(), + import.line_no, + DiagnosticDetails::Code(CodeDiagnostic::PrivateImport { + import_mod_path: import.module_path.to_string(), + usage_module: internal_file.module_config().path.to_string(), + definition_module: import_module_config.path.to_string(), + }), + )]), + InterfaceCheckResult::Exposed { + type_check_result: TypeCheckResult::DidNotMatchInterface { expected }, + } => Ok(vec![Diagnostic::new_located_error( + internal_file.relative_file_path().to_path_buf(), + import.line_no, + DiagnosticDetails::Code(CodeDiagnostic::InvalidDataTypeExport { + import_mod_path: import.module_path.to_string(), + usage_module: internal_file.module_config().path.to_string(), + definition_module: import_module_config.path.to_string(), + expected_data_type: expected.to_string(), + }), + )]), + InterfaceCheckResult::Exposed { + type_check_result: TypeCheckResult::MatchedInterface { .. }, + } + | InterfaceCheckResult::Exposed { + type_check_result: TypeCheckResult::Unknown, + } + | InterfaceCheckResult::NoInterfaces + | InterfaceCheckResult::TopLevelModule => Ok(vec![]), + } + } else { + Ok(vec![Diagnostic::new_global_error( + DiagnosticDetails::Configuration(ConfigurationDiagnostic::ModuleConfigNotFound { + module_path: import.module_path.to_string(), + }), + )]) + } + } +} + +impl<'a> FileChecker<'a> for InterfaceChecker<'a> { + type ProcessedFile = FileModuleInternal<'a>; + type Output = Vec; + + fn check(&'a self, input: &Self::ProcessedFile) -> DiagnosticResult { + let mut diagnostics = vec![]; + for import in input.imports.all_imports() { + diagnostics.extend(self.check_interfaces(import, input)?); + } + + Ok(diagnostics) + } +} diff --git a/src/checks/internal_dependency.rs b/src/checks/internal_dependency.rs new file mode 100644 index 00000000..830353ea --- /dev/null +++ b/src/checks/internal_dependency.rs @@ -0,0 +1,205 @@ +use crate::{ + config::{root_module::RootModuleTreatment, DependencyConfig, ModuleConfig, ProjectConfig}, + diagnostics::{ + CodeDiagnostic, ConfigurationDiagnostic, Diagnostic, DiagnosticDetails, FileChecker, + Result as DiagnosticResult, + }, + modules::ModuleTree, + processors::{file_module::FileModuleInternal, imports::NormalizedImport}, +}; +use std::path::Path; + +#[derive(Debug)] +enum LayerCheckResult { + Ok, + SameLayer, + LayerNotSpecified, + LayerViolation(Diagnostic), + UnknownLayer(Diagnostic), +} + +pub struct InternalDependencyChecker<'a> { + project_config: &'a ProjectConfig, + module_tree: &'a ModuleTree, +} + +impl<'a> InternalDependencyChecker<'a> { + pub fn new(project_config: &'a ProjectConfig, module_tree: &'a ModuleTree) -> Self { + Self { + project_config, + module_tree, + } + } + + fn check_layers( + &self, + import: &NormalizedImport, + layers: &[String], + source_module_config: &ModuleConfig, + target_module_config: &ModuleConfig, + relative_file_path: &Path, + ) -> LayerCheckResult { + match (&source_module_config.layer, &target_module_config.layer) { + (Some(source_layer), Some(target_layer)) => { + let source_index = layers.iter().position(|layer| layer == source_layer); + let target_index = layers.iter().position(|layer| layer == target_layer); + + match (source_index, target_index) { + (Some(source_index), Some(target_index)) => { + if source_index == target_index { + LayerCheckResult::SameLayer + } else if source_index < target_index { + LayerCheckResult::Ok + } else { + LayerCheckResult::LayerViolation(Diagnostic::new_located_error( + relative_file_path.to_path_buf(), + import.line_no, + DiagnosticDetails::Code(CodeDiagnostic::LayerViolation { + import_mod_path: import.module_path.to_string(), + usage_module: source_module_config.path.clone(), + usage_layer: source_layer.clone(), + definition_module: target_module_config.path.clone(), + definition_layer: target_layer.clone(), + }), + )) + } + } + // If either index is not found, the layer is unknown + (Some(_), None) => LayerCheckResult::UnknownLayer( + Diagnostic::new_global_error(DiagnosticDetails::Configuration( + ConfigurationDiagnostic::UnknownLayer { + layer: target_layer.clone(), + }, + )), + ), + (None, Some(_)) => LayerCheckResult::UnknownLayer( + Diagnostic::new_global_error(DiagnosticDetails::Configuration( + ConfigurationDiagnostic::UnknownLayer { + layer: source_layer.clone(), + }, + )), + ), + _ => LayerCheckResult::UnknownLayer(Diagnostic::new_global_error( + DiagnosticDetails::Configuration(ConfigurationDiagnostic::UnknownLayer { + layer: source_layer.clone(), + }), + )), + } + } + _ => LayerCheckResult::LayerNotSpecified, // At least one module does not have a layer + } + } + + fn check_dependencies( + &self, + relative_file_path: &Path, + import: &NormalizedImport, + file_module_config: &ModuleConfig, + import_module_config: &ModuleConfig, + layers: &[String], + ) -> DiagnosticResult> { + if import_module_config == file_module_config { + return Ok(vec![]); + } + + // Layer check should take precedence over other dependency checks + match self.check_layers( + import, + layers, + file_module_config, + import_module_config, + relative_file_path, + ) { + LayerCheckResult::Ok => return Ok(vec![]), // Higher layers can unconditionally import lower layers + LayerCheckResult::LayerViolation(e) | LayerCheckResult::UnknownLayer(e) => { + return Ok(vec![e]); + } + LayerCheckResult::SameLayer | LayerCheckResult::LayerNotSpecified => (), // We need to do further processing to determine if the dependency is allowed + }; + + if file_module_config.depends_on.is_none() { + return Ok(vec![]); + } + + if import_module_config.utility { + return Ok(vec![]); + } + + let file_nearest_module_path = &file_module_config.path; + let import_nearest_module_path = &import_module_config.path; + + match file_module_config + .dependencies_iter() + .find(|dep| &dep.path == import_nearest_module_path) + { + Some(DependencyConfig { + deprecated: true, .. + }) => Ok(vec![Diagnostic::new_located_warning( + relative_file_path.to_path_buf(), + import.line_no, + DiagnosticDetails::Code(CodeDiagnostic::DeprecatedImport { + import_mod_path: import.module_path.to_string(), + usage_module: file_nearest_module_path.to_string(), + definition_module: import_nearest_module_path.to_string(), + }), + )]), + Some(_) => Ok(vec![]), + None => Ok(vec![Diagnostic::new_located_error( + relative_file_path.to_path_buf(), + import.line_no, + DiagnosticDetails::Code(CodeDiagnostic::InvalidImport { + import_mod_path: import.module_path.to_string(), + usage_module: file_nearest_module_path.to_string(), + definition_module: import_nearest_module_path.to_string(), + }), + )]), + } + } + + fn check_import( + &self, + import: &NormalizedImport, + internal_file: &FileModuleInternal, + ) -> DiagnosticResult> { + if let Some(import_module_config) = self + .module_tree + .find_nearest(&import.module_path) + .as_ref() + .and_then(|module| module.config.as_ref()) + { + if import_module_config.is_root() + && self.project_config.root_module == RootModuleTreatment::Ignore + { + return Ok(vec![]); + } + + self.check_dependencies( + internal_file.relative_file_path(), + import, + internal_file.module_config(), + import_module_config, + &self.project_config.layers, + ) + } else { + Ok(vec![Diagnostic::new_global_error( + DiagnosticDetails::Configuration(ConfigurationDiagnostic::ModuleConfigNotFound { + module_path: import.module_path.to_string(), + }), + )]) + } + } +} + +impl<'a> FileChecker<'a> for InternalDependencyChecker<'a> { + type ProcessedFile = FileModuleInternal<'a>; + type Output = Vec; + + fn check(&'a self, processed_file: &Self::ProcessedFile) -> DiagnosticResult { + let mut diagnostics = Vec::new(); + for import in processed_file.imports.all_imports() { + diagnostics.extend(self.check_import(import, processed_file)?); + } + + Ok(diagnostics) + } +} diff --git a/src/checks/mod.rs b/src/checks/mod.rs new file mode 100644 index 00000000..77ef47a9 --- /dev/null +++ b/src/checks/mod.rs @@ -0,0 +1,9 @@ +pub mod external_dependency; +pub mod ignore_directive; +pub mod interface; +pub mod internal_dependency; + +pub use external_dependency::ExternalDependencyChecker; +pub use ignore_directive::IgnoreDirectivePostProcessor; +pub use interface::InterfaceChecker; +pub use internal_dependency::InternalDependencyChecker; diff --git a/src/commands/check/check_external.rs b/src/commands/check/check_external.rs index 2d61c0a8..1e8bf9c4 100644 --- a/src/commands/check/check_external.rs +++ b/src/commands/check/check_external.rs @@ -1,18 +1,112 @@ +use crate::checks::{ExternalDependencyChecker, IgnoreDirectivePostProcessor}; +use crate::config::ProjectConfig; +use crate::diagnostics::{ + CodeDiagnostic, ConfigurationDiagnostic, Diagnostic, DiagnosticDetails, DiagnosticError, + DiagnosticPipeline, FileChecker, FileProcessor, Result as DiagnosticResult, +}; +use crate::external::parsing::{parse_pyproject_toml, ProjectInfo}; +use crate::filesystem::{walk_pyfiles, walk_pyprojects, ProjectFile}; +use crate::interrupt::check_interrupt; +use crate::modules::ModuleNode; +use crate::processors::file_module::FileModuleExternal; +use crate::processors::imports::get_external_imports; use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; +use std::sync::Arc; -use crate::config::ProjectConfig; -use crate::external::parsing::parse_pyproject_toml; -use crate::filesystem::relative_to; -use crate::{filesystem, imports}; +use dashmap::DashSet; +use rayon::prelude::*; -use super::checks::{ - check_import_external, check_missing_ignore_directive_reason, - check_unused_ignore_directive_external, ImportProcessResult, -}; -use super::error::ExternalCheckError; -use crate::diagnostics::{CodeDiagnostic, Diagnostic, DiagnosticDetails}; -pub type Result = std::result::Result; +use super::error::CheckError; + +pub type Result = std::result::Result; + +struct CheckExternalPipeline<'a> { + source_roots: &'a [PathBuf], + project_config: &'a ProjectConfig, + module_mappings: &'a HashMap>, + excluded_external_modules: &'a HashSet, + seen_dependencies: DashSet, + external_dependency_checker: ExternalDependencyChecker<'a>, + ignore_directive_post_processor: IgnoreDirectivePostProcessor<'a>, +} + +impl<'a> CheckExternalPipeline<'a> { + pub fn new( + source_roots: &'a [PathBuf], + project_config: &'a ProjectConfig, + project_info: &'a ProjectInfo, + module_mappings: &'a HashMap>, + stdlib_modules: &'a HashSet, + excluded_external_modules: &'a HashSet, + ) -> Self { + Self { + source_roots, + project_config, + module_mappings, + excluded_external_modules, + seen_dependencies: DashSet::new(), + external_dependency_checker: ExternalDependencyChecker::new( + project_info, + module_mappings, + stdlib_modules, + excluded_external_modules, + ), + ignore_directive_post_processor: IgnoreDirectivePostProcessor::new(project_config), + } + } +} + +impl<'a> FileProcessor<'a, ProjectFile<'a>> for CheckExternalPipeline<'a> { + type ProcessedFile = FileModuleExternal<'a>; + + fn process(&'a self, file_path: ProjectFile<'a>) -> DiagnosticResult { + // NOTE: check-external does not currently make use of the module tree, + // but it is very likely to do so in the future. + let file_module = Arc::new(ModuleNode::empty()); + + let external_imports = get_external_imports( + self.source_roots, + file_path.as_ref(), + self.project_config.ignore_type_checking_imports, + )?; + + external_imports + .all_imports_with_distribution_names(self.module_mappings) + .for_each(|import| { + import + .distribution_names + .iter() + .for_each(|distribution_name| { + self.seen_dependencies.insert(distribution_name.clone()); + }); + }); + + Ok(FileModuleExternal::new( + file_path, + file_module, + external_imports, + )) + } +} + +impl<'a> FileChecker<'a> for CheckExternalPipeline<'a> { + type ProcessedFile = FileModuleExternal<'a>; + type Output = Vec; + + fn check(&'a self, processed_file: &Self::ProcessedFile) -> DiagnosticResult { + let mut diagnostics = Vec::new(); + diagnostics.extend(self.external_dependency_checker.check(processed_file)?); + + self.ignore_directive_post_processor.process_diagnostics( + &processed_file.imports.ignore_directives, + &mut diagnostics, + processed_file.relative_file_path(), + ); + + Ok(diagnostics) + } +} pub fn check( project_root: &Path, @@ -24,111 +118,104 @@ pub fn check( let excluded_external_modules: HashSet = project_config.external.exclude.iter().cloned().collect(); let source_roots: Vec = project_config.prepend_roots(project_root); - let mut diagnostics = vec![]; - for pyproject in filesystem::walk_pyprojects(project_root.to_str().unwrap()) { - let project_info = parse_pyproject_toml(&pyproject)?; - let mut all_dependencies = project_info.dependencies.clone(); - for source_root in &project_info.source_paths { - for file_path in filesystem::walk_pyfiles(source_root.to_str().unwrap()) { - let absolute_file_path = source_root.join(&file_path); - - if let Ok(project_imports) = imports::get_external_imports( - &source_roots, - &absolute_file_path, - project_config.ignore_type_checking_imports, - ) { - for import in project_imports.active_imports() { - match check_import_external( - import, - &project_info, - module_mappings, - &excluded_external_modules, - &stdlib_modules, - ) { - ImportProcessResult::UndeclaredDependency(module_name) => { - diagnostics.push(Diagnostic::new_located_error( - relative_to(&absolute_file_path, project_root)?, - import.import_line_no, - DiagnosticDetails::Code( - CodeDiagnostic::UndeclaredExternalDependency { - import_mod_path: module_name, - }, - ), - )); + + let diagnostics = walk_pyprojects(project_root.to_string_lossy().as_ref()) + .par_bridge() + .flat_map(|pyproject| { + let project_info = match parse_pyproject_toml(&pyproject) { + Ok(project_info) => project_info, + Err(_) => { + return vec![Diagnostic::new_global_error( + DiagnosticDetails::Configuration( + ConfigurationDiagnostic::SkippedPyProjectParsingError { + file_path: pyproject.to_string_lossy().to_string(), + }, + ), + )]; + } + }; + let pipeline = CheckExternalPipeline::new( + &source_roots, + project_config, + &project_info, + module_mappings, + &stdlib_modules, + &excluded_external_modules, + ); + let mut project_diagnostics: Vec = project_info + .source_paths + .par_iter() + .flat_map(|source_root| { + walk_pyfiles(&source_root.display().to_string()) + .par_bridge() + .flat_map(|file_path| { + if check_interrupt().is_err() { + // Since files are being processed in parallel, + // this will essentially short-circuit all remaining files. + // Then, we check for an interrupt right after, and return the Err if it is set + return vec![]; } - ImportProcessResult::UsedDependencies(deps) - | ImportProcessResult::Excluded(deps) => { - for dep in deps { - all_dependencies.remove(&dep); + + match pipeline.diagnostics(ProjectFile::new( + project_root, + source_root, + &file_path, + )) { + Ok(diagnostics) => diagnostics, + Err(DiagnosticError::Io(_)) + | Err(DiagnosticError::Filesystem(_)) => { + vec![Diagnostic::new_global_warning( + DiagnosticDetails::Configuration( + ConfigurationDiagnostic::SkippedFileIoError { + file_path: file_path.display().to_string(), + }, + ), + )] } + Err(DiagnosticError::ImportParse(_)) => { + vec![Diagnostic::new_global_warning( + DiagnosticDetails::Configuration( + ConfigurationDiagnostic::SkippedFileSyntaxError { + file_path: file_path.display().to_string(), + }, + ), + )] + } + Err(_) => vec![Diagnostic::new_global_warning( + DiagnosticDetails::Configuration( + ConfigurationDiagnostic::SkippedUnknownError { + file_path: file_path.display().to_string(), + }, + ), + )], } - } - } - - for directive_ignored_import in project_imports.directive_ignored_imports() { - match check_missing_ignore_directive_reason( - &directive_ignored_import, - project_config, - ) { - Ok(()) => {} - Err(diagnostic) => { - diagnostics.push(diagnostic.into_located( - relative_to(&absolute_file_path, project_root)?, - directive_ignored_import.import.line_no, - )); - } - } - - match check_unused_ignore_directive_external( - &directive_ignored_import, - &project_info, - module_mappings, - &excluded_external_modules, - &stdlib_modules, - project_config, - ) { - Ok(()) => {} - Err(diagnostic) => { - diagnostics.push(diagnostic.into_located( - relative_to(&absolute_file_path, project_root)?, - directive_ignored_import.import.line_no, - )); - } - } - } - - for unused_directive in project_imports.unused_ignore_directives() { - if let Ok(severity) = - (&project_config.rules.unused_ignore_directives).try_into() - { - diagnostics.push(Diagnostic::new_located( - severity, - DiagnosticDetails::Code(CodeDiagnostic::UnusedIgnoreDirective()), - relative_to(&absolute_file_path, project_root)?, - unused_directive.line_no, - )); - } - } - } - } - } + }) + }) + .collect(); - diagnostics.extend( - all_dependencies - .into_iter() - .filter(|dep| !excluded_external_modules.contains(dep)) // 'exclude' should hide unused errors unconditionally + let all_seen_dependencies: HashSet = + pipeline.seen_dependencies.into_iter().collect(); + let unused_dependency_diagnostics = project_info + .dependencies + .difference(&all_seen_dependencies) + .filter(|&dep| !pipeline.excluded_external_modules.contains(dep)) // 'exclude' should hide unused errors unconditionally .map(|dep| { Diagnostic::new_global_error(DiagnosticDetails::Code( CodeDiagnostic::UnusedExternalDependency { - package_module_name: dep, + package_module_name: dep.clone(), }, )) - }) - .collect::>(), - ); + }); + + project_diagnostics.extend(unused_dependency_diagnostics); + project_diagnostics + }); + + if check_interrupt().is_err() { + return Err(CheckError::Interrupt); } - Ok(diagnostics) + Ok(diagnostics.collect()) } #[cfg(test)] diff --git a/src/commands/check/check_internal.rs b/src/commands/check/check_internal.rs index 5efce0bd..f6eac181 100644 --- a/src/commands/check/check_internal.rs +++ b/src/commands/check/check_internal.rs @@ -1,167 +1,151 @@ -use super::checks::{ - check_import_internal, check_missing_ignore_directive_reason, - check_unused_ignore_directive_internal, -}; -use super::error::CheckError; -use crate::diagnostics::{CodeDiagnostic, ConfigurationDiagnostic, Diagnostic, DiagnosticDetails}; -use crate::filesystem::relative_to; - use std::{ path::{Path, PathBuf}, sync::atomic::{AtomicBool, Ordering}, - sync::Arc, }; use rayon::prelude::*; -use crate::modules::error::ModuleTreeError; +use super::error::CheckError; use crate::{ + checks::{IgnoreDirectivePostProcessor, InterfaceChecker, InternalDependencyChecker}, config::{root_module::RootModuleTreatment, ProjectConfig}, + diagnostics::{ + ConfigurationDiagnostic, Diagnostic, DiagnosticDetails, DiagnosticError, + DiagnosticPipeline, FileChecker, FileProcessor, Result as DiagnosticResult, + }, exclusion::set_excluded_paths, - filesystem as fs, - imports::{get_project_imports, ImportParseError}, - interfaces::InterfaceChecker, + filesystem::{self as fs, ProjectFile}, interrupt::check_interrupt, - modules::{build_module_tree, ModuleTree}, + modules::{build_module_tree, error::ModuleTreeError, ModuleTree}, + processors::file_module::FileModuleInternal, + processors::imports::{get_project_imports, NormalizedImports}, }; pub type Result = std::result::Result; -fn process_file( - file_path: PathBuf, - project_root: &Path, - source_root: &Path, - source_roots: &[PathBuf], - module_tree: &ModuleTree, - project_config: &ProjectConfig, - interface_checker: &Option, - check_dependencies: bool, - found_imports: &AtomicBool, -) -> Result> { - let abs_file_path = &source_root.join(&file_path); - let relative_file_path = relative_to(abs_file_path, project_root)?; - let mod_path = fs::file_to_module_path(source_roots, abs_file_path)?; - let nearest_module = module_tree - .find_nearest(&mod_path) - .ok_or(CheckError::ModuleTree(ModuleTreeError::ModuleNotFound( - mod_path.to_string(), - )))?; - - if nearest_module.is_unchecked() { - return Ok(vec![]); +struct CheckInternalPipeline<'a> { + _project_root: &'a Path, + project_config: &'a ProjectConfig, + source_roots: &'a [PathBuf], + module_tree: &'a ModuleTree, + found_imports: &'a AtomicBool, + dependency_checker: Option>, + interface_checker: Option>, + ignore_directive_post_processor: IgnoreDirectivePostProcessor<'a>, +} + +impl<'a> CheckInternalPipeline<'a> { + pub fn new( + project_root: &'a Path, + project_config: &'a ProjectConfig, + source_roots: &'a [PathBuf], + module_tree: &'a ModuleTree, + found_imports: &'a AtomicBool, + ) -> Self { + Self { + _project_root: project_root, + project_config, + source_roots, + module_tree, + found_imports, + dependency_checker: None, + interface_checker: None, + ignore_directive_post_processor: IgnoreDirectivePostProcessor::new(project_config), + } } - if nearest_module.is_root() && project_config.root_module == RootModuleTreatment::Ignore { - return Ok(vec![]); + pub fn with_dependency_checker( + mut self, + dependency_checker: Option>, + ) -> Self { + self.dependency_checker = dependency_checker; + self } - let mut diagnostics = vec![]; - let project_imports = match get_project_imports( - source_roots, - abs_file_path, - project_config.ignore_type_checking_imports, - project_config.include_string_imports, - ) { - Ok(project_imports) => { - if !project_imports.imports.is_empty() && !found_imports.load(Ordering::Relaxed) { - // Only attempt to write if we haven't found imports yet. - // This avoids any potential lock contention. - found_imports.store(true, Ordering::Relaxed); - } - project_imports - } - Err(ImportParseError::Parsing { .. }) => { - return Ok(vec![Diagnostic::new_global_warning( - DiagnosticDetails::Configuration(ConfigurationDiagnostic::SkippedFileSyntaxError { - file_path: relative_file_path.display().to_string(), - }), - )]); + pub fn with_interface_checker( + mut self, + interface_checker: Option>, + ) -> Self { + self.interface_checker = interface_checker; + self + } +} + +impl<'a> FileProcessor<'a, ProjectFile<'a>> for CheckInternalPipeline<'a> { + type ProcessedFile = FileModuleInternal<'a>; + + fn process(&'a self, file_path: ProjectFile<'a>) -> DiagnosticResult { + let mod_path = fs::file_to_module_path(self.source_roots, file_path.as_ref())?; + let file_module = + self.module_tree + .find_nearest(&mod_path) + .ok_or(DiagnosticError::ModuleTree( + ModuleTreeError::ModuleNotFound(mod_path.to_string()), + ))?; + + if file_module.is_unchecked() { + return Ok(FileModuleInternal::new( + file_path, + file_module, + NormalizedImports::empty(), + )); } - Err(ImportParseError::Filesystem(_)) => { - return Ok(vec![Diagnostic::new_global_warning( - DiagnosticDetails::Configuration(ConfigurationDiagnostic::SkippedFileIoError { - file_path: relative_file_path.display().to_string(), - }), - )]); + + if file_module.is_root() && self.project_config.root_module == RootModuleTreatment::Ignore { + return Ok(FileModuleInternal::new( + file_path, + file_module, + NormalizedImports::empty(), + )); } - }; - project_imports.active_imports().for_each(|import| { - if let Err(import_diagnostics) = check_import_internal( - &import.module_path, - module_tree, - Arc::clone(&nearest_module), - &project_config.layers, - project_config.root_module.clone(), - interface_checker, - check_dependencies, - ) { - import_diagnostics - .into_iter() - .for_each(|diagnostic| match &diagnostic { - Diagnostic::Global { - details: DiagnosticDetails::Code(_), - .. - } => { - diagnostics.push( - diagnostic.into_located(relative_file_path.clone(), import.line_no), - ); - } - Diagnostic::Global { - details: DiagnosticDetails::Configuration(_), - .. - } => { - diagnostics.push(diagnostic); - } - _ => {} - }); + let project_imports = get_project_imports( + self.source_roots, + file_path.as_ref(), + self.project_config.ignore_type_checking_imports, + self.project_config.include_string_imports, + )?; + + if !project_imports.imports.is_empty() && !self.found_imports.load(Ordering::Relaxed) { + // Only attempt to write if we haven't found imports yet. + // This avoids any potential lock contention. + self.found_imports.store(true, Ordering::Relaxed); } - }); - project_imports - .directive_ignored_imports() - .for_each(|directive_ignored_import| { - match check_unused_ignore_directive_internal( - &directive_ignored_import, - module_tree, - Arc::clone(&nearest_module), - project_config, - interface_checker, - check_dependencies, - ) { - Ok(()) => {} - Err(diagnostic) => { - diagnostics.push(diagnostic.into_located( - relative_file_path.clone(), - directive_ignored_import.import.line_no, - )); - } - } - match check_missing_ignore_directive_reason(&directive_ignored_import, project_config) { - Ok(()) => {} - Err(diagnostic) => { - diagnostics.push(diagnostic.into_located( - relative_file_path.clone(), - directive_ignored_import.import.line_no, - )); - } - } - }); - - project_imports - .unused_ignore_directives() - .for_each(|ignore_directive| { - if let Ok(severity) = (&project_config.rules.unused_ignore_directives).try_into() { - diagnostics.push(Diagnostic::new_located( - severity, - DiagnosticDetails::Code(CodeDiagnostic::UnusedIgnoreDirective()), - relative_file_path.clone(), - ignore_directive.line_no, - )); - } - }); - - Ok(diagnostics) + Ok(FileModuleInternal::new( + file_path, + file_module, + project_imports, + )) + } +} + +impl<'a> FileChecker<'a> for CheckInternalPipeline<'a> { + type ProcessedFile = FileModuleInternal<'a>; + type Output = Vec; + + fn check(&'a self, processed_file: &Self::ProcessedFile) -> DiagnosticResult { + let mut diagnostics = Vec::new(); + diagnostics.extend( + self.dependency_checker + .as_ref() + .map_or(Ok(vec![]), |checker| checker.check(processed_file))?, + ); + + diagnostics.extend( + self.interface_checker + .as_ref() + .map_or(Ok(vec![]), |checker| checker.check(processed_file))?, + ); + + self.ignore_directive_post_processor.process_diagnostics( + &processed_file.imports.ignore_directives, + &mut diagnostics, + processed_file.relative_file_path(), + ); + + Ok(diagnostics) + } } pub fn check( @@ -212,15 +196,30 @@ pub fn check( project_config.use_regex_matching, )?; + let dependency_checker = if dependencies { + Some(InternalDependencyChecker::new(project_config, &module_tree)) + } else { + None + }; + let interface_checker = if interfaces { - let interface_checker = - InterfaceChecker::new(&project_config.all_interfaces().cloned().collect::>()); + let interface_checker = InterfaceChecker::new(project_config, &module_tree); // This is expensive Some(interface_checker.with_type_check_cache(&valid_modules, &source_roots)?) } else { None }; + let pipeline = CheckInternalPipeline::new( + &project_root, + project_config, + &source_roots, + &module_tree, + &found_imports, + ) + .with_dependency_checker(dependency_checker) + .with_interface_checker(interface_checker); + let diagnostics = source_roots.par_iter().flat_map(|source_root| { fs::walk_pyfiles(&source_root.display().to_string()) .par_bridge() @@ -231,18 +230,36 @@ pub fn check( // Then, we check for an interrupt right after, and return the Err if it is set return vec![]; } - process_file( - file_path, - &project_root, - source_root, - &source_roots, - &module_tree, - project_config, - &interface_checker, - dependencies, - &found_imports, - ) - .unwrap_or_default() + + let internal_file = ProjectFile::new(&project_root, source_root, &file_path); + match pipeline.diagnostics(internal_file) { + Ok(diagnostics) => diagnostics, + Err(DiagnosticError::Io(_)) | Err(DiagnosticError::Filesystem(_)) => { + vec![Diagnostic::new_global_warning( + DiagnosticDetails::Configuration( + ConfigurationDiagnostic::SkippedFileIoError { + file_path: file_path.display().to_string(), + }, + ), + )] + } + Err(DiagnosticError::ImportParse(_)) => { + vec![Diagnostic::new_global_warning( + DiagnosticDetails::Configuration( + ConfigurationDiagnostic::SkippedFileSyntaxError { + file_path: file_path.display().to_string(), + }, + ), + )] + } + Err(_) => vec![Diagnostic::new_global_warning( + DiagnosticDetails::Configuration( + ConfigurationDiagnostic::SkippedUnknownError { + file_path: file_path.display().to_string(), + }, + ), + )], + } }) }); diff --git a/src/commands/check/checks.rs b/src/commands/check/checks.rs deleted file mode 100644 index f112bdb4..00000000 --- a/src/commands/check/checks.rs +++ /dev/null @@ -1,577 +0,0 @@ -use std::{ - collections::{HashMap, HashSet}, - sync::Arc, -}; - -use crate::diagnostics::{CodeDiagnostic, ConfigurationDiagnostic, Diagnostic, DiagnosticDetails}; -use crate::{ - config::{ - root_module::RootModuleTreatment, rules::RuleSetting, DependencyConfig, ModuleConfig, - ProjectConfig, - }, - external::parsing::{normalize_package_name, ProjectInfo}, - imports::{DirectiveIgnoredImport, NormalizedImport}, - interfaces::{ - check::CheckResult as InterfaceCheckResult, data_types::TypeCheckResult, InterfaceChecker, - }, - modules::{ModuleNode, ModuleTree}, -}; - -fn check_dependencies( - import_mod_path: &str, - file_module_config: &ModuleConfig, - import_module_config: &ModuleConfig, - layers: &[String], -) -> Result<(), Diagnostic> { - // Layer check should take precedence over other dependency checks - match check_layers( - import_mod_path, - layers, - file_module_config, - import_module_config, - ) { - LayerCheckResult::Ok => return Ok(()), // Higher layers can unconditionally import lower layers - LayerCheckResult::SameLayer | LayerCheckResult::LayerNotSpecified => (), // We need to do further processing to determine if the dependency is allowed - LayerCheckResult::LayerViolation(e) | LayerCheckResult::UnknownLayer(e) => return Err(e), - }; - - if file_module_config.depends_on.is_none() { - return Ok(()); - } - - if import_module_config.utility { - return Ok(()); - } - - let file_nearest_module_path = &file_module_config.path; - let import_nearest_module_path = &import_module_config.path; - - match file_module_config - .dependencies_iter() - .find(|dep| &dep.path == import_nearest_module_path) - { - Some(DependencyConfig { - deprecated: true, .. - }) => Err(Diagnostic::new_global_warning(DiagnosticDetails::Code( - CodeDiagnostic::DeprecatedImport { - import_mod_path: import_mod_path.to_string(), - usage_module: file_nearest_module_path.to_string(), - definition_module: import_nearest_module_path.to_string(), - }, - ))), - Some(_) => Ok(()), - None => Err(Diagnostic::new_global_error(DiagnosticDetails::Code( - CodeDiagnostic::InvalidImport { - import_mod_path: import_mod_path.to_string(), - usage_module: file_nearest_module_path.to_string(), - definition_module: import_nearest_module_path.to_string(), - }, - ))), - } -} - -fn check_interfaces( - import_mod_path: &str, - import_nearest_module: &ModuleNode, - file_nearest_module: &ModuleNode, - interface_checker: &InterfaceChecker, -) -> Result<(), Diagnostic> { - let import_member = import_mod_path - .strip_prefix(&import_nearest_module.full_path) - .and_then(|s| s.strip_prefix('.')) - .unwrap_or(""); - let check_result = - interface_checker.check_member(import_member, &import_nearest_module.full_path); - match check_result { - InterfaceCheckResult::NotExposed => Err(Diagnostic::new_global_error( - DiagnosticDetails::Code(CodeDiagnostic::PrivateImport { - import_mod_path: import_mod_path.to_string(), - usage_module: file_nearest_module.full_path.to_string(), - definition_module: import_nearest_module.full_path.to_string(), - }), - )), - InterfaceCheckResult::Exposed { - type_check_result: TypeCheckResult::DidNotMatchInterface { expected }, - } => Err(Diagnostic::new_global_error(DiagnosticDetails::Code( - CodeDiagnostic::InvalidDataTypeExport { - import_mod_path: import_mod_path.to_string(), - usage_module: file_nearest_module.full_path.to_string(), - definition_module: import_nearest_module.full_path.to_string(), - expected_data_type: expected.to_string(), - }, - ))), - _ => Ok(()), - } -} - -pub(super) fn check_import_internal( - import_mod_path: &str, - module_tree: &ModuleTree, - file_nearest_module: Arc, - layers: &[String], - root_module_treatment: RootModuleTreatment, - interface_checker: &Option, - should_check_dependencies: bool, -) -> Result<(), Vec> { - let mut diagnostics = Vec::new(); - - if !should_check_dependencies && interface_checker.is_none() { - return Err(vec![Diagnostic::new_global_error( - DiagnosticDetails::Configuration(ConfigurationDiagnostic::NoChecksEnabled()), - )]); - } - - let import_nearest_module = match module_tree.find_nearest(import_mod_path) { - Some(module) => module, - // This should not be none since we intend to filter out any external imports, - // but we should allow external imports if they have made it here. - None => return Ok(()), - }; - - if import_nearest_module.is_root() && root_module_treatment == RootModuleTreatment::Ignore { - return Ok(()); - } - - if import_nearest_module == file_nearest_module { - // Imports within the same module are always allowed - return Ok(()); - } - - let file_module_config = match file_nearest_module.config.as_ref() { - Some(config) => config, - None => { - return Err(vec![Diagnostic::new_global_error( - DiagnosticDetails::Configuration(ConfigurationDiagnostic::ModuleConfigNotFound()), - )]); - } - }; - - let import_module_config = match import_nearest_module.config.as_ref() { - Some(config) => config, - None => { - return Err(vec![Diagnostic::new_global_error( - DiagnosticDetails::Configuration(ConfigurationDiagnostic::ModuleConfigNotFound()), - )]); - } - }; - - if let Some(interface_checker) = interface_checker { - if let Err(err) = check_interfaces( - import_mod_path, - &import_nearest_module, - &file_nearest_module, - interface_checker, - ) { - diagnostics.push(err); - } - } - - if should_check_dependencies { - if let Err(err) = check_dependencies( - import_mod_path, - file_module_config, - import_module_config, - layers, - ) { - diagnostics.push(err); - } - } - - if diagnostics.is_empty() { - Ok(()) - } else { - Err(diagnostics) - } -} - -pub(super) fn check_unused_ignore_directive_internal( - directive_ignored_import: &DirectiveIgnoredImport, - module_tree: &ModuleTree, - nearest_module: Arc, - project_config: &ProjectConfig, - interface_checker: &Option, - check_dependencies: bool, -) -> Result<(), Diagnostic> { - if project_config.rules.unused_ignore_directives == RuleSetting::Off { - return Ok(()); - } - - match check_import_internal( - &directive_ignored_import.import.module_path, - module_tree, - Arc::clone(&nearest_module), - &project_config.layers, - project_config.root_module.clone(), - interface_checker, - check_dependencies, - ) { - Ok(()) => Err(Diagnostic::new_global( - (&project_config.rules.unused_ignore_directives) - .try_into() - .unwrap(), - DiagnosticDetails::Code(CodeDiagnostic::UnnecessarilyIgnoredImport { - import_mod_path: directive_ignored_import.import.module_path.to_string(), - }), - )), - Err(_) => Ok(()), - } -} - -#[derive(Debug)] -pub enum ImportProcessResult { - UndeclaredDependency(String), - UsedDependencies(Vec), - Excluded(Vec), -} - -pub(super) fn check_import_external( - import: &NormalizedImport, - project_info: &ProjectInfo, - module_mappings: &HashMap>, - excluded_external_modules: &HashSet, - stdlib_modules: &HashSet, -) -> ImportProcessResult { - let top_level_module_name = import.top_level_module_name().to_string(); - let default_distribution_names = vec![top_level_module_name.clone()]; - let distribution_names: Vec = module_mappings - .get(&top_level_module_name) - .map(|dist_names| { - dist_names - .iter() - .map(|dist_name| normalize_package_name(dist_name)) - .collect() - }) - .unwrap_or(default_distribution_names); - - if distribution_names - .iter() - .any(|dist_name| excluded_external_modules.contains(dist_name)) - || stdlib_modules.contains(&top_level_module_name) - { - return ImportProcessResult::Excluded(distribution_names); - } - - let is_declared = distribution_names - .iter() - .any(|dist_name| project_info.dependencies.contains(dist_name)); - - if !is_declared { - ImportProcessResult::UndeclaredDependency(top_level_module_name.to_string()) - } else { - ImportProcessResult::UsedDependencies(distribution_names) - } -} - -pub(super) fn check_unused_ignore_directive_external( - directive_ignored_import: &DirectiveIgnoredImport, - project_info: &ProjectInfo, - module_mappings: &HashMap>, - excluded_external_modules: &HashSet, - stdlib_modules: &HashSet, - project_config: &ProjectConfig, -) -> Result<(), Diagnostic> { - if let ImportProcessResult::UsedDependencies(_) | ImportProcessResult::Excluded(_) = - check_import_external( - directive_ignored_import.import, - project_info, - module_mappings, - excluded_external_modules, - stdlib_modules, - ) - { - match project_config.rules.unused_ignore_directives { - RuleSetting::Error => Err(Diagnostic::new_global( - (&project_config.rules.unused_ignore_directives) - .try_into() - .unwrap(), - DiagnosticDetails::Code(CodeDiagnostic::UnnecessarilyIgnoredImport { - import_mod_path: directive_ignored_import.import.module_path.to_string(), - }), - )), - RuleSetting::Warn => Err(Diagnostic::new_global( - (&project_config.rules.unused_ignore_directives) - .try_into() - .unwrap(), - DiagnosticDetails::Code(CodeDiagnostic::UnnecessarilyIgnoredImport { - import_mod_path: directive_ignored_import.import.module_path.to_string(), - }), - )), - RuleSetting::Off => Ok(()), - } - } else { - Ok(()) - } -} - -pub(super) fn check_missing_ignore_directive_reason( - directive_ignored_import: &DirectiveIgnoredImport, - project_config: &ProjectConfig, -) -> Result<(), Diagnostic> { - if project_config.rules.require_ignore_directive_reasons == RuleSetting::Off { - return Ok(()); - } - - if directive_ignored_import.reason.is_empty() { - Err(Diagnostic::new_global( - (&project_config.rules.require_ignore_directive_reasons) - .try_into() - .unwrap(), - DiagnosticDetails::Code(CodeDiagnostic::MissingIgnoreDirectiveReason { - import_mod_path: directive_ignored_import.import.module_path.to_string(), - }), - )) - } else { - Ok(()) - } -} - -#[derive(Debug)] -enum LayerCheckResult { - Ok, - SameLayer, - LayerNotSpecified, - LayerViolation(Diagnostic), - UnknownLayer(Diagnostic), -} - -fn check_layers( - import_mod_path: &str, - layers: &[String], - source_module_config: &ModuleConfig, - target_module_config: &ModuleConfig, -) -> LayerCheckResult { - match (&source_module_config.layer, &target_module_config.layer) { - (Some(source_layer), Some(target_layer)) => { - let source_index = layers.iter().position(|layer| layer == source_layer); - let target_index = layers.iter().position(|layer| layer == target_layer); - - match (source_index, target_index) { - (Some(source_index), Some(target_index)) => { - if source_index == target_index { - LayerCheckResult::SameLayer - } else if source_index < target_index { - LayerCheckResult::Ok - } else { - LayerCheckResult::LayerViolation(Diagnostic::new_global_error( - DiagnosticDetails::Code(CodeDiagnostic::LayerViolation { - import_mod_path: import_mod_path.to_string(), - usage_module: source_module_config.path.clone(), - usage_layer: source_layer.clone(), - definition_module: target_module_config.path.clone(), - definition_layer: target_layer.clone(), - }), - )) - } - } - // If either index is not found, the layer is unknown - (Some(_), None) => LayerCheckResult::UnknownLayer(Diagnostic::new_global_error( - DiagnosticDetails::Configuration(ConfigurationDiagnostic::UnknownLayer { - layer: target_layer.clone(), - }), - )), - (None, Some(_)) => LayerCheckResult::UnknownLayer(Diagnostic::new_global_error( - DiagnosticDetails::Configuration(ConfigurationDiagnostic::UnknownLayer { - layer: source_layer.clone(), - }), - )), - _ => LayerCheckResult::UnknownLayer(Diagnostic::new_global_error( - DiagnosticDetails::Configuration(ConfigurationDiagnostic::UnknownLayer { - layer: source_layer.clone(), - }), - )), - } - } - _ => LayerCheckResult::LayerNotSpecified, // At least one module does not have a layer - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::config::{InterfaceConfig, ModuleConfig}; - use crate::diagnostics::Diagnostic; - use crate::modules::ModuleTree; - use crate::tests::check_internal::fixtures::{ - interface_config, layers, module_config, module_tree, - }; - use std::path::PathBuf; - - use rstest::rstest; - - #[rstest] - #[case("domain_one", "domain_two", true)] // same layer, explicit dependency - #[case("domain_one", "domain_one.subdomain", false)] // same layer, parent->child (deprecated) - #[case("domain_one.subdomain", "domain_one", false)] // same layer, child->parent not allowed - #[case("domain_one", "service_one", true)] // top->middle allowed - #[case("domain_one", "data_one", true)] // top->bottom allowed - #[case("service_one", "service_one.internal", true)] // same layer, explicit dependency - #[case("service_one", "data_one", true)] // middle->bottom allowed - #[case("service_one", "domain_one", false)] // middle->top denied - #[case("data_one", "service_one", false)] // bottom->middle denied - #[case("data_one", "domain_one", false)] // bottom->top denied - fn test_check_import( - module_tree: ModuleTree, - module_config: Vec, - interface_config: Vec, - layers: Vec, - #[case] file_mod_path: &str, - #[case] import_mod_path: &str, - #[case] expected_result: bool, - ) { - let file_module = module_tree.find_nearest(file_mod_path).unwrap(); - let interface_checker = Some( - InterfaceChecker::new(&interface_config) - .with_type_check_cache(&module_config, &[PathBuf::from(".")]) - .unwrap(), - ); - - let check_error = check_import_internal( - import_mod_path, - &module_tree, - file_module.clone(), - &layers, - RootModuleTreatment::Allow, - &interface_checker, - true, - ); - let result = check_error.is_ok(); - assert_eq!(result, expected_result); - } - - #[rstest] - fn test_check_deprecated_import( - module_tree: ModuleTree, - module_config: Vec, - interface_config: Vec, - layers: Vec, - ) { - let file_module = module_tree.find_nearest("domain_one").unwrap(); - let interface_checker = Some( - InterfaceChecker::new(&interface_config) - .with_type_check_cache(&module_config, &[PathBuf::from(".")]) - .unwrap(), - ); - - let check_error = check_import_internal( - "domain_one.subdomain", - &module_tree, - file_module.clone(), - &layers, - RootModuleTreatment::Allow, - &interface_checker, - true, - ); - assert!(check_error.is_err()); - assert!(check_error - .unwrap_err() - .iter() - .any(|err| err.is_deprecated())); - } - - #[rstest] - #[case("top", "top", LayerCheckResult::SameLayer)] - #[case("top", "middle", LayerCheckResult::Ok)] - #[case("top", "bottom", LayerCheckResult::Ok)] - #[case("middle", "bottom", LayerCheckResult::Ok)] - #[case("bottom", "top", LayerCheckResult::LayerViolation(Diagnostic::new_global_error( - DiagnosticDetails::Code(CodeDiagnostic::LayerViolation { - import_mod_path: "".to_string(), - usage_module: "".to_string(), - usage_layer: "bottom".to_string(), - definition_module: "".to_string(), - definition_layer: "top".to_string(), - }), - )))] - #[case("middle", "top", LayerCheckResult::LayerViolation(Diagnostic::new_global_error( - DiagnosticDetails::Code(CodeDiagnostic::LayerViolation { - import_mod_path: "".to_string(), - usage_module: "".to_string(), - usage_layer: "middle".to_string(), - definition_module: "".to_string(), - definition_layer: "top".to_string(), - }), - )))] - #[case("bottom", "middle", LayerCheckResult::LayerViolation(Diagnostic::new_global_error( - DiagnosticDetails::Code(CodeDiagnostic::LayerViolation { - import_mod_path: "".to_string(), - usage_module: "".to_string(), - usage_layer: "bottom".to_string(), - definition_module: "".to_string(), - definition_layer: "middle".to_string(), - }), - )))] - fn test_check_layers_hierarchy( - layers: Vec, - #[case] source_layer: &str, - #[case] target_layer: &str, - #[case] expected_pattern: LayerCheckResult, - ) { - let source_config = ModuleConfig::new_with_layer("source", source_layer); - let target_config = ModuleConfig::new_with_layer("target", target_layer); - - let result = check_layers("target", &layers, &source_config, &target_config); - match (result, expected_pattern) { - (LayerCheckResult::Ok, LayerCheckResult::Ok) => (), - (LayerCheckResult::SameLayer, LayerCheckResult::SameLayer) => (), - (LayerCheckResult::LayerViolation(_), LayerCheckResult::LayerViolation(_)) => (), - (actual, expected) => panic!("Expected {:?} but got {:?}", expected, actual), - } - } - - #[rstest] - fn test_check_layers_missing_layers() { - let layers: Vec = vec![]; - let source_config = ModuleConfig::new_with_layer("source", "any"); - let target_config = ModuleConfig::new_with_layer("target", "any"); - - let result = check_layers("target", &layers, &source_config, &target_config); - assert!(matches!(result, LayerCheckResult::UnknownLayer(_))); - } - - #[rstest] - fn test_check_layers_no_layer_specified() { - let layers = vec!["top".to_string(), "bottom".to_string()]; - let source_config = ModuleConfig::default(); - let target_config = ModuleConfig::default(); - - let result = check_layers("", &layers, &source_config, &target_config); - assert!(matches!(result, LayerCheckResult::LayerNotSpecified)); - } - - #[rstest] - #[case("unrestricted", "domain_one", false)] // middle->top denied due to layer check - #[case("unrestricted", "service_one", true)] // same layer allowed - #[case("unrestricted", "data_one", true)] // middle->bottom allowed - fn test_check_import_unrestricted_dependencies( - module_tree: ModuleTree, - module_config: Vec, - interface_config: Vec, - layers: Vec, - #[case] file_mod_path: &str, - #[case] import_mod_path: &str, - #[case] expected_result: bool, - ) { - let file_module = module_tree.find_nearest(file_mod_path).unwrap(); - let interface_checker = Some( - InterfaceChecker::new(&interface_config) - .with_type_check_cache(&module_config, &[PathBuf::from(".")]) - .unwrap(), - ); - - let check_error = check_import_internal( - import_mod_path, - &module_tree, - file_module.clone(), - &layers, - RootModuleTreatment::Allow, - &interface_checker, - true, - ); - let result = check_error.is_ok(); - assert_eq!( - result, expected_result, - "Expected import from '{}' to '{}' to be {}, but got {}", - file_mod_path, import_mod_path, expected_result, result - ); - } -} diff --git a/src/commands/check/error.rs b/src/commands/check/error.rs index 86879f4d..abfe38d8 100644 --- a/src/commands/check/error.rs +++ b/src/commands/check/error.rs @@ -1,11 +1,8 @@ -use std::io; - use thiserror::Error; +use crate::diagnostics::DiagnosticError; use crate::exclusion; -use crate::external; use crate::filesystem as fs; -use crate::imports; use crate::interfaces::error::InterfaceError; use crate::modules; @@ -25,16 +22,6 @@ pub enum CheckError { Interface(#[from] InterfaceError), #[error("Operation cancelled by user")] Interrupt, -} - -#[derive(Error, Debug)] -pub enum ExternalCheckError { - #[error("Parsing error: {0}")] - Parse(#[from] external::ParsingError), - #[error("Import parsing error: {0}")] - ImportParse(#[from] imports::ImportParseError), - #[error("IO error: {0}")] - Io(#[from] io::Error), - #[error("Filesystem error: {0}")] - Filesystem(#[from] fs::FileSystemError), + #[error("Diagnostic error: {0}")] + Diagnostic(#[from] DiagnosticError), } diff --git a/src/commands/check/format.rs b/src/commands/check/format.rs index ed2ec050..903d1626 100644 --- a/src/commands/check/format.rs +++ b/src/commands/check/format.rs @@ -29,8 +29,8 @@ impl From<&DiagnosticDetails> for DiagnosticGroupKind { CodeDiagnostic::UndeclaredExternalDependency { .. } => Self::ExternalDependency, CodeDiagnostic::UnusedExternalDependency { .. } => Self::ExternalDependency, CodeDiagnostic::UnnecessarilyIgnoredImport { .. } => Self::Other, - CodeDiagnostic::UnusedIgnoreDirective { .. } => Self::Other, - CodeDiagnostic::MissingIgnoreDirectiveReason { .. } => Self::Other, + CodeDiagnostic::UnusedIgnoreDirective() => Self::Other, + CodeDiagnostic::MissingIgnoreDirectiveReason() => Self::Other, }, } } diff --git a/src/commands/check/mod.rs b/src/commands/check/mod.rs index 70bfcdaf..e085e5bc 100644 --- a/src/commands/check/mod.rs +++ b/src/commands/check/mod.rs @@ -1,9 +1,8 @@ pub mod check_external; pub mod check_internal; -pub mod checks; pub mod error; pub mod format; pub use check_external::check as check_external; pub use check_internal::check as check_internal; -pub use error::{CheckError, ExternalCheckError}; +pub use error::CheckError; diff --git a/src/commands/report.rs b/src/commands/report.rs index b23b00ab..8fd6f2a7 100644 --- a/src/commands/report.rs +++ b/src/commands/report.rs @@ -15,9 +15,9 @@ use crate::config::ProjectConfig; use crate::filesystem::{ file_to_module_path, validate_project_modules, walk_pyfiles, FileSystemError, }; -use crate::imports::{get_project_imports, ImportParseError, NormalizedImport}; use crate::interrupt::check_interrupt; use crate::modules::{build_module_tree, error::ModuleTreeError}; +use crate::processors::imports::{get_project_imports, ImportParseError, NormalizedImport}; struct Dependency { file_path: PathBuf, @@ -318,9 +318,8 @@ pub fn create_dependency_report( if !is_module_prefix(&module_path, &import.module_path) { return false; } - file_module.as_ref().map_or(false, |m| { - include_usage_modules.as_ref().map_or( - true, + file_module.as_ref().is_some_and(|m| { + include_usage_modules.as_ref().is_none_or( |included_modules| { included_modules.contains(&m.full_path) }, diff --git a/src/commands/test.rs b/src/commands/test.rs index 4a7a52c8..a59b6c90 100644 --- a/src/commands/test.rs +++ b/src/commands/test.rs @@ -7,8 +7,8 @@ use thiserror::Error; use crate::config::{ModuleConfig, ProjectConfig}; use crate::filesystem::{self as fs}; -use crate::imports::get_project_imports; use crate::modules::{build_module_tree, ModuleTree}; +use crate::processors::imports::get_project_imports; #[derive(Error, Debug)] pub enum TestError { diff --git a/src/config/modules.rs b/src/config/modules.rs index 17985c86..22c0b796 100644 --- a/src/config/modules.rs +++ b/src/config/modules.rs @@ -226,6 +226,14 @@ impl ModuleConfig { pub fn new_root_config() -> Self { Self::new(ROOT_MODULE_SENTINEL_TAG, false) } + + pub fn is_root(&self) -> bool { + self.path == ROOT_MODULE_SENTINEL_TAG + } + + pub fn is_unchecked(&self) -> bool { + self.unchecked + } } #[pymethods] diff --git a/src/diagnostics.rs b/src/diagnostics/diagnostics.rs similarity index 95% rename from src/diagnostics.rs rename to src/diagnostics/diagnostics.rs index 3199ae61..42ff831a 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics/diagnostics.rs @@ -40,8 +40,8 @@ pub enum ConfigurationDiagnostic { #[error("Module containing '{file_mod_path}' not found in project.")] ModuleNotFound { file_mod_path: String }, - #[error("Could not find module configuration.")] - ModuleConfigNotFound(), + #[error("Could not find module configuration for module '{module_path}'.")] + ModuleConfigNotFound { module_path: String }, #[error("Layer '{layer}' is not defined in the project.")] UnknownLayer { layer: String }, @@ -57,6 +57,12 @@ pub enum ConfigurationDiagnostic { #[error("Skipped '{file_path}' due to an I/O error.")] SkippedFileIoError { file_path: String }, + + #[error("Skipped '{file_path}' due to a parsing error.")] + SkippedPyProjectParsingError { file_path: String }, + + #[error("Skipped '{file_path}' due to an unknown error.")] + SkippedUnknownError { file_path: String }, } #[derive(Error, Debug, Clone, Serialize, PartialEq)] @@ -106,8 +112,8 @@ pub enum CodeDiagnostic { #[error("Ignore directive is unused.")] UnusedIgnoreDirective(), - #[error("Import '{import_mod_path}' is ignored without providing a reason.")] - MissingIgnoreDirectiveReason { import_mod_path: String }, + #[error("Ignore directive is missing a reason.")] + MissingIgnoreDirectiveReason(), #[error("Import '{import_mod_path}' does not match any declared dependency.")] UndeclaredExternalDependency { import_mod_path: String }, @@ -137,8 +143,8 @@ impl CodeDiagnostic { | CodeDiagnostic::UnnecessarilyIgnoredImport { import_mod_path, .. } => Some(import_mod_path), - CodeDiagnostic::UnusedIgnoreDirective { .. } => None, - CodeDiagnostic::MissingIgnoreDirectiveReason { .. } => None, + CodeDiagnostic::UnusedIgnoreDirective() => None, + CodeDiagnostic::MissingIgnoreDirectiveReason() => None, CodeDiagnostic::UndeclaredExternalDependency { .. } => None, CodeDiagnostic::UnusedExternalDependency { .. } => None, } diff --git a/src/diagnostics/error.rs b/src/diagnostics/error.rs new file mode 100644 index 00000000..ba6b301f --- /dev/null +++ b/src/diagnostics/error.rs @@ -0,0 +1,25 @@ +use std::io; + +use thiserror::Error; + +use crate::external; +use crate::filesystem as fs; +use crate::interfaces::error::InterfaceError; +use crate::modules; +use crate::processors::imports; + +#[derive(Error, Debug)] +pub enum DiagnosticError { + #[error("Module tree error: {0}")] + ModuleTree(#[from] modules::error::ModuleTreeError), + #[error("Interface error: {0}")] + Interface(#[from] InterfaceError), + #[error("Parsing error: {0}")] + Parse(#[from] external::ParsingError), + #[error("Import parsing error: {0}")] + ImportParse(#[from] imports::ImportParseError), + #[error("IO error: {0}")] + Io(#[from] io::Error), + #[error("Filesystem error: {0}")] + Filesystem(#[from] fs::FileSystemError), +} diff --git a/src/diagnostics/mod.rs b/src/diagnostics/mod.rs new file mode 100644 index 00000000..8b54d6f4 --- /dev/null +++ b/src/diagnostics/mod.rs @@ -0,0 +1,7 @@ +pub mod diagnostics; +pub mod error; +pub mod pipeline; + +pub use diagnostics::*; +pub use error::DiagnosticError; +pub use pipeline::{DiagnosticPipeline, FileChecker, FileProcessor, Result}; diff --git a/src/diagnostics/pipeline.rs b/src/diagnostics/pipeline.rs new file mode 100644 index 00000000..221a202c --- /dev/null +++ b/src/diagnostics/pipeline.rs @@ -0,0 +1,48 @@ +use std::path::Path; + +use super::diagnostics::Diagnostic; +use super::error::DiagnosticError; + +pub type Result = std::result::Result; + +// Turn input into diagnostics +pub trait DiagnosticPipeline<'a, P> { + type Output: IntoIterator; + + fn diagnostics(&'a self, input: P) -> Result; +} + +// Turn filepaths into ProcessedFile (references, imports, etc.) +pub trait FileProcessor<'a, P> +where + P: AsRef, +{ + type ProcessedFile; + + fn process(&'a self, file_path: P) -> Result; +} + +// Turn ProcessedFile into diagnostics +pub trait FileChecker<'a> { + type ProcessedFile; + type Output: IntoIterator; + + fn check(&'a self, processed_file: &Self::ProcessedFile) -> Result; +} + +// If you can turn a filepath into ProcessedFile, and then turn that ProcessedFile into diagnostics, +// then you can turn a filepath into diagnostics. +impl<'a, T, P> DiagnosticPipeline<'a, P> for T +where + P: AsRef, + T: FileProcessor<'a, P> + FileChecker<'a>, + >::ProcessedFile: AsRef<>::ProcessedFile>, +{ + type Output = >::Output; + + fn diagnostics(&'a self, file_path: P) -> Result { + let processed_file = self.process(file_path)?; + let diagnostics = self.check(processed_file.as_ref())?; + Ok(diagnostics) + } +} diff --git a/src/external/parsing.rs b/src/external/parsing.rs index 05b0b522..7770aa1e 100644 --- a/src/external/parsing.rs +++ b/src/external/parsing.rs @@ -30,7 +30,7 @@ pub fn extract_dependencies(toml_value: &Value) -> HashSet { let has_project_deps = toml_value .get("project") .and_then(|p| p.get("dependencies")) - .map_or(false, |deps| { + .is_some_and(|deps| { extract_deps_from_value(&mut dependencies, deps); true }); diff --git a/src/filesystem.rs b/src/filesystem.rs index 610ddd31..91f48829 100644 --- a/src/filesystem.rs +++ b/src/filesystem.rs @@ -260,6 +260,32 @@ fn is_pyfile_or_dir(entry: &DirEntry) -> bool { } } +#[derive(Debug)] +pub struct ProjectFile<'a> { + pub project_root: &'a Path, + pub source_root: &'a Path, + pub file_path: PathBuf, + pub relative_file_path: PathBuf, +} + +impl<'a> ProjectFile<'a> { + pub fn new(project_root: &'a Path, source_root: &'a Path, file_path: &'a Path) -> Self { + let absolute_file_path = source_root.join(file_path); + Self { + project_root, + source_root, + relative_file_path: relative_to(&absolute_file_path, project_root).unwrap(), + file_path: absolute_file_path, + } + } +} + +impl AsRef for ProjectFile<'_> { + fn as_ref(&self) -> &Path { + &self.file_path + } +} + pub fn walk_pyfiles(root: &str) -> impl Iterator { let prefix_root = root.to_string(); WalkDir::new(root) diff --git a/src/interfaces/check.rs b/src/interfaces/check.rs deleted file mode 100644 index f6dac130..00000000 --- a/src/interfaces/check.rs +++ /dev/null @@ -1,71 +0,0 @@ -use std::path::PathBuf; - -use super::compiled::CompiledInterfaces; -use super::data_types::{TypeCheckCache, TypeCheckResult}; -use super::error::InterfaceError; -use crate::config::{InterfaceConfig, ModuleConfig}; - -pub struct InterfaceChecker { - interfaces: CompiledInterfaces, - type_check_cache: Option, -} - -#[derive(Debug)] -pub enum CheckResult { - Exposed { type_check_result: TypeCheckResult }, - NotExposed, - NoInterfaces, - TopLevelModule, -} - -impl InterfaceChecker { - pub fn new(interfaces: &[InterfaceConfig]) -> Self { - let compiled = CompiledInterfaces::build(interfaces); - - Self { - interfaces: compiled, - type_check_cache: None, - } - } - - pub fn with_type_check_cache( - mut self, - modules: &[ModuleConfig], - source_roots: &[PathBuf], - ) -> Result { - let type_check_cache = TypeCheckCache::build(&self.interfaces, modules, source_roots)?; - self.type_check_cache = Some(type_check_cache); - Ok(self) - } - - pub fn check_member(&self, member: &str, module_path: &str) -> CheckResult { - if member.is_empty() { - return CheckResult::TopLevelModule; - } - - let matching_interfaces = self.interfaces.get_interfaces(module_path); - - if matching_interfaces.is_empty() { - return CheckResult::NoInterfaces; - } - - let mut is_exposed = false; - for interface in matching_interfaces { - if interface.expose.iter().any(|re| re.is_match(member)) { - is_exposed = true; - } - } - - if !is_exposed { - return CheckResult::NotExposed; - } - - CheckResult::Exposed { - type_check_result: self - .type_check_cache - .as_ref() - .map(|cache| cache.get_result(member)) - .unwrap_or(TypeCheckResult::Unknown), - } - } -} diff --git a/src/interfaces/compiled.rs b/src/interfaces/compiled.rs index c3f17179..f53eab62 100644 --- a/src/interfaces/compiled.rs +++ b/src/interfaces/compiled.rs @@ -29,10 +29,10 @@ pub struct CompiledInterfaces { interfaces: Vec, } -impl CompiledInterfaces { - pub fn build(interfaces: &[InterfaceConfig]) -> Self { +impl<'a> CompiledInterfaces { + pub fn build(interfaces: impl IntoIterator) -> Self { let compiled = interfaces - .iter() + .into_iter() .map(|interface| CompiledInterface { data_types: interface.data_types.clone(), from_modules: interface diff --git a/src/interfaces/data_types.rs b/src/interfaces/data_types.rs index e6af5a1b..f86ccc53 100644 --- a/src/interfaces/data_types.rs +++ b/src/interfaces/data_types.rs @@ -229,7 +229,7 @@ impl DataTypeChecker for InterfaceDataTypes { if parameters .iter() .all(|p| is_primitive_type(p.annotation.as_deref().unwrap_or(""))) - && return_type.as_ref().map_or(false, |t| is_primitive_type(t)) + && return_type.as_ref().is_some_and(|t| is_primitive_type(t)) { TypeCheckResult::MatchedInterface { expected: self.clone(), diff --git a/src/interfaces/mod.rs b/src/interfaces/mod.rs index b86d63f5..e7acd498 100644 --- a/src/interfaces/mod.rs +++ b/src/interfaces/mod.rs @@ -1,7 +1,5 @@ -pub mod check; pub mod compiled; pub mod data_types; pub mod error; -pub use check::InterfaceChecker; pub use data_types::TypeCheckCache; diff --git a/src/lib.rs b/src/lib.rs index d5cb649d..f705cbfa 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,4 +1,5 @@ pub mod cache; +pub mod checks; pub mod cli; pub mod colors; pub mod commands; @@ -7,7 +8,6 @@ pub mod diagnostics; pub mod exclusion; pub mod external; pub mod filesystem; -pub mod imports; pub mod interfaces; pub mod interrupt; pub mod lsp; @@ -15,6 +15,7 @@ pub mod modularity; pub mod modules; pub mod parsing; pub mod pattern; +pub mod processors; pub mod python; pub mod tests; @@ -33,10 +34,10 @@ mod errors { pyo3::import_exception!(tach.errors, TachSetupError); } -impl From for PyErr { - fn from(err: imports::ImportParseError) -> Self { +impl From for PyErr { + fn from(err: processors::imports::ImportParseError) -> Self { match err { - imports::ImportParseError::Parsing { file: _, source: _ } => { + processors::imports::ImportParseError::Parsing { file: _, source: _ } => { PySyntaxError::new_err(err.to_string()) } _ => PyOSError::new_err(err.to_string()), @@ -65,12 +66,6 @@ impl From for PyErr { } } -impl From for PyErr { - fn from(err: check::ExternalCheckError) -> Self { - PyOSError::new_err(err.to_string()) - } -} - impl From for PyErr { fn from(err: check::CheckError) -> Self { match err { @@ -171,17 +166,18 @@ fn dump_project_config_to_toml( parsing::config::dump_project_config_to_toml(config).map_err(sync::SyncError::TomlSerialize) } +/// Get first-party imports from file_path #[pyfunction] #[pyo3(signature = (source_roots, file_path, ignore_type_checking_imports=false, include_string_imports=false))] -fn get_normalized_imports( +fn get_project_imports( source_roots: Vec, file_path: String, ignore_type_checking_imports: bool, include_string_imports: bool, -) -> imports::Result> { +) -> processors::imports::Result> { let source_roots: Vec = source_roots.iter().map(PathBuf::from).collect(); let file_path = PathBuf::from(file_path); - Ok(imports::get_normalized_imports( + Ok(processors::imports::get_project_imports( &source_roots, &file_path, ignore_type_checking_imports, @@ -191,44 +187,25 @@ fn get_normalized_imports( .imports) } -/// Get first-party imports from file_path +/// Get third-party imports from file_path #[pyfunction] -#[pyo3(signature = (source_roots, file_path, ignore_type_checking_imports=false, include_string_imports=false))] -fn get_project_imports( +#[pyo3(signature = (source_roots, file_path, ignore_type_checking_imports=false))] +fn get_external_imports( source_roots: Vec, file_path: String, ignore_type_checking_imports: bool, - include_string_imports: bool, -) -> imports::Result> { +) -> processors::imports::Result> { let source_roots: Vec = source_roots.iter().map(PathBuf::from).collect(); let file_path = PathBuf::from(file_path); - Ok(imports::get_project_imports( + Ok(processors::imports::get_external_imports( &source_roots, &file_path, ignore_type_checking_imports, - include_string_imports, )? .into_active_imports() .imports) } -/// Get third-party imports from file_path -#[pyfunction] -#[pyo3(signature = (source_roots, file_path, ignore_type_checking_imports=false))] -fn get_external_imports( - source_roots: Vec, - file_path: String, - ignore_type_checking_imports: bool, -) -> imports::Result> { - let source_roots: Vec = source_roots.iter().map(PathBuf::from).collect(); - let file_path = PathBuf::from(file_path); - Ok( - imports::get_external_imports(&source_roots, &file_path, ignore_type_checking_imports)? - .into_active_imports() - .imports, - ) -} - /// Set excluded paths globally. /// This is called separately in order to set up a singleton instance holding regex/glob patterns, /// since they would be expensive to build for every call. @@ -408,7 +385,6 @@ fn extension(_py: Python, m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_function(wrap_pyfunction_bound!(parse_project_config, m)?)?; m.add_function(wrap_pyfunction_bound!(get_project_imports, m)?)?; m.add_function(wrap_pyfunction_bound!(get_external_imports, m)?)?; - m.add_function(wrap_pyfunction_bound!(get_normalized_imports, m)?)?; m.add_function(wrap_pyfunction_bound!(set_excluded_paths, m)?)?; m.add_function(wrap_pyfunction_bound!(check_external_dependencies, m)?)?; m.add_function(wrap_pyfunction_bound!(create_dependency_report, m)?)?; diff --git a/src/modules/tree.rs b/src/modules/tree.rs index d758bed7..684097a9 100644 --- a/src/modules/tree.rs +++ b/src/modules/tree.rs @@ -47,7 +47,7 @@ impl ModuleNode { } pub fn is_unchecked(&self) -> bool { - self.config.as_ref().map_or(false, |c| c.unchecked) + self.config.as_ref().is_some_and(|c| c.unchecked) } pub fn fill(&mut self, config: ModuleConfig, full_path: String) { diff --git a/src/processors/file_module.rs b/src/processors/file_module.rs new file mode 100644 index 00000000..13f86135 --- /dev/null +++ b/src/processors/file_module.rs @@ -0,0 +1,99 @@ +use std::marker::PhantomData; +use std::path::PathBuf; +use std::{path::Path, sync::Arc}; + +use crate::filesystem::ProjectFile; +use crate::{config::ModuleConfig, modules::ModuleNode}; + +use super::imports::{AllImports, ExternalImports, NormalizedImports, ProjectImports}; + +#[derive(Debug)] +pub struct FileModule<'a, State = AllImports> { + pub file: ProjectFile<'a>, + pub module: Arc, + pub imports: NormalizedImports, + _state: PhantomData, +} + +impl FileModule<'_, State> { + pub fn module_config(&self) -> &ModuleConfig { + self.module.config.as_ref().unwrap() + } + + pub fn relative_file_path(&self) -> &Path { + &self.file.relative_file_path + } +} + +impl<'a, State> AsRef> for FileModule<'a, State> { + fn as_ref(&self) -> &FileModule<'a, State> { + self + } +} + +impl<'a> FileModule<'a, AllImports> { + pub fn new( + file: ProjectFile<'a>, + module: Arc, + imports: NormalizedImports, + ) -> Self { + Self { + file, + module, + imports, + _state: PhantomData, + } + } + + pub fn into_internal(self, source_roots: &[PathBuf]) -> FileModule<'a, ProjectImports> { + FileModule { + file: self.file, + module: self.module, + imports: self.imports.into_project_imports(source_roots), + _state: PhantomData, + } + } + + pub fn into_external(self, source_roots: &[PathBuf]) -> FileModule<'a, ExternalImports> { + FileModule { + file: self.file, + module: self.module, + imports: self.imports.into_external_imports(source_roots), + _state: PhantomData, + } + } +} + +pub type FileModuleInternal<'a> = FileModule<'a, ProjectImports>; + +impl<'a> FileModuleInternal<'a> { + pub fn new( + file: ProjectFile<'a>, + module: Arc, + imports: NormalizedImports, + ) -> Self { + Self { + file, + module, + imports, + _state: PhantomData, + } + } +} + +pub type FileModuleExternal<'a> = FileModule<'a, ExternalImports>; + +impl<'a> FileModuleExternal<'a> { + pub fn new( + file: ProjectFile<'a>, + module: Arc, + imports: NormalizedImports, + ) -> Self { + Self { + file, + module, + imports, + _state: PhantomData, + } + } +} diff --git a/src/imports.rs b/src/processors/imports.rs similarity index 83% rename from src/imports.rs rename to src/processors/imports.rs index 9a0cfc4b..69e8a988 100644 --- a/src/imports.rs +++ b/src/processors/imports.rs @@ -1,5 +1,5 @@ use std::collections::hash_map::Entry; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::fmt::Debug; use std::iter; use std::marker::PhantomData; @@ -17,6 +17,8 @@ use ruff_python_ast::visitor::Visitor; use ruff_python_ast::{Expr, Mod, Stmt, StmtIf, StmtImport, StmtImportFrom}; use thiserror::Error; +use crate::diagnostics::Diagnostic; +use crate::external::parsing::normalize_package_name; use crate::python::{error::ParsingError, parsing::parse_python_source}; use crate::{exclusion, filesystem}; @@ -71,6 +73,14 @@ pub struct NormalizedImports { } impl NormalizedImports { + pub fn empty() -> Self { + Self { + imports: vec![], + ignore_directives: IgnoreDirectives::empty(), + _state: PhantomData, + } + } + pub fn new(imports: Vec, ignore_directives: IgnoreDirectives) -> Self { Self { imports, @@ -79,6 +89,14 @@ impl NormalizedImports { } } + pub fn all_imports(&self) -> impl Iterator { + self.imports.iter() + } + + pub fn into_imports(self) -> impl Iterator { + self.imports.into_iter() + } + pub fn active_imports(&self) -> impl Iterator { self.imports .iter() @@ -96,33 +114,6 @@ impl NormalizedImports { _state: PhantomData, } } - - pub fn directive_ignored_imports(&self) -> impl Iterator { - self.imports - .iter() - .filter(|&import| self.ignore_directives.is_ignored(import)) - .map(|import| DirectiveIgnoredImport { - import, - reason: self - .ignore_directives - .get(&import.import_line_no) - .unwrap() - .reason - .clone(), - }) - } - - pub fn unused_ignore_directives(&self) -> impl Iterator { - let mut directive_lines: HashSet = - HashSet::from_iter(self.ignore_directives.lines().cloned()); - self.imports.iter().for_each(|import| { - directive_lines.remove(&import.import_line_no); - }); - directive_lines - .into_iter() - .map(|line| self.ignore_directives.get(&line).unwrap()) - .chain(self.ignore_directives.redundant_directives()) - } } impl NormalizedImports { @@ -131,8 +122,7 @@ impl NormalizedImports { source_roots: &[PathBuf], ) -> (Vec, Vec) { self.imports.into_iter().partition(|normalized_import| { - is_project_import(source_roots, &normalized_import.module_path) - .map_or(false, |is_project| is_project) + is_project_import(source_roots, &normalized_import.module_path).unwrap_or(false) }) } @@ -175,6 +165,37 @@ impl NormalizedImports { } } +pub struct ExternalImportWithDistributionNames<'a> { + pub distribution_names: Vec, + pub import: &'a NormalizedImport, +} + +impl<'a> NormalizedImports { + pub fn all_imports_with_distribution_names( + &'a self, + module_mappings: &'a HashMap>, + ) -> impl Iterator> { + self.all_imports().map(|import| { + let top_level_module_name = import.top_level_module_name().to_string(); + let default_distribution_names = vec![top_level_module_name.clone()]; + let distribution_names: Vec = module_mappings + .get(&top_level_module_name) + .map(|dist_names| { + dist_names + .iter() + .map(|dist_name| normalize_package_name(dist_name)) + .collect() + }) + .unwrap_or(default_distribution_names); + + ExternalImportWithDistributionNames { + distribution_names, + import, + } + }) + } +} + impl Extend> for NormalizedImports { fn extend>>(&mut self, iter: T) { for normalized_imports in iter { @@ -195,7 +216,29 @@ impl IntoPy for NormalizedImport { pub struct IgnoreDirective { pub modules: Vec, pub reason: String, - pub line_no: usize, + pub line_no: usize, // Where is the directive literally written + pub ignored_line_no: usize, // Where is the directive being applied +} + +impl IgnoreDirective { + pub fn matches_diagnostic(&self, diagnostic: &Diagnostic) -> bool { + // If the diagnostic is not on the line that the directive is being applied, it is not a match + if Some(self.ignored_line_no) != diagnostic.line_number() { + return false; + } + + // If the directive is a blanket ignore, it matches any diagnostic + if self.modules.is_empty() { + return true; + } + + // If applicable, check if the diagnostic has specified a matching module path + diagnostic.import_mod_path().is_none_or(|import_mod_path| { + self.modules + .iter() + .any(|module| import_mod_path.ends_with(module)) + }) + } } #[derive(Debug, Default, Clone)] @@ -205,8 +248,19 @@ pub struct IgnoreDirectives { } impl IgnoreDirectives { - pub fn lines(&self) -> impl Iterator { - self.directives.keys() + pub fn empty() -> Self { + Self { + directives: HashMap::new(), + redundant_directives: Vec::new(), + } + } + + pub fn active_directives(&self) -> impl Iterator { + self.directives.values() + } + + pub fn redundant_directives(&self) -> impl Iterator { + self.redundant_directives.iter() } pub fn len(&self) -> usize { @@ -217,8 +271,8 @@ impl IgnoreDirectives { self.directives.is_empty() } - pub fn add_directive(&mut self, directive: IgnoreDirective, ignored_line_no: usize) { - match self.directives.entry(ignored_line_no) { + pub fn add_directive(&mut self, directive: IgnoreDirective) { + match self.directives.entry(directive.ignored_line_no) { Entry::Occupied(_) => { self.redundant_directives.push(directive); } @@ -235,7 +289,7 @@ impl IgnoreDirectives { pub fn is_ignored(&self, normalized_import: &NormalizedImport) -> bool { self.directives .get(&normalized_import.import_line_no) - .map_or(false, |directive| { + .is_some_and(|directive| { if normalized_import.is_absolute { directive.modules.is_empty() || directive.modules.contains(&normalized_import.module_path) @@ -254,10 +308,6 @@ impl IgnoreDirectives { self.redundant_directives .retain(|directive| directive.line_no != normalized_import.import_line_no); } - - pub fn redundant_directives(&self) -> impl Iterator { - self.redundant_directives.iter() - } } impl Extend for IgnoreDirectives { @@ -300,17 +350,18 @@ fn get_ignore_directives(file_content: &str) -> IgnoreDirectives { .collect() }; + let mut ignored_line_no = normal_lineno; + if line.trim_start().starts_with('#') { + ignored_line_no = normal_lineno + 1; + } let directive = IgnoreDirective { modules, reason, line_no: normal_lineno, + ignored_line_no, }; - if line.trim_start().starts_with('#') { - ignores.add_directive(directive, normal_lineno + 1); - } else { - ignores.add_directive(directive, normal_lineno); - } + ignores.add_directive(directive); } } @@ -524,29 +575,32 @@ pub fn is_project_import>(source_roots: &[P], mod_path: &str) -> let resolved_module = filesystem::module_to_file_path(source_roots, mod_path, true); if let Some(module) = resolved_module { // This appears to be a project import, verify it is not excluded - Ok(!exclusion::is_path_excluded(module.file_path)) + Ok(!exclusion::is_path_excluded(&module.file_path)) } else { // This is not a project import Ok(false) } } -pub fn get_normalized_imports( +pub fn get_normalized_imports>( source_roots: &[PathBuf], - file_path: &PathBuf, + file_path: P, ignore_type_checking_imports: bool, include_string_imports: bool, ) -> Result { - let file_contents = filesystem::read_file_content(file_path)?; + let file_contents = filesystem::read_file_content(file_path.as_ref())?; let file_ast = parse_python_source(&file_contents).map_err(|err| ImportParseError::Parsing { - file: file_path.to_str().unwrap().to_string(), + file: file_path.as_ref().to_string_lossy().to_string(), source: err, })?; - let is_package = file_path.ends_with("__init__.py"); + let is_package = file_path + .as_ref() + .to_string_lossy() + .ends_with("__init__.py"); let ignore_directives = get_ignore_directives(file_contents.as_str()); let file_mod_path: Option = - filesystem::file_to_module_path(source_roots, file_path).ok(); + filesystem::file_to_module_path(source_roots, file_path.as_ref()).ok(); let mut import_visitor = ImportVisitor::new( file_mod_path, Locator::new(&file_contents), @@ -583,28 +637,32 @@ pub fn get_normalized_imports( } } -pub fn get_project_imports( +pub fn get_project_imports>( source_roots: &[PathBuf], - file_path: &PathBuf, + file_path: P, ignore_type_checking_imports: bool, include_string_imports: bool, ) -> Result> { let normalized_imports = get_normalized_imports( source_roots, - file_path, + file_path.as_ref(), ignore_type_checking_imports, include_string_imports, )?; Ok(normalized_imports.into_project_imports(source_roots)) } -pub fn get_external_imports( +pub fn get_external_imports>( source_roots: &[PathBuf], - file_path: &PathBuf, + file_path: P, ignore_type_checking_imports: bool, ) -> Result> { - let normalized_imports = - get_normalized_imports(source_roots, file_path, ignore_type_checking_imports, false)?; + let normalized_imports = get_normalized_imports( + source_roots, + file_path.as_ref(), + ignore_type_checking_imports, + false, + )?; Ok(normalized_imports.into_external_imports(source_roots)) } diff --git a/src/processors/mod.rs b/src/processors/mod.rs new file mode 100644 index 00000000..8f0af943 --- /dev/null +++ b/src/processors/mod.rs @@ -0,0 +1,2 @@ +pub mod file_module; +pub mod imports; diff --git a/src/tests/check_internal.rs b/src/tests/check_internal.rs deleted file mode 100644 index 23a368ba..00000000 --- a/src/tests/check_internal.rs +++ /dev/null @@ -1,192 +0,0 @@ -#[cfg(test)] -pub mod fixtures { - use std::{collections::HashMap, path::PathBuf, sync::Arc}; - - use crate::config::{DependencyConfig, InterfaceConfig, InterfaceDataTypes, ModuleConfig}; - use crate::modules::{ModuleNode, ModuleTree}; - use rstest::fixture; - - #[fixture] - pub fn interface_config() -> Vec { - vec![InterfaceConfig { - expose: vec!["public_fn".to_string()], - from_modules: vec!["domain_one".to_string()], - data_types: InterfaceDataTypes::All, - }] - } - - #[fixture] - pub fn layers() -> Vec { - vec![ - "top".to_string(), - "middle".to_string(), - "bottom".to_string(), - ] - } - - #[fixture] - pub fn module_tree() -> ModuleTree { - ModuleTree { - root: Arc::new(ModuleNode { - is_end_of_path: false, - full_path: String::new(), - config: None, - children: HashMap::from([ - ( - "domain_one".to_string(), - Arc::new(ModuleNode { - is_end_of_path: true, - full_path: "domain_one".to_string(), - config: Some(ModuleConfig { - path: "domain_one".to_string(), - depends_on: Some(vec![ - DependencyConfig::from_path("domain_two"), - DependencyConfig::from_deprecated_path("domain_one.subdomain"), - ]), - strict: false, - layer: Some("top".to_string()), - ..Default::default() - }), - children: HashMap::from([( - "subdomain".to_string(), - Arc::new(ModuleNode { - is_end_of_path: true, - full_path: "domain_one.subdomain".to_string(), - config: Some(ModuleConfig::new_with_layer( - "domain_one.subdomain", - "top", - )), - children: HashMap::new(), - }), - )]), - }), - ), - ( - "domain_two".to_string(), - Arc::new(ModuleNode { - is_end_of_path: true, - full_path: "domain_two".to_string(), - config: Some(ModuleConfig::new_with_layer("domain_two", "top")), - children: HashMap::new(), - }), - ), - ( - "service_one".to_string(), - Arc::new(ModuleNode { - is_end_of_path: true, - full_path: "service_one".to_string(), - config: Some(ModuleConfig { - path: "service_one".to_string(), - depends_on: Some(vec![DependencyConfig::from_path( - "service_one.internal", - )]), - strict: false, - layer: Some("middle".to_string()), - ..Default::default() - }), - children: HashMap::from([( - "internal".to_string(), - Arc::new(ModuleNode { - is_end_of_path: true, - full_path: "service_one.internal".to_string(), - config: Some(ModuleConfig::new_with_layer( - "service_one.internal", - "middle", - )), - children: HashMap::new(), - }), - )]), - }), - ), - ( - "data_one".to_string(), - Arc::new(ModuleNode { - is_end_of_path: true, - full_path: "data_one".to_string(), - config: Some(ModuleConfig::new_with_layer("data_one", "bottom")), - children: HashMap::new(), - }), - ), - ( - "unrestricted".to_string(), - Arc::new(ModuleNode { - is_end_of_path: true, - full_path: "unrestricted".to_string(), - config: Some(ModuleConfig { - path: "unrestricted".to_string(), - depends_on: None, - strict: false, - layer: Some("middle".to_string()), - ..Default::default() - }), - children: HashMap::new(), - }), - ), - ]), - }), - } - } - - #[fixture] - pub fn module_config() -> Vec { - vec![ - ModuleConfig { - path: "domain_one".to_string(), - depends_on: Some(vec![ - DependencyConfig::from_path("domain_two"), - DependencyConfig::from_deprecated_path("domain_one.subdomain"), - ]), - strict: false, - layer: Some("top".to_string()), - ..Default::default() - }, - ModuleConfig { - path: "domain_one.subdomain".to_string(), - depends_on: Some(vec![]), - strict: false, - layer: Some("top".to_string()), - ..Default::default() - }, - ModuleConfig { - path: "domain_two".to_string(), - depends_on: Some(vec![]), - strict: false, - layer: Some("top".to_string()), - ..Default::default() - }, - ModuleConfig { - path: "service_one".to_string(), - depends_on: Some(vec![DependencyConfig::from_path("service_one.internal")]), - strict: false, - layer: Some("middle".to_string()), - ..Default::default() - }, - ModuleConfig { - path: "service_one.internal".to_string(), - depends_on: Some(vec![]), - strict: false, - layer: Some("middle".to_string()), - ..Default::default() - }, - ModuleConfig { - path: "data_one".to_string(), - depends_on: Some(vec![]), - strict: false, - layer: Some("bottom".to_string()), - ..Default::default() - }, - ModuleConfig { - path: "unrestricted".to_string(), - depends_on: None, - strict: false, - layer: Some("middle".to_string()), - ..Default::default() - }, - ] - } - - #[fixture] - pub fn source_roots() -> Vec { - vec![PathBuf::from("src")] - } -} diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 459ef9df..c5301d41 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -1,4 +1,3 @@ -pub mod check_internal; pub mod module; pub mod test; #[cfg(test)]