Add a lint rule to enforce noqa validity (#253)

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
 

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

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<Check>, 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<Check>, 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() {

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<String>),
     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<Regex> = Lazy::new(|| {
-    Regex::new(r"(?i)# noqa(?::\s?(?P<codes>([A-Z]+[0-9]+(?:[,\s]+)?)+))?").expect("Invalid regex")
+    Regex::new(r"(?i)(?P<noqa># noqa(?::\s?(?P<codes>([A-Z]+[0-9]+(?:[,\s]+)?)+))?)")
+        .expect("Invalid regex")
 });
 static SPLIT_COMMA_REGEX: Lazy<Regex> = 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,
-        }
-    }
 }

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(

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,

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: ~
+