From 64ee21644b72d83b1965cefdb0d70441b883d1bd Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Wed, 17 May 2023 21:28:01 -0500 Subject: [PATCH 01/15] Set up scaffolding for flake8-fixme --- crates/ruff/src/codes.rs | 6 ++++ crates/ruff/src/registry.rs | 8 +++++ crates/ruff/src/rules/flake8_fixme/mod.rs | 1 + crates/ruff/src/rules/flake8_fixme/rules.rs | 38 +++++++++++++++++++++ crates/ruff/src/rules/mod.rs | 1 + 5 files changed, 54 insertions(+) create mode 100644 crates/ruff/src/rules/flake8_fixme/mod.rs create mode 100644 crates/ruff/src/rules/flake8_fixme/rules.rs diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 1e896a3653f3b..9a3793ca3ed37 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -754,6 +754,12 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { // airflow (Airflow, "001") => (RuleGroup::Unspecified, Rule::AirflowVariableNameTaskIdMismatch), + // flake8-fixme + (Flake8Fixme, "001") => (RuleGroup::Unspecified, Rule::LineContainsFixme), + (Flake8Fixme, "002") => (RuleGroup::Unspecified, Rule::LineContainsTodo), + (Flake8Fixme, "003") => (RuleGroup::Unspecified, Rule::LineContainsXxx), + (Flake8Fixme, "004") => (RuleGroup::Unspecified, Rule::LineContainsHack), + _ => return None, }) } diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 0a642d2fc49ed..2688a22561aba 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -672,6 +672,11 @@ ruff_macros::register_rules!( rules::flake8_todos::rules::MissingSpaceAfterTodoColon, // airflow rules::airflow::rules::AirflowVariableNameTaskIdMismatch, + // flake8-fixme + rules::flake8_fixme::rules::LineContainsTodo, + rules::flake8_fixme::rules::LineContainsHack, + rules::flake8_fixme::rules::LineContainsXxx, + rules::flake8_fixme::rules::LineContainsFixme, ); pub trait AsRule { @@ -821,6 +826,9 @@ pub enum Linter { /// [flake8-todos](https://github.com/orsinium-labs/flake8-todos/) #[prefix = "TD"] Flake8Todos, + /// [flake8-fixme](https://github.com/tommilligan/flake8-fixme) + #[prefix = "T"] + Flake8Fixme, /// [eradicate](https://pypi.org/project/eradicate/) #[prefix = "ERA"] Eradicate, diff --git a/crates/ruff/src/rules/flake8_fixme/mod.rs b/crates/ruff/src/rules/flake8_fixme/mod.rs new file mode 100644 index 0000000000000..811c9e63b67a8 --- /dev/null +++ b/crates/ruff/src/rules/flake8_fixme/mod.rs @@ -0,0 +1 @@ +pub(crate) mod rules; diff --git a/crates/ruff/src/rules/flake8_fixme/rules.rs b/crates/ruff/src/rules/flake8_fixme/rules.rs new file mode 100644 index 0000000000000..b96a1ee98de81 --- /dev/null +++ b/crates/ruff/src/rules/flake8_fixme/rules.rs @@ -0,0 +1,38 @@ +use ruff_diagnostics::Violation; +use ruff_macros::{derive_message_formats, violation}; + +#[violation] +pub struct LineContainsTodo {} +impl Violation for LineContainsTodo { + #[derive_message_formats] + fn message(&self) -> String { + format!("Line contains TODO") + } +} + +#[violation] +pub struct LineContainsFixme {} +impl Violation for LineContainsFixme { + #[derive_message_formats] + fn message(&self) -> String { + format!("Line contains FIXME") + } +} + +#[violation] +pub struct LineContainsXxx {} +impl Violation for LineContainsXxx { + #[derive_message_formats] + fn message(&self) -> String { + format!("Line contains XXX") + } +} + +#[violation] +pub struct LineContainsHack {} +impl Violation for LineContainsHack { + #[derive_message_formats] + fn message(&self) -> String { + format!("Line contains HACK") + } +} diff --git a/crates/ruff/src/rules/mod.rs b/crates/ruff/src/rules/mod.rs index 0c4e5ef84ce43..172631145cce7 100644 --- a/crates/ruff/src/rules/mod.rs +++ b/crates/ruff/src/rules/mod.rs @@ -16,6 +16,7 @@ pub mod flake8_debugger; pub mod flake8_django; pub mod flake8_errmsg; pub mod flake8_executable; +pub mod flake8_fixme; pub mod flake8_future_annotations; pub mod flake8_gettext; pub mod flake8_implicit_str_concat; From 9fe5f812eef6f277d4883547506b61663dd2cfc0 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Thu, 18 May 2023 07:43:33 -0500 Subject: [PATCH 02/15] Move Directive to /checkers/todo_directives/TodoDirective --- crates/ruff/src/checkers/mod.rs | 1 + crates/ruff/src/checkers/todo_directives.rs | 58 ++++++++++++++++++ .../src/rules/flake8_todos/rules/todos.rs | 61 +------------------ 3 files changed, 61 insertions(+), 59 deletions(-) create mode 100644 crates/ruff/src/checkers/todo_directives.rs diff --git a/crates/ruff/src/checkers/mod.rs b/crates/ruff/src/checkers/mod.rs index 5178e7e6647ec..192ea10c99f44 100644 --- a/crates/ruff/src/checkers/mod.rs +++ b/crates/ruff/src/checkers/mod.rs @@ -4,4 +4,5 @@ pub(crate) mod imports; pub(crate) mod logical_lines; pub(crate) mod noqa; pub(crate) mod physical_lines; +pub(crate) mod todo_directives; pub(crate) mod tokens; diff --git a/crates/ruff/src/checkers/todo_directives.rs b/crates/ruff/src/checkers/todo_directives.rs new file mode 100644 index 0000000000000..44feb32d39920 --- /dev/null +++ b/crates/ruff/src/checkers/todo_directives.rs @@ -0,0 +1,58 @@ +use ruff_text_size::{TextLen, TextSize}; + +pub enum TodoDirective { + Todo, + Fixme, + Xxx, +} + +impl TodoDirective { + /// Extract a [`TodoDirective`] from a comment. + /// + /// Returns the offset of the directive within the comment, and the matching directive tag. + pub(crate) fn from_comment(comment: &str) -> Option<(TodoDirective, TextSize)> { + let mut subset_opt = Some(comment); + let mut total_offset = TextSize::new(0); + + // Loop over the comment to catch cases like `# foo # TODO`. + while let Some(subset) = subset_opt { + let trimmed = subset.trim_start_matches('#').trim_start().to_lowercase(); + + let offset = subset.text_len() - trimmed.text_len(); + total_offset += offset; + + let directive = if trimmed.starts_with("fixme") { + Some((TodoDirective::Fixme, total_offset)) + } else if trimmed.starts_with("xxx") { + Some((TodoDirective::Xxx, total_offset)) + } else if trimmed.starts_with("todo") { + Some((TodoDirective::Todo, total_offset)) + } else { + None + }; + + if directive.is_some() { + return directive; + } + + // Shrink the subset to check for the next phrase starting with "#". + subset_opt = if let Some(new_offset) = trimmed.find('#') { + total_offset += TextSize::try_from(new_offset).unwrap(); + subset.get(total_offset.to_usize()..) + } else { + None + }; + } + + None + } + + /// Returns the length of the directive tag. + pub(crate) fn len(&self) -> TextSize { + match self { + TodoDirective::Fixme => TextSize::new(5), + TodoDirective::Todo => TextSize::new(4), + TodoDirective::Xxx => TextSize::new(3), + } + } +} diff --git a/crates/ruff/src/rules/flake8_todos/rules/todos.rs b/crates/ruff/src/rules/flake8_todos/rules/todos.rs index d641a717612d2..e808e85e2fa38 100644 --- a/crates/ruff/src/rules/flake8_todos/rules/todos.rs +++ b/crates/ruff/src/rules/flake8_todos/rules/todos.rs @@ -6,7 +6,7 @@ use ruff_text_size::{TextLen, TextRange, TextSize}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; -use crate::{registry::Rule, settings::Settings}; +use crate::{checkers::todo_directives::TodoDirective, registry::Rule, settings::Settings}; /// ## What it does /// Checks that a TODO comment is labelled with "TODO". @@ -221,63 +221,6 @@ impl Violation for MissingSpaceAfterTodoColon { } } -enum Directive { - Todo, - Fixme, - Xxx, -} - -impl Directive { - /// Extract a [`Directive`] from a comment. - /// - /// Returns the matching directive tag and its offset within the comment. - fn from_comment(comment: &str) -> Option<(Directive, TextSize)> { - let mut subset_opt = Some(comment); - let mut total_offset = TextSize::new(0); - - // Loop over the comment to catch cases like `# foo # TODO`. - while let Some(subset) = subset_opt { - let trimmed = subset.trim_start_matches('#').trim_start().to_lowercase(); - - let offset = subset.text_len() - trimmed.text_len(); - total_offset += offset; - - let directive = if trimmed.starts_with("fixme") { - Some((Directive::Fixme, total_offset)) - } else if trimmed.starts_with("xxx") { - Some((Directive::Xxx, total_offset)) - } else if trimmed.starts_with("todo") { - Some((Directive::Todo, total_offset)) - } else { - None - }; - - if directive.is_some() { - return directive; - } - - // Shrink the subset to check for the next phrase starting with "#". - subset_opt = if let Some(new_offset) = trimmed.find('#') { - total_offset += TextSize::try_from(new_offset).unwrap(); - subset.get(total_offset.to_usize()..) - } else { - None - }; - } - - None - } - - /// Returns the length of the directive tag. - fn len(&self) -> TextSize { - match self { - Directive::Fixme => TextSize::new(5), - Directive::Todo => TextSize::new(4), - Directive::Xxx => TextSize::new(3), - } - } -} - // If this struct ever gets pushed outside of this module, it may be worth creating an enum for // the different tag types + other convenience methods. /// Represents a TODO tag or any of its variants - FIXME, XXX, TODO. @@ -352,7 +295,7 @@ pub(crate) fn todos(indexer: &Indexer, locator: &Locator, settings: &Settings) - /// Returns the tag pulled out of a given comment, if it exists. fn detect_tag(comment: &str, start: TextSize) -> Option { - let (directive, offset) = Directive::from_comment(comment)?; + let (directive, offset) = TodoDirective::from_comment(comment)?; let comment_range = TextRange::at(offset, directive.len()); let tag_range = TextRange::at(start + offset, directive.len()); Some(Tag { From 28e29371a3c1c4ef23602e9c9ac8ca9d45ec4677 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Fri, 26 May 2023 12:35:32 -0500 Subject: [PATCH 03/15] Refactor TodoDirective detection logic so it can be reused --- crates/ruff/src/checkers/tokens.rs | 46 ++++++++- .../src/rules/flake8_todos/rules/todos.rs | 98 +++++-------------- 2 files changed, 66 insertions(+), 78 deletions(-) diff --git a/crates/ruff/src/checkers/tokens.rs b/crates/ruff/src/checkers/tokens.rs index 7ebe8937337f2..111fb19f452d1 100644 --- a/crates/ruff/src/checkers/tokens.rs +++ b/crates/ruff/src/checkers/tokens.rs @@ -1,5 +1,6 @@ //! Lint rules based on token traversal. +use ruff_text_size::TextRange; use rustpython_parser::lexer::LexResult; use rustpython_parser::Tok; @@ -7,13 +8,15 @@ use crate::lex::docstring_detection::StateMachine; use crate::registry::{AsRule, Rule}; use crate::rules::ruff::rules::Context; use crate::rules::{ - eradicate, flake8_commas, flake8_implicit_str_concat, flake8_pyi, flake8_quotes, flake8_todos, - pycodestyle, pylint, pyupgrade, ruff, + eradicate, flake8_commas, flake8_fixme, flake8_implicit_str_concat, flake8_pyi, flake8_quotes, + flake8_todos, pycodestyle, pylint, pyupgrade, ruff, }; use crate::settings::Settings; use ruff_diagnostics::Diagnostic; use ruff_python_ast::source_code::{Indexer, Locator}; +use super::todo_directives::TodoDirective; + pub(crate) fn check_tokens( locator: &Locator, indexer: &Indexer, @@ -60,6 +63,9 @@ pub(crate) fn check_tokens( ]); let enforce_extraneous_parenthesis = settings.rules.enabled(Rule::ExtraneousParentheses); let enforce_type_comment_in_stub = settings.rules.enabled(Rule::TypeCommentInStub); + + // Combine flake8_todos and flake8_fixme so that we can reuse detected [`TodoDirective`]s + // applicable to both linters. let enforce_todos = settings.rules.any_enabled(&[ Rule::InvalidTodoTag, Rule::MissingTodoAuthor, @@ -68,6 +74,10 @@ pub(crate) fn check_tokens( Rule::MissingTodoDescription, Rule::InvalidTodoCapitalization, Rule::MissingSpaceAfterTodoColon, + Rule::LineContainsFixme, + Rule::LineContainsXxx, + Rule::LineContainsTodo, + Rule::LineContainsHack, ]); // RUF001, RUF002, RUF003 @@ -184,9 +194,39 @@ pub(crate) fn check_tokens( } // TD001, TD002, TD003, TD004, TD005, TD006, TD007 + // T001, T002, T003, T004 if enforce_todos { + // The TextRange of the comment, its position in comment_ranges, and the directive's + // TextRange + let mut other_directive_ranges: Vec<(TextRange, usize, TextRange)> = vec![]; + let mut flake8_fixme_directive_ranges: Vec = vec![]; + + // Find all TodoDirectives + for (i, comment_range) in indexer.comment_ranges().iter().enumerate() { + let comment = locator.slice(*comment_range); + let Some((directive, relative_offset)) = TodoDirective::from_comment(comment) else { + continue; + }; + + let directive_range = + TextRange::at(comment_range.start() + relative_offset, directive.len()); + + // TODO, XXX, and FIXME directives are supported by flake8_todos. flake8_fixme supports + // all 4 TodoDirective variants. + if !matches!(directive, TodoDirective::Hack) { + other_directive_ranges.push((*comment_range, i, directive_range)); + } + flake8_fixme_directive_ranges.push(directive_range); + } + + diagnostics.extend( + flake8_todos::rules::todos(other_directive_ranges, indexer, locator, settings) + .into_iter() + .filter(|diagnostic| settings.rules.enabled(diagnostic.kind.rule())), + ); + diagnostics.extend( - flake8_todos::rules::todos(indexer, locator, settings) + flake8_fixme::rules::todos(flake8_fixme_directive_ranges, locator) .into_iter() .filter(|diagnostic| settings.rules.enabled(diagnostic.kind.rule())), ); diff --git a/crates/ruff/src/rules/flake8_todos/rules/todos.rs b/crates/ruff/src/rules/flake8_todos/rules/todos.rs index e808e85e2fa38..ce11065da8549 100644 --- a/crates/ruff/src/rules/flake8_todos/rules/todos.rs +++ b/crates/ruff/src/rules/flake8_todos/rules/todos.rs @@ -239,25 +239,28 @@ static ISSUE_LINK_REGEX_SET: Lazy = Lazy::new(|| { .unwrap() }); -pub(crate) fn todos(indexer: &Indexer, locator: &Locator, settings: &Settings) -> Vec { +pub(crate) fn todos( + comment_directive_ranges: Vec<(TextRange, usize, TextRange)>, + indexer: &Indexer, + locator: &Locator, + settings: &Settings, +) -> Vec { let mut diagnostics: Vec = vec![]; - let mut iter = indexer.comment_ranges().iter().peekable(); - while let Some(comment_range) = iter.next() { - let comment = locator.slice(*comment_range); - - // Check that the comment is a TODO (properly formed or not). - let Some(tag) = detect_tag(comment, comment_range.start()) else { - continue; + for (comment_range, comment_index, directive_range) in comment_directive_ranges { + let comment = locator.slice(comment_range); + let tag = Tag { + content: locator.slice(directive_range), + range: directive_range, }; tag_errors(&tag, &mut diagnostics, settings); - static_errors(&mut diagnostics, comment, *comment_range, &tag); + static_errors(&mut diagnostics, comment, comment_range, &tag); - // TD003 let mut has_issue_link = false; - let mut curr_range = comment_range; - while let Some(next_range) = iter.peek() { + let mut curr_range = &comment_range; + let mut future_comments = indexer.comment_ranges().iter().skip(comment_index + 1); + while let Some(next_range) = future_comments.next() { // Ensure that next_comment_range is in the same multiline comment "block" as // comment_range. if !locator @@ -268,8 +271,8 @@ pub(crate) fn todos(indexer: &Indexer, locator: &Locator, settings: &Settings) - break; } - let next_comment = locator.slice(**next_range); - if detect_tag(next_comment, next_range.start()).is_some() { + let next_comment = locator.slice(*next_range); + if TodoDirective::from_comment(next_comment).is_some() { break; } @@ -280,12 +283,11 @@ pub(crate) fn todos(indexer: &Indexer, locator: &Locator, settings: &Settings) - // If the next_comment isn't a tag or an issue, it's worthles in the context of this // linter. We can increment here instead of waiting for the next iteration of the outer // loop. - // - // Unwrap is safe because peek() is Some() - curr_range = iter.next().unwrap(); + curr_range = next_range; } if !has_issue_link { + // TD-003 diagnostics.push(Diagnostic::new(MissingTodoLink, tag.range)); } } @@ -293,17 +295,6 @@ pub(crate) fn todos(indexer: &Indexer, locator: &Locator, settings: &Settings) - diagnostics } -/// Returns the tag pulled out of a given comment, if it exists. -fn detect_tag(comment: &str, start: TextSize) -> Option { - let (directive, offset) = TodoDirective::from_comment(comment)?; - let comment_range = TextRange::at(offset, directive.len()); - let tag_range = TextRange::at(start + offset, directive.len()); - Some(Tag { - content: &comment[comment_range], - range: tag_range, - }) -} - /// Check that the tag itself is valid. This function modifies `diagnostics` in-place. fn tag_errors(tag: &Tag, diagnostics: &mut Vec, settings: &Settings) { if tag.content == "TODO" { @@ -367,62 +358,19 @@ fn static_errors( let after_author = &post_tag[usize::from(author_end)..]; if let Some(after_colon) = after_author.strip_prefix(':') { if after_colon.is_empty() { + // TD-005 diagnostics.push(Diagnostic::new(MissingTodoDescription, tag.range)); } else if !after_colon.starts_with(char::is_whitespace) { + // TD-007 diagnostics.push(Diagnostic::new(MissingSpaceAfterTodoColon, tag.range)); } } else { + // TD-004 diagnostics.push(Diagnostic::new(MissingTodoColon, tag.range)); if after_author.is_empty() { + // TD-005 diagnostics.push(Diagnostic::new(MissingTodoDescription, tag.range)); } } } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_detect_tag() { - let test_comment = "# TODO: todo tag"; - let expected = Tag { - content: "TODO", - range: TextRange::new(TextSize::new(2), TextSize::new(6)), - }; - assert_eq!(Some(expected), detect_tag(test_comment, TextSize::new(0))); - - let test_comment = "#TODO: todo tag"; - let expected = Tag { - content: "TODO", - range: TextRange::new(TextSize::new(1), TextSize::new(5)), - }; - assert_eq!(Some(expected), detect_tag(test_comment, TextSize::new(0))); - - let test_comment = "# todo: todo tag"; - let expected = Tag { - content: "todo", - range: TextRange::new(TextSize::new(2), TextSize::new(6)), - }; - assert_eq!(Some(expected), detect_tag(test_comment, TextSize::new(0))); - let test_comment = "# fixme: fixme tag"; - let expected = Tag { - content: "fixme", - range: TextRange::new(TextSize::new(2), TextSize::new(7)), - }; - assert_eq!(Some(expected), detect_tag(test_comment, TextSize::new(0))); - let test_comment = "# noqa # TODO: todo"; - let expected = Tag { - content: "TODO", - range: TextRange::new(TextSize::new(9), TextSize::new(13)), - }; - assert_eq!(Some(expected), detect_tag(test_comment, TextSize::new(0))); - let test_comment = "# noqa # XXX"; - let expected = Tag { - content: "XXX", - range: TextRange::new(TextSize::new(9), TextSize::new(12)), - }; - assert_eq!(Some(expected), detect_tag(test_comment, TextSize::new(0))); - } -} From d5b3a29cfd2cdeac5624700d4ace732f164d0cd7 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Fri, 26 May 2023 15:16:38 -0500 Subject: [PATCH 04/15] Add TodoDirective::Hack --- crates/ruff/src/checkers/todo_directives.rs | 24 ++++++++++++++-- crates/ruff/src/checkers/tokens.rs | 3 +- crates/ruff/src/directives.rs | 2 +- crates/ruff/src/registry.rs | 6 +++- crates/ruff/src/rules/flake8_fixme/mod.rs | 28 ++++++++++++++++++- crates/ruff/src/rules/flake8_fixme/rules.rs | 20 ++++++++++--- .../src/rules/flake8_todos/rules/todos.rs | 3 +- 7 files changed, 73 insertions(+), 13 deletions(-) diff --git a/crates/ruff/src/checkers/todo_directives.rs b/crates/ruff/src/checkers/todo_directives.rs index 44feb32d39920..bee5e44df6773 100644 --- a/crates/ruff/src/checkers/todo_directives.rs +++ b/crates/ruff/src/checkers/todo_directives.rs @@ -1,9 +1,26 @@ +use std::str::FromStr; + use ruff_text_size::{TextLen, TextSize}; -pub enum TodoDirective { +pub(crate) enum TodoDirective { Todo, Fixme, Xxx, + Hack, +} + +impl FromStr for TodoDirective { + type Err = (); + + fn from_str(s: &str) -> Result { + match s.to_lowercase().as_str() { + "fixme" => Ok(TodoDirective::Fixme), + "hack" => Ok(TodoDirective::Hack), + "todo" => Ok(TodoDirective::Todo), + "xxx" => Ok(TodoDirective::Xxx), + _ => Err(()), + } + } } impl TodoDirective { @@ -21,12 +38,15 @@ impl TodoDirective { let offset = subset.text_len() - trimmed.text_len(); total_offset += offset; + // TODO: rework this let directive = if trimmed.starts_with("fixme") { Some((TodoDirective::Fixme, total_offset)) } else if trimmed.starts_with("xxx") { Some((TodoDirective::Xxx, total_offset)) } else if trimmed.starts_with("todo") { Some((TodoDirective::Todo, total_offset)) + } else if trimmed.starts_with("hack") { + Some((TodoDirective::Hack, total_offset)) } else { None }; @@ -51,7 +71,7 @@ impl TodoDirective { pub(crate) fn len(&self) -> TextSize { match self { TodoDirective::Fixme => TextSize::new(5), - TodoDirective::Todo => TextSize::new(4), + TodoDirective::Todo | TodoDirective::Hack => TextSize::new(4), TodoDirective::Xxx => TextSize::new(3), } } diff --git a/crates/ruff/src/checkers/tokens.rs b/crates/ruff/src/checkers/tokens.rs index 111fb19f452d1..f5c0bf7c33fb5 100644 --- a/crates/ruff/src/checkers/tokens.rs +++ b/crates/ruff/src/checkers/tokens.rs @@ -64,8 +64,7 @@ pub(crate) fn check_tokens( let enforce_extraneous_parenthesis = settings.rules.enabled(Rule::ExtraneousParentheses); let enforce_type_comment_in_stub = settings.rules.enabled(Rule::TypeCommentInStub); - // Combine flake8_todos and flake8_fixme so that we can reuse detected [`TodoDirective`]s - // applicable to both linters. + // Combine flake8_todos and flake8_fixme so that we can reuse detected [`TodoDirective`]s. let enforce_todos = settings.rules.any_enabled(&[ Rule::InvalidTodoTag, Rule::MissingTodoAuthor, diff --git a/crates/ruff/src/directives.rs b/crates/ruff/src/directives.rs index c626c5d8e2d09..a0eb770df566e 100644 --- a/crates/ruff/src/directives.rs +++ b/crates/ruff/src/directives.rs @@ -1,4 +1,4 @@ -//! Extract `# noqa` and `# isort: skip` directives from tokenized source. +//! Extract `# noqa`, `# isort: skip`, and `# TODO` directives from tokenized source. use bitflags::bitflags; use ruff_text_size::{TextLen, TextRange, TextSize}; diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 2688a22561aba..454311ae07f54 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -963,7 +963,11 @@ impl Rule { | Rule::MissingTodoColon | Rule::MissingTodoDescription | Rule::InvalidTodoCapitalization - | Rule::MissingSpaceAfterTodoColon => LintSource::Tokens, + | Rule::MissingSpaceAfterTodoColon + | Rule::LineContainsFixme + | Rule::LineContainsHack + | Rule::LineContainsTodo + | Rule::LineContainsXxx => LintSource::Tokens, Rule::IOError => LintSource::Io, Rule::UnsortedImports | Rule::MissingRequiredImport => LintSource::Imports, Rule::ImplicitNamespacePackage | Rule::InvalidModuleName => LintSource::Filesystem, diff --git a/crates/ruff/src/rules/flake8_fixme/mod.rs b/crates/ruff/src/rules/flake8_fixme/mod.rs index 811c9e63b67a8..735e61c72e6d4 100644 --- a/crates/ruff/src/rules/flake8_fixme/mod.rs +++ b/crates/ruff/src/rules/flake8_fixme/mod.rs @@ -1 +1,27 @@ -pub(crate) mod rules; +pub mod rules; + +#[cfg(test)] +mod tests { + use std::path::Path; + + use anyhow::Result; + use test_case::test_case; + + use crate::registry::Rule; + use crate::test::test_path; + use crate::{assert_messages, settings}; + + #[test_case(Rule::LineContainsFixme; "T001")] + #[test_case(Rule::LineContainsHack; "T002")] + #[test_case(Rule::LineContainsTodo; "T003")] + #[test_case(Rule::LineContainsXxx; "T004")] + fn rules(rule_code: Rule) -> Result<()> { + let snapshot = format!("{}_T00.py", rule_code.as_ref()); + let diagnostics = test_path( + Path::new("flake8_fixme/T00.py"), + &settings::Settings::for_rule(rule_code), + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } +} diff --git a/crates/ruff/src/rules/flake8_fixme/rules.rs b/crates/ruff/src/rules/flake8_fixme/rules.rs index b96a1ee98de81..d03411ee4d47d 100644 --- a/crates/ruff/src/rules/flake8_fixme/rules.rs +++ b/crates/ruff/src/rules/flake8_fixme/rules.rs @@ -2,7 +2,7 @@ use ruff_diagnostics::Violation; use ruff_macros::{derive_message_formats, violation}; #[violation] -pub struct LineContainsTodo {} +pub struct LineContainsTodo; impl Violation for LineContainsTodo { #[derive_message_formats] fn message(&self) -> String { @@ -11,7 +11,7 @@ impl Violation for LineContainsTodo { } #[violation] -pub struct LineContainsFixme {} +pub struct LineContainsFixme; impl Violation for LineContainsFixme { #[derive_message_formats] fn message(&self) -> String { @@ -20,7 +20,7 @@ impl Violation for LineContainsFixme { } #[violation] -pub struct LineContainsXxx {} +pub struct LineContainsXxx; impl Violation for LineContainsXxx { #[derive_message_formats] fn message(&self) -> String { @@ -29,10 +29,22 @@ impl Violation for LineContainsXxx { } #[violation] -pub struct LineContainsHack {} +pub struct LineContainsHack; impl Violation for LineContainsHack { #[derive_message_formats] fn message(&self) -> String { format!("Line contains HACK") } } + +pub fn todos(directive_ranges: Vec<(TodoDirective, TextRange)>) -> Vec { + directive_ranges + .into_iter() + .map(|(directive, range)| match directive { + TodoDirective::Fixme => Diagnostic::new(LineContainsFixme, range), + TodoDirective::Hack => Diagnostic::new(LineContainsHack, range), + TodoDirective::Todo => Diagnostic::new(LineContainsTodo, range), + TodoDirective::Xxx => Diagnostic::new(LineContainsXxx, range), + }) + .collect::>() +} diff --git a/crates/ruff/src/rules/flake8_todos/rules/todos.rs b/crates/ruff/src/rules/flake8_todos/rules/todos.rs index ce11065da8549..d6222ce841a6d 100644 --- a/crates/ruff/src/rules/flake8_todos/rules/todos.rs +++ b/crates/ruff/src/rules/flake8_todos/rules/todos.rs @@ -259,8 +259,7 @@ pub(crate) fn todos( let mut has_issue_link = false; let mut curr_range = &comment_range; - let mut future_comments = indexer.comment_ranges().iter().skip(comment_index + 1); - while let Some(next_range) = future_comments.next() { + for next_range in indexer.comment_ranges().iter().skip(comment_index + 1) { // Ensure that next_comment_range is in the same multiline comment "block" as // comment_range. if !locator From 5ea47b6ba0464928c1500bb65bd3bf54213ac41c Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Fri, 26 May 2023 21:00:58 -0500 Subject: [PATCH 05/15] Move TodoDirective to directives.rs --- crates/ruff/src/checkers/mod.rs | 1 - crates/ruff/src/checkers/todo_directives.rs | 78 ------------------- crates/ruff/src/checkers/tokens.rs | 9 +-- crates/ruff/src/directives.rs | 77 ++++++++++++++++++ crates/ruff/src/rules/flake8_fixme/rules.rs | 5 +- .../src/rules/flake8_todos/rules/todos.rs | 2 +- ruff.schema.json | 7 ++ 7 files changed, 93 insertions(+), 86 deletions(-) delete mode 100644 crates/ruff/src/checkers/todo_directives.rs diff --git a/crates/ruff/src/checkers/mod.rs b/crates/ruff/src/checkers/mod.rs index 192ea10c99f44..5178e7e6647ec 100644 --- a/crates/ruff/src/checkers/mod.rs +++ b/crates/ruff/src/checkers/mod.rs @@ -4,5 +4,4 @@ pub(crate) mod imports; pub(crate) mod logical_lines; pub(crate) mod noqa; pub(crate) mod physical_lines; -pub(crate) mod todo_directives; pub(crate) mod tokens; diff --git a/crates/ruff/src/checkers/todo_directives.rs b/crates/ruff/src/checkers/todo_directives.rs deleted file mode 100644 index bee5e44df6773..0000000000000 --- a/crates/ruff/src/checkers/todo_directives.rs +++ /dev/null @@ -1,78 +0,0 @@ -use std::str::FromStr; - -use ruff_text_size::{TextLen, TextSize}; - -pub(crate) enum TodoDirective { - Todo, - Fixme, - Xxx, - Hack, -} - -impl FromStr for TodoDirective { - type Err = (); - - fn from_str(s: &str) -> Result { - match s.to_lowercase().as_str() { - "fixme" => Ok(TodoDirective::Fixme), - "hack" => Ok(TodoDirective::Hack), - "todo" => Ok(TodoDirective::Todo), - "xxx" => Ok(TodoDirective::Xxx), - _ => Err(()), - } - } -} - -impl TodoDirective { - /// Extract a [`TodoDirective`] from a comment. - /// - /// Returns the offset of the directive within the comment, and the matching directive tag. - pub(crate) fn from_comment(comment: &str) -> Option<(TodoDirective, TextSize)> { - let mut subset_opt = Some(comment); - let mut total_offset = TextSize::new(0); - - // Loop over the comment to catch cases like `# foo # TODO`. - while let Some(subset) = subset_opt { - let trimmed = subset.trim_start_matches('#').trim_start().to_lowercase(); - - let offset = subset.text_len() - trimmed.text_len(); - total_offset += offset; - - // TODO: rework this - let directive = if trimmed.starts_with("fixme") { - Some((TodoDirective::Fixme, total_offset)) - } else if trimmed.starts_with("xxx") { - Some((TodoDirective::Xxx, total_offset)) - } else if trimmed.starts_with("todo") { - Some((TodoDirective::Todo, total_offset)) - } else if trimmed.starts_with("hack") { - Some((TodoDirective::Hack, total_offset)) - } else { - None - }; - - if directive.is_some() { - return directive; - } - - // Shrink the subset to check for the next phrase starting with "#". - subset_opt = if let Some(new_offset) = trimmed.find('#') { - total_offset += TextSize::try_from(new_offset).unwrap(); - subset.get(total_offset.to_usize()..) - } else { - None - }; - } - - None - } - - /// Returns the length of the directive tag. - pub(crate) fn len(&self) -> TextSize { - match self { - TodoDirective::Fixme => TextSize::new(5), - TodoDirective::Todo | TodoDirective::Hack => TextSize::new(4), - TodoDirective::Xxx => TextSize::new(3), - } - } -} diff --git a/crates/ruff/src/checkers/tokens.rs b/crates/ruff/src/checkers/tokens.rs index f5c0bf7c33fb5..a3c4d1e0e225b 100644 --- a/crates/ruff/src/checkers/tokens.rs +++ b/crates/ruff/src/checkers/tokens.rs @@ -4,6 +4,7 @@ use ruff_text_size::TextRange; use rustpython_parser::lexer::LexResult; use rustpython_parser::Tok; +use crate::directives::TodoDirective; use crate::lex::docstring_detection::StateMachine; use crate::registry::{AsRule, Rule}; use crate::rules::ruff::rules::Context; @@ -15,8 +16,6 @@ use crate::settings::Settings; use ruff_diagnostics::Diagnostic; use ruff_python_ast::source_code::{Indexer, Locator}; -use super::todo_directives::TodoDirective; - pub(crate) fn check_tokens( locator: &Locator, indexer: &Indexer, @@ -198,7 +197,7 @@ pub(crate) fn check_tokens( // The TextRange of the comment, its position in comment_ranges, and the directive's // TextRange let mut other_directive_ranges: Vec<(TextRange, usize, TextRange)> = vec![]; - let mut flake8_fixme_directive_ranges: Vec = vec![]; + let mut flake8_fixme_directive_ranges: Vec<(TodoDirective, TextRange)> = vec![]; // Find all TodoDirectives for (i, comment_range) in indexer.comment_ranges().iter().enumerate() { @@ -215,7 +214,7 @@ pub(crate) fn check_tokens( if !matches!(directive, TodoDirective::Hack) { other_directive_ranges.push((*comment_range, i, directive_range)); } - flake8_fixme_directive_ranges.push(directive_range); + flake8_fixme_directive_ranges.push((directive, directive_range)); } diagnostics.extend( @@ -225,7 +224,7 @@ pub(crate) fn check_tokens( ); diagnostics.extend( - flake8_fixme::rules::todos(flake8_fixme_directive_ranges, locator) + flake8_fixme::rules::todos(flake8_fixme_directive_ranges) .into_iter() .filter(|diagnostic| settings.rules.enabled(diagnostic.kind.rule())), ); diff --git a/crates/ruff/src/directives.rs b/crates/ruff/src/directives.rs index a0eb770df566e..61fd170ed5c19 100644 --- a/crates/ruff/src/directives.rs +++ b/crates/ruff/src/directives.rs @@ -1,5 +1,7 @@ //! Extract `# noqa`, `# isort: skip`, and `# TODO` directives from tokenized source. +use std::str::FromStr; + use bitflags::bitflags; use ruff_text_size::{TextLen, TextRange, TextSize}; use rustpython_parser::lexer::LexResult; @@ -217,6 +219,81 @@ fn extract_isort_directives(lxr: &[LexResult], locator: &Locator) -> IsortDirect } } +pub enum TodoDirective { + Todo, + Fixme, + Xxx, + Hack, +} + +impl FromStr for TodoDirective { + type Err = (); + + fn from_str(s: &str) -> Result { + match s.to_lowercase().as_str() { + "fixme" => Ok(TodoDirective::Fixme), + "hack" => Ok(TodoDirective::Hack), + "todo" => Ok(TodoDirective::Todo), + "xxx" => Ok(TodoDirective::Xxx), + _ => Err(()), + } + } +} + +impl TodoDirective { + /// Extract a [`TodoDirective`] from a comment. + /// + /// Returns the offset of the directive within the comment, and the matching directive tag. + pub fn from_comment(comment: &str) -> Option<(TodoDirective, TextSize)> { + let mut subset_opt = Some(comment); + let mut total_offset = TextSize::new(0); + + // Loop over the comment to catch cases like `# foo # TODO`. + while let Some(subset) = subset_opt { + let trimmed = subset.trim_start_matches('#').trim_start().to_lowercase(); + + let offset = subset.text_len() - trimmed.text_len(); + total_offset += offset; + + // TODO: rework this + let directive = if trimmed.starts_with("fixme") { + Some((TodoDirective::Fixme, total_offset)) + } else if trimmed.starts_with("xxx") { + Some((TodoDirective::Xxx, total_offset)) + } else if trimmed.starts_with("todo") { + Some((TodoDirective::Todo, total_offset)) + } else if trimmed.starts_with("hack") { + Some((TodoDirective::Hack, total_offset)) + } else { + None + }; + + if directive.is_some() { + return directive; + } + + // Shrink the subset to check for the next phrase starting with "#". + subset_opt = if let Some(new_offset) = trimmed.find('#') { + total_offset += TextSize::try_from(new_offset).unwrap(); + subset.get(total_offset.to_usize()..) + } else { + None + }; + } + + None + } + + /// Returns the length of the directive tag. + pub fn len(&self) -> TextSize { + match self { + TodoDirective::Fixme => TextSize::new(5), + TodoDirective::Todo | TodoDirective::Hack => TextSize::new(4), + TodoDirective::Xxx => TextSize::new(3), + } + } +} + #[cfg(test)] mod tests { use ruff_text_size::{TextLen, TextRange, TextSize}; diff --git a/crates/ruff/src/rules/flake8_fixme/rules.rs b/crates/ruff/src/rules/flake8_fixme/rules.rs index d03411ee4d47d..09d3e10ac96a4 100644 --- a/crates/ruff/src/rules/flake8_fixme/rules.rs +++ b/crates/ruff/src/rules/flake8_fixme/rules.rs @@ -1,5 +1,8 @@ -use ruff_diagnostics::Violation; +use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_text_size::TextRange; + +use crate::directives::TodoDirective; #[violation] pub struct LineContainsTodo; diff --git a/crates/ruff/src/rules/flake8_todos/rules/todos.rs b/crates/ruff/src/rules/flake8_todos/rules/todos.rs index d6222ce841a6d..4fef5928a114f 100644 --- a/crates/ruff/src/rules/flake8_todos/rules/todos.rs +++ b/crates/ruff/src/rules/flake8_todos/rules/todos.rs @@ -6,7 +6,7 @@ use ruff_text_size::{TextLen, TextRange, TextSize}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; -use crate::{checkers::todo_directives::TodoDirective, registry::Rule, settings::Settings}; +use crate::{directives::TodoDirective, registry::Rule, settings::Settings}; /// ## What it does /// Checks that a TODO comment is labelled with "TODO". diff --git a/ruff.schema.json b/ruff.schema.json index 31550f9dcb680..877fdc8b11e13 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2368,6 +2368,13 @@ "SLF00", "SLF001", "T", + "T", + "T0", + "T00", + "T001", + "T002", + "T003", + "T004", "T1", "T10", "T100", From d7bb5b03a1ecee1f57d4869817586845b07d8e14 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Sat, 27 May 2023 10:34:20 -0500 Subject: [PATCH 06/15] Add tests for flake8_fixme --- .../test/fixtures/flake8_fixme/T00.py | 8 +++++++ ...me__tests__line-contains-fixme_T00.py.snap | 21 ++++++++++++++++ ...xme__tests__line-contains-hack_T00.py.snap | 24 +++++++++++++++++++ ...xme__tests__line-contains-todo_T00.py.snap | 21 ++++++++++++++++ ...ixme__tests__line-contains-xxx_T00.py.snap | 24 +++++++++++++++++++ 5 files changed, 98 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/flake8_fixme/T00.py create mode 100644 crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-fixme_T00.py.snap create mode 100644 crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-hack_T00.py.snap create mode 100644 crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-todo_T00.py.snap create mode 100644 crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-xxx_T00.py.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_fixme/T00.py b/crates/ruff/resources/test/fixtures/flake8_fixme/T00.py new file mode 100644 index 0000000000000..a398610c1c234 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_fixme/T00.py @@ -0,0 +1,8 @@ +# TODO: todo +# todo: todo +# XXX: xxx +# xxx: xxx +# HACK: hack +# hack: hack +# FIXME: fixme +# fixme: fixme diff --git a/crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-fixme_T00.py.snap b/crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-fixme_T00.py.snap new file mode 100644 index 0000000000000..aaefcbcfec692 --- /dev/null +++ b/crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-fixme_T00.py.snap @@ -0,0 +1,21 @@ +--- +source: crates/ruff/src/rules/flake8_fixme/mod.rs +--- +T00.py:7:3: T001 Line contains FIXME + | + 7 | # HACK: hack + 8 | # hack: hack + 9 | # FIXME: fixme + | ^^^^^ T001 +10 | # fixme: fixme + | + +T00.py:8:3: T001 Line contains FIXME + | + 8 | # hack: hack + 9 | # FIXME: fixme +10 | # fixme: fixme + | ^^^^^ T001 + | + + diff --git a/crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-hack_T00.py.snap b/crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-hack_T00.py.snap new file mode 100644 index 0000000000000..8125bd7c938cd --- /dev/null +++ b/crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-hack_T00.py.snap @@ -0,0 +1,24 @@ +--- +source: crates/ruff/src/rules/flake8_fixme/mod.rs +--- +T00.py:5:3: T004 Line contains HACK + | +5 | # XXX: xxx +6 | # xxx: xxx +7 | # HACK: hack + | ^^^^ T004 +8 | # hack: hack +9 | # FIXME: fixme + | + +T00.py:6:3: T004 Line contains HACK + | + 6 | # xxx: xxx + 7 | # HACK: hack + 8 | # hack: hack + | ^^^^ T004 + 9 | # FIXME: fixme +10 | # fixme: fixme + | + + diff --git a/crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-todo_T00.py.snap b/crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-todo_T00.py.snap new file mode 100644 index 0000000000000..73de9d6898bc9 --- /dev/null +++ b/crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-todo_T00.py.snap @@ -0,0 +1,21 @@ +--- +source: crates/ruff/src/rules/flake8_fixme/mod.rs +--- +T00.py:1:3: T002 Line contains TODO + | +1 | # TODO: todo + | ^^^^ T002 +2 | # todo: todo +3 | # XXX: xxx + | + +T00.py:2:3: T002 Line contains TODO + | +2 | # TODO: todo +3 | # todo: todo + | ^^^^ T002 +4 | # XXX: xxx +5 | # xxx: xxx + | + + diff --git a/crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-xxx_T00.py.snap b/crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-xxx_T00.py.snap new file mode 100644 index 0000000000000..9f826cdbdab91 --- /dev/null +++ b/crates/ruff/src/rules/flake8_fixme/snapshots/ruff__rules__flake8_fixme__tests__line-contains-xxx_T00.py.snap @@ -0,0 +1,24 @@ +--- +source: crates/ruff/src/rules/flake8_fixme/mod.rs +--- +T00.py:3:3: T003 Line contains XXX + | +3 | # TODO: todo +4 | # todo: todo +5 | # XXX: xxx + | ^^^ T003 +6 | # xxx: xxx +7 | # HACK: hack + | + +T00.py:4:3: T003 Line contains XXX + | +4 | # todo: todo +5 | # XXX: xxx +6 | # xxx: xxx + | ^^^ T003 +7 | # HACK: hack +8 | # hack: hack + | + + From 9693fa927029e9eda1fe64998b18f2a87fa4530e Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Sun, 28 May 2023 11:01:54 -0500 Subject: [PATCH 07/15] Clean up TodoDirective detection --- crates/ruff/src/directives.rs | 48 +++++++++++---------- crates/ruff/src/rules/flake8_fixme/rules.rs | 4 ++ 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/crates/ruff/src/directives.rs b/crates/ruff/src/directives.rs index 61fd170ed5c19..4337e4a55462a 100644 --- a/crates/ruff/src/directives.rs +++ b/crates/ruff/src/directives.rs @@ -230,13 +230,30 @@ impl FromStr for TodoDirective { type Err = (); fn from_str(s: &str) -> Result { - match s.to_lowercase().as_str() { - "fixme" => Ok(TodoDirective::Fixme), - "hack" => Ok(TodoDirective::Hack), - "todo" => Ok(TodoDirective::Todo), - "xxx" => Ok(TodoDirective::Xxx), - _ => Err(()), + // The lengths of the respective variant strings: TODO, FIXME, HACK, XXX + for length in [3, 4, 5] { + let Some(substr) = s.get(..length) else { + break; + }; + + match substr.to_lowercase().as_str() { + "fixme" => { + return Ok(TodoDirective::Fixme); + } + "hack" => { + return Ok(TodoDirective::Hack); + } + "todo" => { + return Ok(TodoDirective::Todo); + } + "xxx" => { + return Ok(TodoDirective::Xxx); + } + _ => continue, + } } + + Err(()) } } @@ -250,26 +267,13 @@ impl TodoDirective { // Loop over the comment to catch cases like `# foo # TODO`. while let Some(subset) = subset_opt { - let trimmed = subset.trim_start_matches('#').trim_start().to_lowercase(); + let trimmed = subset.trim_start_matches('#').trim_start(); let offset = subset.text_len() - trimmed.text_len(); total_offset += offset; - // TODO: rework this - let directive = if trimmed.starts_with("fixme") { - Some((TodoDirective::Fixme, total_offset)) - } else if trimmed.starts_with("xxx") { - Some((TodoDirective::Xxx, total_offset)) - } else if trimmed.starts_with("todo") { - Some((TodoDirective::Todo, total_offset)) - } else if trimmed.starts_with("hack") { - Some((TodoDirective::Hack, total_offset)) - } else { - None - }; - - if directive.is_some() { - return directive; + if let Ok(directive) = trimmed.parse::() { + return Some((directive, total_offset)); } // Shrink the subset to check for the next phrase starting with "#". diff --git a/crates/ruff/src/rules/flake8_fixme/rules.rs b/crates/ruff/src/rules/flake8_fixme/rules.rs index 09d3e10ac96a4..0e1811ce61cd0 100644 --- a/crates/ruff/src/rules/flake8_fixme/rules.rs +++ b/crates/ruff/src/rules/flake8_fixme/rules.rs @@ -44,9 +44,13 @@ pub fn todos(directive_ranges: Vec<(TodoDirective, TextRange)>) -> Vec Diagnostic::new(LineContainsFixme, range), + // T-002 TodoDirective::Hack => Diagnostic::new(LineContainsHack, range), + // T-003 TodoDirective::Todo => Diagnostic::new(LineContainsTodo, range), + // T-004 TodoDirective::Xxx => Diagnostic::new(LineContainsXxx, range), }) .collect::>() From a2babd386bf6b8a9b6a7b42351ec560e2a014226 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Tue, 30 May 2023 15:07:28 -0500 Subject: [PATCH 08/15] Add TodoComment and TodoDirective --- crates/ruff/src/directives.rs | 220 +++++++++++++++++++++++++++------- 1 file changed, 178 insertions(+), 42 deletions(-) diff --git a/crates/ruff/src/directives.rs b/crates/ruff/src/directives.rs index 4337e4a55462a..2aa673f05d7a5 100644 --- a/crates/ruff/src/directives.rs +++ b/crates/ruff/src/directives.rs @@ -219,14 +219,117 @@ fn extract_isort_directives(lxr: &[LexResult], locator: &Locator) -> IsortDirect } } -pub enum TodoDirective { +/// A comment that contains a [`TodoDirective`] +pub struct TodoComment<'a> { + /// The comment's text + pub content: &'a str, + /// The directive found within the comment. + pub directive: TodoDirective, + /// The comment's actual [`TextRange`]. + pub range: TextRange, + /// The comment range's position in [`Indexer`].comment_ranges() + pub range_index: usize, +} + +impl<'a> TodoComment<'a> { + /// Attempt to transform a normal comment into a [`TodoComment`]. + pub fn from_comment(content: &'a str, range: &TextRange, range_index: usize) -> Option { + TodoDirective::extract_directive(content, range).map(|directive| Self { + directive, + content, + range: *range, + range_index, + }) + } +} + +#[derive(Debug, PartialEq)] +pub struct TodoDirective { + /// The directive's literal text in the source code file. + pub content: String, + /// The directive's [`TextRange`] in the file. + pub range: TextRange, + /// The directive's kind: HACK, XXX, FIXME, or TODO. + pub kind: TodoDirectiveKind, + /// The directive's offset against the range of the comment that contains it. + pub relative_offset: TextSize, +} + +impl TodoDirective { + pub fn new( + content: String, + directive_range: TextRange, + kind: TodoDirectiveKind, + relative_offset: TextSize, + ) -> Self { + Self { + content, + range: directive_range, + kind, + relative_offset, + } + } + + /// Extract a [`TodoDirective`] from a comment. + pub fn extract_directive(string: &str, string_range: &TextRange) -> Option { + // The directive's offset from the start of the comment. + let mut relative_offset = TextSize::new(0); + let mut subset_opt = Some(string); + + // Loop over `#`-delimited sections of the comment to check for directives. This will + // correctly handle cases like `# foo # TODO`. + while let Some(subset) = subset_opt { + let trimmed = subset.trim_start_matches('#').trim_start(); + + let offset = subset.text_len() - trimmed.text_len(); + relative_offset += offset; + + // If we detect a TodoDirectiveKind variant substring in the comment, construct and + // return the appropriate TodoDirective + if let Ok(directive_kind) = trimmed.parse::() { + let directive_length = directive_kind.len(); + let content = string + .get( + relative_offset.to_usize()..(relative_offset + directive_length).to_usize(), + ) + .unwrap() + .to_string(); + + return Some(Self { + content, + range: TextRange::at(string_range.start() + relative_offset, directive_length), + kind: directive_kind, + relative_offset, + }); + } + + // Shrink the subset to check for the next phrase starting with "#". + subset_opt = if let Some(new_offset) = trimmed.find('#') { + relative_offset += TextSize::try_from(new_offset).unwrap(); + subset.get(relative_offset.to_usize()..) + } else { + None + }; + } + + None + } + + /// Returns the length of the directive's content. + pub fn len(&self) -> TextSize { + self.kind.len() + } +} + +#[derive(Debug, PartialEq)] +pub enum TodoDirectiveKind { Todo, Fixme, Xxx, Hack, } -impl FromStr for TodoDirective { +impl FromStr for TodoDirectiveKind { type Err = (); fn from_str(s: &str) -> Result { @@ -238,16 +341,16 @@ impl FromStr for TodoDirective { match substr.to_lowercase().as_str() { "fixme" => { - return Ok(TodoDirective::Fixme); + return Ok(TodoDirectiveKind::Fixme); } "hack" => { - return Ok(TodoDirective::Hack); + return Ok(TodoDirectiveKind::Hack); } "todo" => { - return Ok(TodoDirective::Todo); + return Ok(TodoDirectiveKind::Todo); } "xxx" => { - return Ok(TodoDirective::Xxx); + return Ok(TodoDirectiveKind::Xxx); } _ => continue, } @@ -257,43 +360,12 @@ impl FromStr for TodoDirective { } } -impl TodoDirective { - /// Extract a [`TodoDirective`] from a comment. - /// - /// Returns the offset of the directive within the comment, and the matching directive tag. - pub fn from_comment(comment: &str) -> Option<(TodoDirective, TextSize)> { - let mut subset_opt = Some(comment); - let mut total_offset = TextSize::new(0); - - // Loop over the comment to catch cases like `# foo # TODO`. - while let Some(subset) = subset_opt { - let trimmed = subset.trim_start_matches('#').trim_start(); - - let offset = subset.text_len() - trimmed.text_len(); - total_offset += offset; - - if let Ok(directive) = trimmed.parse::() { - return Some((directive, total_offset)); - } - - // Shrink the subset to check for the next phrase starting with "#". - subset_opt = if let Some(new_offset) = trimmed.find('#') { - total_offset += TextSize::try_from(new_offset).unwrap(); - subset.get(total_offset.to_usize()..) - } else { - None - }; - } - - None - } - - /// Returns the length of the directive tag. +impl TodoDirectiveKind { pub fn len(&self) -> TextSize { match self { - TodoDirective::Fixme => TextSize::new(5), - TodoDirective::Todo | TodoDirective::Hack => TextSize::new(4), - TodoDirective::Xxx => TextSize::new(3), + TodoDirectiveKind::Xxx => TextSize::new(3), + TodoDirectiveKind::Hack | TodoDirectiveKind::Todo => TextSize::new(4), + TodoDirectiveKind::Fixme => TextSize::new(5), } } } @@ -306,7 +378,9 @@ mod tests { use ruff_python_ast::source_code::{Indexer, Locator}; - use crate::directives::{extract_isort_directives, extract_noqa_line_for}; + use crate::directives::{ + extract_isort_directives, extract_noqa_line_for, TodoDirective, TodoDirectiveKind, + }; use crate::noqa::NoqaMapping; fn noqa_mappings(contents: &str) -> NoqaMapping { @@ -509,4 +583,66 @@ z = x + 1"; vec![TextSize::from(13)] ); } + + #[test] + fn todo_directives() { + let test_comment = "# TODO: todo tag"; + let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len()); + let expected = TodoDirective { + content: String::from("TODO"), + range: TextRange::new(TextSize::new(2), TextSize::new(6)), + kind: TodoDirectiveKind::Todo, + relative_offset: TextSize::new(2), + }; + assert_eq!( + expected, + TodoDirective::extract_directive(test_comment, &test_comment_range).unwrap() + ); + + let test_comment = "#TODO: todo tag"; + let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len()); + let expected = TodoDirective { + content: String::from("TODO"), + range: TextRange::new(TextSize::new(1), TextSize::new(5)), + kind: TodoDirectiveKind::Todo, + relative_offset: TextSize::new(1), + }; + assert_eq!( + expected, + TodoDirective::extract_directive(test_comment, &test_comment_range).unwrap() + ); + + let test_comment = "# fixme: fixme tag"; + let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len()); + let expected = TodoDirective { + content: String::from("fixme"), + range: TextRange::new(TextSize::new(2), TextSize::new(7)), + kind: TodoDirectiveKind::Fixme, + relative_offset: TextSize::new(2), + }; + assert_eq!( + expected, + TodoDirective::extract_directive(test_comment, &test_comment_range).unwrap() + ); + + let test_comment = "# noqa # TODO: todo"; + let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len()); + let expected = TodoDirective { + content: String::from("TODO"), + range: TextRange::new(TextSize::new(9), TextSize::new(13)), + kind: TodoDirectiveKind::Todo, + relative_offset: TextSize::new(9), + }; + assert_eq!( + expected, + TodoDirective::extract_directive(test_comment, &test_comment_range).unwrap() + ); + + let test_comment = "# no directive"; + let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len()); + assert_eq!( + None, + TodoDirective::extract_directive(test_comment, &test_comment_range) + ); + } } From 11e9c69e3fa11e28ab593b322f3176359404832a Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Tue, 30 May 2023 15:07:47 -0500 Subject: [PATCH 09/15] Refactor flake8_fixme to work with TodoComment/TodoDirective --- crates/ruff/src/checkers/tokens.rs | 38 +++++++-------------- crates/ruff/src/rules/flake8_fixme/rules.rs | 17 +++++---- 2 files changed, 20 insertions(+), 35 deletions(-) diff --git a/crates/ruff/src/checkers/tokens.rs b/crates/ruff/src/checkers/tokens.rs index a3c4d1e0e225b..c9a48aacdab30 100644 --- a/crates/ruff/src/checkers/tokens.rs +++ b/crates/ruff/src/checkers/tokens.rs @@ -1,10 +1,9 @@ //! Lint rules based on token traversal. -use ruff_text_size::TextRange; use rustpython_parser::lexer::LexResult; use rustpython_parser::Tok; -use crate::directives::TodoDirective; +use crate::directives::TodoComment; use crate::lex::docstring_detection::StateMachine; use crate::registry::{AsRule, Rule}; use crate::rules::ruff::rules::Context; @@ -194,37 +193,24 @@ pub(crate) fn check_tokens( // TD001, TD002, TD003, TD004, TD005, TD006, TD007 // T001, T002, T003, T004 if enforce_todos { - // The TextRange of the comment, its position in comment_ranges, and the directive's - // TextRange - let mut other_directive_ranges: Vec<(TextRange, usize, TextRange)> = vec![]; - let mut flake8_fixme_directive_ranges: Vec<(TodoDirective, TextRange)> = vec![]; - - // Find all TodoDirectives - for (i, comment_range) in indexer.comment_ranges().iter().enumerate() { - let comment = locator.slice(*comment_range); - let Some((directive, relative_offset)) = TodoDirective::from_comment(comment) else { - continue; - }; - - let directive_range = - TextRange::at(comment_range.start() + relative_offset, directive.len()); - - // TODO, XXX, and FIXME directives are supported by flake8_todos. flake8_fixme supports - // all 4 TodoDirective variants. - if !matches!(directive, TodoDirective::Hack) { - other_directive_ranges.push((*comment_range, i, directive_range)); - } - flake8_fixme_directive_ranges.push((directive, directive_range)); - } + let todo_comments: Vec = indexer + .comment_ranges() + .iter() + .enumerate() + .filter_map(|(i, comment_range)| { + let comment = locator.slice(*comment_range); + TodoComment::from_comment(comment, comment_range, i).map(|comment| comment) + }) + .collect::>(); diagnostics.extend( - flake8_todos::rules::todos(other_directive_ranges, indexer, locator, settings) + flake8_todos::rules::todos(&todo_comments, indexer, locator, settings) .into_iter() .filter(|diagnostic| settings.rules.enabled(diagnostic.kind.rule())), ); diagnostics.extend( - flake8_fixme::rules::todos(flake8_fixme_directive_ranges) + flake8_fixme::rules::todos(&todo_comments) .into_iter() .filter(|diagnostic| settings.rules.enabled(diagnostic.kind.rule())), ); diff --git a/crates/ruff/src/rules/flake8_fixme/rules.rs b/crates/ruff/src/rules/flake8_fixme/rules.rs index 0e1811ce61cd0..e706063f24a57 100644 --- a/crates/ruff/src/rules/flake8_fixme/rules.rs +++ b/crates/ruff/src/rules/flake8_fixme/rules.rs @@ -1,8 +1,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_text_size::TextRange; -use crate::directives::TodoDirective; +use crate::directives::{TodoComment, TodoDirectiveKind}; #[violation] pub struct LineContainsTodo; @@ -40,18 +39,18 @@ impl Violation for LineContainsHack { } } -pub fn todos(directive_ranges: Vec<(TodoDirective, TextRange)>) -> Vec { +pub fn todos(directive_ranges: &Vec) -> Vec { directive_ranges - .into_iter() - .map(|(directive, range)| match directive { + .iter() + .map(|TodoComment { directive, .. }| match directive.kind { // T-001 - TodoDirective::Fixme => Diagnostic::new(LineContainsFixme, range), + TodoDirectiveKind::Fixme => Diagnostic::new(LineContainsFixme, directive.range), // T-002 - TodoDirective::Hack => Diagnostic::new(LineContainsHack, range), + TodoDirectiveKind::Hack => Diagnostic::new(LineContainsHack, directive.range), // T-003 - TodoDirective::Todo => Diagnostic::new(LineContainsTodo, range), + TodoDirectiveKind::Todo => Diagnostic::new(LineContainsTodo, directive.range), // T-004 - TodoDirective::Xxx => Diagnostic::new(LineContainsXxx, range), + TodoDirectiveKind::Xxx => Diagnostic::new(LineContainsXxx, directive.range), }) .collect::>() } From 62742d159e3d20508591fb7fafa39110209c9656 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Tue, 30 May 2023 15:08:11 -0500 Subject: [PATCH 10/15] Refactor flake8_todos to work with TodoComment/TodoDirective --- .../src/rules/flake8_todos/rules/todos.rs | 69 ++++++++++--------- 1 file changed, 37 insertions(+), 32 deletions(-) diff --git a/crates/ruff/src/rules/flake8_todos/rules/todos.rs b/crates/ruff/src/rules/flake8_todos/rules/todos.rs index 4fef5928a114f..16bb4c9cde969 100644 --- a/crates/ruff/src/rules/flake8_todos/rules/todos.rs +++ b/crates/ruff/src/rules/flake8_todos/rules/todos.rs @@ -6,7 +6,11 @@ use ruff_text_size::{TextLen, TextRange, TextSize}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; -use crate::{directives::TodoDirective, registry::Rule, settings::Settings}; +use crate::{ + directives::{TodoComment, TodoDirective}, + registry::Rule, + settings::Settings, +}; /// ## What it does /// Checks that a TODO comment is labelled with "TODO". @@ -240,26 +244,27 @@ static ISSUE_LINK_REGEX_SET: Lazy = Lazy::new(|| { }); pub(crate) fn todos( - comment_directive_ranges: Vec<(TextRange, usize, TextRange)>, + todo_comments: &Vec, indexer: &Indexer, locator: &Locator, settings: &Settings, ) -> Vec { let mut diagnostics: Vec = vec![]; - for (comment_range, comment_index, directive_range) in comment_directive_ranges { - let comment = locator.slice(comment_range); - let tag = Tag { - content: locator.slice(directive_range), - range: directive_range, - }; + for todo_comment in todo_comments { + let TodoComment { + directive, + content, + range, + range_index, + } = todo_comment; - tag_errors(&tag, &mut diagnostics, settings); - static_errors(&mut diagnostics, comment, comment_range, &tag); + tag_errors(directive, &mut diagnostics, settings); + static_errors(&mut diagnostics, content, *range, directive); let mut has_issue_link = false; - let mut curr_range = &comment_range; - for next_range in indexer.comment_ranges().iter().skip(comment_index + 1) { + let mut curr_range = range; + for next_range in indexer.comment_ranges().iter().skip(range_index + 1) { // Ensure that next_comment_range is in the same multiline comment "block" as // comment_range. if !locator @@ -271,7 +276,7 @@ pub(crate) fn todos( } let next_comment = locator.slice(*next_range); - if TodoDirective::from_comment(next_comment).is_some() { + if TodoDirective::extract_directive(next_comment, next_range).is_some() { break; } @@ -287,7 +292,7 @@ pub(crate) fn todos( if !has_issue_link { // TD-003 - diagnostics.push(Diagnostic::new(MissingTodoLink, tag.range)); + diagnostics.push(Diagnostic::new(MissingTodoLink, directive.range)); } } @@ -295,24 +300,24 @@ pub(crate) fn todos( } /// Check that the tag itself is valid. This function modifies `diagnostics` in-place. -fn tag_errors(tag: &Tag, diagnostics: &mut Vec, settings: &Settings) { - if tag.content == "TODO" { +fn tag_errors(directive: &TodoDirective, diagnostics: &mut Vec, settings: &Settings) { + if directive.content == "TODO" { return; } - if tag.content.to_uppercase() == "TODO" { + if directive.content.to_uppercase() == "TODO" { // TD006 let mut diagnostic = Diagnostic::new( InvalidTodoCapitalization { - tag: tag.content.to_string(), + tag: directive.content.to_string(), }, - tag.range, + directive.range, ); if settings.rules.should_fix(Rule::InvalidTodoCapitalization) { diagnostic.set_fix(Fix::automatic(Edit::range_replacement( "TODO".to_string(), - tag.range, + directive.range, ))); } @@ -321,9 +326,9 @@ fn tag_errors(tag: &Tag, diagnostics: &mut Vec, settings: &Settings) // TD001 diagnostics.push(Diagnostic::new( InvalidTodoTag { - tag: tag.content.to_string(), + tag: directive.content.to_string(), }, - tag.range, + directive.range, )); } } @@ -334,11 +339,11 @@ fn static_errors( diagnostics: &mut Vec, comment: &str, comment_range: TextRange, - tag: &Tag, + directive: &TodoDirective, ) { - let post_tag = &comment[usize::from(tag.range.end() - comment_range.start())..]; - let trimmed = post_tag.trim_start(); - let content_offset = post_tag.text_len() - trimmed.text_len(); + let post_directive = &comment[usize::from(directive.range.end() - comment_range.start())..]; + let trimmed = post_directive.trim_start(); + let content_offset = post_directive.text_len() - trimmed.text_len(); let author_end = content_offset + if trimmed.starts_with('(') { @@ -349,27 +354,27 @@ fn static_errors( } } else { // TD-002 - diagnostics.push(Diagnostic::new(MissingTodoAuthor, tag.range)); + diagnostics.push(Diagnostic::new(MissingTodoAuthor, directive.range)); TextSize::new(0) }; - let after_author = &post_tag[usize::from(author_end)..]; + let after_author = &post_directive[usize::from(author_end)..]; if let Some(after_colon) = after_author.strip_prefix(':') { if after_colon.is_empty() { // TD-005 - diagnostics.push(Diagnostic::new(MissingTodoDescription, tag.range)); + diagnostics.push(Diagnostic::new(MissingTodoDescription, directive.range)); } else if !after_colon.starts_with(char::is_whitespace) { // TD-007 - diagnostics.push(Diagnostic::new(MissingSpaceAfterTodoColon, tag.range)); + diagnostics.push(Diagnostic::new(MissingSpaceAfterTodoColon, directive.range)); } } else { // TD-004 - diagnostics.push(Diagnostic::new(MissingTodoColon, tag.range)); + diagnostics.push(Diagnostic::new(MissingTodoColon, directive.range)); if after_author.is_empty() { // TD-005 - diagnostics.push(Diagnostic::new(MissingTodoDescription, tag.range)); + diagnostics.push(Diagnostic::new(MissingTodoDescription, directive.range)); } } } From 1aaebb79e6eaf1ad330844afe33ac0b2205ddaae Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Tue, 30 May 2023 15:12:40 -0500 Subject: [PATCH 11/15] Remove HACK support from flake8_todos --- crates/ruff/src/rules/flake8_todos/rules/todos.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/flake8_todos/rules/todos.rs b/crates/ruff/src/rules/flake8_todos/rules/todos.rs index 16bb4c9cde969..fa6658ef631f5 100644 --- a/crates/ruff/src/rules/flake8_todos/rules/todos.rs +++ b/crates/ruff/src/rules/flake8_todos/rules/todos.rs @@ -7,7 +7,7 @@ use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix, Violat use ruff_macros::{derive_message_formats, violation}; use crate::{ - directives::{TodoComment, TodoDirective}, + directives::{TodoComment, TodoDirective, TodoDirectiveKind}, registry::Rule, settings::Settings, }; @@ -259,6 +259,11 @@ pub(crate) fn todos( range_index, } = todo_comment; + // flake8-todos doesn't support HACK directives. + if matches!(directive.kind, TodoDirectiveKind::Hack) { + continue; + } + tag_errors(directive, &mut diagnostics, settings); static_errors(&mut diagnostics, content, *range, directive); From 4b8e28e20cafd241698ca9a66db116085f08e502 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Tue, 30 May 2023 15:27:00 -0500 Subject: [PATCH 12/15] Move relative_offset onto TodoComment --- crates/ruff/src/directives.rs | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/crates/ruff/src/directives.rs b/crates/ruff/src/directives.rs index 2aa673f05d7a5..072f00e9cbdd3 100644 --- a/crates/ruff/src/directives.rs +++ b/crates/ruff/src/directives.rs @@ -241,6 +241,12 @@ impl<'a> TodoComment<'a> { range_index, }) } + + /// Determine the starting location for the [`TodoDirective`], relative to the comment's + /// starting offset + pub fn directive_offset(&self) -> TextSize { + self.directive.range.start() - self.range.start() + } } #[derive(Debug, PartialEq)] @@ -251,22 +257,14 @@ pub struct TodoDirective { pub range: TextRange, /// The directive's kind: HACK, XXX, FIXME, or TODO. pub kind: TodoDirectiveKind, - /// The directive's offset against the range of the comment that contains it. - pub relative_offset: TextSize, } impl TodoDirective { - pub fn new( - content: String, - directive_range: TextRange, - kind: TodoDirectiveKind, - relative_offset: TextSize, - ) -> Self { + pub fn new(content: String, directive_range: TextRange, kind: TodoDirectiveKind) -> Self { Self { content, range: directive_range, kind, - relative_offset, } } @@ -299,7 +297,6 @@ impl TodoDirective { content, range: TextRange::at(string_range.start() + relative_offset, directive_length), kind: directive_kind, - relative_offset, }); } @@ -592,7 +589,6 @@ z = x + 1"; content: String::from("TODO"), range: TextRange::new(TextSize::new(2), TextSize::new(6)), kind: TodoDirectiveKind::Todo, - relative_offset: TextSize::new(2), }; assert_eq!( expected, @@ -605,7 +601,6 @@ z = x + 1"; content: String::from("TODO"), range: TextRange::new(TextSize::new(1), TextSize::new(5)), kind: TodoDirectiveKind::Todo, - relative_offset: TextSize::new(1), }; assert_eq!( expected, @@ -618,7 +613,6 @@ z = x + 1"; content: String::from("fixme"), range: TextRange::new(TextSize::new(2), TextSize::new(7)), kind: TodoDirectiveKind::Fixme, - relative_offset: TextSize::new(2), }; assert_eq!( expected, @@ -631,7 +625,6 @@ z = x + 1"; content: String::from("TODO"), range: TextRange::new(TextSize::new(9), TextSize::new(13)), kind: TodoDirectiveKind::Todo, - relative_offset: TextSize::new(9), }; assert_eq!( expected, From 6a81c486398e423ff4714d8bc284e355a892f4a1 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Tue, 30 May 2023 16:39:37 -0500 Subject: [PATCH 13/15] Fix clippy --- crates/ruff/src/checkers/tokens.rs | 2 +- crates/ruff/src/rules/flake8_fixme/rules.rs | 2 +- crates/ruff/src/rules/flake8_todos/rules/todos.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ruff/src/checkers/tokens.rs b/crates/ruff/src/checkers/tokens.rs index c9a48aacdab30..250f5e411487c 100644 --- a/crates/ruff/src/checkers/tokens.rs +++ b/crates/ruff/src/checkers/tokens.rs @@ -199,7 +199,7 @@ pub(crate) fn check_tokens( .enumerate() .filter_map(|(i, comment_range)| { let comment = locator.slice(*comment_range); - TodoComment::from_comment(comment, comment_range, i).map(|comment| comment) + TodoComment::from_comment(comment, comment_range, i) }) .collect::>(); diff --git a/crates/ruff/src/rules/flake8_fixme/rules.rs b/crates/ruff/src/rules/flake8_fixme/rules.rs index e706063f24a57..39771cec54d5c 100644 --- a/crates/ruff/src/rules/flake8_fixme/rules.rs +++ b/crates/ruff/src/rules/flake8_fixme/rules.rs @@ -39,7 +39,7 @@ impl Violation for LineContainsHack { } } -pub fn todos(directive_ranges: &Vec) -> Vec { +pub fn todos(directive_ranges: &[TodoComment]) -> Vec { directive_ranges .iter() .map(|TodoComment { directive, .. }| match directive.kind { diff --git a/crates/ruff/src/rules/flake8_todos/rules/todos.rs b/crates/ruff/src/rules/flake8_todos/rules/todos.rs index fa6658ef631f5..d57c10745b787 100644 --- a/crates/ruff/src/rules/flake8_todos/rules/todos.rs +++ b/crates/ruff/src/rules/flake8_todos/rules/todos.rs @@ -244,7 +244,7 @@ static ISSUE_LINK_REGEX_SET: Lazy = Lazy::new(|| { }); pub(crate) fn todos( - todo_comments: &Vec, + todo_comments: &[TodoComment], indexer: &Indexer, locator: &Locator, settings: &Settings, From 4be1cddcb8eb2cbaa365675223411efc45520151 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Wed, 31 May 2023 09:52:07 -0500 Subject: [PATCH 14/15] Implement reviewer comments --- crates/ruff/src/checkers/tokens.rs | 2 +- crates/ruff/src/directives.rs | 54 +++++++------------ .../src/rules/flake8_todos/rules/todos.rs | 12 +++-- 3 files changed, 29 insertions(+), 39 deletions(-) diff --git a/crates/ruff/src/checkers/tokens.rs b/crates/ruff/src/checkers/tokens.rs index 250f5e411487c..a02c599955f75 100644 --- a/crates/ruff/src/checkers/tokens.rs +++ b/crates/ruff/src/checkers/tokens.rs @@ -201,7 +201,7 @@ pub(crate) fn check_tokens( let comment = locator.slice(*comment_range); TodoComment::from_comment(comment, comment_range, i) }) - .collect::>(); + .collect(); diagnostics.extend( flake8_todos::rules::todos(&todo_comments, indexer, locator, settings) diff --git a/crates/ruff/src/directives.rs b/crates/ruff/src/directives.rs index 072f00e9cbdd3..1c962d4984343 100644 --- a/crates/ruff/src/directives.rs +++ b/crates/ruff/src/directives.rs @@ -224,7 +224,7 @@ pub struct TodoComment<'a> { /// The comment's text pub content: &'a str, /// The directive found within the comment. - pub directive: TodoDirective, + pub directive: TodoDirective<'a>, /// The comment's actual [`TextRange`]. pub range: TextRange, /// The comment range's position in [`Indexer`].comment_ranges() @@ -234,7 +234,7 @@ pub struct TodoComment<'a> { impl<'a> TodoComment<'a> { /// Attempt to transform a normal comment into a [`TodoComment`]. pub fn from_comment(content: &'a str, range: &TextRange, range_index: usize) -> Option { - TodoDirective::extract_directive(content, range).map(|directive| Self { + TodoDirective::from_comment(content, range).map(|directive| Self { directive, content, range: *range, @@ -250,29 +250,21 @@ impl<'a> TodoComment<'a> { } #[derive(Debug, PartialEq)] -pub struct TodoDirective { - /// The directive's literal text in the source code file. - pub content: String, +pub struct TodoDirective<'a> { + /// The actual directive + pub content: &'a str, /// The directive's [`TextRange`] in the file. pub range: TextRange, /// The directive's kind: HACK, XXX, FIXME, or TODO. pub kind: TodoDirectiveKind, } -impl TodoDirective { - pub fn new(content: String, directive_range: TextRange, kind: TodoDirectiveKind) -> Self { - Self { - content, - range: directive_range, - kind, - } - } - +impl<'a> TodoDirective<'a> { /// Extract a [`TodoDirective`] from a comment. - pub fn extract_directive(string: &str, string_range: &TextRange) -> Option { + pub fn from_comment(comment: &'a str, comment_range: &TextRange) -> Option { // The directive's offset from the start of the comment. let mut relative_offset = TextSize::new(0); - let mut subset_opt = Some(string); + let mut subset_opt = Some(comment); // Loop over `#`-delimited sections of the comment to check for directives. This will // correctly handle cases like `# foo # TODO`. @@ -285,17 +277,11 @@ impl TodoDirective { // If we detect a TodoDirectiveKind variant substring in the comment, construct and // return the appropriate TodoDirective if let Ok(directive_kind) = trimmed.parse::() { - let directive_length = directive_kind.len(); - let content = string - .get( - relative_offset.to_usize()..(relative_offset + directive_length).to_usize(), - ) - .unwrap() - .to_string(); + let len = directive_kind.len(); return Some(Self { - content, - range: TextRange::at(string_range.start() + relative_offset, directive_length), + content: &comment[TextRange::at(relative_offset, len)], + range: TextRange::at(comment_range.start() + relative_offset, len), kind: directive_kind, }); } @@ -586,56 +572,56 @@ z = x + 1"; let test_comment = "# TODO: todo tag"; let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len()); let expected = TodoDirective { - content: String::from("TODO"), + content: "TODO", range: TextRange::new(TextSize::new(2), TextSize::new(6)), kind: TodoDirectiveKind::Todo, }; assert_eq!( expected, - TodoDirective::extract_directive(test_comment, &test_comment_range).unwrap() + TodoDirective::from_comment(test_comment, &test_comment_range).unwrap() ); let test_comment = "#TODO: todo tag"; let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len()); let expected = TodoDirective { - content: String::from("TODO"), + content: "TODO", range: TextRange::new(TextSize::new(1), TextSize::new(5)), kind: TodoDirectiveKind::Todo, }; assert_eq!( expected, - TodoDirective::extract_directive(test_comment, &test_comment_range).unwrap() + TodoDirective::from_comment(test_comment, &test_comment_range).unwrap() ); let test_comment = "# fixme: fixme tag"; let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len()); let expected = TodoDirective { - content: String::from("fixme"), + content: "fixme", range: TextRange::new(TextSize::new(2), TextSize::new(7)), kind: TodoDirectiveKind::Fixme, }; assert_eq!( expected, - TodoDirective::extract_directive(test_comment, &test_comment_range).unwrap() + TodoDirective::from_comment(test_comment, &test_comment_range).unwrap() ); let test_comment = "# noqa # TODO: todo"; let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len()); let expected = TodoDirective { - content: String::from("TODO"), + content: "TODO", range: TextRange::new(TextSize::new(9), TextSize::new(13)), kind: TodoDirectiveKind::Todo, }; assert_eq!( expected, - TodoDirective::extract_directive(test_comment, &test_comment_range).unwrap() + TodoDirective::from_comment(test_comment, &test_comment_range).unwrap() ); let test_comment = "# no directive"; let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len()); assert_eq!( None, - TodoDirective::extract_directive(test_comment, &test_comment_range) + TodoDirective::from_comment(test_comment, &test_comment_range) ); } } diff --git a/crates/ruff/src/rules/flake8_todos/rules/todos.rs b/crates/ruff/src/rules/flake8_todos/rules/todos.rs index d57c10745b787..f3aa9608a498a 100644 --- a/crates/ruff/src/rules/flake8_todos/rules/todos.rs +++ b/crates/ruff/src/rules/flake8_todos/rules/todos.rs @@ -264,7 +264,7 @@ pub(crate) fn todos( continue; } - tag_errors(directive, &mut diagnostics, settings); + directive_errors(directive, &mut diagnostics, settings); static_errors(&mut diagnostics, content, *range, directive); let mut has_issue_link = false; @@ -281,7 +281,7 @@ pub(crate) fn todos( } let next_comment = locator.slice(*next_range); - if TodoDirective::extract_directive(next_comment, next_range).is_some() { + if TodoDirective::from_comment(next_comment, next_range).is_some() { break; } @@ -304,8 +304,12 @@ pub(crate) fn todos( diagnostics } -/// Check that the tag itself is valid. This function modifies `diagnostics` in-place. -fn tag_errors(directive: &TodoDirective, diagnostics: &mut Vec, settings: &Settings) { +/// Check that the directive itself is valid. This function modifies `diagnostics` in-place. +fn directive_errors( + directive: &TodoDirective, + diagnostics: &mut Vec, + settings: &Settings, +) { if directive.content == "TODO" { return; } From e438c9d4fdb07df2b74cc0d77eab5bb12094937d Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Thu, 1 Jun 2023 11:41:07 -0500 Subject: [PATCH 15/15] Change visibility to pub(crate), fix clippy --- crates/ruff/src/checkers/tokens.rs | 2 +- crates/ruff/src/directives.rs | 55 ++++++++----------- crates/ruff/src/rules/flake8_fixme/rules.rs | 2 +- .../src/rules/flake8_todos/rules/todos.rs | 23 ++++---- 4 files changed, 36 insertions(+), 46 deletions(-) diff --git a/crates/ruff/src/checkers/tokens.rs b/crates/ruff/src/checkers/tokens.rs index a02c599955f75..4f71610a7cdbd 100644 --- a/crates/ruff/src/checkers/tokens.rs +++ b/crates/ruff/src/checkers/tokens.rs @@ -199,7 +199,7 @@ pub(crate) fn check_tokens( .enumerate() .filter_map(|(i, comment_range)| { let comment = locator.slice(*comment_range); - TodoComment::from_comment(comment, comment_range, i) + TodoComment::from_comment(comment, *comment_range, i) }) .collect(); diff --git a/crates/ruff/src/directives.rs b/crates/ruff/src/directives.rs index 1c962d4984343..2f3c0187ac4a9 100644 --- a/crates/ruff/src/directives.rs +++ b/crates/ruff/src/directives.rs @@ -220,48 +220,46 @@ fn extract_isort_directives(lxr: &[LexResult], locator: &Locator) -> IsortDirect } /// A comment that contains a [`TodoDirective`] -pub struct TodoComment<'a> { +pub(crate) struct TodoComment<'a> { /// The comment's text - pub content: &'a str, + pub(crate) content: &'a str, /// The directive found within the comment. - pub directive: TodoDirective<'a>, + pub(crate) directive: TodoDirective<'a>, /// The comment's actual [`TextRange`]. - pub range: TextRange, + pub(crate) range: TextRange, /// The comment range's position in [`Indexer`].comment_ranges() - pub range_index: usize, + pub(crate) range_index: usize, } impl<'a> TodoComment<'a> { /// Attempt to transform a normal comment into a [`TodoComment`]. - pub fn from_comment(content: &'a str, range: &TextRange, range_index: usize) -> Option { + pub(crate) fn from_comment( + content: &'a str, + range: TextRange, + range_index: usize, + ) -> Option { TodoDirective::from_comment(content, range).map(|directive| Self { - directive, content, - range: *range, + directive, + range, range_index, }) } - - /// Determine the starting location for the [`TodoDirective`], relative to the comment's - /// starting offset - pub fn directive_offset(&self) -> TextSize { - self.directive.range.start() - self.range.start() - } } #[derive(Debug, PartialEq)] -pub struct TodoDirective<'a> { +pub(crate) struct TodoDirective<'a> { /// The actual directive - pub content: &'a str, + pub(crate) content: &'a str, /// The directive's [`TextRange`] in the file. - pub range: TextRange, + pub(crate) range: TextRange, /// The directive's kind: HACK, XXX, FIXME, or TODO. - pub kind: TodoDirectiveKind, + pub(crate) kind: TodoDirectiveKind, } impl<'a> TodoDirective<'a> { /// Extract a [`TodoDirective`] from a comment. - pub fn from_comment(comment: &'a str, comment_range: &TextRange) -> Option { + pub(crate) fn from_comment(comment: &'a str, comment_range: TextRange) -> Option { // The directive's offset from the start of the comment. let mut relative_offset = TextSize::new(0); let mut subset_opt = Some(comment); @@ -297,15 +295,10 @@ impl<'a> TodoDirective<'a> { None } - - /// Returns the length of the directive's content. - pub fn len(&self) -> TextSize { - self.kind.len() - } } #[derive(Debug, PartialEq)] -pub enum TodoDirectiveKind { +pub(crate) enum TodoDirectiveKind { Todo, Fixme, Xxx, @@ -344,7 +337,7 @@ impl FromStr for TodoDirectiveKind { } impl TodoDirectiveKind { - pub fn len(&self) -> TextSize { + fn len(&self) -> TextSize { match self { TodoDirectiveKind::Xxx => TextSize::new(3), TodoDirectiveKind::Hack | TodoDirectiveKind::Todo => TextSize::new(4), @@ -578,7 +571,7 @@ z = x + 1"; }; assert_eq!( expected, - TodoDirective::from_comment(test_comment, &test_comment_range).unwrap() + TodoDirective::from_comment(test_comment, test_comment_range).unwrap() ); let test_comment = "#TODO: todo tag"; @@ -590,7 +583,7 @@ z = x + 1"; }; assert_eq!( expected, - TodoDirective::from_comment(test_comment, &test_comment_range).unwrap() + TodoDirective::from_comment(test_comment, test_comment_range).unwrap() ); let test_comment = "# fixme: fixme tag"; @@ -602,7 +595,7 @@ z = x + 1"; }; assert_eq!( expected, - TodoDirective::from_comment(test_comment, &test_comment_range).unwrap() + TodoDirective::from_comment(test_comment, test_comment_range).unwrap() ); let test_comment = "# noqa # TODO: todo"; @@ -614,14 +607,14 @@ z = x + 1"; }; assert_eq!( expected, - TodoDirective::from_comment(test_comment, &test_comment_range).unwrap() + TodoDirective::from_comment(test_comment, test_comment_range).unwrap() ); let test_comment = "# no directive"; let test_comment_range = TextRange::at(TextSize::new(0), test_comment.text_len()); assert_eq!( None, - TodoDirective::from_comment(test_comment, &test_comment_range) + TodoDirective::from_comment(test_comment, test_comment_range) ); } } diff --git a/crates/ruff/src/rules/flake8_fixme/rules.rs b/crates/ruff/src/rules/flake8_fixme/rules.rs index 39771cec54d5c..1d6d4d05c234d 100644 --- a/crates/ruff/src/rules/flake8_fixme/rules.rs +++ b/crates/ruff/src/rules/flake8_fixme/rules.rs @@ -39,7 +39,7 @@ impl Violation for LineContainsHack { } } -pub fn todos(directive_ranges: &[TodoComment]) -> Vec { +pub(crate) fn todos(directive_ranges: &[TodoComment]) -> Vec { directive_ranges .iter() .map(|TodoComment { directive, .. }| match directive.kind { diff --git a/crates/ruff/src/rules/flake8_todos/rules/todos.rs b/crates/ruff/src/rules/flake8_todos/rules/todos.rs index f3aa9608a498a..bbc456a9f1d6d 100644 --- a/crates/ruff/src/rules/flake8_todos/rules/todos.rs +++ b/crates/ruff/src/rules/flake8_todos/rules/todos.rs @@ -225,15 +225,6 @@ impl Violation for MissingSpaceAfterTodoColon { } } -// If this struct ever gets pushed outside of this module, it may be worth creating an enum for -// the different tag types + other convenience methods. -/// Represents a TODO tag or any of its variants - FIXME, XXX, TODO. -#[derive(Debug, PartialEq, Eq)] -struct Tag<'a> { - range: TextRange, - content: &'a str, -} - static ISSUE_LINK_REGEX_SET: Lazy = Lazy::new(|| { RegexSet::new([ r#"^#\s*(http|https)://.*"#, // issue link @@ -255,9 +246,10 @@ pub(crate) fn todos( let TodoComment { directive, content, - range, range_index, + .. } = todo_comment; + let range = todo_comment.range; // flake8-todos doesn't support HACK directives. if matches!(directive.kind, TodoDirectiveKind::Hack) { @@ -265,11 +257,16 @@ pub(crate) fn todos( } directive_errors(directive, &mut diagnostics, settings); - static_errors(&mut diagnostics, content, *range, directive); + static_errors(&mut diagnostics, content, range, directive); let mut has_issue_link = false; let mut curr_range = range; - for next_range in indexer.comment_ranges().iter().skip(range_index + 1) { + for next_range in indexer + .comment_ranges() + .iter() + .skip(range_index + 1) + .copied() + { // Ensure that next_comment_range is in the same multiline comment "block" as // comment_range. if !locator @@ -280,7 +277,7 @@ pub(crate) fn todos( break; } - let next_comment = locator.slice(*next_range); + let next_comment = locator.slice(next_range); if TodoDirective::from_comment(next_comment, next_range).is_some() { break; }