Avoid attempting to fix PT018 in multi-statement lines (#6829)

## Summary

These fixes will _always_ fail, so we should avoid trying to construct
them in the first place.

Closes https://github.com/astral-sh/ruff/issues/6812.
This commit is contained in:
Charlie Marsh 2023-08-23 19:09:34 -04:00 committed by GitHub
parent 9b6e008cf1
commit 847432cacf
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 109 additions and 52 deletions

View File

@ -43,3 +43,12 @@ message
assert something # OK assert something # OK
assert something and something_else # Error assert something and something_else # Error
assert something and something_else and something_third # Error assert something and something_else and something_third # Error
def test_multiline():
assert something and something_else; x = 1
x = 1; assert something and something_else
x = 1; \
assert something and something_else

View File

@ -226,25 +226,25 @@ fn trailing_semicolon(offset: TextSize, locator: &Locator) -> Option<TextSize> {
fn next_stmt_break(semicolon: TextSize, locator: &Locator) -> TextSize { fn next_stmt_break(semicolon: TextSize, locator: &Locator) -> TextSize {
let start_location = semicolon + TextSize::from(1); let start_location = semicolon + TextSize::from(1);
let contents = &locator.contents()[usize::from(start_location)..]; for line in
for line in NewlineWithTrailingNewline::from(contents) { NewlineWithTrailingNewline::with_offset(locator.after(start_location), start_location)
{
let trimmed = line.trim_whitespace(); let trimmed = line.trim_whitespace();
// Skip past any continuations. // Skip past any continuations.
if trimmed.starts_with('\\') { if trimmed.starts_with('\\') {
continue; continue;
} }
return start_location return if trimmed.is_empty() {
+ if trimmed.is_empty() { // If the line is empty, then despite the previous statement ending in a
// If the line is empty, then despite the previous statement ending in a // semicolon, we know that it's not a multi-statement line.
// semicolon, we know that it's not a multi-statement line. line.start()
line.start() } else {
} else { // Otherwise, find the start of the next statement. (Or, anything that isn't
// Otherwise, find the start of the next statement. (Or, anything that isn't // whitespace.)
// whitespace.) let relative_offset = line.find(|c: char| !is_python_whitespace(c)).unwrap();
let relative_offset = line.find(|c: char| !is_python_whitespace(c)).unwrap(); line.start() + TextSize::try_from(relative_offset).unwrap()
line.start() + TextSize::try_from(relative_offset).unwrap() };
};
} }
locator.line_end(start_location) locator.line_end(start_location)

View File

@ -562,7 +562,7 @@ fn add_noqa_inner(
let mut prev_end = TextSize::default(); let mut prev_end = TextSize::default();
for (offset, (rules, directive)) in matches_by_line { for (offset, (rules, directive)) in matches_by_line {
output.push_str(&locator.contents()[TextRange::new(prev_end, offset)]); output.push_str(locator.slice(TextRange::new(prev_end, offset)));
let line = locator.full_line(offset); let line = locator.full_line(offset);
@ -619,7 +619,7 @@ fn add_noqa_inner(
prev_end = offset + line.text_len(); prev_end = offset + line.text_len();
} }
output.push_str(&locator.contents()[usize::from(prev_end)..]); output.push_str(locator.after(prev_end));
(count, output) (count, output)
} }

View File

@ -140,7 +140,7 @@ fn move_initialization(
let mut body = function_def.body.iter(); let mut body = function_def.body.iter();
let statement = body.next()?; let statement = body.next()?;
if indexer.preceded_by_multi_statement_line(statement, locator) { if indexer.in_multi_statement_line(statement, locator) {
return None; return None;
} }
@ -170,7 +170,7 @@ fn move_initialization(
if let Some(statement) = body.next() { if let Some(statement) = body.next() {
// If there's a second statement, insert _before_ it, but ensure this isn't a // If there's a second statement, insert _before_ it, but ensure this isn't a
// multi-statement line. // multi-statement line.
if indexer.preceded_by_multi_statement_line(statement, locator) { if indexer.in_multi_statement_line(statement, locator) {
return None; return None;
} }
Edit::insertion(content, locator.line_start(statement.start())) Edit::insertion(content, locator.line_start(statement.start()))

View File

@ -126,8 +126,8 @@ pub(crate) fn implicit(
} }
fn concatenate_strings(a_range: TextRange, b_range: TextRange, locator: &Locator) -> Option<Fix> { fn concatenate_strings(a_range: TextRange, b_range: TextRange, locator: &Locator) -> Option<Fix> {
let a_text = &locator.contents()[a_range]; let a_text = locator.slice(a_range);
let b_text = &locator.contents()[b_range]; let b_text = locator.slice(b_range);
let a_leading_quote = leading_quote(a_text)?; let a_leading_quote = leading_quote(a_text)?;
let b_leading_quote = leading_quote(b_text)?; let b_leading_quote = leading_quote(b_text)?;

View File

@ -1,7 +1,7 @@
use std::borrow::Cow; use std::borrow::Cow;
use anyhow::bail;
use anyhow::Result; use anyhow::Result;
use anyhow::{bail, Context};
use libcst_native::{ use libcst_native::{
self, Assert, BooleanOp, CompoundStatement, Expression, ParenthesizableWhitespace, self, Assert, BooleanOp, CompoundStatement, Expression, ParenthesizableWhitespace,
ParenthesizedNode, SimpleStatementLine, SimpleWhitespace, SmallStatement, Statement, ParenthesizedNode, SimpleStatementLine, SimpleWhitespace, SmallStatement, Statement,
@ -635,9 +635,8 @@ fn parenthesize<'a>(expression: Expression<'a>, parent: &Expression<'a>) -> Expr
/// `assert a == "hello"` and `assert b == "world"`. /// `assert a == "hello"` and `assert b == "world"`.
fn fix_composite_condition(stmt: &Stmt, locator: &Locator, stylist: &Stylist) -> Result<Edit> { fn fix_composite_condition(stmt: &Stmt, locator: &Locator, stylist: &Stylist) -> Result<Edit> {
// Infer the indentation of the outer block. // Infer the indentation of the outer block.
let Some(outer_indent) = whitespace::indentation(locator, stmt) else { let outer_indent =
bail!("Unable to fix multiline statement"); whitespace::indentation(locator, stmt).context("Unable to fix multiline statement")?;
};
// Extract the module text. // Extract the module text.
let contents = locator.lines(stmt.range()); let contents = locator.lines(stmt.range());
@ -672,11 +671,11 @@ fn fix_composite_condition(stmt: &Stmt, locator: &Locator, stylist: &Stylist) ->
&mut indented_block.body &mut indented_block.body
}; };
let [Statement::Simple(simple_statement_line)] = &statements[..] else { let [Statement::Simple(simple_statement_line)] = statements.as_slice() else {
bail!("Expected one simple statement") bail!("Expected one simple statement")
}; };
let [SmallStatement::Assert(assert_statement)] = &simple_statement_line.body[..] else { let [SmallStatement::Assert(assert_statement)] = simple_statement_line.body.as_slice() else {
bail!("Expected simple statement to be an assert") bail!("Expected simple statement to be an assert")
}; };
@ -754,6 +753,9 @@ pub(crate) fn composite_condition(
if matches!(composite, CompositionKind::Simple) if matches!(composite, CompositionKind::Simple)
&& msg.is_none() && msg.is_none()
&& !checker.indexer().comment_ranges().intersects(stmt.range()) && !checker.indexer().comment_ranges().intersects(stmt.range())
&& !checker
.indexer()
.in_multi_statement_line(stmt, checker.locator())
{ {
diagnostic.try_set_fix(|| { diagnostic.try_set_fix(|| {
fix_composite_condition(stmt, checker.locator(), checker.stylist()) fix_composite_condition(stmt, checker.locator(), checker.stylist())

View File

@ -299,6 +299,8 @@ PT018.py:44:1: PT018 [*] Assertion should be broken down into multiple parts
44 |+assert something 44 |+assert something
45 |+assert something_else 45 |+assert something_else
45 46 | assert something and something_else and something_third # Error 45 46 | assert something and something_else and something_third # Error
46 47 |
47 48 |
PT018.py:45:1: PT018 [*] Assertion should be broken down into multiple parts PT018.py:45:1: PT018 [*] Assertion should be broken down into multiple parts
| |
@ -316,5 +318,37 @@ PT018.py:45:1: PT018 [*] Assertion should be broken down into multiple parts
45 |-assert something and something_else and something_third # Error 45 |-assert something and something_else and something_third # Error
45 |+assert something and something_else 45 |+assert something and something_else
46 |+assert something_third 46 |+assert something_third
46 47 |
47 48 |
48 49 | def test_multiline():
PT018.py:49:5: PT018 Assertion should be broken down into multiple parts
|
48 | def test_multiline():
49 | assert something and something_else; x = 1
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
50 |
51 | x = 1; assert something and something_else
|
= help: Break down assertion into multiple parts
PT018.py:51:12: PT018 Assertion should be broken down into multiple parts
|
49 | assert something and something_else; x = 1
50 |
51 | x = 1; assert something and something_else
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
52 |
53 | x = 1; \
|
= help: Break down assertion into multiple parts
PT018.py:54:9: PT018 Assertion should be broken down into multiple parts
|
53 | x = 1; \
54 | assert something and something_else
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
|
= help: Break down assertion into multiple parts

View File

@ -1,16 +1,16 @@
use std::path::Path; use std::path::Path;
use itertools::{EitherOrBoth, Itertools}; use itertools::{EitherOrBoth, Itertools};
use ruff_python_ast::{PySourceType, Ranged, Stmt};
use ruff_text_size::TextRange;
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::whitespace::{followed_by_multi_statement_line, trailing_lines_end}; use ruff_python_ast::whitespace::trailing_lines_end;
use ruff_python_ast::{PySourceType, Ranged, Stmt};
use ruff_python_codegen::Stylist; use ruff_python_codegen::Stylist;
use ruff_python_index::Indexer; use ruff_python_index::Indexer;
use ruff_python_trivia::{leading_indentation, textwrap::indent, PythonWhitespace}; use ruff_python_trivia::{leading_indentation, textwrap::indent, PythonWhitespace};
use ruff_source_file::{Locator, UniversalNewlines}; use ruff_source_file::{Locator, UniversalNewlines};
use ruff_text_size::TextRange;
use crate::line_width::LineWidth; use crate::line_width::LineWidth;
use crate::registry::AsRule; use crate::registry::AsRule;
@ -97,7 +97,7 @@ pub(crate) fn organize_imports(
// Special-cases: there's leading or trailing content in the import block. These // Special-cases: there's leading or trailing content in the import block. These
// are too hard to get right, and relatively rare, so flag but don't fix. // are too hard to get right, and relatively rare, so flag but don't fix.
if indexer.preceded_by_multi_statement_line(block.imports.first().unwrap(), locator) if indexer.preceded_by_multi_statement_line(block.imports.first().unwrap(), locator)
|| followed_by_multi_statement_line(block.imports.last().unwrap(), locator) || indexer.followed_by_multi_statement_line(block.imports.last().unwrap(), locator)
{ {
return Some(Diagnostic::new(UnsortedImports, range)); return Some(Diagnostic::new(UnsortedImports, range));
} }

View File

@ -62,8 +62,7 @@ fn find_useless_f_strings<'a>(
kind: StringKind::FString | StringKind::RawFString, kind: StringKind::FString | StringKind::RawFString,
.. ..
} => { } => {
let first_char = let first_char = locator.slice(TextRange::at(range.start(), TextSize::from(1)));
&locator.contents()[TextRange::at(range.start(), TextSize::from(1))];
// f"..." => f_position = 0 // f"..." => f_position = 0
// fr"..." => f_position = 0 // fr"..." => f_position = 0
// rf"..." => f_position = 1 // rf"..." => f_position = 1

View File

@ -60,6 +60,17 @@ impl Diagnostic {
} }
} }
/// Set the [`Fix`] used to fix the diagnostic, if the provided function returns `Ok`.
/// Otherwise, log the error.
#[inline]
pub fn try_set_optional_fix(&mut self, func: impl FnOnce() -> Result<Option<Fix>>) {
match func() {
Ok(None) => {}
Ok(Some(fix)) => self.fix = Some(fix),
Err(err) => error!("Failed to create fix for {}: {}", self.kind.name, err),
}
}
pub const fn range(&self) -> TextRange { pub const fn range(&self) -> TextRange {
self.range self.range
} }

View File

@ -1,10 +1,8 @@
use crate::{Ranged, Stmt}; use ruff_python_trivia::{indentation_at_offset, is_python_whitespace, PythonWhitespace};
use ruff_source_file::{newlines::UniversalNewlineIterator, Locator};
use ruff_text_size::{TextRange, TextSize}; use ruff_text_size::{TextRange, TextSize};
use ruff_python_trivia::{ use crate::{Ranged, Stmt};
has_trailing_content, indentation_at_offset, is_python_whitespace, PythonWhitespace,
};
use ruff_source_file::{newlines::UniversalNewlineIterator, Locator};
/// Extract the leading indentation from a line. /// Extract the leading indentation from a line.
#[inline] #[inline]
@ -18,20 +16,12 @@ where
/// Return the end offset at which the empty lines following a statement. /// Return the end offset at which the empty lines following a statement.
pub fn trailing_lines_end(stmt: &Stmt, locator: &Locator) -> TextSize { pub fn trailing_lines_end(stmt: &Stmt, locator: &Locator) -> TextSize {
let line_end = locator.full_line_end(stmt.end()); let line_end = locator.full_line_end(stmt.end());
let rest = &locator.contents()[usize::from(line_end)..]; UniversalNewlineIterator::with_offset(locator.after(line_end), 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, |line| line.full_end()) .map_or(line_end, |line| line.full_end())
} }
/// 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.end(), locator)
}
/// If a [`Ranged`] has a trailing comment, return the index of the hash. /// If a [`Ranged`] has a trailing comment, return the index of the hash.
pub fn trailing_comment_start_offset<T>(located: &T, locator: &Locator) -> Option<TextSize> pub fn trailing_comment_start_offset<T>(located: &T, locator: &Locator) -> Option<TextSize>
where where
@ -39,7 +29,7 @@ where
{ {
let line_end = locator.line_end(located.end()); let line_end = locator.line_end(located.end());
let trailing = &locator.contents()[TextRange::new(located.end(), line_end)]; let trailing = locator.slice(TextRange::new(located.end(), line_end));
for (index, char) in trailing.char_indices() { for (index, char) in trailing.char_indices() {
if char == '#' { if char == '#' {

View File

@ -39,7 +39,7 @@ impl Indexer {
let mut line_start = TextSize::default(); let mut line_start = TextSize::default();
for (tok, range) in tokens.iter().flatten() { for (tok, range) in tokens.iter().flatten() {
let trivia = &locator.contents()[TextRange::new(prev_end, range.start())]; let trivia = locator.slice(TextRange::new(prev_end, range.start()));
// Get the trivia between the previous and the current token and detect any newlines. // Get the trivia between the previous and the current token and detect any newlines.
// This is necessary because `RustPython` doesn't emit `[Tok::Newline]` tokens // This is necessary because `RustPython` doesn't emit `[Tok::Newline]` tokens
@ -250,14 +250,26 @@ impl Indexer {
Some(continuation) 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 preceded by other statements in a multi-statement
/// other statements preceding it. /// line.
pub fn preceded_by_multi_statement_line(&self, stmt: &Stmt, locator: &Locator) -> bool { pub fn preceded_by_multi_statement_line(&self, stmt: &Stmt, locator: &Locator) -> bool {
has_leading_content(stmt.start(), locator) has_leading_content(stmt.start(), locator)
|| self || self
.preceded_by_continuations(stmt.start(), locator) .preceded_by_continuations(stmt.start(), locator)
.is_some() .is_some()
} }
/// Return `true` if a [`Stmt`] appears to be followed by other statements in a multi-statement
/// line.
pub fn followed_by_multi_statement_line(&self, stmt: &Stmt, locator: &Locator) -> bool {
has_trailing_content(stmt.end(), locator)
}
/// Return `true` if a [`Stmt`] appears to be part of a multi-statement line.
pub fn in_multi_statement_line(&self, stmt: &Stmt, locator: &Locator) -> bool {
self.followed_by_multi_statement_line(stmt, locator)
|| self.preceded_by_multi_statement_line(stmt, locator)
}
} }
#[cfg(test)] #[cfg(test)]

View File

@ -4,7 +4,7 @@ use ruff_text_size::{TextRange, TextSize};
/// Extract the leading indentation from a line. /// Extract the leading indentation from a line.
pub fn indentation_at_offset<'a>(offset: TextSize, locator: &'a Locator) -> Option<&'a str> { pub fn indentation_at_offset<'a>(offset: TextSize, locator: &'a Locator) -> Option<&'a str> {
let line_start = locator.line_start(offset); let line_start = locator.line_start(offset);
let indentation = &locator.contents()[TextRange::new(line_start, offset)]; let indentation = locator.slice(TextRange::new(line_start, offset));
if indentation.chars().all(is_python_whitespace) { if indentation.chars().all(is_python_whitespace) {
Some(indentation) Some(indentation)
@ -16,14 +16,14 @@ pub fn indentation_at_offset<'a>(offset: TextSize, locator: &'a Locator) -> Opti
/// Return `true` if the node starting the given [`TextSize`] has leading content. /// Return `true` if the node starting the given [`TextSize`] has leading content.
pub fn has_leading_content(offset: TextSize, locator: &Locator) -> bool { pub fn has_leading_content(offset: TextSize, locator: &Locator) -> bool {
let line_start = locator.line_start(offset); let line_start = locator.line_start(offset);
let leading = &locator.contents()[TextRange::new(line_start, offset)]; let leading = locator.slice(TextRange::new(line_start, offset));
leading.chars().any(|char| !is_python_whitespace(char)) leading.chars().any(|char| !is_python_whitespace(char))
} }
/// Return `true` if the node ending at the given [`TextSize`] has trailing content. /// Return `true` if the node ending at the given [`TextSize`] has trailing content.
pub fn has_trailing_content(offset: TextSize, locator: &Locator) -> bool { pub fn has_trailing_content(offset: TextSize, locator: &Locator) -> bool {
let line_end = locator.line_end(offset); let line_end = locator.line_end(offset);
let trailing = &locator.contents()[TextRange::new(offset, line_end)]; let trailing = locator.slice(TextRange::new(offset, line_end));
for char in trailing.chars() { for char in trailing.chars() {
if char == '#' { if char == '#' {