Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support parenthesized expressions when splitting compound assertions #5219

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Support parenthesized expressions when splitting compound assertions #5219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions crates/ruff/resources/test/fixtures/flake8_pytest_style/PT018.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ def test_error():
assert something and something_else == """error
message
"""
assert (
something
and something_else
== """error
message
"""
)

# recursive case
assert not (a or not (b or c))
Expand All @@ -31,14 +38,6 @@ def test_error():
assert not (something or something_else and something_third), "with message"
# detected, but no autofix for mixed conditions (e.g. `a or b and c`)
assert not (something or something_else and something_third)
# detected, but no autofix for parenthesized conditions
assert (
something
and something_else
== """error
message
"""
)


assert something # OK
Expand Down
62 changes: 53 additions & 9 deletions crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ use libcst_native::{
};
use rustpython_parser::ast::{self, BoolOp, ExceptHandler, Expr, Keyword, Ranged, Stmt, UnaryOp};

use crate::autofix::codemods::CodegenStylist;
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::{has_comments_in, Truthiness};
use ruff_python_ast::source_code::{Locator, Stylist};
use ruff_python_ast::visitor::Visitor;
use ruff_python_ast::{visitor, whitespace};

use crate::autofix::codemods::CodegenStylist;
use crate::checkers::ast::Checker;
use crate::cst::matchers::match_indented_block;
use crate::cst::matchers::match_module;
Expand Down Expand Up @@ -315,6 +315,54 @@ fn negate<'a>(expression: &Expression<'a>) -> Expression<'a> {
}))
}

/// Propagate parentheses from a parent to a child expression, if necessary.
///
/// For example, when splitting:
/// ```python
/// assert (a and b ==
/// """)
/// ```
///
/// The parentheses need to be propagated to the right-most expression:
/// ```python
/// assert a
/// assert (b ==
/// "")
/// ```
fn parenthesize<'a>(expression: Expression<'a>, parent: &Expression<'a>) -> Expression<'a> {
if matches!(
expression,
Expression::Comparison(_)
| Expression::UnaryOperation(_)
| Expression::BinaryOperation(_)
| Expression::BooleanOperation(_)
| Expression::Attribute(_)
| Expression::Tuple(_)
| Expression::Call(_)
| Expression::GeneratorExp(_)
| Expression::ListComp(_)
| Expression::SetComp(_)
| Expression::DictComp(_)
| Expression::List(_)
| Expression::Set(_)
| Expression::Dict(_)
| Expression::Subscript(_)
| Expression::StarredElement(_)
| Expression::IfExp(_)
| Expression::Lambda(_)
| Expression::Yield(_)
| Expression::Await(_)
| Expression::ConcatenatedString(_)
| Expression::FormattedString(_)
| Expression::NamedExpr(_)
) {
if let (Some(left), Some(right)) = (parent.lpar().first(), parent.rpar().first()) {
return expression.with_parens(left.clone(), right.clone());
}
}
expression
}

/// Replace composite condition `assert a == "hello" and b == "world"` with two statements
/// `assert a == "hello"` and `assert b == "world"`.
fn fix_composite_condition(stmt: &Stmt, locator: &Locator, stylist: &Stylist) -> Result<Edit> {
Expand Down Expand Up @@ -363,19 +411,15 @@ fn fix_composite_condition(stmt: &Stmt, locator: &Locator, stylist: &Stylist) ->
bail!("Expected simple statement to be an assert")
};

if !(assert_statement.test.lpar().is_empty() && assert_statement.test.rpar().is_empty()) {
bail!("Unable to split parenthesized condition");
}

// Extract the individual conditions.
let mut conditions: Vec<Expression> = Vec::with_capacity(2);
match &assert_statement.test {
Expression::UnaryOperation(op) => {
if matches!(op.operator, libcst_native::UnaryOp::Not { .. }) {
if let Expression::BooleanOperation(op) = &*op.expression {
if matches!(op.operator, BooleanOp::Or { .. }) {
conditions.push(negate(&op.left));
conditions.push(negate(&op.right));
conditions.push(parenthesize(negate(&op.left), &assert_statement.test));
conditions.push(parenthesize(negate(&op.right), &assert_statement.test));
} else {
bail!("Expected assert statement to be a composite condition");
}
Expand All @@ -386,8 +430,8 @@ fn fix_composite_condition(stmt: &Stmt, locator: &Locator, stylist: &Stylist) ->
}
Expression::BooleanOperation(op) => {
if matches!(op.operator, BooleanOp::And { .. }) {
conditions.push(*op.left.clone());
conditions.push(*op.right.clone());
conditions.push(parenthesize(*op.left.clone(), &assert_statement.test));
conditions.push(parenthesize(*op.right.clone(), &assert_statement.test));
} else {
bail!("Expected assert statement to be a composite condition");
}
Expand Down
181 changes: 97 additions & 84 deletions ...c/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT018.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,8 @@ PT018.py:21:5: PT018 [*] Assertion should be broken down into multiple parts
22 | | message
23 | | """
| |_______^ PT018
24 |
25 | # recursive case
24 | assert (
25 | something
|
= help: Break down assertion into multiple parts

Expand All @@ -177,131 +177,144 @@ PT018.py:21:5: PT018 [*] Assertion should be broken down into multiple parts
22 |+ assert something_else == """error
22 23 | message
23 24 | """
24 25 |
24 25 | assert (

PT018.py:26:5: PT018 [*] Assertion should be broken down into multiple parts
PT018.py:24:5: PT018 [*] Assertion should be broken down into multiple parts
|
25 | # recursive case
26 | assert not (a or not (b or c))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
27 | assert not (a or not (b and c))
22 | message
23 | """
24 | assert (
| _____^
25 | | something
26 | | and something_else
27 | | == """error
28 | | message
29 | | """
30 | | )
| |_____^ PT018
31 |
32 | # recursive case
|
= help: Break down assertion into multiple parts

ℹ Suggested fix
21 21 | assert something and something_else == """error
22 22 | message
23 23 | """
24 24 |
25 25 | # recursive case
26 |- assert not (a or not (b or c))
26 |+ assert not a
27 |+ assert (b or c)
27 28 | assert not (a or not (b and c))
28 29 |
29 30 | # detected, but no autofix for messages
24 |+ assert something
24 25 | assert (
25 |- something
26 |- and something_else
26 |+ something_else
27 27 | == """error
28 28 | message
29 29 | """

PT018.py:27:5: PT018 [*] Assertion should be broken down into multiple parts
PT018.py:33:5: PT018 [*] Assertion should be broken down into multiple parts
|
25 | # recursive case
26 | assert not (a or not (b or c))
27 | assert not (a or not (b and c))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
28 |
29 | # detected, but no autofix for messages
32 | # recursive case
33 | assert not (a or not (b or c))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
34 | assert not (a or not (b and c))
|
= help: Break down assertion into multiple parts

ℹ Suggested fix
24 24 |
25 25 | # recursive case
26 26 | assert not (a or not (b or c))
27 |- assert not (a or not (b and c))
27 |+ assert not a
28 |+ assert (b and c)
28 29 |
29 30 | # detected, but no autofix for messages
30 31 | assert something and something_else, "error message"
30 30 | )
31 31 |
32 32 | # recursive case
33 |- assert not (a or not (b or c))
33 |+ assert not a
34 |+ assert (b or c)
34 35 | assert not (a or not (b and c))
35 36 |
36 37 | # detected, but no autofix for messages

PT018.py:30:5: PT018 Assertion should be broken down into multiple parts
PT018.py:34:5: PT018 [*] Assertion should be broken down into multiple parts
|
29 | # detected, but no autofix for messages
30 | assert something and something_else, "error message"
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
31 | assert not (something or something_else and something_third), "with message"
32 | # detected, but no autofix for mixed conditions (e.g. `a or b and c`)
32 | # recursive case
33 | assert not (a or not (b or c))
34 | assert not (a or not (b and c))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
35 |
36 | # detected, but no autofix for messages
|
= help: Break down assertion into multiple parts

PT018.py:31:5: PT018 Assertion should be broken down into multiple parts
ℹ Suggested fix
31 31 |
32 32 | # recursive case
33 33 | assert not (a or not (b or c))
34 |- assert not (a or not (b and c))
34 |+ assert not a
35 |+ assert (b and c)
35 36 |
36 37 | # detected, but no autofix for messages
37 38 | assert something and something_else, "error message"

PT018.py:37:5: PT018 Assertion should be broken down into multiple parts
|
29 | # detected, but no autofix for messages
30 | assert something and something_else, "error message"
31 | assert not (something or something_else and something_third), "with message"
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
32 | # detected, but no autofix for mixed conditions (e.g. `a or b and c`)
33 | assert not (something or something_else and something_third)
36 | # detected, but no autofix for messages
37 | assert something and something_else, "error message"
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
38 | assert not (something or something_else and something_third), "with message"
39 | # detected, but no autofix for mixed conditions (e.g. `a or b and c`)
|
= help: Break down assertion into multiple parts

PT018.py:33:5: PT018 Assertion should be broken down into multiple parts
PT018.py:38:5: PT018 Assertion should be broken down into multiple parts
|
31 | assert not (something or something_else and something_third), "with message"
32 | # detected, but no autofix for mixed conditions (e.g. `a or b and c`)
33 | assert not (something or something_else and something_third)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
34 | # detected, but no autofix for parenthesized conditions
35 | assert (
36 | # detected, but no autofix for messages
37 | assert something and something_else, "error message"
38 | assert not (something or something_else and something_third), "with message"
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
39 | # detected, but no autofix for mixed conditions (e.g. `a or b and c`)
40 | assert not (something or something_else and something_third)
|
= help: Break down assertion into multiple parts

PT018.py:35:5: PT018 Assertion should be broken down into multiple parts
PT018.py:40:5: PT018 Assertion should be broken down into multiple parts
|
33 | assert not (something or something_else and something_third)
34 | # detected, but no autofix for parenthesized conditions
35 | assert (
| _____^
36 | | something
37 | | and something_else
38 | | == """error
39 | | message
40 | | """
41 | | )
| |_____^ PT018
38 | assert not (something or something_else and something_third), "with message"
39 | # detected, but no autofix for mixed conditions (e.g. `a or b and c`)
40 | assert not (something or something_else and something_third)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
|
= help: Break down assertion into multiple parts

PT018.py:45:1: PT018 [*] Assertion should be broken down into multiple parts
PT018.py:44:1: PT018 [*] Assertion should be broken down into multiple parts
|
44 | assert something # OK
45 | assert something and something_else # Error
43 | assert something # OK
44 | assert something and something_else # Error
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
46 | assert something and something_else and something_third # Error
45 | assert something and something_else and something_third # Error
|
= help: Break down assertion into multiple parts

ℹ Suggested fix
41 41 |
42 42 |
43 43 |
44 44 | assert something # OK
45 |-assert something and something_else # Error
45 |+assert something
46 |+assert something_else
46 47 | assert something and something_else and something_third # Error
43 43 | assert something # OK
44 |-assert something and something_else # Error
44 |+assert something
45 |+assert something_else
45 46 | assert something and something_else and something_third # Error

PT018.py:46:1: PT018 [*] Assertion should be broken down into multiple parts
PT018.py:45:1: PT018 [*] Assertion should be broken down into multiple parts
|
44 | assert something # OK
45 | assert something and something_else # Error
46 | assert something and something_else and something_third # Error
43 | assert something # OK
44 | assert something and something_else # Error
45 | assert something and something_else and something_third # Error
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT018
|
= help: Break down assertion into multiple parts

ℹ Suggested fix
43 43 |
44 44 | assert something # OK
45 45 | assert something and something_else # Error
46 |-assert something and something_else and something_third # Error
46 |+assert something and something_else
47 |+assert something_third
42 42 |
43 43 | assert something # OK
44 44 | assert something and something_else # Error
45 |-assert something and something_else and something_third # Error
45 |+assert something and something_else
46 |+assert something_third


Loading