Skip to content

Commit

Permalink
Avoid attempting to fix PT018 in multi-statement lines (#6829)
Browse files Browse the repository at this point in the history
## Summary

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

Closes #6812.
  • Loading branch information
charliermarsh authored Aug 23, 2023
1 parent 9b6e008 commit 847432c
Show file tree
Hide file tree
Showing 13 changed files with 109 additions and 52 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,12 @@ def test_error():
assert something # OK
assert something and something_else # 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
26 changes: 13 additions & 13 deletions crates/ruff/src/autofix/edits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -226,25 +226,25 @@ fn trailing_semicolon(offset: TextSize, locator: &Locator) -> Option<TextSize> {
fn next_stmt_break(semicolon: TextSize, locator: &Locator) -> TextSize {
let start_location = semicolon + TextSize::from(1);

let contents = &locator.contents()[usize::from(start_location)..];
for line in NewlineWithTrailingNewline::from(contents) {
for line in
NewlineWithTrailingNewline::with_offset(locator.after(start_location), start_location)
{
let trimmed = line.trim_whitespace();
// Skip past any continuations.
if trimmed.starts_with('\\') {
continue;
}

return start_location
+ if trimmed.is_empty() {
// If the line is empty, then despite the previous statement ending in a
// semicolon, we know that it's not a multi-statement line.
line.start()
} else {
// Otherwise, find the start of the next statement. (Or, anything that isn't
// whitespace.)
let relative_offset = line.find(|c: char| !is_python_whitespace(c)).unwrap();
line.start() + TextSize::try_from(relative_offset).unwrap()
};
return if trimmed.is_empty() {
// If the line is empty, then despite the previous statement ending in a
// semicolon, we know that it's not a multi-statement line.
line.start()
} else {
// Otherwise, find the start of the next statement. (Or, anything that isn't
// whitespace.)
let relative_offset = line.find(|c: char| !is_python_whitespace(c)).unwrap();
line.start() + TextSize::try_from(relative_offset).unwrap()
};
}

locator.line_end(start_location)
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff/src/noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,7 @@ fn add_noqa_inner(
let mut prev_end = TextSize::default();

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);

Expand Down Expand Up @@ -619,7 +619,7 @@ fn add_noqa_inner(
prev_end = offset + line.text_len();
}

output.push_str(&locator.contents()[usize::from(prev_end)..]);
output.push_str(locator.after(prev_end));

(count, output)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ fn move_initialization(
let mut body = function_def.body.iter();

let statement = body.next()?;
if indexer.preceded_by_multi_statement_line(statement, locator) {
if indexer.in_multi_statement_line(statement, locator) {
return None;
}

Expand Down Expand Up @@ -170,7 +170,7 @@ fn move_initialization(
if let Some(statement) = body.next() {
// If there's a second statement, insert _before_ it, but ensure this isn't a
// multi-statement line.
if indexer.preceded_by_multi_statement_line(statement, locator) {
if indexer.in_multi_statement_line(statement, locator) {
return None;
}
Edit::insertion(content, locator.line_start(statement.start()))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,8 @@ pub(crate) fn implicit(
}

fn concatenate_strings(a_range: TextRange, b_range: TextRange, locator: &Locator) -> Option<Fix> {
let a_text = &locator.contents()[a_range];
let b_text = &locator.contents()[b_range];
let a_text = locator.slice(a_range);
let b_text = locator.slice(b_range);

let a_leading_quote = leading_quote(a_text)?;
let b_leading_quote = leading_quote(b_text)?;
Expand Down
14 changes: 8 additions & 6 deletions crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::borrow::Cow;

use anyhow::bail;
use anyhow::Result;
use anyhow::{bail, Context};
use libcst_native::{
self, Assert, BooleanOp, CompoundStatement, Expression, ParenthesizableWhitespace,
ParenthesizedNode, SimpleStatementLine, SimpleWhitespace, SmallStatement, Statement,
Expand Down Expand Up @@ -635,9 +635,8 @@ fn parenthesize<'a>(expression: Expression<'a>, parent: &Expression<'a>) -> Expr
/// `assert a == "hello"` and `assert b == "world"`.
fn fix_composite_condition(stmt: &Stmt, locator: &Locator, stylist: &Stylist) -> Result<Edit> {
// Infer the indentation of the outer block.
let Some(outer_indent) = whitespace::indentation(locator, stmt) else {
bail!("Unable to fix multiline statement");
};
let outer_indent =
whitespace::indentation(locator, stmt).context("Unable to fix multiline statement")?;

// Extract the module text.
let contents = locator.lines(stmt.range());
Expand Down Expand Up @@ -672,11 +671,11 @@ fn fix_composite_condition(stmt: &Stmt, locator: &Locator, stylist: &Stylist) ->
&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")
};

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")
};

Expand Down Expand Up @@ -754,6 +753,9 @@ pub(crate) fn composite_condition(
if matches!(composite, CompositionKind::Simple)
&& msg.is_none()
&& !checker.indexer().comment_ranges().intersects(stmt.range())
&& !checker
.indexer()
.in_multi_statement_line(stmt, checker.locator())
{
diagnostic.try_set_fix(|| {
fix_composite_condition(stmt, checker.locator(), checker.stylist())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,8 @@ PT018.py:44:1: PT018 [*] Assertion should be broken down into multiple parts
44 |+assert something
45 |+assert something_else
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
|
Expand All @@ -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
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


8 changes: 4 additions & 4 deletions crates/ruff/src/rules/isort/rules/organize_imports.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
use std::path::Path;

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_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_index::Indexer;
use ruff_python_trivia::{leading_indentation, textwrap::indent, PythonWhitespace};
use ruff_source_file::{Locator, UniversalNewlines};
use ruff_text_size::TextRange;

use crate::line_width::LineWidth;
use crate::registry::AsRule;
Expand Down Expand Up @@ -97,7 +97,7 @@ pub(crate) fn organize_imports(
// 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.
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));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ fn find_useless_f_strings<'a>(
kind: StringKind::FString | StringKind::RawFString,
..
} => {
let first_char =
&locator.contents()[TextRange::at(range.start(), TextSize::from(1))];
let first_char = locator.slice(TextRange::at(range.start(), TextSize::from(1)));
// f"..." => f_position = 0
// fr"..." => f_position = 0
// rf"..." => f_position = 1
Expand Down
11 changes: 11 additions & 0 deletions crates/ruff_diagnostics/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
self.range
}
Expand Down
20 changes: 5 additions & 15 deletions crates/ruff_python_ast/src/whitespace.rs
Original file line number Diff line number Diff line change
@@ -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_python_trivia::{
has_trailing_content, indentation_at_offset, is_python_whitespace, PythonWhitespace,
};
use ruff_source_file::{newlines::UniversalNewlineIterator, Locator};
use crate::{Ranged, Stmt};

/// Extract the leading indentation from a line.
#[inline]
Expand All @@ -18,28 +16,20 @@ where
/// Return the end offset at which the empty lines following a statement.
pub fn trailing_lines_end(stmt: &Stmt, locator: &Locator) -> TextSize {
let line_end = locator.full_line_end(stmt.end());
let rest = &locator.contents()[usize::from(line_end)..];

UniversalNewlineIterator::with_offset(rest, line_end)
UniversalNewlineIterator::with_offset(locator.after(line_end), line_end)
.take_while(|line| line.trim_whitespace().is_empty())
.last()
.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.
pub fn trailing_comment_start_offset<T>(located: &T, locator: &Locator) -> Option<TextSize>
where
T: Ranged,
{
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() {
if char == '#' {
Expand Down
18 changes: 15 additions & 3 deletions crates/ruff_python_index/src/indexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ impl Indexer {
let mut line_start = TextSize::default();

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.
// This is necessary because `RustPython` doesn't emit `[Tok::Newline]` tokens
Expand Down Expand Up @@ -250,14 +250,26 @@ impl Indexer {
Some(continuation)
}

/// Return `true` if a `Stmt` appears to be part of a multi-statement line, with
/// other statements preceding it.
/// Return `true` if a [`Stmt`] appears to be preceded by other statements in a multi-statement
/// line.
pub fn preceded_by_multi_statement_line(&self, stmt: &Stmt, locator: &Locator) -> bool {
has_leading_content(stmt.start(), locator)
|| self
.preceded_by_continuations(stmt.start(), locator)
.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)]
Expand Down
6 changes: 3 additions & 3 deletions crates/ruff_python_trivia/src/whitespace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use ruff_text_size::{TextRange, TextSize};
/// Extract the leading indentation from a line.
pub fn indentation_at_offset<'a>(offset: TextSize, locator: &'a Locator) -> Option<&'a str> {
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) {
Some(indentation)
Expand All @@ -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.
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)];
let leading = locator.slice(TextRange::new(line_start, offset));
leading.chars().any(|char| !is_python_whitespace(char))
}

/// 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)];
let trailing = locator.slice(TextRange::new(offset, line_end));

for char in trailing.chars() {
if char == '#' {
Expand Down

0 comments on commit 847432c

Please sign in to comment.