Skip to content

Commit

Permalink
Avoid removing trailing comments on pass statements (#2235)
Browse files Browse the repository at this point in the history
This isn't super consistent with some other rules, but... if you have a lone body, with a `pass`, followed by a comment, it's probably surprising if it gets removed. Let's retain the comment.

Closes #2231.
  • Loading branch information
charliermarsh authored Jan 26, 2023
1 parent 9d3a553 commit a6ec2eb
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 29 deletions.
35 changes: 20 additions & 15 deletions resources/test/fixtures/flake8_pie/PIE790.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
class Foo:
"""buzz"""

pass # PIE790
pass


if foo:
"""foo"""
pass # PIE790
pass


def multi_statement() -> None:
Expand All @@ -18,51 +18,51 @@ def multi_statement() -> None:
pass
else:
"""bar"""
pass # PIE790
pass


while True:
pass
else:
"""bar"""
pass # PIE790
pass


for _ in range(10):
pass
else:
"""bar"""
pass # PIE790
pass


async for _ in range(10):
pass
else:
"""bar"""
pass # PIE790
pass


def foo() -> None:
"""
buzz
"""

pass # PIE790
pass


async def foo():
"""
buzz
"""

pass # PIE790
pass


try:
"""
buzz
"""
pass # PIE790
pass
except ValueError:
pass

Expand All @@ -71,29 +71,34 @@ async def foo():
bar()
except ValueError:
"""bar"""
pass # PIE790
pass


for _ in range(10):
"""buzz"""
pass # PIE790
pass

async for _ in range(10):
"""buzz"""
pass # PIE790
pass

while cond:
"""buzz"""
pass # PIE790
pass


with bar:
"""buzz"""
pass # PIE790
pass

async with bar:
"""buzz"""
pass # PIE790
pass


def foo() -> None:
"""buzz"""
pass # bar


class Foo:
Expand Down
18 changes: 18 additions & 0 deletions src/ast/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,24 @@ pub fn match_trailing_content(stmt: &Stmt, locator: &Locator) -> bool {
false
}

/// If a `Stmt` has a trailing comment, return the index of the hash.
pub fn match_trailing_comment(stmt: &Stmt, locator: &Locator) -> Option<usize> {
let range = Range::new(
stmt.end_location.unwrap(),
Location::new(stmt.end_location.unwrap().row() + 1, 0),
);
let suffix = locator.slice_source_code_range(&range);
for (i, char) in suffix.chars().enumerate() {
if char == '#' {
return Some(i);
}
if !char.is_whitespace() {
return None;
}
}
None
}

/// Return the number of trailing empty lines following a statement.
pub fn count_trailing_lines(stmt: &Stmt, locator: &Locator) -> usize {
let suffix =
Expand Down
39 changes: 25 additions & 14 deletions src/rules/flake8_pie/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@ use rustc_hash::FxHashSet;
use rustpython_ast::{Constant, Expr, ExprKind, Keyword, Stmt, StmtKind};

use crate::ast::comparable::ComparableExpr;
use crate::ast::helpers::unparse_expr;
use crate::ast::helpers::{match_trailing_comment, unparse_expr};
use crate::ast::types::{Range, RefEquality};
use crate::autofix::helpers::delete_stmt;
use crate::checkers::ast::Checker;
use crate::define_violation;
use crate::fix::Fix;
use crate::message::Location;
use crate::python::identifiers::is_identifier;
use crate::registry::{Diagnostic, Rule};
use crate::violation::{AlwaysAutofixableViolation, Violation};
Expand Down Expand Up @@ -112,19 +113,29 @@ pub fn no_unnecessary_pass(checker: &mut Checker, body: &[Stmt]) {
let mut diagnostic =
Diagnostic::new(NoUnnecessaryPass, Range::from_located(pass_stmt));
if checker.patch(&Rule::NoUnnecessaryPass) {
match delete_stmt(
pass_stmt,
None,
&[],
checker.locator,
checker.indexer,
checker.stylist,
) {
Ok(fix) => {
diagnostic.amend(fix);
}
Err(e) => {
error!("Failed to delete `pass` statement: {}", e);
if let Some(index) = match_trailing_comment(pass_stmt, checker.locator) {
diagnostic.amend(Fix::deletion(
pass_stmt.location,
Location::new(
pass_stmt.end_location.unwrap().row(),
pass_stmt.end_location.unwrap().column() + index,
),
));
} else {
match delete_stmt(
pass_stmt,
None,
&[],
checker.locator,
checker.indexer,
checker.stylist,
) {
Ok(fix) => {
diagnostic.amend(fix);
}
Err(e) => {
error!("Failed to delete `pass` statement: {}", e);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,4 +290,22 @@ expression: diagnostics
row: 97
column: 0
parent: ~
- kind:
NoUnnecessaryPass: ~
location:
row: 101
column: 4
end_location:
row: 101
column: 8
fix:
content:
- ""
location:
row: 101
column: 4
end_location:
row: 101
column: 10
parent: ~

0 comments on commit a6ec2eb

Please sign in to comment.