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

[pylint] Implement empty-comment (PLR2044) #9174

Merged
merged 7 commits into from
Dec 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

53 changes: 53 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pylint/empty_comment.py
Copy link
Contributor Author

@mdbernard mdbernard Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Add test for "empty comment" lines in multi-line strings/comments
  • Add test for "empty comment" lines in block comments

Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# this line has a non-empty comment and is OK
# this line is also OK, but the three following lines are not
#
#
#

# this non-empty comment has trailing whitespace and is OK

# Many codebases use multiple `#` characters on a single line to visually
# separate sections of code, so we don't consider these empty comments.

##########################################

# trailing `#` characters are not considered empty comments ###


def foo(): # this comment is OK, the one below is not
pass #


# the lines below have no comments and are OK
def bar():
pass


# "Empty comments" are common in block comments
# to add legibility. For example:
#
# The above line's "empty comment" is likely
# intentional and is considered OK.


# lines in multi-line strings/comments whose last non-whitespace character is a `#`
# do not count as empty comments
"""
The following lines are all fine:
#
#
#
"""

# These should be removed, despite being an empty "block comment".

#
#

# These should also be removed.

x = 1

#
##
#
1 change: 1 addition & 0 deletions crates/ruff_linter/src/checkers/physical_lines.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
//! Lint rules based on checking physical lines.

use ruff_diagnostics::Diagnostic;
use ruff_python_codegen::Stylist;
use ruff_python_index::Indexer;
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff_linter/src/checkers/tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ pub(crate) fn check_tokens(
pygrep_hooks::rules::blanket_type_ignore(&mut diagnostics, indexer, locator);
}

if settings.rules.enabled(Rule::EmptyComment) {
pylint::rules::empty_comments(&mut diagnostics, indexer, locator);
}

if settings.rules.any_enabled(&[
Rule::AmbiguousUnicodeCharacterString,
Rule::AmbiguousUnicodeCharacterDocstring,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup),
(Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup),
(Pylint, "R2004") => (RuleGroup::Stable, rules::pylint::rules::MagicValueComparison),
(Pylint, "R2044") => (RuleGroup::Preview, rules::pylint::rules::EmptyComment),
(Pylint, "R5501") => (RuleGroup::Stable, rules::pylint::rules::CollapsibleElseIf),
(Pylint, "R6201") => (RuleGroup::Preview, rules::pylint::rules::LiteralMembership),
#[allow(deprecated)]
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ impl Rule {
| Rule::BlanketNOQA
| Rule::BlanketTypeIgnore
| Rule::CommentedOutCode
| Rule::EmptyComment
| Rule::ExtraneousParentheses
| Rule::InvalidCharacterBackspace
| Rule::InvalidCharacterEsc
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ mod tests {
)]
#[test_case(Rule::ComparisonWithItself, Path::new("comparison_with_itself.py"))]
#[test_case(Rule::EqWithoutHash, Path::new("eq_without_hash.py"))]
#[test_case(Rule::EmptyComment, Path::new("empty_comment.py"))]
#[test_case(Rule::ManualFromImport, Path::new("import_aliasing.py"))]
#[test_case(Rule::SingleStringSlots, Path::new("single_string_slots.py"))]
#[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_0.py"))]
Expand Down
106 changes: 106 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/empty_comment.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_index::Indexer;
use ruff_python_trivia::is_python_whitespace;

use ruff_source_file::Locator;
use ruff_text_size::{TextRange, TextSize};
use rustc_hash::FxHashSet;

/// ## What it does
/// Checks for a # symbol appearing on a line not followed by an actual comment.
///
/// ## Why is this bad?
/// Empty comments don't provide any clarity to the code, and just add clutter.
/// Either add a comment or delete the empty comment.
///
/// ## Example
/// ```python
/// class Foo: #
/// pass
/// ```
///
/// Use instead:
/// ```python
/// class Foo:
/// pass
/// ```
///
/// ## References
/// - [Pylint documentation](https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/empty-comment.html)
#[violation]
pub struct EmptyComment;

impl Violation for EmptyComment {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Always;

#[derive_message_formats]
fn message(&self) -> String {
format!("Line with empty comment")
}

fn fix_title(&self) -> Option<String> {
Some(format!("Delete the empty comment"))
}
}

/// PLR2044
pub(crate) fn empty_comments(
diagnostics: &mut Vec<Diagnostic>,
indexer: &Indexer,
locator: &Locator,
) {
let block_comments = FxHashSet::from_iter(indexer.comment_ranges().block_comments(locator));

for range in indexer.comment_ranges() {
// Ignore comments that are part of multi-line "comment blocks".
if block_comments.contains(&range.start()) {
continue;
}

// If the line contains an empty comment, add a diagnostic.
if let Some(diagnostic) = empty_comment(*range, locator) {
diagnostics.push(diagnostic);
}
}
}

/// Return a [`Diagnostic`] if the comment at the given [`TextRange`] is empty.
fn empty_comment(range: TextRange, locator: &Locator) -> Option<Diagnostic> {
// Check: is the comment empty?
if !locator
.slice(range)
.chars()
.skip(1)
.all(is_python_whitespace)
{
return None;
}

// Find the location of the `#`.
let first_hash_col = range.start();

// Find the start of the line.
let line = locator.line_range(first_hash_col);

// Find the last character in the line that precedes the comment, if any.
let deletion_start_col = locator
.slice(TextRange::new(line.start(), first_hash_col))
.char_indices()
.rev()
.find(|&(_, c)| !is_python_whitespace(c) && c != '#')
.map(|(last_non_whitespace_non_comment_col, _)| {
// SAFETY: (last_non_whitespace_non_comment_col + 1) <= first_hash_col
TextSize::try_from(last_non_whitespace_non_comment_col).unwrap() + TextSize::new(1)
});

Some(
Diagnostic::new(EmptyComment, TextRange::new(first_hash_col, line.end())).with_fix(
Fix::safe_edit(if let Some(deletion_start_col) = deletion_start_col {
Edit::deletion(line.start() + deletion_start_col, line.end())
} else {
Edit::range_deletion(locator.full_line_range(first_hash_col))
}),
),
)
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub(crate) use comparison_of_constant::*;
pub(crate) use comparison_with_itself::*;
pub(crate) use continue_in_finally::*;
pub(crate) use duplicate_bases::*;
pub(crate) use empty_comment::*;
pub(crate) use eq_without_hash::*;
pub(crate) use global_at_module_level::*;
pub(crate) use global_statement::*;
Expand Down Expand Up @@ -92,6 +93,7 @@ mod comparison_of_constant;
mod comparison_with_itself;
mod continue_in_finally;
mod duplicate_bases;
mod empty_comment;
mod eq_without_hash;
mod global_at_module_level;
mod global_statement;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
empty_comment.py:3:1: PLR2044 [*] Line with empty comment
|
1 | # this line has a non-empty comment and is OK
2 | # this line is also OK, but the three following lines are not
3 | #
| ^ PLR2044
4 | #
5 | #
|
= help: Delete the empty comment

ℹ Safe fix
1 1 | # this line has a non-empty comment and is OK
2 2 | # this line is also OK, but the three following lines are not
3 |-#
4 3 | #
5 4 | #
6 5 |

empty_comment.py:4:5: PLR2044 [*] Line with empty comment
|
2 | # this line is also OK, but the three following lines are not
3 | #
4 | #
| ^ PLR2044
5 | #
|
= help: Delete the empty comment

ℹ Safe fix
1 1 | # this line has a non-empty comment and is OK
2 2 | # this line is also OK, but the three following lines are not
3 3 | #
4 |- #
5 4 | #
6 5 |
7 6 | # this non-empty comment has trailing whitespace and is OK

empty_comment.py:5:9: PLR2044 [*] Line with empty comment
|
3 | #
4 | #
5 | #
| ^ PLR2044
6 |
7 | # this non-empty comment has trailing whitespace and is OK
|
= help: Delete the empty comment

ℹ Safe fix
2 2 | # this line is also OK, but the three following lines are not
3 3 | #
4 4 | #
5 |- #
6 5 |
7 6 | # this non-empty comment has trailing whitespace and is OK
8 7 |

empty_comment.py:18:11: PLR2044 [*] Line with empty comment
|
17 | def foo(): # this comment is OK, the one below is not
18 | pass #
| ^ PLR2044
|
= help: Delete the empty comment

ℹ Safe fix
15 15 |
16 16 |
17 17 | def foo(): # this comment is OK, the one below is not
18 |- pass #
18 |+ pass
19 19 |
20 20 |
21 21 | # the lines below have no comments and are OK

empty_comment.py:44:1: PLR2044 [*] Line with empty comment
|
42 | # These should be removed, despite being an empty "block comment".
43 |
44 | #
| ^ PLR2044
45 | #
|
= help: Delete the empty comment

ℹ Safe fix
42 42 | # These should be removed, despite being an empty "block comment".
43 43 |
44 44 | #
45 |-#
46 45 |
47 46 | # These should also be removed.
48 47 |

empty_comment.py:45:1: PLR2044 [*] Line with empty comment
|
44 | #
45 | #
| ^ PLR2044
46 |
47 | # These should also be removed.
|
= help: Delete the empty comment

ℹ Safe fix
42 42 | # These should be removed, despite being an empty "block comment".
43 43 |
44 44 | #
45 |-#
46 45 |
47 46 | # These should also be removed.
48 47 |


1 change: 1 addition & 0 deletions crates/ruff_python_trivia/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ unicode-ident = { workspace = true }
insta = { workspace = true }
ruff_python_ast = { path = "../ruff_python_ast" }
ruff_python_parser = { path = "../ruff_python_parser" }
ruff_python_index = { path = "../ruff_python_index" }

[lints]
workspace = true
Loading
Loading