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

Extend unnecessary-pass (PIE790) to trigger on all unnecessary pass statements #7697

Merged
merged 3 commits into from Sep 29, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
11 changes: 11 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE790.py
Expand Up @@ -123,3 +123,14 @@ class Error(Exception):

def foo() -> None:
pass


def foo():
print("foo")
pass


def foo():
"""A docstring."""
print("foo")
pass
Expand Up @@ -3,7 +3,6 @@ use ruff_python_ast::Stmt;
use ruff_diagnostics::AlwaysFixableViolation;
use ruff_diagnostics::{Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::is_docstring_stmt;
use ruff_python_ast::whitespace::trailing_comment_start_offset;
use ruff_text_size::Ranged;

Expand All @@ -17,8 +16,8 @@ use crate::registry::AsRule;
/// present).
///
/// ## Why is this bad?
/// When a function or class definition contains a docstring, an additional
/// `pass` statement is redundant.
/// When a function or class definition contains more than one statement, the
/// `pass` statement is not needed.
///
/// ## Example
/// ```python
Expand Down Expand Up @@ -51,28 +50,23 @@ impl AlwaysFixableViolation for UnnecessaryPass {

/// PIE790
pub(crate) fn no_unnecessary_pass(checker: &mut Checker, body: &[Stmt]) {
let [first, second, ..] = body else {
return;
};
// This only catches the case in which a docstring makes a `pass` statement
// redundant. Consider removing all `pass` statements instead.
if !is_docstring_stmt(first) {
if body.len() < 2 {
return;
}

// The second statement must be a `pass` statement.
if !second.is_pass_stmt() {
return;
}

let mut diagnostic = Diagnostic::new(UnnecessaryPass, second.range());
if checker.patch(diagnostic.kind.rule()) {
let edit = if let Some(index) = trailing_comment_start_offset(second, checker.locator()) {
Edit::range_deletion(second.range().add_end(index))
} else {
fix::edits::delete_stmt(second, None, checker.locator(), checker.indexer())
};
diagnostic.set_fix(Fix::automatic(edit));
}
checker.diagnostics.push(diagnostic);
body.iter()
.filter(|stmt| stmt.is_pass_stmt())
.for_each(|stmt| {
let mut diagnostic = Diagnostic::new(UnnecessaryPass, stmt.range());
if checker.patch(diagnostic.kind.rule()) {
let edit =
if let Some(index) = trailing_comment_start_offset(stmt, checker.locator()) {
Edit::range_deletion(stmt.range().add_end(index))
} else {
fix::edits::delete_stmt(stmt, None, checker.locator(), checker.indexer())
};
diagnostic.set_fix(Fix::automatic(edit));
}
checker.diagnostics.push(diagnostic);
});
Copy link
Member

Choose a reason for hiding this comment

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

I'm genuinely unsure what's faster: this, or passing the pass statement itself to no_unnecessary_pass, and then using traversal::suite to find the containing body and see if it's of length one or not.

Copy link
Member

Choose a reason for hiding this comment

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

Like this is probably faster if pass statements are common, but they're relatively rare.

}
Expand Up @@ -317,4 +317,37 @@ PIE790.py:101:5: PIE790 [*] Unnecessary `pass` statement
103 103 |
104 104 | class Foo:

PIE790.py:130:5: PIE790 [*] Unnecessary `pass` statement
|
128 | def foo():
129 | print("foo")
130 | pass
| ^^^^ PIE790
|
= help: Remove unnecessary `pass`

ℹ Fix
127 127 |
128 128 | def foo():
129 129 | print("foo")
130 |- pass
131 130 |
132 131 |
133 132 | def foo():

PIE790.py:136:5: PIE790 [*] Unnecessary `pass` statement
|
134 | """A docstring."""
135 | print("foo")
136 | pass
| ^^^^ PIE790
|
= help: Remove unnecessary `pass`

ℹ Fix
133 133 | def foo():
134 134 | """A docstring."""
135 135 | print("foo")
136 |- pass