From 8e06140d1d3646b51e1fc572043916f03e15d034 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 19 Jun 2023 22:04:28 -0400 Subject: [PATCH] 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). --- .../pyflakes/multi_statement_lines.py | 23 +- crates/ruff/src/autofix/edits.rs | 48 +-- .../rules/duplicate_class_field_definition.rs | 1 - .../flake8_pie/rules/no_unnecessary_pass.rs | 8 +- .../rules/ellipsis_in_non_empty_class_body.rs | 9 +- .../flake8_pyi/rules/pass_in_class_body.rs | 9 +- .../rules/str_or_repr_defined_in_stub.rs | 8 +- .../src/rules/flake8_return/rules/function.rs | 9 +- .../rules/empty_type_checking_block.rs | 8 +- .../pycodestyle/rules/lambda_assignment.rs | 4 +- .../rules/pyflakes/rules/unused_variable.rs | 16 +- ...yflakes__tests__multi_statement_lines.snap | 388 ++++++++++-------- crates/ruff/src/rules/pylint/rules/logging.rs | 4 +- .../src/rules/pylint/rules/useless_return.rs | 9 +- .../pyupgrade/rules/outdated_version_block.rs | 1 - .../pyupgrade/rules/useless_metaclass_type.rs | 8 +- crates/ruff_python_ast/src/helpers.rs | 158 ++++--- 17 files changed, 394 insertions(+), 317 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/pyflakes/multi_statement_lines.py b/crates/ruff/resources/test/fixtures/pyflakes/multi_statement_lines.py index d4e8712933..16233c83db 100644 --- a/crates/ruff/resources/test/fixtures/pyflakes/multi_statement_lines.py +++ b/crates/ruff/resources/test/fixtures/pyflakes/multi_statement_lines.py @@ -1,4 +1,3 @@ - if True: import foo1; x = 1 import foo2; x = 1 @@ -11,7 +10,6 @@ if True: import foo4 \ ; x = 1 - if True: x = 1; import foo5 @@ -20,12 +18,10 @@ if True: x = 1; \ import foo6 - if True: x = 1 \ ; import foo7 - if True: x = 1; import foo8; x = 1 x = 1; import foo9; x = 1 @@ -40,12 +36,27 @@ if True: ;import foo11 \ ;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. x = 1; \ -import foo12 +import foo15 # Continuation, followed by end-of-file. (Removing `import foo` would cause a syntax # error.) x = 1; \ -import foo13 +import foo16 \ No newline at end of file diff --git a/crates/ruff/src/autofix/edits.rs b/crates/ruff/src/autofix/edits.rs index c6218e5060..285d42c258 100644 --- a/crates/ruff/src/autofix/edits.rs +++ b/crates/ruff/src/autofix/edits.rs @@ -29,7 +29,6 @@ pub(crate) fn delete_stmt( parent: Option<&Stmt>, locator: &Locator, indexer: &Indexer, - stylist: &Stylist, ) -> Edit { if parent .map(|parent| is_lone_child(stmt, parent)) @@ -39,18 +38,15 @@ pub(crate) fn delete_stmt( // it with a `pass`. Edit::range_replacement("pass".to_string(), stmt.range()) } 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); 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()) - } else if helpers::preceded_by_continuation(stmt, indexer, locator) { - if is_end_of_file(stmt, locator) && locator.is_at_start_of_line(stmt.start()) { - // Special-case: a file can't end in a continuation. - Edit::range_replacement(stylist.line_ending().to_string(), stmt.range()) - } else { - Edit::range_deletion(stmt.range()) - } + } else if let Some(start) = + helpers::preceded_by_continuations(stmt.start(), locator, indexer) + { + Edit::range_deletion(TextRange::new(start, stmt.end())) } else { let range = locator.full_lines_range(stmt.range()); Edit::range_deletion(range) @@ -68,7 +64,7 @@ pub(crate) fn remove_unused_imports<'a>( stylist: &Stylist, ) -> Result { 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())), } } @@ -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 /// of a multi-statement line. -fn trailing_semicolon(stmt: &Stmt, locator: &Locator) -> Option { - let contents = locator.after(stmt.end()); +fn trailing_semicolon(offset: TextSize, locator: &Locator) -> Option { + let contents = locator.after(offset); for line in NewlineWithTrailingNewline::from(contents) { let trimmed = line.trim_whitespace_start(); if trimmed.starts_with(';') { 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('\\') { @@ -284,16 +280,11 @@ fn next_stmt_break(semicolon: TextSize, locator: &Locator) -> TextSize { 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)] mod tests { use anyhow::Result; use ruff_text_size::TextSize; - use rustpython_parser::ast::Suite; + use rustpython_parser::ast::{Ranged, Suite}; use rustpython_parser::Parse; use ruff_python_ast::source_code::Locator; @@ -306,19 +297,25 @@ mod tests { let program = Suite::parse(contents, "")?; let stmt = program.first().unwrap(); 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 program = Suite::parse(contents, "")?; let stmt = program.first().unwrap(); 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 program = Suite::parse(contents, "")?; let stmt = program.first().unwrap(); 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#" x = 1 \ @@ -328,7 +325,10 @@ x = 1 \ let program = Suite::parse(contents, "")?; let stmt = program.first().unwrap(); 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(()) } diff --git a/crates/ruff/src/rules/flake8_pie/rules/duplicate_class_field_definition.rs b/crates/ruff/src/rules/flake8_pie/rules/duplicate_class_field_definition.rs index ce0991ac98..6351778925 100644 --- a/crates/ruff/src/rules/flake8_pie/rules/duplicate_class_field_definition.rs +++ b/crates/ruff/src/rules/flake8_pie/rules/duplicate_class_field_definition.rs @@ -93,7 +93,6 @@ pub(crate) fn duplicate_class_field_definition<'a, 'b>( Some(parent), checker.locator, checker.indexer, - checker.stylist, ); diagnostic.set_fix(Fix::suggested(edit).isolate(checker.isolation(Some(parent)))); } diff --git a/crates/ruff/src/rules/flake8_pie/rules/no_unnecessary_pass.rs b/crates/ruff/src/rules/flake8_pie/rules/no_unnecessary_pass.rs index f9047c5004..1cc40d052e 100644 --- a/crates/ruff/src/rules/flake8_pie/rules/no_unnecessary_pass.rs +++ b/crates/ruff/src/rules/flake8_pie/rules/no_unnecessary_pass.rs @@ -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) { Edit::range_deletion(stmt.range().add_end(index)) } else { - autofix::edits::delete_stmt( - stmt, - None, - checker.locator, - checker.indexer, - checker.stylist, - ) + autofix::edits::delete_stmt(stmt, None, checker.locator, checker.indexer) }; diagnostic.set_fix(Fix::automatic(edit)); } diff --git a/crates/ruff/src/rules/flake8_pyi/rules/ellipsis_in_non_empty_class_body.rs b/crates/ruff/src/rules/flake8_pyi/rules/ellipsis_in_non_empty_class_body.rs index 3648d27345..5239611c0f 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/ellipsis_in_non_empty_class_body.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/ellipsis_in_non_empty_class_body.rs @@ -69,13 +69,8 @@ pub(crate) fn ellipsis_in_non_empty_class_body<'a>( let mut diagnostic = Diagnostic::new(EllipsisInNonEmptyClassBody, stmt.range()); if checker.patch(diagnostic.kind.rule()) { - let edit = autofix::edits::delete_stmt( - stmt, - Some(parent), - checker.locator, - checker.indexer, - checker.stylist, - ); + let edit = + autofix::edits::delete_stmt(stmt, Some(parent), checker.locator, checker.indexer); diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(Some(parent)))); } checker.diagnostics.push(diagnostic); diff --git a/crates/ruff/src/rules/flake8_pyi/rules/pass_in_class_body.rs b/crates/ruff/src/rules/flake8_pyi/rules/pass_in_class_body.rs index 43f1829e27..d6f947fd34 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/pass_in_class_body.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/pass_in_class_body.rs @@ -39,13 +39,8 @@ pub(crate) fn pass_in_class_body<'a>( let mut diagnostic = Diagnostic::new(PassInClassBody, stmt.range()); if checker.patch(diagnostic.kind.rule()) { - let edit = autofix::edits::delete_stmt( - stmt, - Some(parent), - checker.locator, - checker.indexer, - checker.stylist, - ); + let edit = + autofix::edits::delete_stmt(stmt, Some(parent), checker.locator, checker.indexer); diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(Some(parent)))); } checker.diagnostics.push(diagnostic); diff --git a/crates/ruff/src/rules/flake8_pyi/rules/str_or_repr_defined_in_stub.rs b/crates/ruff/src/rules/flake8_pyi/rules/str_or_repr_defined_in_stub.rs index 3ae9eab2f8..54611b75e3 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/str_or_repr_defined_in_stub.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/str_or_repr_defined_in_stub.rs @@ -95,13 +95,7 @@ pub(crate) fn str_or_repr_defined_in_stub(checker: &mut Checker, stmt: &Stmt) { if checker.patch(diagnostic.kind.rule()) { let stmt = checker.semantic().stmt(); let parent = checker.semantic().stmt_parent(); - let edit = delete_stmt( - stmt, - parent, - checker.locator, - checker.indexer, - checker.stylist, - ); + let edit = delete_stmt(stmt, parent, checker.locator, checker.indexer); diagnostic.set_fix( Fix::automatic(edit).isolate(checker.isolation(checker.semantic().stmt_parent())), ); diff --git a/crates/ruff/src/rules/flake8_return/rules/function.rs b/crates/ruff/src/rules/flake8_return/rules/function.rs index 1fb986ca2b..11b5582579 100644 --- a/crates/ruff/src/rules/flake8_return/rules/function.rs +++ b/crates/ruff/src/rules/flake8_return/rules/function.rs @@ -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 // edit, since we're editing the preceding statement, so no conflicting edit would // be allowed to remove that preceding statement. - let delete_return = edits::delete_stmt( - stmt, - None, - checker.locator, - checker.indexer, - checker.stylist, - ); + let delete_return = + edits::delete_stmt(stmt, None, checker.locator, checker.indexer); // Replace the `x = 1` statement with `return 1`. let content = checker.locator.slice(assign.range()); diff --git a/crates/ruff/src/rules/flake8_type_checking/rules/empty_type_checking_block.rs b/crates/ruff/src/rules/flake8_type_checking/rules/empty_type_checking_block.rs index 88ed829041..f3d0929615 100644 --- a/crates/ruff/src/rules/flake8_type_checking/rules/empty_type_checking_block.rs +++ b/crates/ruff/src/rules/flake8_type_checking/rules/empty_type_checking_block.rs @@ -62,13 +62,7 @@ pub(crate) fn empty_type_checking_block(checker: &mut Checker, stmt: &ast::StmtI // Delete the entire type-checking block. let stmt = checker.semantic().stmt(); let parent = checker.semantic().stmt_parent(); - let edit = autofix::edits::delete_stmt( - stmt, - parent, - checker.locator, - checker.indexer, - checker.stylist, - ); + let edit = autofix::edits::delete_stmt(stmt, parent, checker.locator, checker.indexer); diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(parent))); } checker.diagnostics.push(diagnostic); diff --git a/crates/ruff/src/rules/pycodestyle/rules/lambda_assignment.rs b/crates/ruff/src/rules/pycodestyle/rules/lambda_assignment.rs index d6126674c7..9d65aa6692 100644 --- a/crates/ruff/src/rules/pycodestyle/rules/lambda_assignment.rs +++ b/crates/ruff/src/rules/pycodestyle/rules/lambda_assignment.rs @@ -78,8 +78,8 @@ pub(crate) fn lambda_assignment( // See https://github.com/astral-sh/ruff/issues/3046 if checker.patch(diagnostic.kind.rule()) && !checker.semantic().scope().kind.is_class() - && !has_leading_content(stmt, checker.locator) - && !has_trailing_content(stmt, checker.locator) + && !has_leading_content(stmt.start(), checker.locator) + && !has_trailing_content(stmt.end(), checker.locator) { let first_line = checker.locator.line(stmt.start()); let indentation = leading_indentation(first_line); diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs index f9ede7b63b..a611eb8375 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs @@ -210,13 +210,7 @@ fn remove_unused_variable( Some(Fix::suggested(edit)) } else { // If (e.g.) assigning to a constant (`x = 1`), delete the entire statement. - let edit = delete_stmt( - stmt, - parent, - checker.locator, - checker.indexer, - checker.stylist, - ); + let edit = delete_stmt(stmt, parent, checker.locator, checker.indexer); Some(Fix::suggested(edit).isolate(checker.isolation(parent))) }; } @@ -241,13 +235,7 @@ fn remove_unused_variable( Some(Fix::suggested(edit)) } else { // If (e.g.) assigning to a constant (`x = 1`), delete the entire statement. - let edit = delete_stmt( - stmt, - parent, - checker.locator, - checker.indexer, - checker.stylist, - ); + let edit = delete_stmt(stmt, parent, checker.locator, checker.indexer); Some(Fix::suggested(edit).isolate(checker.isolation(parent))) }; } diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__multi_statement_lines.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__multi_statement_lines.snap index 00f0e38f00..f384bf81ca 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__multi_statement_lines.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__multi_statement_lines.snap @@ -1,257 +1,325 @@ --- 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: -3 | import foo1; x = 1 +1 | if True: +2 | import foo1; x = 1 | ^^^^ F401 -4 | import foo2; x = 1 +3 | import foo2; x = 1 | = help: Remove unused import: `foo1` ℹ Fix -1 1 | -2 2 | if True: -3 |- import foo1; x = 1 - 3 |+ x = 1 -4 4 | import foo2; x = 1 -5 5 | -6 6 | if True: +1 1 | if True: +2 |- import foo1; x = 1 + 2 |+ x = 1 +3 3 | import foo2; x = 1 +4 4 | +5 5 | 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: -3 | import foo1; x = 1 -4 | import foo2; x = 1 +1 | if True: +2 | import foo1; x = 1 +3 | import foo2; x = 1 | ^^^^ F401 -5 | -6 | if True: +4 | +5 | if True: | = help: Remove unused import: `foo2` ℹ Fix -1 1 | -2 2 | if True: -3 3 | import foo1; x = 1 -4 |- import foo2; x = 1 - 4 |+ x = 1 -5 5 | -6 6 | if True: -7 7 | import foo3; \ +1 1 | if True: +2 2 | import foo1; x = 1 +3 |- import foo2; x = 1 + 3 |+ x = 1 +4 4 | +5 5 | if True: +6 6 | 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: -7 | import foo3; \ +5 | if True: +6 | import foo3; \ | ^^^^ F401 -8 | x = 1 +7 | x = 1 | = help: Remove unused import: `foo3` ℹ Fix -4 4 | import foo2; x = 1 -5 5 | -6 6 | if True: -7 |- import foo3; \ -8 |-x = 1 - 7 |+ x = 1 -9 8 | -10 9 | if True: -11 10 | import foo4 \ +3 3 | import foo2; x = 1 +4 4 | +5 5 | if True: +6 |- import foo3; \ +7 |-x = 1 + 6 |+ x = 1 +8 7 | +9 8 | if True: +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: -11 | import foo4 \ + 9 | if True: +10 | import foo4 \ | ^^^^ F401 -12 | ; x = 1 +11 | ; x = 1 | = help: Remove unused import: `foo4` ℹ Fix -8 8 | x = 1 -9 9 | -10 10 | if True: -11 |- import foo4 \ -12 |- ; x = 1 - 11 |+ x = 1 -13 12 | -14 13 | -15 14 | if True: +7 7 | x = 1 +8 8 | +9 9 | if True: +10 |- import foo4 \ +11 |- ; x = 1 + 10 |+ x = 1 +12 11 | +13 12 | 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: -16 | x = 1; import foo5 +13 | if True: +14 | x = 1; import foo5 | ^^^^ F401 | = help: Remove unused import: `foo5` ℹ Fix -13 13 | -14 14 | -15 15 | if True: -16 |- x = 1; import foo5 - 16 |+ x = 1; -17 17 | -18 18 | -19 19 | if True: +11 11 | ; x = 1 +12 12 | +13 13 | if True: +14 |- x = 1; import foo5 + 14 |+ x = 1; +15 15 | +16 16 | +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: -20 | x = 1; \ -21 | import foo6 +17 | if True: +18 | x = 1; \ +19 | import foo6 | ^^^^ F401 +20 | +21 | if True: | = help: Remove unused import: `foo6` ℹ Fix -18 18 | -19 19 | if True: -20 20 | x = 1; \ -21 |- import foo6 - 21 |+ -22 22 | -23 23 | -24 24 | if True: +15 15 | +16 16 | +17 17 | if True: +18 |- x = 1; \ +19 |- import foo6 + 18 |+ x = 1; +20 19 | +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: -25 | x = 1 \ -26 | ; import foo7 +21 | if True: +22 | x = 1 \ +23 | ; import foo7 | ^^^^ F401 +24 | +25 | if True: | = help: Remove unused import: `foo7` ℹ Fix -23 23 | -24 24 | if True: -25 25 | x = 1 \ -26 |- ; import foo7 - 26 |+ ; -27 27 | -28 28 | -29 29 | if True: +20 20 | +21 21 | if True: +22 22 | x = 1 \ +23 |- ; import foo7 + 23 |+ ; +24 24 | +25 25 | 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: -30 | x = 1; import foo8; x = 1 +25 | if True: +26 | x = 1; import foo8; x = 1 | ^^^^ F401 -31 | x = 1; import foo9; x = 1 +27 | x = 1; import foo9; x = 1 | = help: Remove unused import: `foo8` ℹ 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 | 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: -30 | x = 1; import foo8; x = 1 -31 | x = 1; import foo9; x = 1 +25 | if True: +26 | x = 1; import foo8; x = 1 +27 | x = 1; import foo9; x = 1 | ^^^^ F401 -32 | -33 | if True: +28 | +29 | if True: | = help: Remove unused import: `foo9` ℹ 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 | 29 29 | if True: -30 30 | x = 1; import foo8; x = 1 -31 |- x = 1; import foo9; x = 1 - 31 |+ x = 1; x = 1 -32 32 | -33 33 | if True: -34 34 | x = 1; \ +30 30 | 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: -34 | x = 1; \ -35 | import foo10; \ +29 | if True: +30 | x = 1; \ +31 | import foo10; \ | ^^^^^ F401 -36 | x = 1 +32 | x = 1 | = help: Remove unused import: `foo10` ℹ Fix -32 32 | -33 33 | if True: -34 34 | x = 1; \ -35 |- import foo10; \ -36 |- x = 1 - 35 |+ x = 1 -37 36 | -38 37 | if True: -39 38 | x = 1 \ +28 28 | +29 29 | if True: +30 30 | x = 1; \ +31 |- import foo10; \ +32 |- x = 1 + 31 |+ x = 1 +33 32 | +34 33 | if True: +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: -39 | x = 1 \ -40 | ;import foo11 \ +34 | if True: +35 | x = 1 \ +36 | ;import foo11 \ | ^^^^^ F401 -41 | ;x = 1 +37 | ;x = 1 | = help: Remove unused import: `foo11` ℹ Fix -37 37 | -38 38 | if True: -39 39 | x = 1 \ -40 |- ;import foo11 \ -41 40 | ;x = 1 -42 41 | -43 42 | +33 33 | +34 34 | if True: +35 35 | x = 1 \ +36 |- ;import foo11 \ +37 36 | ;x = 1 +38 37 | +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. -45 | x = 1; \ -46 | import foo12 - | ^^^^^ F401 -47 | -48 | # Continuation, followed by end-of-file. (Removing `import foo` would cause a syntax +40 | x = 1; \ +41 | \ +42 | import foo12 + | ^^^^^ F401 +43 | +44 | if True: | = help: Remove unused import: `foo12` ℹ Fix -43 43 | -44 44 | # Continuation, but not as the last content in the file. -45 45 | x = 1; \ -46 |-import foo12 -47 46 | - 47 |+ -48 48 | # Continuation, followed by end-of-file. (Removing `import foo` would cause a syntax -49 49 | # error.) -50 50 | x = 1; \ +37 37 | ;x = 1 +38 38 | +39 39 | if True: +40 |- x = 1; \ +41 |- \ +42 |- import foo12 + 40 |+ x = 1; +43 41 | +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.) -50 | x = 1; \ -51 | import foo13 - | ^^^^^ F401 +45 | x = 1; \ +46 | \ +47 | import foo13 + | ^^^^^ F401 | = help: Remove unused import: `foo13` ℹ Fix -48 48 | # Continuation, followed by end-of-file. (Removing `import foo` would cause a syntax -49 49 | # error.) -50 50 | x = 1; \ -51 |-import foo13 - 51 |+ +42 42 | import foo12 +43 43 | +44 44 | if True: +45 |- x = 1; \ +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; diff --git a/crates/ruff/src/rules/pylint/rules/logging.rs b/crates/ruff/src/rules/pylint/rules/logging.rs index 157844d818..8391113612 100644 --- a/crates/ruff/src/rules/pylint/rules/logging.rs +++ b/crates/ruff/src/rules/pylint/rules/logging.rs @@ -122,7 +122,7 @@ pub(crate) fn logging_call( return; } - let message_args = call_args.args.len() - 1; + let message_args = call_args.num_args() - 1; if checker.enabled(Rule::LoggingTooManyArgs) { if summary.num_positional < message_args { @@ -134,7 +134,7 @@ pub(crate) fn logging_call( if checker.enabled(Rule::LoggingTooFewArgs) { if message_args > 0 - && call_args.kwargs.is_empty() + && call_args.num_kwargs() == 0 && summary.num_positional > message_args { checker diff --git a/crates/ruff/src/rules/pylint/rules/useless_return.rs b/crates/ruff/src/rules/pylint/rules/useless_return.rs index 82436e976e..58b6b1b46e 100644 --- a/crates/ruff/src/rules/pylint/rules/useless_return.rs +++ b/crates/ruff/src/rules/pylint/rules/useless_return.rs @@ -103,13 +103,8 @@ pub(crate) fn useless_return<'a>( let mut diagnostic = Diagnostic::new(UselessReturn, last_stmt.range()); if checker.patch(diagnostic.kind.rule()) { - let edit = autofix::edits::delete_stmt( - last_stmt, - Some(stmt), - checker.locator, - checker.indexer, - checker.stylist, - ); + let edit = + autofix::edits::delete_stmt(last_stmt, Some(stmt), checker.locator, checker.indexer); diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(Some(stmt)))); } checker.diagnostics.push(diagnostic); diff --git a/crates/ruff/src/rules/pyupgrade/rules/outdated_version_block.rs b/crates/ruff/src/rules/pyupgrade/rules/outdated_version_block.rs index 5598b2a76d..8fad415be1 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/outdated_version_block.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/outdated_version_block.rs @@ -226,7 +226,6 @@ fn fix_py2_block( if matches!(block.leading_token.tok, StartTok::If) { parent } else { None }, checker.locator, checker.indexer, - checker.stylist, ); return Some(Fix::suggested(edit)); }; diff --git a/crates/ruff/src/rules/pyupgrade/rules/useless_metaclass_type.rs b/crates/ruff/src/rules/pyupgrade/rules/useless_metaclass_type.rs index a739ccad3e..bb54016701 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/useless_metaclass_type.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/useless_metaclass_type.rs @@ -68,13 +68,7 @@ pub(crate) fn useless_metaclass_type( if checker.patch(diagnostic.kind.rule()) { let stmt = checker.semantic().stmt(); let parent = checker.semantic().stmt_parent(); - let edit = autofix::edits::delete_stmt( - stmt, - parent, - checker.locator, - checker.indexer, - checker.stylist, - ); + let edit = autofix::edits::delete_stmt(stmt, parent, checker.locator, checker.indexer); diagnostic.set_fix(Fix::automatic(edit).isolate(checker.isolation(parent))); } checker.diagnostics.push(diagnostic); diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index d9a64049a8..1c6c65e0d5 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -12,7 +12,7 @@ use rustpython_parser::ast::{ use rustpython_parser::{lexer, Mode, Tok}; 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::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. -pub fn has_comments(located: &T, locator: &Locator) -> bool +pub fn has_comments(node: &T, locator: &Locator) -> bool where T: Ranged, { - let start = if has_leading_content(located, locator) { - located.start() + let start = if has_leading_content(node.start(), locator) { + node.start() } else { - locator.line_start(located.start()) + locator.line_start(node.start()) }; - let end = if has_trailing_content(located, locator) { - located.end() + let end = if has_trailing_content(node.end(), locator) { + node.end() } else { - locator.line_end(located.end()) + locator.line_end(node.end()) }; has_comments_in(TextRange::new(start, end), locator) @@ -927,7 +927,7 @@ where { fn visit_stmt(&mut self, stmt: &'b Stmt) { match stmt { - Stmt::FunctionDef(_) | Stmt::AsyncFunctionDef(_) => { + Stmt::FunctionDef(_) | Stmt::AsyncFunctionDef(_) | Stmt::ClassDef(_) => { // Don't recurse. } Stmt::Return(stmt) => self.returns.push(stmt), @@ -982,29 +982,23 @@ where } } -/// Return `true` if a [`Ranged`] has leading content. -pub fn has_leading_content(located: &T, locator: &Locator) -> bool -where - T: Ranged, -{ - 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 the node starting the given [`TextSize`] has leading content. +pub fn has_leading_content(offset: TextSize, locator: &Locator) -> bool { + let line_start = locator.line_start(offset); + let leading = &locator.contents()[TextRange::new(line_start, offset)]; + leading.chars().any(|char| !is_python_whitespace(char)) } -/// Return `true` if a [`Ranged`] has trailing content. -pub fn has_trailing_content(located: &T, locator: &Locator) -> bool -where - T: Ranged, -{ - let line_end = locator.line_end(located.end()); - let trailing = &locator.contents()[TextRange::new(located.end(), line_end)]; +/// Return `true` if the node ending at the given [`TextSize`] has trailing content. +pub fn has_trailing_content(offset: TextSize, locator: &Locator) -> bool { + let line_end = locator.line_end(offset); + let trailing = &locator.contents()[TextRange::new(offset, line_end)]; for char in trailing.chars() { if char == '#' { return false; } - if !char.is_whitespace() { + if !is_python_whitespace(char) { return true; } } @@ -1020,11 +1014,11 @@ where 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 == '#' { - return TextSize::try_from(i).ok(); + return TextSize::try_from(index).ok(); } - if !char.is_whitespace() { + if !is_python_whitespace(char) { return None; } } @@ -1040,7 +1034,7 @@ pub fn trailing_lines_end(stmt: &Stmt, locator: &Locator) -> TextSize { UniversalNewlineIterator::with_offset(rest, line_end) .take_while(|line| line.trim_whitespace().is_empty()) .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`]. @@ -1081,7 +1075,7 @@ pub fn first_colon_range(range: TextRange, locator: &Locator) -> Option Option bool { - let previous_line_end = locator.line_start(stmt.start()); - let newline_pos = usize::from(previous_line_end).saturating_sub(1); +/// Given an offset at the end of a line (including newlines), return the offset of the +/// continuation at the end of that line. +fn find_continuation(offset: TextSize, locator: &Locator, indexer: &Indexer) -> Option { + let newline_pos = usize::from(offset).saturating_sub(1); - // Compute start of preceding line + // Skip the newline. let newline_len = match locator.contents().as_bytes()[newline_pos] { b'\n' => { if locator @@ -1126,24 +1119,77 @@ pub fn preceded_by_continuation(stmt: &Stmt, indexer: &Indexer, locator: &Locato } } b'\r' => 1, - // No preceding line - _ => return false, + // No preceding line. + _ => return None, }; - // See if the position is in the continuation line starts - indexer.is_continuation(previous_line_end - TextSize::from(newline_len), locator) + indexer + .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 { + // 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 /// other statements preceding it. 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 /// other statements following it. 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. @@ -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. +#[derive(Default)] pub struct SimpleCallArgs<'a> { - pub args: Vec<&'a Expr>, - pub kwargs: FxHashMap<&'a str, &'a Expr>, + args: Vec<&'a Expr>, + kwargs: FxHashMap<&'a str, &'a Expr>, } impl<'a> SimpleCallArgs<'a> { @@ -1213,6 +1259,16 @@ impl<'a> SimpleCallArgs<'a> { 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. pub fn is_empty(&self) -> bool { self.len() == 0 @@ -1507,7 +1563,7 @@ mod tests { use anyhow::Result; 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::Parse; @@ -1523,25 +1579,25 @@ mod tests { let program = Suite::parse(contents, "")?; let stmt = program.first().unwrap(); 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 program = Suite::parse(contents, "")?; let stmt = program.first().unwrap(); let locator = Locator::new(contents); - assert!(has_trailing_content(stmt, &locator)); + assert!(has_trailing_content(stmt.end(), &locator)); let contents = "x = 1 "; let program = Suite::parse(contents, "")?; let stmt = program.first().unwrap(); let locator = Locator::new(contents); - assert!(!has_trailing_content(stmt, &locator)); + assert!(!has_trailing_content(stmt.end(), &locator)); let contents = "x = 1 # Comment"; let program = Suite::parse(contents, "")?; let stmt = program.first().unwrap(); let locator = Locator::new(contents); - assert!(!has_trailing_content(stmt, &locator)); + assert!(!has_trailing_content(stmt.end(), &locator)); let contents = r#" x = 1 @@ -1551,7 +1607,7 @@ y = 2 let program = Suite::parse(contents, "")?; let stmt = program.first().unwrap(); let locator = Locator::new(contents); - assert!(!has_trailing_content(stmt, &locator)); + assert!(!has_trailing_content(stmt.end(), &locator)); Ok(()) }