Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize checks #586

Merged
merged 10 commits into from
Feb 5, 2025
15 changes: 15 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down
6 changes: 0 additions & 6 deletions python/tach/extension.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -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: ...
Expand Down
25 changes: 24 additions & 1 deletion python/tests/test_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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
83 changes: 83 additions & 0 deletions src/checks/external_dependency.rs
Original file line number Diff line number Diff line change
@@ -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<String, Vec<String>>,
stdlib_modules: &'a HashSet<String>,
excluded_external_modules: &'a HashSet<String>,
}

impl<'a> ExternalDependencyChecker<'a> {
pub fn new(
project_info: &'a ProjectInfo,
module_mappings: &'a HashMap<String, Vec<String>>,
stdlib_modules: &'a HashSet<String>,
excluded_external_modules: &'a HashSet<String>,
) -> Self {
Self {
project_info,
module_mappings,
stdlib_modules,
excluded_external_modules,
}
}

fn check_import(
&'a self,
import: ExternalImportWithDistributionNames<'a>,
processed_file: &FileModuleExternal<'a>,
) -> Option<Diagnostic> {
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<Diagnostic>;

fn check(&'a self, processed_file: &Self::ProcessedFile) -> DiagnosticResult<Self::Output> {
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)
}
}
138 changes: 138 additions & 0 deletions src/checks/ignore_directive.rs
Original file line number Diff line number Diff line change
@@ -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<Diagnostic>,
relative_file_path: &Path,
) -> Option<Diagnostic> {
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<Diagnostic> {
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<Diagnostic>,
relative_file_path: &Path,
) -> Vec<Diagnostic> {
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<Diagnostic>,
relative_file_path: &Path,
) -> Vec<Diagnostic> {
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<Diagnostic>,
) {
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<Diagnostic>,
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);
}
}
Loading