Skip to content

Commit

Permalink
Refactor static errors and point all issues to tag
Browse files Browse the repository at this point in the history
  • Loading branch information
evanrittenhouse committed May 12, 2023
1 parent 0704bf2 commit dfee2f0
Show file tree
Hide file tree
Showing 8 changed files with 74 additions and 158 deletions.
2 changes: 1 addition & 1 deletion crates/ruff/src/checkers/tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ pub(crate) fn check_tokens(
// T001, T002, T003, T004, T005, T006, T007
if enforce_todos {
diagnostics.extend(
flake8_todo::rules::check_todos(tokens, autofix, settings)
flake8_todo::rules::check_todos(tokens, settings)
.into_iter()
.filter(|diagnostic| settings.rules.enabled(diagnostic.kind.rule())),
);
Expand Down
148 changes: 32 additions & 116 deletions crates/ruff/src/rules/flake8_todo/rules.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@


use once_cell::sync::Lazy;

use regex::RegexSet;
Expand All @@ -9,10 +7,7 @@ use ruff_text_size::{TextLen, TextRange, TextSize};
use rustpython_parser::lexer::LexResult;
use rustpython_parser::Tok;

use crate::{
registry::Rule,
settings::{flags, Settings},
};
use crate::{registry::Rule, settings::Settings};

/// ## What it does
/// Checks that a TODO comment is actually labelled with "TODO".
Expand Down Expand Up @@ -240,11 +235,7 @@ struct Tag<'a> {
content: &'a str,
}

pub fn check_todos(
tokens: &[LexResult],
autofix: flags::Autofix,
settings: &Settings,
) -> Vec<Diagnostic> {
pub(crate) fn check_todos(tokens: &[LexResult], settings: &Settings) -> Vec<Diagnostic> {
let mut diagnostics: Vec<Diagnostic> = vec![];

let mut iter = tokens.iter().flatten().peekable();
Expand All @@ -258,7 +249,7 @@ pub fn check_todos(
continue;
};

check_for_tag_errors(&tag, &mut diagnostics, autofix, settings);
check_for_tag_errors(&tag, &mut diagnostics, settings);
check_for_static_errors(comment, *token_range, &tag, &mut diagnostics);

// TD-003
Expand Down Expand Up @@ -306,12 +297,7 @@ fn detect_tag<'a>(comment: &'a str, comment_range: &'a TextRange) -> Option<Tag<
}

/// Check that the tag is valid. This function modifies `diagnostics` in-place.
fn check_for_tag_errors(
tag: &Tag,
diagnostics: &mut Vec<Diagnostic>,
autofix: flags::Autofix,
settings: &Settings,
) {
fn check_for_tag_errors(tag: &Tag, diagnostics: &mut Vec<Diagnostic>, settings: &Settings) {
if tag.content == "TODO" {
return;
}
Expand All @@ -325,8 +311,8 @@ fn check_for_tag_errors(
tag.range,
);

if autofix.into() && settings.rules.should_fix(Rule::InvalidCapitalizationInTodo) {
invalid_capitalization.set_fix(Fix::unspecified(Edit::range_replacement(
if settings.rules.should_fix(Rule::InvalidCapitalizationInTodo) {
invalid_capitalization.set_fix(Fix::automatic(Edit::range_replacement(
"TODO".to_string(),
tag.range,
)));
Expand All @@ -352,110 +338,40 @@ fn check_for_static_errors(
tag: &Tag,
diagnostics: &mut Vec<Diagnostic>,
) {
// Relative offset of the current character from the start of the comment.
let mut relative_offset: usize = usize::from(tag.range.end() - comment_range.start());
let mut comment_chars = comment.chars().skip(relative_offset).peekable();
// Absolute offset of the comment's author block from the start of the file.
let mut author_range: Option<TextRange> = None;
// Absolute offset of the comment's colon from the start of the file.
let mut colon_offset: Option<TextSize> = None;

let comment_rest = &comment[usize::from(relative_offset)..];
let trimmed_start = comment_rest.trim_start();
// Relative offset from the end of the tag to the start of the rest of the comment
let whitespace_offset = comment_rest.text_len();

// An "author block" must be contained in parentheses, like "(ruff)". To check if it exists,
// we can check the first non-whitespace character after the tag. If that first character is a
// left parenthesis, we can say that we have an author's block.
for char in comment_chars.by_ref() {
relative_offset += 1;
if char.is_whitespace() {
continue;
}

// We can guarantee that there's no author if the colon directly follows the TODO tag.
if char == ':' {
colon_offset =
Some(comment_range.start() + TextSize::try_from(relative_offset).ok().unwrap());
break;
}

if char == '(' {
author_range = Some(TextRange::at(
comment_range.start() + TextSize::try_from(relative_offset).ok().unwrap(),
TextSize::new(1),
));
}

break;
}

if let Some(range) = author_range {
let mut author_block_length = 0usize;
for char in comment_chars.by_ref() {
relative_offset += 1;
author_block_length += 1;

if char == ')' {
break;
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 author_end = content_offset
+ if trimmed.starts_with('(') {
if let Some(end_index) = trimmed.find(')') {
TextSize::try_from(end_index + 1).unwrap()
} else {
trimmed.text_len()
}
}

author_range = Some(range.add_end(TextSize::try_from(author_block_length).ok().unwrap()));
} else {
diagnostics.push(Diagnostic::new(
MissingAuthorInTodo,
TextRange::at(tag.range.end(), TextSize::new(1)),
));
}

// A valid colon must be the character after the author block (or after the tag, if the author
// block doesn't exist).
if colon_offset.is_none() {
if let Some(char) = comment_chars.next() {
relative_offset += 1;
} else {
diagnostics.push(Diagnostic::new(MissingAuthorInTodo, tag.range));

if char == ':' {
colon_offset =
Some(comment_range.start() + TextSize::try_from(relative_offset).ok().unwrap());
}
}
}
TextSize::new(0)
};

if let Some(range) = colon_offset {
if let Some(char) = comment_chars.next() {
if char == ' ' {
return;
}
let post_author = &post_tag[usize::from(author_end)..];

diagnostics.push(Diagnostic::new(
MissingSpaceAfterColonInTodo,
TextRange::at(range, TextSize::new(1)),
));
let post_colon = if let Some((_colon, after_colon)) = post_author.split_once(':') {
if let Some(stripped) = after_colon.strip_prefix(' ') {
stripped
} else {
diagnostics.push(Diagnostic::new(MissingSpaceAfterColonInTodo, tag.range));
after_colon
}
} else {
// Adjust where the colon should be based on the length of the author block, if it exists.
let adjusted_colon_position = tag.range.end()
+ if let Some(author_range) = author_range {
author_range.len()
} else {
TextSize::new(0)
};
diagnostics.push(Diagnostic::new(MissingColonInTodo, tag.range));
""
};

diagnostics.push(Diagnostic::new(
MissingColonInTodo,
TextRange::at(adjusted_colon_position, TextSize::new(1)),
));
if post_colon.is_empty() {
diagnostics.push(Diagnostic::new(MissingTextInTodo, tag.range));
}

match comment_chars.next() {
Some(_) => {}
None => diagnostics.push(Diagnostic::new(
MissingTextInTodo,
TextRange::at(comment_range.end(), TextSize::new(1)),
)),
};
}

#[cfg(test)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ TD006.py:4:3: TD006 [*] Invalid TODO capitalization: `ToDo` should be `TODO`
|
= help: Replace `ToDo` with `TODO`

Suggested fix
Fix
1 1 | # TDO006 - accepted
2 2 | # TODO (evanrittenhouse): this is a valid TODO
3 3 | # TDO006 - error
Expand All @@ -28,7 +28,7 @@ TD006.py:5:3: TD006 [*] Invalid TODO capitalization: `todo` should be `TODO`
|
= help: Replace `todo` with `TODO`

Suggested fix
Fix
2 2 | # TODO (evanrittenhouse): this is a valid TODO
3 3 | # TDO006 - error
4 4 | # ToDo (evanrittenhouse): invalid capitalization
Expand Down
Original file line number Diff line number Diff line change
@@ -1,31 +1,31 @@
---
source: crates/ruff/src/rules/flake8_todo/mod.rs
---
TD002.py:5:7: TD002 Missing author in TODO. Try: # TODO (<author_name>): ...
TD002.py:5:3: TD002 Missing author in TODO. Try: # TODO (<author_name>): ...
|
5 | # TODO(evanrittenhouse): this also has an author
6 | # T002 - errors
7 | # TODO: this has no author
| ^ TD002
| ^^^^ TD002
8 | # FIXME: neither does this
9 | # TODO : and neither does this
|

TD002.py:6:8: TD002 Missing author in TODO. Try: # TODO (<author_name>): ...
TD002.py:6:3: TD002 Missing author in TODO. Try: # TODO (<author_name>): ...
|
6 | # T002 - errors
7 | # TODO: this has no author
8 | # FIXME: neither does this
| ^ TD002
| ^^^^^ TD002
9 | # TODO : and neither does this
|

TD002.py:7:7: TD002 Missing author in TODO. Try: # TODO (<author_name>): ...
TD002.py:7:3: TD002 Missing author in TODO. Try: # TODO (<author_name>): ...
|
7 | # TODO: this has no author
8 | # FIXME: neither does this
9 | # TODO : and neither does this
| ^ TD002
| ^^^^ TD002
|


Original file line number Diff line number Diff line change
@@ -1,31 +1,31 @@
---
source: crates/ruff/src/rules/flake8_todo/mod.rs
---
TD004.py:4:7: TD004 Missing colon in TODO. Try: # TODO: ...
TD004.py:4:3: TD004 Missing colon in TODO. Try: # TODO: ...
|
4 | # TODO(evanrittenhouse): this has a colon
5 | # T004 - errors
6 | # TODO this has no colon
| ^ TD004
7 | # TODO(evanrittenhouse) this has no colon
| ^^^^ TD004
7 | # TODO(evanrittenhouse 😀) this has no colon
8 | # FIXME add a colon
|

TD004.py:5:24: TD004 Missing colon in TODO. Try: # TODO: ...
TD004.py:5:3: TD004 Missing colon in TODO. Try: # TODO: ...
|
5 | # T004 - errors
6 | # TODO this has no colon
7 | # TODO(evanrittenhouse) this has no colon
| ^ TD004
7 | # TODO(evanrittenhouse 😀) this has no colon
| ^^^^ TD004
8 | # FIXME add a colon
|

TD004.py:6:8: TD004 Missing colon in TODO. Try: # TODO: ...
TD004.py:6:3: TD004 Missing colon in TODO. Try: # TODO: ...
|
6 | # TODO this has no colon
7 | # TODO(evanrittenhouse) this has no colon
7 | # TODO(evanrittenhouse 😀) this has no colon
8 | # FIXME add a colon
| ^ TD004
| ^^^^^ TD004
|


Original file line number Diff line number Diff line change
@@ -1,41 +1,41 @@
---
source: crates/ruff/src/rules/flake8_todo/mod.rs
---
TD007.py:5:25: TD007 Missing space after colon in TODO
TD007.py:5:3: TD007 Missing space after colon in TODO
|
5 | # TODO: so does this
6 | # T007 - errors
7 | # TODO(evanrittenhouse):this has no space after a colon
| ^ TD007
| ^^^^ TD007
8 | # TODO (evanrittenhouse):this doesn't either
9 | # TODO:neither does this
|

TD007.py:6:26: TD007 Missing space after colon in TODO
TD007.py:6:3: TD007 Missing space after colon in TODO
|
6 | # T007 - errors
7 | # TODO(evanrittenhouse):this has no space after a colon
8 | # TODO (evanrittenhouse):this doesn't either
| ^ TD007
| ^^^^ TD007
9 | # TODO:neither does this
10 | # FIXME:and lastly neither does this
|

TD007.py:7:8: TD007 Missing space after colon in TODO
TD007.py:7:3: TD007 Missing space after colon in TODO
|
7 | # TODO(evanrittenhouse):this has no space after a colon
8 | # TODO (evanrittenhouse):this doesn't either
9 | # TODO:neither does this
| ^ TD007
| ^^^^ TD007
10 | # FIXME:and lastly neither does this
|

TD007.py:8:9: TD007 Missing space after colon in TODO
TD007.py:8:3: TD007 Missing space after colon in TODO
|
8 | # TODO (evanrittenhouse):this doesn't either
9 | # TODO:neither does this
10 | # FIXME:and lastly neither does this
| ^ TD007
| ^^^^^ TD007
|


Original file line number Diff line number Diff line change
@@ -1,31 +1,31 @@
---
source: crates/ruff/src/rules/flake8_todo/mod.rs
---
TD005.py:4:25: TD005 Missing text after 'TODO'
TD005.py:4:3: TD005 Missing text after 'TODO'
|
4 | # TODO(evanrittenhouse): this has text, while the errors do not
5 | # T005 - errors
6 | # TODO(evanrittenhouse):
| ^ TD005
| ^^^^ TD005
7 | # TODO(evanrittenhouse)
8 | # FIXME
|

TD005.py:5:24: TD005 Missing text after 'TODO'
TD005.py:5:3: TD005 Missing text after 'TODO'
|
5 | # T005 - errors
6 | # TODO(evanrittenhouse):
7 | # TODO(evanrittenhouse)
| ^ TD005
| ^^^^ TD005
8 | # FIXME
|

TD005.py:6:8: TD005 Missing text after 'TODO'
TD005.py:6:3: TD005 Missing text after 'TODO'
|
6 | # TODO(evanrittenhouse):
7 | # TODO(evanrittenhouse)
8 | # FIXME
| ^ TD005
| ^^^^^ TD005
|


Loading

0 comments on commit dfee2f0

Please sign in to comment.