-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
## Summary This PR is a continuation of #6897 and #6702 and me replicating `refurb` rules (#1348). It adds support for [FURB132](/~https://github.com/dosisod/refurb/blob/master/refurb/checks/builtin/set_discard.py) ## Test Plan I included a new test + checked that all other tests pass.
- Loading branch information
1 parent
c448b40
commit 7e36284
Showing
9 changed files
with
395 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
from typing import Set | ||
from some.package.name import foo, bar | ||
|
||
|
||
s = set() | ||
s2 = {} | ||
s3: set[int] = foo() | ||
|
||
# these should match | ||
|
||
# FURB132 | ||
if "x" in s: | ||
s.remove("x") | ||
|
||
|
||
# FURB132 | ||
if "x" in s2: | ||
s2.remove("x") | ||
|
||
|
||
# FURB132 | ||
if "x" in s3: | ||
s3.remove("x") | ||
|
||
|
||
var = "y" | ||
# FURB132 | ||
if var in s: | ||
s.remove(var) | ||
|
||
|
||
if f"{var}:{var}" in s: | ||
s.remove(f"{var}:{var}") | ||
|
||
|
||
def identity(x): | ||
return x | ||
|
||
|
||
# TODO: FURB132 | ||
if identity("x") in s2: | ||
s2.remove(identity("x")) | ||
|
||
|
||
# these should not | ||
|
||
if "x" in s: | ||
s.remove("y") | ||
|
||
s.discard("x") | ||
|
||
s2 = set() | ||
|
||
if "x" in s: | ||
s2.remove("x") | ||
|
||
if "x" in s: | ||
s.remove("x") | ||
print("removed item") | ||
|
||
if bar() in s: | ||
# bar() might have a side effect | ||
s.remove(bar()) | ||
|
||
if "x" in s: | ||
s.remove("x") | ||
else: | ||
print("not found") | ||
|
||
class Container: | ||
def remove(self, item) -> None: | ||
return | ||
|
||
def __contains__(self, other) -> bool: | ||
return True | ||
|
||
c = Container() | ||
|
||
if "x" in c: | ||
c.remove("x") |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
206 changes: 206 additions & 0 deletions
206
crates/ruff/src/rules/refurb/rules/check_and_remove_from_set.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,206 @@ | ||
use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; | ||
use ruff_macros::{derive_message_formats, violation}; | ||
use ruff_python_ast::comparable::ComparableExpr; | ||
use ruff_python_ast::helpers::contains_effect; | ||
use ruff_python_ast::{self as ast, CmpOp, Expr, Stmt}; | ||
use ruff_python_codegen::Generator; | ||
use ruff_text_size::{Ranged, TextRange}; | ||
|
||
use crate::autofix::snippet::SourceCodeSnippet; | ||
use crate::checkers::ast::Checker; | ||
use crate::registry::AsRule; | ||
use crate::rules::refurb::helpers::is_set; | ||
|
||
/// ## What it does | ||
/// Checks for uses of `set#remove` that can be replaced with `set#discard`. | ||
/// | ||
/// ## Why is this bad? | ||
/// If an element should be removed from a set if it is present, it is more | ||
/// succinct and idiomatic to use `discard`. | ||
/// | ||
/// ## Example | ||
/// ```python | ||
/// nums = {123, 456} | ||
/// | ||
/// if 123 in nums: | ||
/// nums.remove(123) | ||
/// ``` | ||
/// | ||
/// Use instead: | ||
/// ```python | ||
/// nums = {123, 456} | ||
/// | ||
/// nums.discard(123) | ||
/// ``` | ||
/// | ||
/// ## References | ||
/// - [Python documentation: `set.discard()`](https://docs.python.org/3/library/stdtypes.html?highlight=list#frozenset.discard) | ||
#[violation] | ||
pub struct CheckAndRemoveFromSet { | ||
element: SourceCodeSnippet, | ||
set: String, | ||
} | ||
|
||
impl CheckAndRemoveFromSet { | ||
fn suggestion(&self) -> String { | ||
let set = &self.set; | ||
let element = self.element.truncated_display(); | ||
format!("{set}.discard({element})") | ||
} | ||
} | ||
|
||
impl AlwaysAutofixableViolation for CheckAndRemoveFromSet { | ||
#[derive_message_formats] | ||
fn message(&self) -> String { | ||
let suggestion = self.suggestion(); | ||
format!("Use `{suggestion}` instead of check and `remove`") | ||
} | ||
|
||
fn autofix_title(&self) -> String { | ||
let suggestion = self.suggestion(); | ||
format!("Replace with `{suggestion}`") | ||
} | ||
} | ||
|
||
/// FURB132 | ||
pub(crate) fn check_and_remove_from_set(checker: &mut Checker, if_stmt: &ast::StmtIf) { | ||
// In order to fit the profile, we need if without else clauses and with only one statement in its body. | ||
if if_stmt.body.len() != 1 || !if_stmt.elif_else_clauses.is_empty() { | ||
return; | ||
} | ||
|
||
// The `if` test should be `element in set`. | ||
let Some((check_element, check_set)) = match_check(if_stmt) else { | ||
return; | ||
}; | ||
|
||
// The `if` body should be `set.remove(element)`. | ||
let Some((remove_element, remove_set)) = match_remove(if_stmt) else { | ||
return; | ||
}; | ||
|
||
// ` | ||
// `set` in the check should be the same as `set` in the body | ||
if check_set.id != remove_set.id | ||
// `element` in the check should be the same as `element` in the body | ||
|| !compare(&check_element.into(), &remove_element.into()) | ||
// `element` shouldn't have a side effect, otherwise we might change the semantics of the program. | ||
|| contains_effect(check_element, |id| checker.semantic().is_builtin(id)) | ||
{ | ||
return; | ||
} | ||
|
||
// Check if what we assume is set is indeed a set. | ||
if !checker | ||
.semantic() | ||
.resolve_name(check_set) | ||
.map(|binding_id| checker.semantic().binding(binding_id)) | ||
.map_or(false, |binding| { | ||
is_set(binding, &check_set.id, checker.semantic()) | ||
}) | ||
{ | ||
return; | ||
}; | ||
|
||
let mut diagnostic = Diagnostic::new( | ||
CheckAndRemoveFromSet { | ||
element: SourceCodeSnippet::from_str(checker.locator().slice(check_element)), | ||
set: check_set.id.to_string(), | ||
}, | ||
if_stmt.range(), | ||
); | ||
if checker.patch(diagnostic.kind.rule()) { | ||
diagnostic.set_fix(Fix::suggested(Edit::replacement( | ||
make_suggestion(check_set, check_element, checker.generator()), | ||
if_stmt.start(), | ||
if_stmt.end(), | ||
))); | ||
} | ||
checker.diagnostics.push(diagnostic); | ||
} | ||
|
||
fn compare(lhs: &ComparableExpr, rhs: &ComparableExpr) -> bool { | ||
lhs == rhs | ||
} | ||
|
||
/// Match `if` condition to be `expr in name`, returns a tuple of (`expr`, `name`) on success. | ||
fn match_check(if_stmt: &ast::StmtIf) -> Option<(&Expr, &ast::ExprName)> { | ||
let ast::ExprCompare { | ||
ops, | ||
left, | ||
comparators, | ||
.. | ||
} = if_stmt.test.as_compare_expr()?; | ||
|
||
if ops.as_slice() != [CmpOp::In] { | ||
return None; | ||
} | ||
|
||
let [Expr::Name(right @ ast::ExprName { .. })] = comparators.as_slice() else { | ||
return None; | ||
}; | ||
|
||
Some((left.as_ref(), right)) | ||
} | ||
|
||
/// Match `if` body to be `name.remove(expr)`, returns a tuple of (`expr`, `name`) on success. | ||
fn match_remove(if_stmt: &ast::StmtIf) -> Option<(&Expr, &ast::ExprName)> { | ||
let [Stmt::Expr(ast::StmtExpr { value: expr, .. })] = if_stmt.body.as_slice() else { | ||
return None; | ||
}; | ||
|
||
let ast::ExprCall { | ||
func: attr, | ||
arguments: ast::Arguments { args, keywords, .. }, | ||
.. | ||
} = expr.as_call_expr()?; | ||
|
||
let ast::ExprAttribute { | ||
value: receiver, | ||
attr: func_name, | ||
.. | ||
} = attr.as_attribute_expr()?; | ||
|
||
let Expr::Name(ref set @ ast::ExprName { .. }) = receiver.as_ref() else { | ||
return None; | ||
}; | ||
|
||
let [arg] = args.as_slice() else { | ||
return None; | ||
}; | ||
|
||
if func_name != "remove" || !keywords.is_empty() { | ||
return None; | ||
} | ||
|
||
Some((arg, set)) | ||
} | ||
|
||
/// Construct the fix suggestion, ie `set.discard(element)`. | ||
fn make_suggestion(set: &ast::ExprName, element: &Expr, generator: Generator) -> String { | ||
// Here we construct `set.discard(element)` | ||
// | ||
// Let's make `set.discard`. | ||
let attr = ast::ExprAttribute { | ||
value: Box::new(set.clone().into()), | ||
attr: ast::Identifier::new("discard".to_string(), TextRange::default()), | ||
ctx: ast::ExprContext::Load, | ||
range: TextRange::default(), | ||
}; | ||
// Make the actual call `set.discard(element)` | ||
let call = ast::ExprCall { | ||
func: Box::new(attr.into()), | ||
arguments: ast::Arguments { | ||
args: vec![element.clone()], | ||
keywords: vec![], | ||
range: TextRange::default(), | ||
}, | ||
range: TextRange::default(), | ||
}; | ||
// And finally, turn it into a statement. | ||
let stmt = ast::StmtExpr { | ||
value: Box::new(call.into()), | ||
range: TextRange::default(), | ||
}; | ||
generator.stmt(&stmt.into()) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
pub(crate) use check_and_remove_from_set::*; | ||
pub(crate) use delete_full_slice::*; | ||
pub(crate) use repeated_append::*; | ||
|
||
mod check_and_remove_from_set; | ||
mod delete_full_slice; | ||
|
||
mod repeated_append; |
Oops, something went wrong.