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

Fix SIM222 and SIM223 false positive #3832

Merged
merged 2 commits into from
Mar 31, 2023
Merged

Fix SIM222 and SIM223 false positive #3832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b51c5cf
Fix SIM222 and SIM223 false positive
JonathanPlasse Mar 31, 2023
70009ff
Add fixed false positives to fixtures
JonathanPlasse Mar 31, 2023
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
13 changes: 13 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_simplify/SIM222.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,16 @@ def validate(self, value):

if a or True or f() or b or g(): # SIM222
pass


if a and f() and b and g() and False: # OK
pass

if a and f() and False and g() and b: # OK
pass

if False and f() and a and g() and b: # OK
pass

if a and False and f() and b and g(): # OK
pass
19 changes: 16 additions & 3 deletions crates/ruff/resources/test/fixtures/flake8_simplify/SIM223.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,24 @@
if a and f() and b and g() and False: # OK
pass

if a and f() and False and g() and b: # SIM222
if a and f() and False and g() and b: # SIM223
pass

if False and f() and a and g() and b: # SIM222
if False and f() and a and g() and b: # SIM223
pass

if a and False and f() and b and g(): # SIM222
if a and False and f() and b and g(): # SIM223
pass


if a or f() or b or g() or True: # OK
pass

if a or f() or True or g() or b: # OK
pass

if True or f() or a or g() or b: # OK
pass

if a or True or f() or b or g(): # OK
pass
13 changes: 10 additions & 3 deletions crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,10 +503,17 @@ pub fn expr_or_not_expr(checker: &mut Checker, expr: &Expr) {
}
}

pub fn is_short_circuit(ctx: &Context, expr: &Expr) -> Option<(Location, Location)> {
pub fn is_short_circuit(
ctx: &Context,
expr: &Expr,
expected_op: &Boolop,
) -> Option<(Location, Location)> {
let ExprKind::BoolOp { op, values, } = &expr.node else {
return None;
};
if op != expected_op {
return None;
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we modify the fixtures, such that they would've caught this?

let short_circuit_value = match op {
Boolop::And => false,
Boolop::Or => true,
Expand Down Expand Up @@ -551,7 +558,7 @@ pub fn is_short_circuit(ctx: &Context, expr: &Expr) -> Option<(Location, Locatio

/// SIM222
pub fn expr_or_true(checker: &mut Checker, expr: &Expr) {
let Some((location, end_location)) = is_short_circuit(&checker.ctx, expr) else {
let Some((location, end_location)) = is_short_circuit(&checker.ctx, expr, &Boolop::Or) else {
return;
};
let mut diagnostic = Diagnostic::new(
Expand All @@ -573,7 +580,7 @@ pub fn expr_or_true(checker: &mut Checker, expr: &Expr) {

/// SIM223
pub fn expr_and_false(checker: &mut Checker, expr: &Expr) {
let Some((location, end_location)) = is_short_circuit(&checker.ctx, expr) else {
let Some((location, end_location)) = is_short_circuit(&checker.ctx, expr, &Boolop::And) else {
return;
};
let mut diagnostic = Diagnostic::new(
Expand Down