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

Retain parentheses when fixing SIM210 #7118

Merged
merged 1 commit into from
Sep 3, 2023
Merged

Retain parentheses when fixing SIM210 #7118

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
5 changes: 5 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_simplify/SIM210.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,8 @@ def bool():
return False

a = True if b else False


# Regression test for: https://github.com/astral-sh/ruff/issues/7076
samesld = True if (psl.privatesuffix(urlparse(response.url).netloc) ==
psl.privatesuffix(src.netloc)) else False
8 changes: 2 additions & 6 deletions crates/ruff/src/checkers/ast/analyze/expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1253,14 +1253,10 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
range: _,
}) => {
if checker.enabled(Rule::IfExprWithTrueFalse) {
flake8_simplify::rules::explicit_true_false_in_ifexpr(
checker, expr, test, body, orelse,
);
flake8_simplify::rules::if_expr_with_true_false(checker, expr, test, body, orelse);
}
if checker.enabled(Rule::IfExprWithFalseTrue) {
flake8_simplify::rules::explicit_false_true_in_ifexpr(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These names were wrong.

checker, expr, test, body, orelse,
);
flake8_simplify::rules::if_expr_with_false_true(checker, expr, test, body, orelse);
}
if checker.enabled(Rule::IfExprWithTwistedArms) {
flake8_simplify::rules::twisted_arms_in_ifexpr(checker, expr, test, body, orelse);
Expand Down
99 changes: 55 additions & 44 deletions crates/ruff/src/rules/flake8_simplify/rules/ast_ifexp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use ruff_text_size::{Ranged, TextRange};
use ruff_diagnostics::{AlwaysAutofixableViolation, AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::{is_const_false, is_const_true};
use ruff_python_ast::parenthesize::parenthesized_range;

use crate::checkers::ast::Checker;
use crate::registry::AsRule;
Expand All @@ -30,21 +31,29 @@ use crate::registry::AsRule;
/// - [Python documentation: Truth Value Testing](https://docs.python.org/3/library/stdtypes.html#truth-value-testing)
#[violation]
pub struct IfExprWithTrueFalse {
expr: String,
is_compare: bool,
}

impl Violation for IfExprWithTrueFalse {
const AUTOFIX: AutofixKind = AutofixKind::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let IfExprWithTrueFalse { expr } = self;
format!("Use `bool({expr})` instead of `True if {expr} else False`")
let IfExprWithTrueFalse { is_compare } = self;
if *is_compare {
format!("Remove unnecessary `True if ... else False`")
} else {
format!("Use `bool(...)` instead of `True if ... else False`")
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to remove the raw source code from these. I think we're hoping that this information is instead conveyed by showing code frames.

}

fn autofix_title(&self) -> Option<String> {
let IfExprWithTrueFalse { expr } = self;
Some(format!("Replace with `not {expr}"))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message was wrong (mixed up with the next rule).

let IfExprWithTrueFalse { is_compare } = self;
if *is_compare {
Some(format!("Remove unnecessary `True if ... else False`"))
} else {
Some(format!("Replace with `bool(...)"))
}
}
}

Expand All @@ -70,20 +79,16 @@ impl Violation for IfExprWithTrueFalse {
/// ## References
/// - [Python documentation: Truth Value Testing](https://docs.python.org/3/library/stdtypes.html#truth-value-testing)
#[violation]
pub struct IfExprWithFalseTrue {
expr: String,
}
pub struct IfExprWithFalseTrue;

impl AlwaysAutofixableViolation for IfExprWithFalseTrue {
#[derive_message_formats]
fn message(&self) -> String {
let IfExprWithFalseTrue { expr } = self;
format!("Use `not {expr}` instead of `False if {expr} else True`")
format!("Use `not ...` instead of `False if ... else True`")
}

fn autofix_title(&self) -> String {
let IfExprWithFalseTrue { expr } = self;
format!("Replace with `bool({expr})")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message was wrong (mixed up with the previous rule).

format!("Replace with `not ...`")
}
}

Expand Down Expand Up @@ -135,7 +140,7 @@ impl AlwaysAutofixableViolation for IfExprWithTwistedArms {
}

/// SIM210
pub(crate) fn explicit_true_false_in_ifexpr(
pub(crate) fn if_expr_with_true_false(
checker: &mut Checker,
expr: &Expr,
test: &Expr,
Expand All @@ -148,33 +153,43 @@ pub(crate) fn explicit_true_false_in_ifexpr(

let mut diagnostic = Diagnostic::new(
IfExprWithTrueFalse {
expr: checker.generator().expr(test),
is_compare: test.is_compare_expr(),
},
expr.range(),
);
if checker.patch(diagnostic.kind.rule()) {
if test.is_compare_expr() {
diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
checker.locator().slice(test).to_string(),
checker
.locator()
.slice(
parenthesized_range(test.into(), expr.into(), checker.locator().contents())
.unwrap_or(test.range()),
)
.to_string(),
expr.range(),
)));
} else if checker.semantic().is_builtin("bool") {
let node = ast::ExprName {
id: "bool".into(),
ctx: ExprContext::Load,
range: TextRange::default(),
};
let node1 = ast::ExprCall {
func: Box::new(node.into()),
arguments: Arguments {
args: vec![test.clone()],
keywords: vec![],
range: TextRange::default(),
},
range: TextRange::default(),
};
diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
checker.generator().expr(&node1.into()),
checker.generator().expr(
&ast::ExprCall {
func: Box::new(
ast::ExprName {
id: "bool".into(),
ctx: ExprContext::Load,
range: TextRange::default(),
}
.into(),
),
arguments: Arguments {
args: vec![test.clone()],
keywords: vec![],
range: TextRange::default(),
},
range: TextRange::default(),
}
.into(),
),
expr.range(),
)));
};
Expand All @@ -183,7 +198,7 @@ pub(crate) fn explicit_true_false_in_ifexpr(
}

/// SIM211
pub(crate) fn explicit_false_true_in_ifexpr(
pub(crate) fn if_expr_with_false_true(
checker: &mut Checker,
expr: &Expr,
test: &Expr,
Expand All @@ -194,21 +209,17 @@ pub(crate) fn explicit_false_true_in_ifexpr(
return;
}

let mut diagnostic = Diagnostic::new(
IfExprWithFalseTrue {
expr: checker.generator().expr(test),
},
expr.range(),
);
let mut diagnostic = Diagnostic::new(IfExprWithFalseTrue, expr.range());
if checker.patch(diagnostic.kind.rule()) {
let node = test.clone();
let node1 = ast::ExprUnaryOp {
op: UnaryOp::Not,
operand: Box::new(node),
range: TextRange::default(),
};
diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
checker.generator().expr(&node1.into()),
checker.generator().expr(
&ast::ExprUnaryOp {
op: UnaryOp::Not,
operand: Box::new(test.clone()),
range: TextRange::default(),
}
.into(),
),
expr.range(),
)));
}
Expand Down
35 changes: 27 additions & 8 deletions ...ules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM210_SIM210.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
---
source: crates/ruff/src/rules/flake8_simplify/mod.rs
---
SIM210.py:1:5: SIM210 [*] Use `bool(b)` instead of `True if b else False`
SIM210.py:1:5: SIM210 [*] Use `bool(...)` instead of `True if ... else False`
|
1 | a = True if b else False # SIM210
| ^^^^^^^^^^^^^^^^^^^^ SIM210
2 |
3 | a = True if b != c else False # SIM210
|
= help: Replace with `not b
= help: Replace with `bool(...)

ℹ Suggested fix
1 |-a = True if b else False # SIM210
Expand All @@ -17,7 +17,7 @@ SIM210.py:1:5: SIM210 [*] Use `bool(b)` instead of `True if b else False`
3 3 | a = True if b != c else False # SIM210
4 4 |

SIM210.py:3:5: SIM210 [*] Use `bool(b != c)` instead of `True if b != c else False`
SIM210.py:3:5: SIM210 [*] Remove unnecessary `True if ... else False`
|
1 | a = True if b else False # SIM210
2 |
Expand All @@ -26,7 +26,7 @@ SIM210.py:3:5: SIM210 [*] Use `bool(b != c)` instead of `True if b != c else Fal
4 |
5 | a = True if b + c else False # SIM210
|
= help: Replace with `not b != c
= help: Remove unnecessary `True if ... else False`

ℹ Suggested fix
1 1 | a = True if b else False # SIM210
Expand All @@ -37,7 +37,7 @@ SIM210.py:3:5: SIM210 [*] Use `bool(b != c)` instead of `True if b != c else Fal
5 5 | a = True if b + c else False # SIM210
6 6 |

SIM210.py:5:5: SIM210 [*] Use `bool(b + c)` instead of `True if b + c else False`
SIM210.py:5:5: SIM210 [*] Use `bool(...)` instead of `True if ... else False`
|
3 | a = True if b != c else False # SIM210
4 |
Expand All @@ -46,7 +46,7 @@ SIM210.py:5:5: SIM210 [*] Use `bool(b + c)` instead of `True if b + c else False
6 |
7 | a = False if b else True # OK
|
= help: Replace with `not b + c
= help: Replace with `bool(...)

ℹ Suggested fix
2 2 |
Expand All @@ -58,13 +58,32 @@ SIM210.py:5:5: SIM210 [*] Use `bool(b + c)` instead of `True if b + c else False
7 7 | a = False if b else True # OK
8 8 |

SIM210.py:15:9: SIM210 Use `bool(b)` instead of `True if b else False`
SIM210.py:15:9: SIM210 Use `bool(...)` instead of `True if ... else False`
|
13 | return False
14 |
15 | a = True if b else False
| ^^^^^^^^^^^^^^^^^^^^ SIM210
|
= help: Replace with `not b
= help: Replace with `bool(...)

SIM210.py:19:11: SIM210 [*] Remove unnecessary `True if ... else False`
|
18 | # Regression test for: https://github.com/astral-sh/ruff/issues/7076
19 | samesld = True if (psl.privatesuffix(urlparse(response.url).netloc) ==
| ___________^
20 | | psl.privatesuffix(src.netloc)) else False
| |____________________________________________________________________________^ SIM210
|
= help: Remove unnecessary `True if ... else False`

ℹ Suggested fix
16 16 |
17 17 |
18 18 | # Regression test for: https://github.com/astral-sh/ruff/issues/7076
19 |-samesld = True if (psl.privatesuffix(urlparse(response.url).netloc) ==
20 |- psl.privatesuffix(src.netloc)) else False
19 |+samesld = (psl.privatesuffix(urlparse(response.url).netloc) ==
20 |+ psl.privatesuffix(src.netloc))


12 changes: 6 additions & 6 deletions ...ules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM211_SIM211.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
---
source: crates/ruff/src/rules/flake8_simplify/mod.rs
---
SIM211.py:1:5: SIM211 [*] Use `not b` instead of `False if b else True`
SIM211.py:1:5: SIM211 [*] Use `not ...` instead of `False if ... else True`
|
1 | a = False if b else True # SIM211
| ^^^^^^^^^^^^^^^^^^^^ SIM211
2 |
3 | a = False if b != c else True # SIM211
|
= help: Replace with `bool(b)
= help: Replace with `not ...`

ℹ Suggested fix
1 |-a = False if b else True # SIM211
Expand All @@ -17,7 +17,7 @@ SIM211.py:1:5: SIM211 [*] Use `not b` instead of `False if b else True`
3 3 | a = False if b != c else True # SIM211
4 4 |

SIM211.py:3:5: SIM211 [*] Use `not b != c` instead of `False if b != c else True`
SIM211.py:3:5: SIM211 [*] Use `not ...` instead of `False if ... else True`
|
1 | a = False if b else True # SIM211
2 |
Expand All @@ -26,7 +26,7 @@ SIM211.py:3:5: SIM211 [*] Use `not b != c` instead of `False if b != c else True
4 |
5 | a = False if b + c else True # SIM211
|
= help: Replace with `bool(b != c)
= help: Replace with `not ...`

ℹ Suggested fix
1 1 | a = False if b else True # SIM211
Expand All @@ -37,7 +37,7 @@ SIM211.py:3:5: SIM211 [*] Use `not b != c` instead of `False if b != c else True
5 5 | a = False if b + c else True # SIM211
6 6 |

SIM211.py:5:5: SIM211 [*] Use `not b + c` instead of `False if b + c else True`
SIM211.py:5:5: SIM211 [*] Use `not ...` instead of `False if ... else True`
|
3 | a = False if b != c else True # SIM211
4 |
Expand All @@ -46,7 +46,7 @@ SIM211.py:5:5: SIM211 [*] Use `not b + c` instead of `False if b + c else True`
6 |
7 | a = True if b else False # OK
|
= help: Replace with `bool(b + c)
= help: Replace with `not ...`

ℹ Suggested fix
2 2 |
Expand Down