Skip to content

Commit

Permalink
Merge pull request #447 from gauge-sh/require-ignore-directive-reasons
Browse files Browse the repository at this point in the history 
Add rules config to require ignore directives have reasons attached
  • Loading branch information
@emdoyle
emdoyle authored Dec 1, 2024
2 parents 6c94690 + fb7af6a commit 4608c13
Show file tree
Hide file tree
Showing 5 changed files with 146 additions and 67 deletions.
1 change: 1 addition & 0 deletions python/tach/extension.pyi
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ RuleSetting = Literal["error", "warn", "off"]

class RulesConfig:
unused_ignore_directives: RuleSetting
require_ignore_directive_reasons: RuleSetting

class ProjectConfig:
modules: list[ModuleConfig]
Expand Down
83 changes: 59 additions & 24 deletions src/check_int.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ pub enum ImportCheckError {
#[error("Import '{import_mod_path}' is unnecessarily ignored by a directive.")]
UnusedIgnoreDirective { import_mod_path: String },

#[error("Import '{import_mod_path}' is ignored without providing a reason.")]
MissingIgnoreDirectiveReason { import_mod_path: String },

#[error("No checks enabled. At least one of dependencies or interfaces must be enabled.")]
NoChecksEnabled(),
}
Expand Down Expand Up @@ -380,44 +383,76 @@ pub fn check(
boundary_errors.push(boundary_error);
}
}

if project_config.rules.unused_ignore_directives == RuleSetting::Off {
// Skip directive-related checks if both rules are off
if project_config.rules.unused_ignore_directives == RuleSetting::Off
&& project_config.rules.require_ignore_directive_reasons == RuleSetting::Off
{
continue;
}

for directive_ignored_import in project_imports.directive_ignored_imports {
if check_import(
&directive_ignored_import.module_path,
&module_tree,
Arc::clone(&nearest_module),
project_config.root_module.clone(),
&interface_checker,
dependencies,
interfaces,
)
.is_ok()
// Check for missing ignore directive reasons
if project_config.rules.require_ignore_directive_reasons != RuleSetting::Off
&& directive_ignored_import.reason.is_empty()
{
let message = format!(
"Import '{}' is unnecessarily ignored by a directive.",
directive_ignored_import.module_path
);
match project_config.rules.unused_ignore_directives {
RuleSetting::Error => {
let error = BoundaryError {
file_path: file_path.clone(),
line_number: directive_ignored_import.import.line_no,
import_mod_path: directive_ignored_import.import.module_path.to_string(),
error_info: ImportCheckError::MissingIgnoreDirectiveReason {
import_mod_path: directive_ignored_import
.import
.module_path
.to_string(),
},
};
if project_config.rules.require_ignore_directive_reasons == RuleSetting::Error {
boundary_errors.push(error);
} else {
warnings.push(format!(
"Import '{}' is ignored without providing a reason",
directive_ignored_import.import.module_path
));
}
}

// Check for unnecessary ignore directives
if project_config.rules.unused_ignore_directives != RuleSetting::Off {
let is_unnecessary = check_import(
&directive_ignored_import.import.module_path,
&module_tree,
Arc::clone(&nearest_module),
project_config.root_module.clone(),
&interface_checker,
dependencies,
interfaces,
)
.is_ok();

if is_unnecessary {
let message = format!(
"Import '{}' is unnecessarily ignored by a directive.",
directive_ignored_import.import.module_path
);

if project_config.rules.unused_ignore_directives == RuleSetting::Error {
boundary_errors.push(BoundaryError {
file_path: file_path.clone(),
line_number: directive_ignored_import.line_no,
import_mod_path: directive_ignored_import.module_path.to_string(),
line_number: directive_ignored_import.import.line_no,
import_mod_path: directive_ignored_import
.import
.module_path
.to_string(),
error_info: ImportCheckError::UnusedIgnoreDirective {
import_mod_path: directive_ignored_import
.import
.module_path
.to_string(),
},
});
}
RuleSetting::Warn => {
} else {
warnings.push(message);
}
// Should never be reached
RuleSetting::Off => {}
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/core/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,12 +291,18 @@ pub struct RulesConfig {
skip_serializing_if = "RuleSetting::is_warn"
)]
pub unused_ignore_directives: RuleSetting,
#[serde(
default = "RuleSetting::off",
skip_serializing_if = "RuleSetting::is_off"
)]
pub require_ignore_directive_reasons: RuleSetting,
}

impl Default for RulesConfig {
fn default() -> Self {
Self {
unused_ignore_directives: RuleSetting::warn(),
require_ignore_directive_reasons: RuleSetting::off(),
}
}
}
Expand Down
122 changes: 79 additions & 43 deletions src/imports.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,16 @@ impl NormalizedImport {
}
}

#[derive(Debug)]
pub struct DirectiveIgnoredImport {
pub import: NormalizedImport,
pub reason: String,
}

#[derive(Debug, Default)]
pub struct NormalizedImports {
pub imports: Vec<NormalizedImport>,
pub directive_ignored_imports: Vec<NormalizedImport>,
pub directive_ignored_imports: Vec<DirectiveIgnoredImport>,
}

impl NormalizedImports {
Expand All @@ -69,18 +75,32 @@ impl IntoPy<PyObject> for NormalizedImport {
}
}

pub type IgnoreDirectives = HashMap<usize, Vec<String>>;
#[derive(Debug)]
pub struct ProjectImports {
pub imports: Vec<NormalizedImport>,
pub directive_ignored_imports: Vec<DirectiveIgnoredImport>,
}

pub struct IgnoreDirective {
modules: Vec<String>,
reason: String,
}

pub type IgnoreDirectives = HashMap<usize, IgnoreDirective>;

static TACH_IGNORE_REGEX: Lazy<regex::Regex> =
Lazy::new(|| Regex::new(r"# *tach-ignore(?:\([^)]*\))?((?:\s+[\w.]+)*)\s*$").unwrap());
Lazy::new(|| Regex::new(r"# *tach-ignore(?:\(([^)]*)\))?((?:\s+[\w.]+)*)\s*$").unwrap());

fn get_ignore_directives(file_content: &str) -> IgnoreDirectives {
let mut ignores: IgnoreDirectives = HashMap::new();

for (lineno, line) in file_content.lines().enumerate() {
let normal_lineno = lineno + 1;
if let Some(captures) = TACH_IGNORE_REGEX.captures(line) {
let ignored_modules = captures.get(1).map_or("", |m| m.as_str());
let reason = captures
.get(1)
.map_or("".to_string(), |m| m.as_str().to_string());
let ignored_modules = captures.get(2).map_or("", |m| m.as_str());
let modules: Vec<String> = if ignored_modules.is_empty() {
Vec::new()
} else {
Expand All @@ -90,10 +110,12 @@ fn get_ignore_directives(file_content: &str) -> IgnoreDirectives {
.collect()
};

let directive = IgnoreDirective { modules, reason };

if line.starts_with('#') {
ignores.insert(normal_lineno + 1, modules);
ignores.insert(normal_lineno + 1, directive);
} else {
ignores.insert(normal_lineno, modules);
ignores.insert(normal_lineno, directive);
}
}
}
Expand Down Expand Up @@ -134,21 +156,27 @@ impl<'a> ImportVisitor<'a> {
.locator
.compute_line_index(import_statement.range.start())
.get();
let ignored_modules: Option<&Vec<String>> = self.ignore_directives.get(&line_no);
let ignored_directive: Option<&IgnoreDirective> = self.ignore_directives.get(&line_no);

if let Some(ignored) = ignored_modules {
if ignored.is_empty() {
if let Some(ignored) = ignored_directive {
if ignored.modules.is_empty() {
// Blanket ignore of current import - add all to directive_ignored_imports
normalized_imports.directive_ignored_imports.extend(
import_statement.names.iter().map(|alias| NormalizedImport {
module_path: alias.name.to_string(),
line_no: self
.locator
.compute_line_index(alias.range.start())
.get()
.try_into()
.unwrap(),
}),
import_statement
.names
.iter()
.map(|alias| DirectiveIgnoredImport {
import: NormalizedImport {
module_path: alias.name.to_string(),
line_no: self
.locator
.compute_line_index(alias.range.start())
.get()
.try_into()
.unwrap(),
},
reason: ignored.reason.clone(),
}),
);
return normalized_imports;
}
Expand All @@ -165,9 +193,14 @@ impl<'a> ImportVisitor<'a> {
.unwrap(),
};

if let Some(ignored) = ignored_modules {
if ignored.contains(alias.name.as_ref()) {
normalized_imports.directive_ignored_imports.push(import);
if let Some(ignored) = ignored_directive {
if ignored.modules.contains(alias.name.as_ref()) {
normalized_imports
.directive_ignored_imports
.push(DirectiveIgnoredImport {
import,
reason: ignored.reason.clone(),
});
} else {
normalized_imports.imports.push(import);
}
Expand Down Expand Up @@ -235,23 +268,26 @@ impl<'a> ImportVisitor<'a> {
.locator
.compute_line_index(import_statement.range.start())
.get();
let ignored_modules: Option<&Vec<String>> = self.ignore_directives.get(&line_no);
let ignored_directive: Option<&IgnoreDirective> = self.ignore_directives.get(&line_no);

if let Some(ignored) = ignored_modules {
if ignored.is_empty() {
if let Some(ignored) = ignored_directive {
if ignored.modules.is_empty() {
// Blanket ignore - add all imports to directive_ignored_imports
for name in &import_statement.names {
let global_mod_path = format!("{}.{}", base_mod_path, name.name.as_str());
normalized_imports
.directive_ignored_imports
.push(NormalizedImport {
module_path: global_mod_path,
line_no: self
.locator
.compute_line_index(name.range.start())
.get()
.try_into()
.unwrap(),
.push(DirectiveIgnoredImport {
import: NormalizedImport {
module_path: global_mod_path,
line_no: self
.locator
.compute_line_index(name.range.start())
.get()
.try_into()
.unwrap(),
},
reason: ignored.reason.clone(),
});
}
return normalized_imports;
Expand All @@ -270,15 +306,20 @@ impl<'a> ImportVisitor<'a> {
.unwrap(),
};

if let Some(ignored) = ignored_modules {
if ignored.contains(
if let Some(ignored) = ignored_directive {
if ignored.modules.contains(
&name
.asname
.as_deref()
.unwrap_or(name.name.as_ref())
.to_string(),
) {
normalized_imports.directive_ignored_imports.push(import);
normalized_imports
.directive_ignored_imports
.push(DirectiveIgnoredImport {
import,
reason: ignored.reason.clone(),
});
} else {
normalized_imports.imports.push(import);
}
Expand Down Expand Up @@ -430,11 +471,6 @@ pub fn get_normalized_imports(
}
}

pub struct ProjectImports {
pub imports: Vec<NormalizedImport>,
pub directive_ignored_imports: Vec<NormalizedImport>,
}

pub fn get_project_imports(
source_roots: &[PathBuf],
file_path: &PathBuf,
Expand All @@ -461,10 +497,10 @@ pub fn get_project_imports(
directive_ignored_imports: normalized_imports
.directive_ignored_imports
.into_iter()
.filter_map(|normalized_import| {
is_project_import(source_roots, &normalized_import.module_path)
.filter_map(|directive_ignored_import| {
is_project_import(source_roots, &directive_ignored_import.import.module_path)
.map_or(None, |is_project_import| {
is_project_import.then_some(normalized_import)
is_project_import.then_some(directive_ignored_import)
})
})
.collect(),
Expand Down
1 change: 1 addition & 0 deletions tach.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,3 +333,4 @@ exclude = [

[rules]
unused_ignore_directives = "error"
require_ignore_directive_reasons = "error"

0 comments on commit 4608c13

Please sign in to comment.