Skip to content

Commit

Permalink
Avoid attempting to fix PT018 in multi-statement lines
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Aug 23, 2023
1 parent 9b6e008 commit 5c7a507
Show file tree
Hide file tree
Showing 9 changed files with 86 additions and 26 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
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
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
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
14 changes: 3 additions & 11 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 @@ -26,12 +24,6 @@ pub fn trailing_lines_end(stmt: &Stmt, locator: &Locator) -> TextSize {
.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
Expand Down
16 changes: 14 additions & 2 deletions crates/ruff_python_index/src/indexer.rs
Original file line number Diff line number Diff line change
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
2 changes: 1 addition & 1 deletion 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 Down

0 comments on commit 5c7a507

Please sign in to comment.