From 732f208e47a556a1feddceec59e05f238621f46b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 21 Sep 2022 19:56:38 -0400 Subject: [PATCH] Add a lint rule to enforce noqa validity (#253) --- README.md | 1 + resources/test/fixtures/M001.py | 16 +++ src/check_lines.rs | 62 +++++++++- src/checks.rs | 113 +++++++++++++++---- src/linter.rs | 12 ++ src/settings.rs | 4 +- src/snapshots/ruff__linter__tests__m001.snap | 23 ++++ 7 files changed, 199 insertions(+), 32 deletions(-) create mode 100644 resources/test/fixtures/M001.py create mode 100644 src/snapshots/ruff__linter__tests__m001.snap diff --git a/README.md b/README.md index 16cbaa57bc..e60ff5fb63 100644 --- a/README.md +++ b/README.md @@ -209,6 +209,7 @@ Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis F | F901 | RaiseNotImplemented | `raise NotImplemented` should be `raise NotImplementedError` | | R001 | UselessObjectInheritance | Class `...` inherits from object | | R002 | NoAssertEquals | `assertEquals` is deprecated, use `assertEqual` instead | +| M001 | UnusedNOQA | Unused `noqa` directive | ## Development diff --git a/resources/test/fixtures/M001.py b/resources/test/fixtures/M001.py new file mode 100644 index 0000000000..5b83cdf596 --- /dev/null +++ b/resources/test/fixtures/M001.py @@ -0,0 +1,16 @@ +def f() -> None: + # Valid + a = 1 # noqa + + # Valid + b = 2 # noqa: F841 + + # Invalid + c = 1 # noqa + print(c) + + # Invalid + d = 1 # noqa: E501 + + # Invalid + d = 1 # noqa: F841, E501 diff --git a/src/check_lines.rs b/src/check_lines.rs index c3cfa046a6..9d209f2fdf 100644 --- a/src/check_lines.rs +++ b/src/check_lines.rs @@ -1,6 +1,7 @@ +use once_cell::sync::Lazy; use rustpython_parser::ast::Location; -use crate::checks::{Check, CheckCode, CheckKind}; +use crate::checks::{extract_noqa_directive, Check, CheckCode, CheckKind, Directive}; use crate::settings::Settings; /// Whether the given line is too long and should be reported. @@ -21,15 +22,31 @@ fn should_enforce_line_length(line: &str, length: usize, limit: usize) -> bool { pub fn check_lines(checks: &mut Vec, contents: &str, settings: &Settings) { let enforce_line_too_long = settings.select.contains(&CheckCode::E501); + let enforce_noqa = settings.select.contains(&CheckCode::M001); let mut line_checks = vec![]; let mut ignored = vec![]; for (row, line) in contents.lines().enumerate() { + let noqa_directive = Lazy::new(|| extract_noqa_directive(line)); + let mut line_ignored: Vec<&str> = vec![]; + // Remove any ignored checks. // TODO(charlie): Only validate checks for the current line. for (index, check) in checks.iter().enumerate() { - if check.location.row() == row + 1 && check.is_inline_ignored(line) { - ignored.push(index); + if check.location.row() == row + 1 { + match &*noqa_directive { + Directive::All(_) => { + line_ignored.push(check.kind.code().as_str()); + ignored.push(index) + } + Directive::Codes(_, codes) => { + if codes.contains(&check.kind.code().as_str()) { + line_ignored.push(check.kind.code().as_str()); + ignored.push(index); + } + } + Directive::None => {} + } } } @@ -41,11 +58,46 @@ pub fn check_lines(checks: &mut Vec, contents: &str, settings: &Settings) CheckKind::LineTooLong(line_length, settings.line_length), Location::new(row + 1, settings.line_length + 1), ); - if !check.is_inline_ignored(line) { - line_checks.push(check); + match &*noqa_directive { + Directive::All(_) => { + line_ignored.push(check.kind.code().as_str()); + } + Directive::Codes(_, codes) => { + if codes.contains(&check.kind.code().as_str()) { + line_ignored.push(check.kind.code().as_str()); + } else { + line_checks.push(check); + } + } + Directive::None => line_checks.push(check), } } } + + // Enforce that the noqa was actually used. + if enforce_noqa { + match &*noqa_directive { + Directive::All(column) => { + if line_ignored.is_empty() { + line_checks.push(Check::new( + CheckKind::UnusedNOQA(None), + Location::new(row + 1, column + 1), + )); + } + } + Directive::Codes(column, codes) => { + for code in codes { + if !line_ignored.contains(code) { + line_checks.push(Check::new( + CheckKind::UnusedNOQA(Some(code.to_string())), + Location::new(row + 1, column + 1), + )); + } + } + } + Directive::None => {} + } + } } ignored.sort(); for index in ignored.iter().rev() { diff --git a/src/checks.rs b/src/checks.rs index ea985d03d7..35cf842791 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -6,7 +6,7 @@ use regex::Regex; use rustpython_parser::ast::Location; use serde::{Deserialize, Serialize}; -pub const ALL_CHECK_CODES: [CheckCode; 44] = [ +pub const DEFAULT_CHECK_CODES: [CheckCode; 42] = [ CheckCode::E402, CheckCode::E501, CheckCode::E711, @@ -49,6 +49,52 @@ pub const ALL_CHECK_CODES: [CheckCode; 44] = [ CheckCode::F831, CheckCode::F841, CheckCode::F901, +]; + +pub const ALL_CHECK_CODES: [CheckCode; 45] = [ + CheckCode::E402, + CheckCode::E501, + CheckCode::E711, + CheckCode::E712, + CheckCode::E713, + CheckCode::E714, + CheckCode::E721, + CheckCode::E722, + CheckCode::E731, + CheckCode::E741, + CheckCode::E742, + CheckCode::E743, + CheckCode::E902, + CheckCode::E999, + CheckCode::F401, + CheckCode::F402, + CheckCode::F403, + CheckCode::F404, + CheckCode::F405, + CheckCode::F406, + CheckCode::F407, + CheckCode::F541, + CheckCode::F601, + CheckCode::F602, + CheckCode::F621, + CheckCode::F622, + CheckCode::F631, + CheckCode::F632, + CheckCode::F633, + CheckCode::F634, + CheckCode::F701, + CheckCode::F702, + CheckCode::F704, + CheckCode::F706, + CheckCode::F707, + CheckCode::F722, + CheckCode::F821, + CheckCode::F822, + CheckCode::F823, + CheckCode::F831, + CheckCode::F841, + CheckCode::F901, + CheckCode::M001, CheckCode::R001, CheckCode::R002, ]; @@ -99,6 +145,7 @@ pub enum CheckCode { F901, R001, R002, + M001, } impl FromStr for CheckCode { @@ -150,6 +197,7 @@ impl FromStr for CheckCode { "F901" => Ok(CheckCode::F901), "R001" => Ok(CheckCode::R001), "R002" => Ok(CheckCode::R002), + "M001" => Ok(CheckCode::M001), _ => Err(anyhow::anyhow!("Unknown check code: {s}")), } } @@ -202,13 +250,14 @@ impl CheckCode { CheckCode::F901 => "F901", CheckCode::R001 => "R001", CheckCode::R002 => "R002", + CheckCode::M001 => "M001", } } /// The source for the check (either the AST, the filesystem, or the physical lines). pub fn lint_source(&self) -> &'static LintSource { match self { - CheckCode::E501 => &LintSource::Lines, + CheckCode::E501 | CheckCode::M001 => &LintSource::Lines, CheckCode::E902 | CheckCode::E999 => &LintSource::FileSystem, _ => &LintSource::AST, } @@ -259,6 +308,7 @@ impl CheckCode { CheckCode::F831 => CheckKind::DuplicateArgumentName, CheckCode::F841 => CheckKind::UnusedVariable("...".to_string()), CheckCode::F901 => CheckKind::RaiseNotImplemented, + CheckCode::M001 => CheckKind::UnusedNOQA(None), CheckCode::R001 => CheckKind::UselessObjectInheritance("...".to_string()), CheckCode::R002 => CheckKind::NoAssertEquals, } @@ -280,6 +330,7 @@ pub enum RejectedCmpop { #[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] pub enum CheckKind { + UnusedNOQA(Option), AmbiguousClassName(String), AmbiguousFunctionName(String), AmbiguousVariableName(String), @@ -376,6 +427,7 @@ impl CheckKind { CheckKind::UnusedVariable(_) => "UnusedVariable", CheckKind::UselessObjectInheritance(_) => "UselessObjectInheritance", CheckKind::YieldOutsideFunction => "YieldOutsideFunction", + CheckKind::UnusedNOQA(_) => "UnusedNOQA", } } @@ -399,8 +451,8 @@ impl CheckKind { CheckKind::IfTuple => &CheckCode::F634, CheckKind::ImportShadowedByLoopVar(_, _) => &CheckCode::F402, CheckKind::ImportStarNotPermitted(_) => &CheckCode::F406, - CheckKind::ImportStarUsed(_) => &CheckCode::F403, CheckKind::ImportStarUsage(_, _) => &CheckCode::F405, + CheckKind::ImportStarUsed(_) => &CheckCode::F403, CheckKind::InvalidPrintSyntax => &CheckCode::F633, CheckKind::IsLiteral => &CheckCode::F632, CheckKind::LateFutureImport => &CheckCode::F404, @@ -423,6 +475,7 @@ impl CheckKind { CheckKind::UndefinedLocal(_) => &CheckCode::F823, CheckKind::UndefinedName(_) => &CheckCode::F821, CheckKind::UnusedImport(_) => &CheckCode::F401, + CheckKind::UnusedNOQA(_) => &CheckCode::M001, CheckKind::UnusedVariable(_) => &CheckCode::F841, CheckKind::UselessObjectInheritance(_) => &CheckCode::R001, CheckKind::YieldOutsideFunction => &CheckCode::F704, @@ -556,6 +609,10 @@ impl CheckKind { CheckKind::YieldOutsideFunction => { "a `yield` or `yield from` statement outside of a function/method".to_string() } + CheckKind::UnusedNOQA(code) => match code { + None => "Unused `noqa` directive".to_string(), + Some(code) => format!("Unused `noqa` directive for {code}"), + }, } } @@ -584,10 +641,37 @@ pub struct Check { } static NO_QA_REGEX: Lazy = Lazy::new(|| { - Regex::new(r"(?i)# noqa(?::\s?(?P([A-Z]+[0-9]+(?:[,\s]+)?)+))?").expect("Invalid regex") + Regex::new(r"(?i)(?P# noqa(?::\s?(?P([A-Z]+[0-9]+(?:[,\s]+)?)+))?)") + .expect("Invalid regex") }); static SPLIT_COMMA_REGEX: Lazy = Lazy::new(|| Regex::new(r"[,\s]").expect("Invalid regex")); +pub enum Directive<'a> { + None, + All(usize), + Codes(usize, Vec<&'a str>), +} + +pub fn extract_noqa_directive(line: &str) -> Directive { + match NO_QA_REGEX.captures(line) { + Some(caps) => match caps.name("noqa") { + Some(noqa) => match caps.name("codes") { + Some(codes) => Directive::Codes( + noqa.start(), + SPLIT_COMMA_REGEX + .split(codes.as_str()) + .map(|code| code.trim()) + .filter(|code| !code.is_empty()) + .collect(), + ), + None => Directive::All(noqa.start()), + }, + None => Directive::None, + }, + None => Directive::None, + } +} + impl Check { pub fn new(kind: CheckKind, location: Location) -> Self { Self { @@ -600,25 +684,4 @@ impl Check { pub fn amend(&mut self, fix: Fix) { self.fix = Some(fix); } - - pub fn is_inline_ignored(&self, line: &str) -> bool { - match NO_QA_REGEX.captures(line) { - Some(caps) => match caps.name("codes") { - Some(codes) => { - for code in SPLIT_COMMA_REGEX - .split(codes.as_str()) - .map(|code| code.trim()) - .filter(|code| !code.is_empty()) - { - if code == self.kind.code().as_str() { - return true; - } - } - false - } - None => true, - }, - None => false, - } - } } diff --git a/src/linter.rs b/src/linter.rs index 5b51a5bd6a..04e853212e 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -578,6 +578,18 @@ mod tests { Ok(()) } + #[test] + fn m001() -> Result<()> { + let mut checks = check_path( + Path::new("./resources/test/fixtures/M001.py"), + &settings::Settings::for_rules(vec![CheckCode::M001, CheckCode::F841]), + &fixer::Mode::Generate, + )?; + checks.sort_by_key(|check| check.location); + insta::assert_yaml_snapshot!(checks); + Ok(()) + } + #[test] fn r001() -> Result<()> { let mut checks = check_path( diff --git a/src/settings.rs b/src/settings.rs index 37cf5112a4..0fbd699635 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -5,7 +5,7 @@ use std::path::{Path, PathBuf}; use glob::Pattern; use once_cell::sync::Lazy; -use crate::checks::{CheckCode, ALL_CHECK_CODES}; +use crate::checks::{CheckCode, DEFAULT_CHECK_CODES}; use crate::fs; use crate::pyproject::load_config; @@ -127,7 +127,7 @@ impl Settings { select: if let Some(select) = config.select { BTreeSet::from_iter(select) } else { - BTreeSet::from_iter(ALL_CHECK_CODES) + BTreeSet::from_iter(DEFAULT_CHECK_CODES) }, pyproject, project_root, diff --git a/src/snapshots/ruff__linter__tests__m001.snap b/src/snapshots/ruff__linter__tests__m001.snap new file mode 100644 index 0000000000..20b6c095f4 --- /dev/null +++ b/src/snapshots/ruff__linter__tests__m001.snap @@ -0,0 +1,23 @@ +--- +source: src/linter.rs +expression: checks +--- +- kind: + UnusedNOQA: ~ + location: + row: 9 + column: 12 + fix: ~ +- kind: + UnusedNOQA: E501 + location: + row: 13 + column: 12 + fix: ~ +- kind: + UnusedNOQA: E501 + location: + row: 16 + column: 12 + fix: ~ +