Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement flake8_fixme and refactor TodoDirective #4681

Merged
merged 15 commits into from Jun 2, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions 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
30 changes: 27 additions & 3 deletions crates/ruff/src/checkers/tokens.rs
Expand Up @@ -3,12 +3,13 @@
use rustpython_parser::lexer::LexResult;
use rustpython_parser::Tok;

use crate::directives::TodoComment;
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;
Expand Down Expand Up @@ -60,6 +61,8 @@ 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.
let enforce_todos = settings.rules.any_enabled(&[
Rule::InvalidTodoTag,
Rule::MissingTodoAuthor,
Expand All @@ -68,6 +71,10 @@ pub(crate) fn check_tokens(
Rule::MissingTodoDescription,
Rule::InvalidTodoCapitalization,
Rule::MissingSpaceAfterTodoColon,
Rule::LineContainsFixme,
Rule::LineContainsXxx,
Rule::LineContainsTodo,
Rule::LineContainsHack,
]);

// RUF001, RUF002, RUF003
Expand Down Expand Up @@ -184,9 +191,26 @@ pub(crate) fn check_tokens(
}

// TD001, TD002, TD003, TD004, TD005, TD006, TD007
// T001, T002, T003, T004
if enforce_todos {
let todo_comments: Vec<TodoComment> = indexer
.comment_ranges()
.iter()
.enumerate()
.filter_map(|(i, comment_range)| {
let comment = locator.slice(*comment_range);
TodoComment::from_comment(comment, comment_range, i)
})
.collect::<Vec<TodoComment>>();
evanrittenhouse marked this conversation as resolved.
Show resolved Hide resolved

diagnostics.extend(
flake8_todos::rules::todos(&todo_comments, 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(&todo_comments)
.into_iter()
.filter(|diagnostic| settings.rules.enabled(diagnostic.kind.rule())),
);
Expand Down
6 changes: 6 additions & 0 deletions crates/ruff/src/codes.rs
Expand Up @@ -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,
})
}
214 changes: 212 additions & 2 deletions crates/ruff/src/directives.rs
@@ -1,4 +1,6 @@
//! Extract `# noqa` and `# isort: skip` directives from tokenized source.
//! Extract `# noqa`, `# isort: skip`, and `# TODO` directives from tokenized source.

use std::str::FromStr;

use bitflags::bitflags;
use ruff_text_size::{TextLen, TextRange, TextSize};
Expand Down Expand Up @@ -217,6 +219,154 @@ fn extract_isort_directives(lxr: &[LexResult], locator: &Locator) -> IsortDirect
}
}

/// A comment that contains a [`TodoDirective`]
pub struct TodoComment<'a> {
/// The comment's text
evanrittenhouse marked this conversation as resolved.
Show resolved Hide resolved
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<Self> {
TodoDirective::extract_directive(content, range).map(|directive| Self {
directive,
content,
range: *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 {
/// The directive's literal text in the source code file.
pub content: String,
/// The directive's [`TextRange`] in the file.
evanrittenhouse marked this conversation as resolved.
Show resolved Hide resolved
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,
}
}

evanrittenhouse marked this conversation as resolved.
Show resolved Hide resolved
/// Extract a [`TodoDirective`] from a comment.
pub fn extract_directive(string: &str, string_range: &TextRange) -> Option<Self> {
// 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::<TodoDirectiveKind>() {
let directive_length = directive_kind.len();
let content = string
.get(
relative_offset.to_usize()..(relative_offset + directive_length).to_usize(),
)
.unwrap()
.to_string();

evanrittenhouse marked this conversation as resolved.
Show resolved Hide resolved
return Some(Self {
content,
range: TextRange::at(string_range.start() + relative_offset, directive_length),
kind: directive_kind,
});
}

// 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 TodoDirectiveKind {
type Err = ();

fn from_str(s: &str) -> Result<Self, Self::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(TodoDirectiveKind::Fixme);
}
"hack" => {
return Ok(TodoDirectiveKind::Hack);
}
"todo" => {
return Ok(TodoDirectiveKind::Todo);
}
"xxx" => {
return Ok(TodoDirectiveKind::Xxx);
}
_ => continue,
}
}

Err(())
}
}

impl TodoDirectiveKind {
pub fn len(&self) -> TextSize {
match self {
TodoDirectiveKind::Xxx => TextSize::new(3),
TodoDirectiveKind::Hack | TodoDirectiveKind::Todo => TextSize::new(4),
TodoDirectiveKind::Fixme => TextSize::new(5),
}
}
}

#[cfg(test)]
mod tests {
use ruff_text_size::{TextLen, TextRange, TextSize};
Expand All @@ -225,7 +375,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 {
Expand Down Expand Up @@ -428,4 +580,62 @@ 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,
};
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,
};
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,
};
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,
};
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)
);
}
}
14 changes: 13 additions & 1 deletion crates/ruff/src/registry.rs
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -955,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,
Expand Down
27 changes: 27 additions & 0 deletions crates/ruff/src/rules/flake8_fixme/mod.rs
@@ -0,0 +1,27 @@
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(())
}
}