diff --git a/crates/ruff/src/rules/flake8_todo/rules.rs b/crates/ruff/src/rules/flake8_todo/rules.rs index 0084fada9fd1ac..c7c86acd6373ee 100644 --- a/crates/ruff/src/rules/flake8_todo/rules.rs +++ b/crates/ruff/src/rules/flake8_todo/rules.rs @@ -6,7 +6,6 @@ use regex::{CaptureMatches, Regex}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::types::Range; -use rustpython_parser::ast::Location; use rustpython_parser::lexer::LexResult; use rustpython_parser::Tok; @@ -105,9 +104,9 @@ impl Violation for MissingSpaceAfterColonInTodo { // // Note: Tags taken from https://github.com/orsinium-labs/flake8-todos/blob/master/flake8_todos/_rules.py#L12. static TODO_REGEX: Lazy = Lazy::new(|| { - // TODO BEFORE COMMITTING - should be a nested group inside of Regex::new(r"^#\s*([tT][oO][dD][oO]|BUG|FIXME|XXX)(\(.*\))?(:)?( )?(.+)?$").unwrap() }); + static NUM_CAPTURE_GROUPS: usize = 5usize; pub fn check_rules(tokens: &[LexResult]) -> Vec { @@ -118,8 +117,43 @@ pub fn check_rules(tokens: &[LexResult]) -> Vec { continue; }; - if get_captured_matches(comment).peek().is_some() { - diagnostics.extend(get_tag_regex_errors(comment, *start, *end)); + if let Some(captures_ref) = get_captured_matches(comment).peek() { + let diagnostics_ref = &mut diagnostics; + let range = Range::new(*start, *end); + let captures = captures_ref.to_owned(); + + // captures.get(1) is the tag, which is required for the regex to match. The unwrap() + // call is therefore safe + let tag = captures.get(1).unwrap().as_str(); + if tag != "TODO" { + diagnostics_ref.push(Diagnostic::new( + InvalidTodoTag { + tag: String::from(tag), + }, + range, + )); + + if tag.to_uppercase() == "TODO" { + diagnostics_ref.push(Diagnostic::new( + InvalidCapitalizationInTodo { + tag: String::from(tag), + }, + range, + )); + } + } + + for capture_group_index in 2..=NUM_CAPTURE_GROUPS { + if captures.get(capture_group_index).is_some() { + continue; + } + + if let Some(diagnostic) = + get_regex_error(capture_group_index, &range, diagnostics_ref) + { + diagnostics_ref.push(diagnostic); + }; + } } } @@ -130,55 +164,23 @@ fn get_captured_matches(text: &str) -> Peekable { TODO_REGEX.captures_iter(text).peekable() } -fn get_tag_regex_errors(text: &str, start: Location, end: Location) -> Vec { - let mut diagnostics: Vec = vec![]; - - for capture in TODO_REGEX.captures_iter(text) { - // The tag is required for capturing the regex, so this is safe. - let tag = capture.get(1).unwrap().as_str(); - if tag != "TODO" { - diagnostics.push(Diagnostic::new( - InvalidTodoTag { - tag: String::from(tag), - }, - Range::new(start, end), - )); - - if tag.to_uppercase() == "TODO" { - diagnostics.push(Diagnostic::new( - InvalidCapitalizationInTodo { - tag: String::from(tag), - }, - Range::new(start, end), - )); - } - } - - // Note: This initially looks bad from a speed perspective, but is O(1) given that we - // know that there will only ever be 1 `capture` (due to regex anchors) and constant - // capture groups. - for capture_group_index in 2..=NUM_CAPTURE_GROUPS { - if capture.get(capture_group_index).is_none() { - let range = Range::new(start, end); - diagnostics.push(match capture_group_index { - 2usize => Diagnostic::new(MissingAuthorInTodo, range), - 3usize => Diagnostic::new(MissingColonInTodo, range), - 4usize => { - if diagnostics - .last() - .map_or(true, |last| last.kind != MissingColonInTodo.into()) - { - Diagnostic::new(MissingSpaceAfterColonInTodo, range) - } else { - continue; - } - } - 5usize => Diagnostic::new(MissingTextInTodo, range), - _ => break, - }); +/// Mapper for static regex errors caused by a capture group at index i (i > 1 since the tag +/// capture group could lead to multiple diagnostics being pushed) +fn get_regex_error(i: usize, range: &Range, diagnostics: &mut [Diagnostic]) -> Option { + match i { + 2usize => Some(Diagnostic::new(MissingAuthorInTodo, *range)), + 3usize => Some(Diagnostic::new(MissingColonInTodo, *range)), + 4usize => { + if diagnostics + .last() + .map_or(true, |last| last.kind != MissingColonInTodo.into()) + { + Some(Diagnostic::new(MissingSpaceAfterColonInTodo, *range)) + } else { + None } } + 5usize => Some(Diagnostic::new(MissingTextInTodo, *range)), + _ => None, } - - diagnostics }