Remove continuations when deleting statements (#5198)

## Summary

This PR modifies our statement deletion logic to delete any preceding
continuation lines.

For example, given:

```py
x = 1; \
  import os
```

We'll now rewrite to:

```py
x = 1;
```

In addition, the logic can now handle multiple preceding continuations
(which is unlikely, but valid).
This commit is contained in:
Charlie Marsh 2023-06-19 22:04:28 -04:00 committed by GitHub
parent 015895bcae
commit 8e06140d1d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
17 changed files with 394 additions and 317 deletions

View file

@ -1,4 +1,3 @@
if True: if True:
import foo1; x = 1 import foo1; x = 1
import foo2; x = 1 import foo2; x = 1
@ -11,7 +10,6 @@ if True:
import foo4 \ import foo4 \
; x = 1 ; x = 1
if True: if True:
x = 1; import foo5 x = 1; import foo5
@ -20,12 +18,10 @@ if True:
x = 1; \ x = 1; \
import foo6 import foo6
if True: if True:
x = 1 \ x = 1 \
; import foo7 ; import foo7
if True: if True:
x = 1; import foo8; x = 1 x = 1; import foo8; x = 1
x = 1; import foo9; x = 1 x = 1; import foo9; x = 1
@ -40,12 +36,27 @@ if True:
;import foo11 \ ;import foo11 \
;x = 1 ;x = 1
if True:
x = 1; \
\
import foo12
if True:
x = 1; \
\
import foo13
if True:
x = 1; \
# \
import foo14
# Continuation, but not as the last content in the file. # Continuation, but not as the last content in the file.
x = 1; \ x = 1; \
import foo12 import foo15
# Continuation, followed by end-of-file. (Removing `import foo` would cause a syntax # Continuation, followed by end-of-file. (Removing `import foo` would cause a syntax
# error.) # error.)
x = 1; \ x = 1; \
import foo13 import foo16

View file

@ -29,7 +29,6 @@ pub(crate) fn delete_stmt(
parent: Option<&Stmt>, parent: Option<&Stmt>,
locator: &Locator, locator: &Locator,
indexer: &Indexer, indexer: &Indexer,
stylist: &Stylist,
) -> Edit { ) -> Edit {
if parent if parent
.map(|parent| is_lone_child(stmt, parent)) .map(|parent| is_lone_child(stmt, parent))
@ -39,18 +38,15 @@ pub(crate) fn delete_stmt(
// it with a `pass`. // it with a `pass`.
Edit::range_replacement("pass".to_string(), stmt.range()) Edit::range_replacement("pass".to_string(), stmt.range())
} else { } else {
if let Some(semicolon) = trailing_semicolon(stmt, locator) { if let Some(semicolon) = trailing_semicolon(stmt.end(), locator) {
let next = next_stmt_break(semicolon, locator); let next = next_stmt_break(semicolon, locator);
Edit::deletion(stmt.start(), next) Edit::deletion(stmt.start(), next)
} else if helpers::has_leading_content(stmt, locator) { } else if helpers::has_leading_content(stmt.start(), locator) {
Edit::range_deletion(stmt.range()) Edit::range_deletion(stmt.range())
} else if helpers::preceded_by_continuation(stmt, indexer, locator) { } else if let Some(start) =
if is_end_of_file(stmt, locator) && locator.is_at_start_of_line(stmt.start()) { helpers::preceded_by_continuations(stmt.start(), locator, indexer)
// Special-case: a file can't end in a continuation. {
Edit::range_replacement(stylist.line_ending().to_string(), stmt.range()) Edit::range_deletion(TextRange::new(start, stmt.end()))
} else {
Edit::range_deletion(stmt.range())
}
} else { } else {
let range = locator.full_lines_range(stmt.range()); let range = locator.full_lines_range(stmt.range());
Edit::range_deletion(range) Edit::range_deletion(range)
@ -68,7 +64,7 @@ pub(crate) fn remove_unused_imports<'a>(
stylist: &Stylist, stylist: &Stylist,
) -> Result<Edit> { ) -> Result<Edit> {
match codemods::remove_imports(unused_imports, stmt, locator, stylist)? { match codemods::remove_imports(unused_imports, stmt, locator, stylist)? {
None => Ok(delete_stmt(stmt, parent, locator, indexer, stylist)), None => Ok(delete_stmt(stmt, parent, locator, indexer)),
Some(content) => Ok(Edit::range_replacement(content, stmt.range())), Some(content) => Ok(Edit::range_replacement(content, stmt.range())),
} }
} }
@ -238,15 +234,15 @@ fn is_lone_child(child: &Stmt, parent: &Stmt) -> bool {
/// Return the location of a trailing semicolon following a `Stmt`, if it's part /// Return the location of a trailing semicolon following a `Stmt`, if it's part
/// of a multi-statement line. /// of a multi-statement line.
fn trailing_semicolon(stmt: &Stmt, locator: &Locator) -> Option<TextSize> { fn trailing_semicolon(offset: TextSize, locator: &Locator) -> Option<TextSize> {
let contents = locator.after(stmt.end()); let contents = locator.after(offset);
for line in NewlineWithTrailingNewline::from(contents) { for line in NewlineWithTrailingNewline::from(contents) {
let trimmed = line.trim_whitespace_start(); let trimmed = line.trim_whitespace_start();
if trimmed.starts_with(';') { if trimmed.starts_with(';') {
let colon_offset = line.text_len() - trimmed.text_len(); let colon_offset = line.text_len() - trimmed.text_len();
return Some(stmt.end() + line.start() + colon_offset); return Some(offset + line.start() + colon_offset);
} }
if !trimmed.starts_with('\\') { if !trimmed.starts_with('\\') {
@ -284,16 +280,11 @@ fn next_stmt_break(semicolon: TextSize, locator: &Locator) -> TextSize {
locator.line_end(start_location) locator.line_end(start_location)
} }
/// Return `true` if a `Stmt` occurs at the end of a file.
fn is_end_of_file(stmt: &Stmt, locator: &Locator) -> bool {
stmt.end() == locator.contents().text_len()
}
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use anyhow::Result; use anyhow::Result;
use ruff_text_size::TextSize; use ruff_text_size::TextSize;
use rustpython_parser::ast::Suite; use rustpython_parser::ast::{Ranged, Suite};
use rustpython_parser::Parse; use rustpython_parser::Parse;
use ruff_python_ast::source_code::Locator; use ruff_python_ast::source_code::Locator;
@ -306,19 +297,25 @@ mod tests {
let program = Suite::parse(contents, "<filename>")?; let program = Suite::parse(contents, "<filename>")?;
let stmt = program.first().unwrap(); let stmt = program.first().unwrap();
let locator = Locator::new(contents); let locator = Locator::new(contents);
assert_eq!(trailing_semicolon(stmt, &locator), None); assert_eq!(trailing_semicolon(stmt.end(), &locator), None);
let contents = "x = 1; y = 1"; let contents = "x = 1; y = 1";
let program = Suite::parse(contents, "<filename>")?; let program = Suite::parse(contents, "<filename>")?;
let stmt = program.first().unwrap(); let stmt = program.first().unwrap();
let locator = Locator::new(contents); let locator = Locator::new(contents);
assert_eq!(trailing_semicolon(stmt, &locator), Some(TextSize::from(5))); assert_eq!(
trailing_semicolon(stmt.end(), &locator),
Some(TextSize::from(5))
);
let contents = "x = 1 ; y = 1"; let contents = "x = 1 ; y = 1";
let program = Suite::parse(contents, "<filename>")?; let program = Suite::parse(contents, "<filename>")?;
let stmt = program.first().unwrap(); let stmt = program.first().unwrap();
let locator = Locator::new(contents); let locator = Locator::new(contents);
assert_eq!(trailing_semicolon(stmt, &locator), Some(TextSize::from(6))); assert_eq!(
trailing_semicolon(stmt.end(), &locator),
Some(TextSize::from(6))
);
let contents = r#" let contents = r#"
x = 1 \ x = 1 \
@ -328,7 +325,10 @@ x = 1 \
let program = Suite::parse(contents, "<filename>")?; let program = Suite::parse(contents, "<filename>")?;
let stmt = program.first().unwrap(); let stmt = program.first().unwrap();
let locator = Locator::new(contents); let locator = Locator::new(contents);
assert_eq!(trailing_semicolon(stmt, &locator), Some(TextSize::from(10))); assert_eq!(
trailing_semicolon(stmt.end(), &locator),
Some(TextSize::from(10))
);
Ok(()) Ok(())
} }

View file

@ -93,7 +93,6 @@ pub(crate) fn duplicate_class_field_definition<'a, 'b>(
Some(parent), Some(parent),
checker.locator, checker.locator,
checker.indexer, checker.indexer,
checker.stylist,
); );
diagnostic.set_fix(Fix::suggested(edit).isolate(checker.isolation(Some(parent)))); diagnostic.set_fix(Fix::suggested(edit).isolate(checker.isolation(Some(parent))));
} }

View file

@ -67,13 +67,7 @@ pub(crate) fn no_unnecessary_pass(checker: &mut Checker, body: &[Stmt]) {
let edit = if let Some(index) = trailing_comment_start_offset(stmt, checker.locator) { let edit = if let Some(index) = trailing_comment_start_offset(stmt, checker.locator) {
Edit::range_deletion(stmt.range().add_end(index)) Edit::range_deletion(stmt.range().add_end(index))
} else { } else {
autofix::edits::delete_stmt( autofix::edits::delete_stmt(stmt, None, checker.locator, checker.indexer)
stmt,
None,
checker.locator,
checker.indexer,
checker.stylist,
)
}; };
diagnostic.set_fix(Fix::automatic(edit)); diagnostic.set_fix(Fix::automatic(edit));
} }

View file

@ -69,13 +69,8 @@ pub(crate) fn ellipsis_in_non_empty_class_body<'a>(
let mut diagnostic = Diagnostic::new(EllipsisInNonEmptyClassBody, stmt.range()); let mut diagnostic = Diagnostic::new(EllipsisInNonEmptyClassBody, stmt.range());
if checker.patch(diagnostic.kind.rule()) { if checker.patch(diagnostic.kind.rule()) {
let edit = autofix::edits::delete_stmt( let edit =
stmt, autofix::edits::delete_stmt(stmt, Some(parent), checker.locator, checker.indexer);
Some(parent),
checker.locator,
checker.indexer,
checker.stylist,
);
diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(Some(parent)))); diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(Some(parent))));
} }
checker.diagnostics.push(diagnostic); checker.diagnostics.push(diagnostic);

View file

@ -39,13 +39,8 @@ pub(crate) fn pass_in_class_body<'a>(
let mut diagnostic = Diagnostic::new(PassInClassBody, stmt.range()); let mut diagnostic = Diagnostic::new(PassInClassBody, stmt.range());
if checker.patch(diagnostic.kind.rule()) { if checker.patch(diagnostic.kind.rule()) {
let edit = autofix::edits::delete_stmt( let edit =
stmt, autofix::edits::delete_stmt(stmt, Some(parent), checker.locator, checker.indexer);
Some(parent),
checker.locator,
checker.indexer,
checker.stylist,
);
diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(Some(parent)))); diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(Some(parent))));
} }
checker.diagnostics.push(diagnostic); checker.diagnostics.push(diagnostic);

View file

@ -95,13 +95,7 @@ pub(crate) fn str_or_repr_defined_in_stub(checker: &mut Checker, stmt: &Stmt) {
if checker.patch(diagnostic.kind.rule()) { if checker.patch(diagnostic.kind.rule()) {
let stmt = checker.semantic().stmt(); let stmt = checker.semantic().stmt();
let parent = checker.semantic().stmt_parent(); let parent = checker.semantic().stmt_parent();
let edit = delete_stmt( let edit = delete_stmt(stmt, parent, checker.locator, checker.indexer);
stmt,
parent,
checker.locator,
checker.indexer,
checker.stylist,
);
diagnostic.set_fix( diagnostic.set_fix(
Fix::automatic(edit).isolate(checker.isolation(checker.semantic().stmt_parent())), Fix::automatic(edit).isolate(checker.isolation(checker.semantic().stmt_parent())),
); );

View file

@ -517,13 +517,8 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack) {
// Delete the `return` statement. There's no need to treat this as an isolated // Delete the `return` statement. There's no need to treat this as an isolated
// edit, since we're editing the preceding statement, so no conflicting edit would // edit, since we're editing the preceding statement, so no conflicting edit would
// be allowed to remove that preceding statement. // be allowed to remove that preceding statement.
let delete_return = edits::delete_stmt( let delete_return =
stmt, edits::delete_stmt(stmt, None, checker.locator, checker.indexer);
None,
checker.locator,
checker.indexer,
checker.stylist,
);
// Replace the `x = 1` statement with `return 1`. // Replace the `x = 1` statement with `return 1`.
let content = checker.locator.slice(assign.range()); let content = checker.locator.slice(assign.range());

View file

@ -62,13 +62,7 @@ pub(crate) fn empty_type_checking_block(checker: &mut Checker, stmt: &ast::StmtI
// Delete the entire type-checking block. // Delete the entire type-checking block.
let stmt = checker.semantic().stmt(); let stmt = checker.semantic().stmt();
let parent = checker.semantic().stmt_parent(); let parent = checker.semantic().stmt_parent();
let edit = autofix::edits::delete_stmt( let edit = autofix::edits::delete_stmt(stmt, parent, checker.locator, checker.indexer);
stmt,
parent,
checker.locator,
checker.indexer,
checker.stylist,
);
diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(parent))); diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(parent)));
} }
checker.diagnostics.push(diagnostic); checker.diagnostics.push(diagnostic);

View file

@ -78,8 +78,8 @@ pub(crate) fn lambda_assignment(
// See https://github.com/astral-sh/ruff/issues/3046 // See https://github.com/astral-sh/ruff/issues/3046
if checker.patch(diagnostic.kind.rule()) if checker.patch(diagnostic.kind.rule())
&& !checker.semantic().scope().kind.is_class() && !checker.semantic().scope().kind.is_class()
&& !has_leading_content(stmt, checker.locator) && !has_leading_content(stmt.start(), checker.locator)
&& !has_trailing_content(stmt, checker.locator) && !has_trailing_content(stmt.end(), checker.locator)
{ {
let first_line = checker.locator.line(stmt.start()); let first_line = checker.locator.line(stmt.start());
let indentation = leading_indentation(first_line); let indentation = leading_indentation(first_line);

View file

@ -210,13 +210,7 @@ fn remove_unused_variable(
Some(Fix::suggested(edit)) Some(Fix::suggested(edit))
} else { } else {
// If (e.g.) assigning to a constant (`x = 1`), delete the entire statement. // If (e.g.) assigning to a constant (`x = 1`), delete the entire statement.
let edit = delete_stmt( let edit = delete_stmt(stmt, parent, checker.locator, checker.indexer);
stmt,
parent,
checker.locator,
checker.indexer,
checker.stylist,
);
Some(Fix::suggested(edit).isolate(checker.isolation(parent))) Some(Fix::suggested(edit).isolate(checker.isolation(parent)))
}; };
} }
@ -241,13 +235,7 @@ fn remove_unused_variable(
Some(Fix::suggested(edit)) Some(Fix::suggested(edit))
} else { } else {
// If (e.g.) assigning to a constant (`x = 1`), delete the entire statement. // If (e.g.) assigning to a constant (`x = 1`), delete the entire statement.
let edit = delete_stmt( let edit = delete_stmt(stmt, parent, checker.locator, checker.indexer);
stmt,
parent,
checker.locator,
checker.indexer,
checker.stylist,
);
Some(Fix::suggested(edit).isolate(checker.isolation(parent))) Some(Fix::suggested(edit).isolate(checker.isolation(parent)))
}; };
} }

View file

@ -1,257 +1,325 @@
--- ---
source: crates/ruff/src/rules/pyflakes/mod.rs source: crates/ruff/src/rules/pyflakes/mod.rs
--- ---
multi_statement_lines.py:3:12: F401 [*] `foo1` imported but unused multi_statement_lines.py:2:12: F401 [*] `foo1` imported but unused
| |
2 | if True: 1 | if True:
3 | import foo1; x = 1 2 | import foo1; x = 1
| ^^^^ F401 | ^^^^ F401
4 | import foo2; x = 1 3 | import foo2; x = 1
| |
= help: Remove unused import: `foo1` = help: Remove unused import: `foo1`
Fix Fix
1 1 | 1 1 | if True:
2 2 | if True: 2 |- import foo1; x = 1
3 |- import foo1; x = 1 2 |+ x = 1
3 |+ x = 1 3 3 | import foo2; x = 1
4 4 | import foo2; x = 1 4 4 |
5 5 | 5 5 | if True:
6 6 | if True:
multi_statement_lines.py:4:12: F401 [*] `foo2` imported but unused multi_statement_lines.py:3:12: F401 [*] `foo2` imported but unused
| |
2 | if True: 1 | if True:
3 | import foo1; x = 1 2 | import foo1; x = 1
4 | import foo2; x = 1 3 | import foo2; x = 1
| ^^^^ F401 | ^^^^ F401
5 | 4 |
6 | if True: 5 | if True:
| |
= help: Remove unused import: `foo2` = help: Remove unused import: `foo2`
Fix Fix
1 1 | 1 1 | if True:
2 2 | if True: 2 2 | import foo1; x = 1
3 3 | import foo1; x = 1 3 |- import foo2; x = 1
4 |- import foo2; x = 1 3 |+ x = 1
4 |+ x = 1 4 4 |
5 5 | 5 5 | if True:
6 6 | if True: 6 6 | import foo3; \
7 7 | import foo3; \
multi_statement_lines.py:7:12: F401 [*] `foo3` imported but unused multi_statement_lines.py:6:12: F401 [*] `foo3` imported but unused
| |
6 | if True: 5 | if True:
7 | import foo3; \ 6 | import foo3; \
| ^^^^ F401 | ^^^^ F401
8 | x = 1 7 | x = 1
| |
= help: Remove unused import: `foo3` = help: Remove unused import: `foo3`
Fix Fix
4 4 | import foo2; x = 1 3 3 | import foo2; x = 1
5 5 | 4 4 |
6 6 | if True: 5 5 | if True:
7 |- import foo3; \ 6 |- import foo3; \
8 |-x = 1 7 |-x = 1
7 |+ x = 1 6 |+ x = 1
9 8 | 8 7 |
10 9 | if True: 9 8 | if True:
11 10 | import foo4 \ 10 9 | import foo4 \
multi_statement_lines.py:11:12: F401 [*] `foo4` imported but unused multi_statement_lines.py:10:12: F401 [*] `foo4` imported but unused
| |
10 | if True: 9 | if True:
11 | import foo4 \ 10 | import foo4 \
| ^^^^ F401 | ^^^^ F401
12 | ; x = 1 11 | ; x = 1
| |
= help: Remove unused import: `foo4` = help: Remove unused import: `foo4`
Fix Fix
8 8 | x = 1 7 7 | x = 1
9 9 | 8 8 |
10 10 | if True: 9 9 | if True:
11 |- import foo4 \ 10 |- import foo4 \
12 |- ; x = 1 11 |- ; x = 1
11 |+ x = 1 10 |+ x = 1
13 12 | 12 11 |
14 13 | 13 12 | if True:
15 14 | if True: 14 13 | x = 1; import foo5
multi_statement_lines.py:16:19: F401 [*] `foo5` imported but unused multi_statement_lines.py:14:19: F401 [*] `foo5` imported but unused
| |
15 | if True: 13 | if True:
16 | x = 1; import foo5 14 | x = 1; import foo5
| ^^^^ F401 | ^^^^ F401
| |
= help: Remove unused import: `foo5` = help: Remove unused import: `foo5`
Fix Fix
13 13 | 11 11 | ; x = 1
14 14 | 12 12 |
15 15 | if True: 13 13 | if True:
16 |- x = 1; import foo5 14 |- x = 1; import foo5
16 |+ x = 1; 14 |+ x = 1;
17 17 | 15 15 |
18 18 | 16 16 |
19 19 | if True: 17 17 | if True:
multi_statement_lines.py:21:17: F401 [*] `foo6` imported but unused multi_statement_lines.py:19:17: F401 [*] `foo6` imported but unused
| |
19 | if True: 17 | if True:
20 | x = 1; \ 18 | x = 1; \
21 | import foo6 19 | import foo6
| ^^^^ F401 | ^^^^ F401
20 |
21 | if True:
| |
= help: Remove unused import: `foo6` = help: Remove unused import: `foo6`
Fix Fix
18 18 | 15 15 |
19 19 | if True: 16 16 |
20 20 | x = 1; \ 17 17 | if True:
21 |- import foo6 18 |- x = 1; \
21 |+ 19 |- import foo6
22 22 | 18 |+ x = 1;
23 23 | 20 19 |
24 24 | if True: 21 20 | if True:
22 21 | x = 1 \
multi_statement_lines.py:26:18: F401 [*] `foo7` imported but unused multi_statement_lines.py:23:18: F401 [*] `foo7` imported but unused
| |
24 | if True: 21 | if True:
25 | x = 1 \ 22 | x = 1 \
26 | ; import foo7 23 | ; import foo7
| ^^^^ F401 | ^^^^ F401
24 |
25 | if True:
| |
= help: Remove unused import: `foo7` = help: Remove unused import: `foo7`
Fix Fix
23 23 | 20 20 |
24 24 | if True: 21 21 | if True:
25 25 | x = 1 \ 22 22 | x = 1 \
26 |- ; import foo7 23 |- ; import foo7
26 |+ ; 23 |+ ;
27 27 | 24 24 |
28 28 | 25 25 | if True:
29 29 | if True: 26 26 | x = 1; import foo8; x = 1
multi_statement_lines.py:30:19: F401 [*] `foo8` imported but unused multi_statement_lines.py:26:19: F401 [*] `foo8` imported but unused
| |
29 | if True: 25 | if True:
30 | x = 1; import foo8; x = 1 26 | x = 1; import foo8; x = 1
| ^^^^ F401 | ^^^^ F401
31 | x = 1; import foo9; x = 1 27 | x = 1; import foo9; x = 1
| |
= help: Remove unused import: `foo8` = help: Remove unused import: `foo8`
Fix Fix
27 27 | 23 23 | ; import foo7
24 24 |
25 25 | if True:
26 |- x = 1; import foo8; x = 1
26 |+ x = 1; x = 1
27 27 | x = 1; import foo9; x = 1
28 28 | 28 28 |
29 29 | if True: 29 29 | if True:
30 |- x = 1; import foo8; x = 1
30 |+ x = 1; x = 1
31 31 | x = 1; import foo9; x = 1
32 32 |
33 33 | if True:
multi_statement_lines.py:31:23: F401 [*] `foo9` imported but unused multi_statement_lines.py:27:23: F401 [*] `foo9` imported but unused
| |
29 | if True: 25 | if True:
30 | x = 1; import foo8; x = 1 26 | x = 1; import foo8; x = 1
31 | x = 1; import foo9; x = 1 27 | x = 1; import foo9; x = 1
| ^^^^ F401 | ^^^^ F401
32 | 28 |
33 | if True: 29 | if True:
| |
= help: Remove unused import: `foo9` = help: Remove unused import: `foo9`
Fix Fix
24 24 |
25 25 | if True:
26 26 | x = 1; import foo8; x = 1
27 |- x = 1; import foo9; x = 1
27 |+ x = 1; x = 1
28 28 | 28 28 |
29 29 | if True: 29 29 | if True:
30 30 | x = 1; import foo8; x = 1 30 30 | x = 1; \
31 |- x = 1; import foo9; x = 1
31 |+ x = 1; x = 1
32 32 |
33 33 | if True:
34 34 | x = 1; \
multi_statement_lines.py:35:16: F401 [*] `foo10` imported but unused multi_statement_lines.py:31:16: F401 [*] `foo10` imported but unused
| |
33 | if True: 29 | if True:
34 | x = 1; \ 30 | x = 1; \
35 | import foo10; \ 31 | import foo10; \
| ^^^^^ F401 | ^^^^^ F401
36 | x = 1 32 | x = 1
| |
= help: Remove unused import: `foo10` = help: Remove unused import: `foo10`
Fix Fix
32 32 | 28 28 |
33 33 | if True: 29 29 | if True:
34 34 | x = 1; \ 30 30 | x = 1; \
35 |- import foo10; \ 31 |- import foo10; \
36 |- x = 1 32 |- x = 1
35 |+ x = 1 31 |+ x = 1
37 36 | 33 32 |
38 37 | if True: 34 33 | if True:
39 38 | x = 1 \ 35 34 | x = 1 \
multi_statement_lines.py:40:17: F401 [*] `foo11` imported but unused multi_statement_lines.py:36:17: F401 [*] `foo11` imported but unused
| |
38 | if True: 34 | if True:
39 | x = 1 \ 35 | x = 1 \
40 | ;import foo11 \ 36 | ;import foo11 \
| ^^^^^ F401 | ^^^^^ F401
41 | ;x = 1 37 | ;x = 1
| |
= help: Remove unused import: `foo11` = help: Remove unused import: `foo11`
Fix Fix
37 37 | 33 33 |
38 38 | if True: 34 34 | if True:
39 39 | x = 1 \ 35 35 | x = 1 \
40 |- ;import foo11 \ 36 |- ;import foo11 \
41 40 | ;x = 1 37 36 | ;x = 1
42 41 | 38 37 |
43 42 | 39 38 | if True:
multi_statement_lines.py:46:8: F401 [*] `foo12` imported but unused multi_statement_lines.py:42:16: F401 [*] `foo12` imported but unused
| |
44 | # Continuation, but not as the last content in the file. 40 | x = 1; \
45 | x = 1; \ 41 | \
46 | import foo12 42 | import foo12
| ^^^^^ F401 | ^^^^^ F401
47 | 43 |
48 | # Continuation, followed by end-of-file. (Removing `import foo` would cause a syntax 44 | if True:
| |
= help: Remove unused import: `foo12` = help: Remove unused import: `foo12`
Fix Fix
43 43 | 37 37 | ;x = 1
44 44 | # Continuation, but not as the last content in the file. 38 38 |
45 45 | x = 1; \ 39 39 | if True:
46 |-import foo12 40 |- x = 1; \
47 46 | 41 |- \
47 |+ 42 |- import foo12
48 48 | # Continuation, followed by end-of-file. (Removing `import foo` would cause a syntax 40 |+ x = 1;
49 49 | # error.) 43 41 |
50 50 | x = 1; \ 44 42 | if True:
45 43 | x = 1; \
multi_statement_lines.py:51:8: F401 [*] `foo13` imported but unused multi_statement_lines.py:47:12: F401 [*] `foo13` imported but unused
| |
49 | # error.) 45 | x = 1; \
50 | x = 1; \ 46 | \
51 | import foo13 47 | import foo13
| ^^^^^ F401 | ^^^^^ F401
| |
= help: Remove unused import: `foo13` = help: Remove unused import: `foo13`
Fix Fix
48 48 | # Continuation, followed by end-of-file. (Removing `import foo` would cause a syntax 42 42 | import foo12
49 49 | # error.) 43 43 |
50 50 | x = 1; \ 44 44 | if True:
51 |-import foo13 45 |- x = 1; \
51 |+ 46 |-\
47 |- import foo13
45 |+ x = 1;
48 46 |
49 47 |
50 48 | if True:
multi_statement_lines.py:53:12: F401 [*] `foo14` imported but unused
|
51 | x = 1; \
52 | # \
53 | import foo14
| ^^^^^ F401
54 |
55 | # Continuation, but not as the last content in the file.
|
= help: Remove unused import: `foo14`
Fix
50 50 | if True:
51 51 | x = 1; \
52 52 | # \
53 |- import foo14
54 53 |
55 54 | # Continuation, but not as the last content in the file.
56 55 | x = 1; \
multi_statement_lines.py:57:8: F401 [*] `foo15` imported but unused
|
55 | # Continuation, but not as the last content in the file.
56 | x = 1; \
57 | import foo15
| ^^^^^ F401
58 |
59 | # Continuation, followed by end-of-file. (Removing `import foo` would cause a syntax
|
= help: Remove unused import: `foo15`
Fix
53 53 | import foo14
54 54 |
55 55 | # Continuation, but not as the last content in the file.
56 |-x = 1; \
57 |-import foo15
56 |+x = 1;
58 57 |
59 58 | # Continuation, followed by end-of-file. (Removing `import foo` would cause a syntax
60 59 | # error.)
multi_statement_lines.py:62:8: F401 [*] `foo16` imported but unused
|
60 | # error.)
61 | x = 1; \
62 | import foo16
| ^^^^^ F401
|
= help: Remove unused import: `foo16`
Fix
58 58 |
59 59 | # Continuation, followed by end-of-file. (Removing `import foo` would cause a syntax
60 60 | # error.)
61 |-x = 1; \
62 |-import foo16
61 |+x = 1;

View file

@ -122,7 +122,7 @@ pub(crate) fn logging_call(
return; return;
} }
let message_args = call_args.args.len() - 1; let message_args = call_args.num_args() - 1;
if checker.enabled(Rule::LoggingTooManyArgs) { if checker.enabled(Rule::LoggingTooManyArgs) {
if summary.num_positional < message_args { if summary.num_positional < message_args {
@ -134,7 +134,7 @@ pub(crate) fn logging_call(
if checker.enabled(Rule::LoggingTooFewArgs) { if checker.enabled(Rule::LoggingTooFewArgs) {
if message_args > 0 if message_args > 0
&& call_args.kwargs.is_empty() && call_args.num_kwargs() == 0
&& summary.num_positional > message_args && summary.num_positional > message_args
{ {
checker checker

View file

@ -103,13 +103,8 @@ pub(crate) fn useless_return<'a>(
let mut diagnostic = Diagnostic::new(UselessReturn, last_stmt.range()); let mut diagnostic = Diagnostic::new(UselessReturn, last_stmt.range());
if checker.patch(diagnostic.kind.rule()) { if checker.patch(diagnostic.kind.rule()) {
let edit = autofix::edits::delete_stmt( let edit =
last_stmt, autofix::edits::delete_stmt(last_stmt, Some(stmt), checker.locator, checker.indexer);
Some(stmt),
checker.locator,
checker.indexer,
checker.stylist,
);
diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(Some(stmt)))); diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(Some(stmt))));
} }
checker.diagnostics.push(diagnostic); checker.diagnostics.push(diagnostic);

View file

@ -226,7 +226,6 @@ fn fix_py2_block(
if matches!(block.leading_token.tok, StartTok::If) { parent } else { None }, if matches!(block.leading_token.tok, StartTok::If) { parent } else { None },
checker.locator, checker.locator,
checker.indexer, checker.indexer,
checker.stylist,
); );
return Some(Fix::suggested(edit)); return Some(Fix::suggested(edit));
}; };

View file

@ -68,13 +68,7 @@ pub(crate) fn useless_metaclass_type(
if checker.patch(diagnostic.kind.rule()) { if checker.patch(diagnostic.kind.rule()) {
let stmt = checker.semantic().stmt(); let stmt = checker.semantic().stmt();
let parent = checker.semantic().stmt_parent(); let parent = checker.semantic().stmt_parent();
let edit = autofix::edits::delete_stmt( let edit = autofix::edits::delete_stmt(stmt, parent, checker.locator, checker.indexer);
stmt,
parent,
checker.locator,
checker.indexer,
checker.stylist,
);
diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(parent))); diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(parent)));
} }
checker.diagnostics.push(diagnostic); checker.diagnostics.push(diagnostic);

View file

@ -12,7 +12,7 @@ use rustpython_parser::ast::{
use rustpython_parser::{lexer, Mode, Tok}; use rustpython_parser::{lexer, Mode, Tok};
use smallvec::SmallVec; use smallvec::SmallVec;
use ruff_python_whitespace::{PythonWhitespace, UniversalNewlineIterator}; use ruff_python_whitespace::{is_python_whitespace, PythonWhitespace, UniversalNewlineIterator};
use crate::call_path::CallPath; use crate::call_path::CallPath;
use crate::source_code::{Indexer, Locator}; use crate::source_code::{Indexer, Locator};
@ -717,19 +717,19 @@ pub fn map_subscript(expr: &Expr) -> &Expr {
} }
/// Returns `true` if a statement or expression includes at least one comment. /// Returns `true` if a statement or expression includes at least one comment.
pub fn has_comments<T>(located: &T, locator: &Locator) -> bool pub fn has_comments<T>(node: &T, locator: &Locator) -> bool
where where
T: Ranged, T: Ranged,
{ {
let start = if has_leading_content(located, locator) { let start = if has_leading_content(node.start(), locator) {
located.start() node.start()
} else { } else {
locator.line_start(located.start()) locator.line_start(node.start())
}; };
let end = if has_trailing_content(located, locator) { let end = if has_trailing_content(node.end(), locator) {
located.end() node.end()
} else { } else {
locator.line_end(located.end()) locator.line_end(node.end())
}; };
has_comments_in(TextRange::new(start, end), locator) has_comments_in(TextRange::new(start, end), locator)
@ -927,7 +927,7 @@ where
{ {
fn visit_stmt(&mut self, stmt: &'b Stmt) { fn visit_stmt(&mut self, stmt: &'b Stmt) {
match stmt { match stmt {
Stmt::FunctionDef(_) | Stmt::AsyncFunctionDef(_) => { Stmt::FunctionDef(_) | Stmt::AsyncFunctionDef(_) | Stmt::ClassDef(_) => {
// Don't recurse. // Don't recurse.
} }
Stmt::Return(stmt) => self.returns.push(stmt), Stmt::Return(stmt) => self.returns.push(stmt),
@ -982,29 +982,23 @@ where
} }
} }
/// Return `true` if a [`Ranged`] has leading content. /// Return `true` if the node starting the given [`TextSize`] has leading content.
pub fn has_leading_content<T>(located: &T, locator: &Locator) -> bool pub fn has_leading_content(offset: TextSize, locator: &Locator) -> bool {
where let line_start = locator.line_start(offset);
T: Ranged, let leading = &locator.contents()[TextRange::new(line_start, offset)];
{ leading.chars().any(|char| !is_python_whitespace(char))
let line_start = locator.line_start(located.start());
let leading = &locator.contents()[TextRange::new(line_start, located.start())];
leading.chars().any(|char| !char.is_whitespace())
} }
/// Return `true` if a [`Ranged`] has trailing content. /// Return `true` if the node ending at the given [`TextSize`] has trailing content.
pub fn has_trailing_content<T>(located: &T, locator: &Locator) -> bool pub fn has_trailing_content(offset: TextSize, locator: &Locator) -> bool {
where let line_end = locator.line_end(offset);
T: Ranged, let trailing = &locator.contents()[TextRange::new(offset, line_end)];
{
let line_end = locator.line_end(located.end());
let trailing = &locator.contents()[TextRange::new(located.end(), line_end)];
for char in trailing.chars() { for char in trailing.chars() {
if char == '#' { if char == '#' {
return false; return false;
} }
if !char.is_whitespace() { if !is_python_whitespace(char) {
return true; return true;
} }
} }
@ -1020,11 +1014,11 @@ where
let trailing = &locator.contents()[TextRange::new(located.end(), line_end)]; let trailing = &locator.contents()[TextRange::new(located.end(), line_end)];
for (i, char) in trailing.chars().enumerate() { for (index, char) in trailing.char_indices() {
if char == '#' { if char == '#' {
return TextSize::try_from(i).ok(); return TextSize::try_from(index).ok();
} }
if !char.is_whitespace() { if !is_python_whitespace(char) {
return None; return None;
} }
} }
@ -1040,7 +1034,7 @@ pub fn trailing_lines_end(stmt: &Stmt, locator: &Locator) -> TextSize {
UniversalNewlineIterator::with_offset(rest, line_end) UniversalNewlineIterator::with_offset(rest, line_end)
.take_while(|line| line.trim_whitespace().is_empty()) .take_while(|line| line.trim_whitespace().is_empty())
.last() .last()
.map_or(line_end, |l| l.full_end()) .map_or(line_end, |line| line.full_end())
} }
/// Return the range of the first parenthesis pair after a given [`TextSize`]. /// Return the range of the first parenthesis pair after a given [`TextSize`].
@ -1081,7 +1075,7 @@ pub fn first_colon_range(range: TextRange, locator: &Locator) -> Option<TextRang
let contents = &locator.contents()[range]; let contents = &locator.contents()[range];
let range = lexer::lex_starts_at(contents, Mode::Module, range.start()) let range = lexer::lex_starts_at(contents, Mode::Module, range.start())
.flatten() .flatten()
.find(|(kind, _)| matches!(kind, Tok::Colon)) .find(|(tok, _)| tok.is_colon())
.map(|(_, range)| range); .map(|(_, range)| range);
range range
} }
@ -1105,13 +1099,12 @@ pub fn elif_else_range(stmt: &ast::StmtIf, locator: &Locator) -> Option<TextRang
.map(|(_, range)| range) .map(|(_, range)| range)
} }
/// Return `true` if a `Stmt` appears to be part of a multi-statement line, with /// Given an offset at the end of a line (including newlines), return the offset of the
/// other statements preceding it. /// continuation at the end of that line.
pub fn preceded_by_continuation(stmt: &Stmt, indexer: &Indexer, locator: &Locator) -> bool { fn find_continuation(offset: TextSize, locator: &Locator, indexer: &Indexer) -> Option<TextSize> {
let previous_line_end = locator.line_start(stmt.start()); let newline_pos = usize::from(offset).saturating_sub(1);
let newline_pos = usize::from(previous_line_end).saturating_sub(1);
// Compute start of preceding line // Skip the newline.
let newline_len = match locator.contents().as_bytes()[newline_pos] { let newline_len = match locator.contents().as_bytes()[newline_pos] {
b'\n' => { b'\n' => {
if locator if locator
@ -1126,24 +1119,77 @@ pub fn preceded_by_continuation(stmt: &Stmt, indexer: &Indexer, locator: &Locato
} }
} }
b'\r' => 1, b'\r' => 1,
// No preceding line // No preceding line.
_ => return false, _ => return None,
}; };
// See if the position is in the continuation line starts indexer
indexer.is_continuation(previous_line_end - TextSize::from(newline_len), locator) .is_continuation(offset - TextSize::from(newline_len), locator)
.then(|| offset - TextSize::from(newline_len) - TextSize::from(1))
}
/// If the node starting at the given [`TextSize`] is preceded by at least one continuation line
/// (i.e., a line ending in a backslash), return the starting offset of the first such continuation
/// character.
///
/// For example, given:
/// ```python
/// x = 1; \
/// y = 2
/// ```
///
/// When passed the offset of `y`, this function will return the offset of the backslash at the end
/// of the first line.
///
/// Similarly, given:
/// ```python
/// x = 1; \
/// \
/// y = 2;
/// ```
///
/// When passed the offset of `y`, this function will again return the offset of the backslash at
/// the end of the first line.
pub fn preceded_by_continuations(
offset: TextSize,
locator: &Locator,
indexer: &Indexer,
) -> Option<TextSize> {
// Find the first preceding continuation.
let mut continuation = find_continuation(locator.line_start(offset), locator, indexer)?;
// Continue searching for continuations, in the unlikely event that we have multiple
// continuations in a row.
loop {
let previous_line_end = locator.line_start(continuation);
if locator
.slice(TextRange::new(previous_line_end, continuation))
.chars()
.all(is_python_whitespace)
{
if let Some(next_continuation) = find_continuation(previous_line_end, locator, indexer)
{
continuation = next_continuation;
continue;
}
}
break;
}
Some(continuation)
} }
/// Return `true` if a `Stmt` appears to be part of a multi-statement line, with /// Return `true` if a `Stmt` appears to be part of a multi-statement line, with
/// other statements preceding it. /// other statements preceding it.
pub fn preceded_by_multi_statement_line(stmt: &Stmt, locator: &Locator, indexer: &Indexer) -> bool { pub fn preceded_by_multi_statement_line(stmt: &Stmt, locator: &Locator, indexer: &Indexer) -> bool {
has_leading_content(stmt, locator) || preceded_by_continuation(stmt, indexer, locator) has_leading_content(stmt.start(), locator)
|| preceded_by_continuations(stmt.start(), locator, indexer).is_some()
} }
/// Return `true` if a `Stmt` appears to be part of a multi-statement line, with /// Return `true` if a `Stmt` appears to be part of a multi-statement line, with
/// other statements following it. /// other statements following it.
pub fn followed_by_multi_statement_line(stmt: &Stmt, locator: &Locator) -> bool { pub fn followed_by_multi_statement_line(stmt: &Stmt, locator: &Locator) -> bool {
has_trailing_content(stmt, locator) has_trailing_content(stmt.end(), locator)
} }
/// Return `true` if a `Stmt` is a docstring. /// Return `true` if a `Stmt` is a docstring.
@ -1165,11 +1211,11 @@ pub fn is_docstring_stmt(stmt: &Stmt) -> bool {
} }
} }
#[derive(Default)]
/// A simple representation of a call's positional and keyword arguments. /// A simple representation of a call's positional and keyword arguments.
#[derive(Default)]
pub struct SimpleCallArgs<'a> { pub struct SimpleCallArgs<'a> {
pub args: Vec<&'a Expr>, args: Vec<&'a Expr>,
pub kwargs: FxHashMap<&'a str, &'a Expr>, kwargs: FxHashMap<&'a str, &'a Expr>,
} }
impl<'a> SimpleCallArgs<'a> { impl<'a> SimpleCallArgs<'a> {
@ -1213,6 +1259,16 @@ impl<'a> SimpleCallArgs<'a> {
self.args.len() + self.kwargs.len() self.args.len() + self.kwargs.len()
} }
/// Return the number of positional arguments.
pub fn num_args(&self) -> usize {
self.args.len()
}
/// Return the number of keyword arguments.
pub fn num_kwargs(&self) -> usize {
self.kwargs.len()
}
/// Return `true` if there are no positional or keyword arguments. /// Return `true` if there are no positional or keyword arguments.
pub fn is_empty(&self) -> bool { pub fn is_empty(&self) -> bool {
self.len() == 0 self.len() == 0
@ -1507,7 +1563,7 @@ mod tests {
use anyhow::Result; use anyhow::Result;
use ruff_text_size::{TextLen, TextRange, TextSize}; use ruff_text_size::{TextLen, TextRange, TextSize};
use rustpython_ast::{CmpOp, Expr, Stmt}; use rustpython_ast::{CmpOp, Expr, Ranged, Stmt};
use rustpython_parser::ast::Suite; use rustpython_parser::ast::Suite;
use rustpython_parser::Parse; use rustpython_parser::Parse;
@ -1523,25 +1579,25 @@ mod tests {
let program = Suite::parse(contents, "<filename>")?; let program = Suite::parse(contents, "<filename>")?;
let stmt = program.first().unwrap(); let stmt = program.first().unwrap();
let locator = Locator::new(contents); let locator = Locator::new(contents);
assert!(!has_trailing_content(stmt, &locator)); assert!(!has_trailing_content(stmt.end(), &locator));
let contents = "x = 1; y = 2"; let contents = "x = 1; y = 2";
let program = Suite::parse(contents, "<filename>")?; let program = Suite::parse(contents, "<filename>")?;
let stmt = program.first().unwrap(); let stmt = program.first().unwrap();
let locator = Locator::new(contents); let locator = Locator::new(contents);
assert!(has_trailing_content(stmt, &locator)); assert!(has_trailing_content(stmt.end(), &locator));
let contents = "x = 1 "; let contents = "x = 1 ";
let program = Suite::parse(contents, "<filename>")?; let program = Suite::parse(contents, "<filename>")?;
let stmt = program.first().unwrap(); let stmt = program.first().unwrap();
let locator = Locator::new(contents); let locator = Locator::new(contents);
assert!(!has_trailing_content(stmt, &locator)); assert!(!has_trailing_content(stmt.end(), &locator));
let contents = "x = 1 # Comment"; let contents = "x = 1 # Comment";
let program = Suite::parse(contents, "<filename>")?; let program = Suite::parse(contents, "<filename>")?;
let stmt = program.first().unwrap(); let stmt = program.first().unwrap();
let locator = Locator::new(contents); let locator = Locator::new(contents);
assert!(!has_trailing_content(stmt, &locator)); assert!(!has_trailing_content(stmt.end(), &locator));
let contents = r#" let contents = r#"
x = 1 x = 1
@ -1551,7 +1607,7 @@ y = 2
let program = Suite::parse(contents, "<filename>")?; let program = Suite::parse(contents, "<filename>")?;
let stmt = program.first().unwrap(); let stmt = program.first().unwrap();
let locator = Locator::new(contents); let locator = Locator::new(contents);
assert!(!has_trailing_content(stmt, &locator)); assert!(!has_trailing_content(stmt.end(), &locator));
Ok(()) Ok(())
} }