Refactor out common exemption-parsing logic (#3670)

crates/ruff/src/checkers/noqa.rs

@@ -1,6 +1,5 @@
 //! `NoQA` enforcement and validation.
 
-use log::warn;
 use nohash_hasher::IntMap;
 use rustpython_parser::ast::Location;
 
@@ -10,7 +9,7 @@ use ruff_python_ast::types::Range;
 
 use crate::codes::NoqaCode;
 use crate::noqa;
-use crate::noqa::{extract_file_exemption, Directive, Exemption};
+use crate::noqa::{Directive, FileExemption};
 use crate::registry::{AsRule, Rule};
 use crate::rule_redirects::get_redirect_target;
 use crate::rules::ruff::rules::{UnusedCodes, UnusedNOQA};
@@ -26,63 +25,47 @@ pub fn check_noqa(
 ) -> Vec<usize> {
     let enforce_noqa = settings.rules.enabled(Rule::UnusedNOQA);
 
-    // Whether the file is exempted from all checks.
+    let lines: Vec<&str> = contents.universal_newlines().collect();
-    let mut file_exempted = false;
 
-    // Codes that are globally exempted (within the current file).
+    // Identify any codes that are globally exempted (within the current file).
-    let mut file_exemptions: Vec<NoqaCode> = vec![];
+    let exemption = noqa::file_exemption(&lines, commented_lines);
 
     // Map from line number to `noqa` directive on that line, along with any codes
     // that were matched by the directive.
     let mut noqa_directives: IntMap<usize, (Directive, Vec<NoqaCode>)> = IntMap::default();
 
-    // Indices of diagnostics that were ignored by a `noqa` directive.
+    // Extract all `noqa` directives.
-    let mut ignored_diagnostics = vec![];
+    if enforce_noqa {
-
+        for lineno in commented_lines {
-    let lines: Vec<&str> = contents.universal_newlines().collect();
-    for lineno in commented_lines {
-        match extract_file_exemption(lines[lineno - 1]) {
-            Exemption::All => {
-                file_exempted = true;
-            }
-            Exemption::Codes(codes) => {
-                file_exemptions.extend(codes.into_iter().filter_map(|code| {
-                    if let Ok(rule) = Rule::from_code(get_redirect_target(code).unwrap_or(code)) {
-                        Some(rule.noqa_code())
-                    } else {
-                        warn!("Invalid code provided to `# ruff: noqa`: {}", code);
-                        None
-                    }
-                }));
-            }
-            Exemption::None => {}
-        }
-
-        if enforce_noqa {
             noqa_directives
                 .entry(lineno - 1)
                 .or_insert_with(|| (noqa::extract_noqa_directive(lines[lineno - 1]), vec![]));
         }
     }
 
+    // Indices of diagnostics that were ignored by a `noqa` directive.
+    let mut ignored_diagnostics = vec![];
+
     // Remove any ignored diagnostics.
     for (index, diagnostic) in diagnostics.iter().enumerate() {
         if matches!(diagnostic.kind.rule(), Rule::BlanketNOQA) {
             continue;
         }
 
-        // If the file is exempted, ignore all diagnostics.
+        match &exemption {
-        if file_exempted {
+            FileExemption::All => {
-            ignored_diagnostics.push(index);
+                // If the file is exempted, ignore all diagnostics.
-            continue;
-        }
-
-        // If the diagnostic is ignored by a global exemption, ignore it.
-        if !file_exemptions.is_empty() {
-            if file_exemptions.contains(&diagnostic.kind.rule().noqa_code()) {
                 ignored_diagnostics.push(index);
                 continue;
             }
+            FileExemption::Codes(codes) => {
+                // If the diagnostic is ignored by a global exemption, ignore it.
+                if codes.contains(&diagnostic.kind.rule().noqa_code()) {
+                    ignored_diagnostics.push(index);
+                    continue;
+                }
+            }
+            FileExemption::None => {}
         }
 
         // Is the violation ignored by a `noqa` directive on the parent line?

crates/ruff/src/noqa.rs

@@ -28,49 +28,6 @@ static NOQA_LINE_REGEX: Lazy<Regex> = Lazy::new(|| {
 });
 static SPLIT_COMMA_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"[,\s]").unwrap());
 
-#[derive(Debug)]
-pub enum Exemption<'a> {
-    None,
-    All,
-    Codes(Vec<&'a str>),
-}
-
-/// Return `true` if a file is exempt from checking based on the contents of the
-/// given line.
-pub fn extract_file_exemption(line: &str) -> Exemption {
-    let line = line.trim_start();
-
-    if line.starts_with("# flake8: noqa")
-        || line.starts_with("# flake8: NOQA")
-        || line.starts_with("# flake8: NoQA")
-    {
-        return Exemption::All;
-    }
-
-    if let Some(remainder) = line
-        .strip_prefix("# ruff: noqa")
-        .or_else(|| line.strip_prefix("# ruff: NOQA"))
-        .or_else(|| line.strip_prefix("# ruff: NoQA"))
-    {
-        if remainder.is_empty() {
-            return Exemption::All;
-        } else if let Some(codes) = remainder.strip_prefix(':') {
-            let codes: Vec<&str> = SPLIT_COMMA_REGEX
-                .split(codes.trim())
-                .map(str::trim)
-                .filter(|code| !code.is_empty())
-                .collect();
-            if codes.is_empty() {
-                warn!("Expected rule codes on `noqa` directive: \"{line}\"");
-            }
-            return Exemption::Codes(codes);
-        }
-        warn!("Unexpected suffix on `noqa` directive: \"{line}\"");
-    }
-
-    Exemption::None
-}
-
 #[derive(Debug)]
 pub enum Directive<'a> {
     None,
@@ -119,6 +76,47 @@ pub fn extract_noqa_directive(line: &str) -> Directive {
     }
 }
 
+enum ParsedExemption<'a> {
+    None,
+    All,
+    Codes(Vec<&'a str>),
+}
+
+/// Return a [`ParsedExemption`] for a given comment line.
+fn parse_file_exemption(line: &str) -> ParsedExemption {
+    let line = line.trim_start();
+
+    if line.starts_with("# flake8: noqa")
+        || line.starts_with("# flake8: NOQA")
+        || line.starts_with("# flake8: NoQA")
+    {
+        return ParsedExemption::All;
+    }
+
+    if let Some(remainder) = line
+        .strip_prefix("# ruff: noqa")
+        .or_else(|| line.strip_prefix("# ruff: NOQA"))
+        .or_else(|| line.strip_prefix("# ruff: NoQA"))
+    {
+        if remainder.is_empty() {
+            return ParsedExemption::All;
+        } else if let Some(codes) = remainder.strip_prefix(':') {
+            let codes: Vec<&str> = SPLIT_COMMA_REGEX
+                .split(codes.trim())
+                .map(str::trim)
+                .filter(|code| !code.is_empty())
+                .collect();
+            if codes.is_empty() {
+                warn!("Expected rule codes on `noqa` directive: \"{line}\"");
+            }
+            return ParsedExemption::Codes(codes);
+        }
+        warn!("Unexpected suffix on `noqa` directive: \"{line}\"");
+    }
+
+    ParsedExemption::None
+}
+
 /// Returns `true` if the string list of `codes` includes `code` (or an alias
 /// thereof).
 pub fn includes(needle: Rule, haystack: &[&str]) -> bool {
@@ -147,6 +145,43 @@ pub fn rule_is_ignored(
     }
 }
 
+pub enum FileExemption {
+    None,
+    All,
+    Codes(Vec<NoqaCode>),
+}
+
+/// Extract the [`FileExemption`] for a given Python source file, enumerating any rules that are
+/// globally ignored within the file.
+pub fn file_exemption(lines: &[&str], commented_lines: &[usize]) -> FileExemption {
+    let mut exempt_codes: Vec<NoqaCode> = vec![];
+
+    for lineno in commented_lines {
+        match parse_file_exemption(lines[lineno - 1]) {
+            ParsedExemption::All => {
+                return FileExemption::All;
+            }
+            ParsedExemption::Codes(codes) => {
+                exempt_codes.extend(codes.into_iter().filter_map(|code| {
+                    if let Ok(rule) = Rule::from_code(get_redirect_target(code).unwrap_or(code)) {
+                        Some(rule.noqa_code())
+                    } else {
+                        warn!("Invalid code provided to `# ruff: noqa`: {}", code);
+                        None
+                    }
+                }));
+            }
+            ParsedExemption::None => {}
+        }
+    }
+
+    if exempt_codes.is_empty() {
+        FileExemption::None
+    } else {
+        FileExemption::Codes(exempt_codes)
+    }
+}
+
 pub fn add_noqa(
     path: &Path,
     diagnostics: &[Diagnostic],
@@ -176,44 +211,26 @@ fn add_noqa_inner(
     // Map of line number to set of (non-ignored) diagnostic codes that are triggered on that line.
     let mut matches_by_line: FxHashMap<usize, RuleSet> = FxHashMap::default();
 
-    // Whether the file is exempted from all checks.
-    let mut file_exempted = false;
-
-    // Codes that are globally exempted (within the current file).
-    let mut file_exemptions: Vec<NoqaCode> = vec![];
-
     let lines: Vec<&str> = contents.universal_newlines().collect();
-    for lineno in commented_lines {
+
-        match extract_file_exemption(lines[lineno - 1]) {
+    // Whether the file is exempted from all checks.
-            Exemption::All => {
+    // Codes that are globally exempted (within the current file).
-                file_exempted = true;
+    let exemption = file_exemption(&lines, commented_lines);
-            }
-            Exemption::Codes(codes) => {
-                file_exemptions.extend(codes.into_iter().filter_map(|code| {
-                    if let Ok(rule) = Rule::from_code(get_redirect_target(code).unwrap_or(code)) {
-                        Some(rule.noqa_code())
-                    } else {
-                        warn!("Invalid code provided to `# ruff: noqa`: {}", code);
-                        None
-                    }
-                }));
-            }
-            Exemption::None => {}
-        }
-    }
 
     // Mark any non-ignored diagnostics.
     for diagnostic in diagnostics {
-        // If the file is exempted, don't add any noqa directives.
+        match &exemption {
-        if file_exempted {
+            FileExemption::All => {
-            continue;
+                // If the file is exempted, don't add any noqa directives.
-        }
-
-        // If the diagnostic is ignored by a global exemption, don't add a noqa directive.
-        if !file_exemptions.is_empty() {
-            if file_exemptions.contains(&diagnostic.kind.rule().noqa_code()) {
                 continue;
             }
+            FileExemption::Codes(codes) => {
+                // If the diagnostic is ignored by a global exemption, don't add a noqa directive.
+                if codes.contains(&diagnostic.kind.rule().noqa_code()) {
+                    continue;
+                }
+            }
+            FileExemption::None => {}
         }
 
         // Is the violation ignored by a `noqa` directive on the parent line?