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

Show negated condition in needless-bool diagnostics #10854

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Show negated condition in needless-bool diagnostics #10854

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM103.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def f():


def f():
# SIM103 (but not fixable)
# SIM103
if a:
return False
else:
Expand All @@ -77,7 +77,7 @@ def f():


def f():
# OK
# SIM103 (but not fixable)
def bool():
return False
if a:
Expand All @@ -86,6 +86,14 @@ def bool():
return False


def f():
# SIM103
if keys is not None and notice.key not in keys:
return False
else:
return True


###
# Positive cases (preview)
###
Expand Down
71 changes: 38 additions & 33 deletions crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,30 +41,31 @@ use crate::fix::snippet::SourceCodeSnippet;
/// [preview]: https://docs.astral.sh/ruff/preview/
#[violation]
pub struct NeedlessBool {
condition: SourceCodeSnippet,
replacement: Option<SourceCodeSnippet>,
condition: Option<SourceCodeSnippet>,
negate: bool,
}

impl Violation for NeedlessBool {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let NeedlessBool { condition, .. } = self;
if let Some(condition) = condition.full_display() {
let NeedlessBool { condition, negate } = self;

if let Some(condition) = condition.as_ref().and_then(SourceCodeSnippet::full_display) {
format!("Return the condition `{condition}` directly")
} else if *negate {
format!("Return the negated condition directly")
} else {
format!("Return the condition directly")
}
}

fn fix_title(&self) -> Option<String> {
let NeedlessBool { replacement, .. } = self;
if let Some(replacement) = replacement
.as_ref()
.and_then(SourceCodeSnippet::full_display)
{
Some(format!("Replace with `{replacement}`"))
let NeedlessBool { condition, .. } = self;

if let Some(condition) = condition.as_ref().and_then(SourceCodeSnippet::full_display) {
Some(format!("Replace with `return {condition}`"))
} else {
Some(format!("Inline condition"))
}
Expand Down Expand Up @@ -191,37 +192,29 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) {
return;
}

let condition = checker.locator().slice(if_test);
let replacement = if checker.indexer().has_comments(&range, checker.locator()) {
// Generate the replacement condition.
let condition = if checker.indexer().has_comments(&range, checker.locator()) {
None
} else {
// If the return values are inverted, wrap the condition in a `not`.
if inverted {
let node = ast::StmtReturn {
value: Some(Box::new(Expr::UnaryOp(ast::ExprUnaryOp {
op: ast::UnaryOp::Not,
operand: Box::new(if_test.clone()),
range: TextRange::default(),
}))),
Some(Expr::UnaryOp(ast::ExprUnaryOp {
op: ast::UnaryOp::Not,
operand: Box::new(if_test.clone()),
range: TextRange::default(),
};
Some(checker.generator().stmt(&node.into()))
}))
} else if if_test.is_compare_expr() {
// If the condition is a comparison, we can replace it with the condition, since we
// know it's a boolean.
let node = ast::StmtReturn {
value: Some(Box::new(if_test.clone())),
range: TextRange::default(),
};
Some(checker.generator().stmt(&node.into()))
Some(if_test.clone())
} else if checker.semantic().is_builtin("bool") {
// Otherwise, we need to wrap the condition in a call to `bool`.
let func_node = ast::ExprName {
id: "bool".into(),
ctx: ExprContext::Load,
range: TextRange::default(),
};
let value_node = ast::ExprCall {
let call_node = ast::ExprCall {
func: Box::new(func_node.into()),
arguments: Arguments {
args: Box::from([if_test.clone()]),
Expand All @@ -230,20 +223,32 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) {
},
range: TextRange::default(),
};
let return_node = ast::StmtReturn {
value: Some(Box::new(value_node.into())),
range: TextRange::default(),
};
Some(checker.generator().stmt(&return_node.into()))
Some(Expr::Call(call_node))
} else {
None
}
};

// Generate the replacement `return` statement.
let replacement = condition.as_ref().map(|expr| {
Stmt::Return(ast::StmtReturn {
value: Some(Box::new(expr.clone())),
range: TextRange::default(),
})
});

// Generate source code.
let replacement = replacement
.as_ref()
.map(|stmt| checker.generator().stmt(stmt));
let condition = condition
.as_ref()
.map(|expr| checker.generator().expr(expr));

let mut diagnostic = Diagnostic::new(
NeedlessBool {
condition: SourceCodeSnippet::from_str(condition),
replacement: replacement.clone().map(SourceCodeSnippet::new),
condition: condition.map(SourceCodeSnippet::new),
negate: inverted,
},
range,
);
Expand Down
40 changes: 33 additions & 7 deletions ...ake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
source: crates/ruff_linter/src/rules/flake8_simplify/mod.rs
---
SIM103.py:3:5: SIM103 [*] Return the condition `a` directly
SIM103.py:3:5: SIM103 [*] Return the condition `bool(a)` directly
|
1 | def f():
2 | # SIM103
Expand Down Expand Up @@ -52,7 +52,7 @@ SIM103.py:11:5: SIM103 [*] Return the condition `a == b` directly
16 13 |
17 14 | def f():

SIM103.py:21:5: SIM103 [*] Return the condition `b` directly
SIM103.py:21:5: SIM103 [*] Return the condition `bool(b)` directly
|
19 | if a:
20 | return 1
Expand All @@ -78,7 +78,7 @@ SIM103.py:21:5: SIM103 [*] Return the condition `b` directly
26 23 |
27 24 | def f():

SIM103.py:32:9: SIM103 [*] Return the condition `b` directly
SIM103.py:32:9: SIM103 [*] Return the condition `bool(b)` directly
|
30 | return 1
31 | else:
Expand All @@ -104,10 +104,10 @@ SIM103.py:32:9: SIM103 [*] Return the condition `b` directly
37 34 |
38 35 | def f():

SIM103.py:57:5: SIM103 [*] Return the condition `a` directly
SIM103.py:57:5: SIM103 [*] Return the condition `not a` directly
|
55 | def f():
56 | # SIM103 (but not fixable)
56 | # SIM103
57 | if a:
| _____^
58 | | return False
Expand All @@ -120,7 +120,7 @@ SIM103.py:57:5: SIM103 [*] Return the condition `a` directly
ℹ Unsafe fix
54 54 |
55 55 | def f():
56 56 | # SIM103 (but not fixable)
56 56 | # SIM103
57 |- if a:
58 |- return False
59 |- else:
Expand All @@ -130,7 +130,7 @@ SIM103.py:57:5: SIM103 [*] Return the condition `a` directly
62 59 |
63 60 | def f():

SIM103.py:83:5: SIM103 Return the condition `a` directly
SIM103.py:83:5: SIM103 Return the condition directly
|
81 | def bool():
82 | return False
Expand All @@ -142,3 +142,29 @@ SIM103.py:83:5: SIM103 Return the condition `a` directly
| |____________________^ SIM103
|
= help: Inline condition

SIM103.py:91:5: SIM103 [*] Return the condition `not (keys is not None and notice.key not in keys)` directly
|
89 | def f():
90 | # SIM103
91 | if keys is not None and notice.key not in keys:
| _____^
92 | | return False
93 | | else:
94 | | return True
| |___________________^ SIM103
|
= help: Replace with `return not (keys is not None and notice.key not in keys)`

ℹ Unsafe fix
88 88 |
89 89 | def f():
90 90 | # SIM103
91 |- if keys is not None and notice.key not in keys:
92 |- return False
93 |- else:
94 |- return True
91 |+ return not (keys is not None and notice.key not in keys)
95 92 |
96 93 |
97 94 | ###