-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[refurb] Implement
delete-full-slice
rule (FURB131
)
- Loading branch information
1 parent
6180ee0
commit 77ee1e6
Showing
10 changed files
with
492 additions
and
80 deletions.
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,64 @@ | ||
from typing import Dict, List | ||
|
||
names = {"key": "value"} | ||
nums = [1, 2, 3] | ||
x = 42 | ||
y = "hello" | ||
|
||
# these should match | ||
|
||
# FURB131 | ||
del nums[:] | ||
|
||
|
||
# FURB131 | ||
del names[:] | ||
|
||
|
||
# FURB131 | ||
del x, nums[:] | ||
|
||
|
||
# FURB131 | ||
del y, names[:], x | ||
|
||
|
||
def yes_one(x: list[int]): | ||
# FURB131 | ||
del x[:] | ||
|
||
|
||
def yes_two(x: dict[int, str]): | ||
# FURB131 | ||
del x[:] | ||
|
||
|
||
def yes_three(x: List[int]): | ||
# FURB131 | ||
del x[:] | ||
|
||
|
||
def yes_four(x: Dict[int, str]): | ||
# FURB131 | ||
del x[:] | ||
|
||
|
||
# these should not | ||
|
||
del names["key"] | ||
del nums[0] | ||
|
||
x = 1 | ||
del x | ||
|
||
|
||
del nums[1:2] | ||
|
||
|
||
del nums[:2] | ||
del nums[1:] | ||
del nums[::2] | ||
|
||
|
||
def no_one(param): | ||
del param[:] |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,129 @@ | ||
use ast::{ParameterWithDefault, Parameters}; | ||
use ruff_python_ast::{self as ast, Expr, Stmt}; | ||
use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType}; | ||
use ruff_python_semantic::{Binding, BindingKind}; | ||
|
||
use crate::checkers::ast::Checker; | ||
|
||
trait TypeChecker { | ||
const EXPR_TYPE: PythonType; | ||
fn match_annotation(checker: &Checker, annotation: &Expr) -> bool; | ||
} | ||
|
||
fn check_type<'a, T: TypeChecker>(checker: &'a Checker, binding: &'a Binding, name: &str) -> bool { | ||
assert!(binding.source.is_some()); | ||
let stmt = checker.semantic().statement(binding.source.unwrap()); | ||
|
||
match binding.kind { | ||
BindingKind::Assignment => match stmt { | ||
Stmt::Assign(ast::StmtAssign { value, .. }) => { | ||
let value_type: ResolvedPythonType = value.as_ref().into(); | ||
let ResolvedPythonType::Atom(candidate) = value_type else { | ||
return false; | ||
}; | ||
candidate == T::EXPR_TYPE | ||
} | ||
Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. }) => { | ||
T::match_annotation(checker, annotation.as_ref()) | ||
} | ||
_ => false, | ||
}, | ||
BindingKind::Argument => match stmt { | ||
Stmt::FunctionDef(ast::StmtFunctionDef { parameters, .. }) => { | ||
let Some(parameter) = find_parameter_by_name(parameters.as_ref(), name) else { | ||
return false; | ||
}; | ||
let Some(ref annotation) = parameter.parameter.annotation else { | ||
return false; | ||
}; | ||
T::match_annotation(checker, annotation.as_ref()) | ||
} | ||
_ => false, | ||
}, | ||
BindingKind::Annotation => match stmt { | ||
Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. }) => { | ||
T::match_annotation(checker, annotation.as_ref()) | ||
} | ||
_ => false, | ||
}, | ||
_ => false, | ||
} | ||
} | ||
|
||
trait AnnotationTypeChecker { | ||
const TYPING_NAME: &'static str; | ||
const BUILTIN_TYPE_NAME: &'static str; | ||
|
||
fn match_annotation(checker: &Checker, annotation: &Expr) -> bool { | ||
let Expr::Subscript(ast::ExprSubscript { value, .. }) = annotation else { | ||
return false; | ||
}; | ||
Self::match_builtin_type(checker, value) | ||
|| checker | ||
.semantic() | ||
.match_typing_expr(value, Self::TYPING_NAME) | ||
} | ||
|
||
fn match_builtin_type(checker: &Checker, type_expr: &Expr) -> bool { | ||
let Expr::Name(ast::ExprName { id, .. }) = type_expr else { | ||
return false; | ||
}; | ||
id == Self::BUILTIN_TYPE_NAME && checker.semantic().is_builtin(Self::BUILTIN_TYPE_NAME) | ||
} | ||
} | ||
|
||
struct ListChecker; | ||
|
||
impl AnnotationTypeChecker for ListChecker { | ||
const TYPING_NAME: &'static str = "List"; | ||
const BUILTIN_TYPE_NAME: &'static str = "list"; | ||
} | ||
|
||
impl TypeChecker for ListChecker { | ||
const EXPR_TYPE: PythonType = PythonType::List; | ||
fn match_annotation(checker: &Checker, annotation: &Expr) -> bool { | ||
<Self as AnnotationTypeChecker>::match_annotation(checker, annotation) | ||
} | ||
} | ||
|
||
struct DictChecker; | ||
|
||
impl AnnotationTypeChecker for DictChecker { | ||
const TYPING_NAME: &'static str = "Dict"; | ||
const BUILTIN_TYPE_NAME: &'static str = "dict"; | ||
} | ||
|
||
impl TypeChecker for DictChecker { | ||
const EXPR_TYPE: PythonType = PythonType::Dict; | ||
fn match_annotation(checker: &Checker, annotation: &Expr) -> bool { | ||
<Self as AnnotationTypeChecker>::match_annotation(checker, annotation) | ||
} | ||
} | ||
|
||
pub(super) fn is_list<'a>(checker: &'a Checker, binding: &'a Binding, name: &str) -> bool { | ||
check_type::<ListChecker>(checker, binding, name) | ||
} | ||
|
||
pub(super) fn is_dict<'a>(checker: &'a Checker, binding: &'a Binding, name: &str) -> bool { | ||
check_type::<DictChecker>(checker, binding, name) | ||
} | ||
|
||
#[inline] | ||
fn find_parameter_by_name<'a>( | ||
parameters: &'a Parameters, | ||
name: &'a str, | ||
) -> Option<&'a ParameterWithDefault> { | ||
find_parameter_by_name_impl(¶meters.args, name) | ||
.or_else(|| find_parameter_by_name_impl(¶meters.posonlyargs, name)) | ||
.or_else(|| find_parameter_by_name_impl(¶meters.kwonlyargs, name)) | ||
} | ||
|
||
#[inline] | ||
fn find_parameter_by_name_impl<'a>( | ||
parameters: &'a [ParameterWithDefault], | ||
name: &'a str, | ||
) -> Option<&'a ParameterWithDefault> { | ||
parameters | ||
.iter() | ||
.find(|arg| arg.parameter.name.as_str() == name) | ||
} |
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
153 changes: 153 additions & 0 deletions
153
crates/ruff/src/rules/refurb/rules/delete_full_slice.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,153 @@ | ||
use ast::Ranged; | ||
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; | ||
use ruff_macros::{derive_message_formats, violation}; | ||
use ruff_python_ast::{self as ast, Expr}; | ||
use ruff_python_codegen::Generator; | ||
use ruff_python_semantic::Binding; | ||
use ruff_text_size::TextRange; | ||
|
||
use crate::checkers::ast::Checker; | ||
use crate::registry::AsRule; | ||
use crate::rules::refurb::helpers::{is_dict, is_list}; | ||
|
||
/// ## What it does | ||
/// Checks for delete statements with full slice on lists and dictionaries. | ||
/// | ||
/// ## Why is this bad? | ||
/// It is faster and more succinct to remove all items via the `clear()` method. | ||
/// | ||
/// ## Example | ||
/// ```python | ||
/// names = {"key": "value"} | ||
/// nums = [1, 2, 3] | ||
/// | ||
/// del names[:] | ||
/// del nums[:] | ||
/// ``` | ||
/// | ||
/// Use instead: | ||
/// ```python | ||
/// names = {"key": "value"} | ||
/// nums = [1, 2, 3] | ||
/// | ||
/// names.clear() | ||
/// nums.clear() | ||
/// ``` | ||
#[violation] | ||
pub struct DeleteFullSlice; | ||
|
||
impl Violation for DeleteFullSlice { | ||
const AUTOFIX: AutofixKind = AutofixKind::Sometimes; | ||
|
||
#[derive_message_formats] | ||
fn message(&self) -> String { | ||
format!("Prefer `clear` over deleting the full slice") | ||
} | ||
|
||
fn autofix_title(&self) -> Option<String> { | ||
Some("Replace with `clear()`".to_string()) | ||
} | ||
} | ||
|
||
// FURB131 | ||
pub(crate) fn delete_full_slice(checker: &mut Checker, delete: &ast::StmtDelete) { | ||
// ATM, we can only auto-fix when delete has a single target. | ||
let only_target = delete.targets.len() == 1; | ||
for target in &delete.targets { | ||
let Some(name) = match_full_slice(checker, target) else { | ||
continue; | ||
}; | ||
let mut diagnostic = Diagnostic::new(DeleteFullSlice {}, delete.range); | ||
|
||
if checker.patch(diagnostic.kind.rule()) && only_target { | ||
let replacement = make_suggestion(name, checker.generator()); | ||
diagnostic.set_fix(Fix::suggested(Edit::replacement( | ||
replacement, | ||
delete.start(), | ||
delete.end(), | ||
))); | ||
} | ||
|
||
checker.diagnostics.push(diagnostic); | ||
} | ||
} | ||
|
||
fn make_suggestion(name: &str, generator: Generator) -> String { | ||
// Here we construct `var.clear()` | ||
// | ||
// And start with construction of `var` | ||
let var = ast::ExprName { | ||
id: name.to_string(), | ||
ctx: ast::ExprContext::Load, | ||
range: TextRange::default(), | ||
}; | ||
// Make `var.clear`. | ||
let attr = ast::ExprAttribute { | ||
value: Box::new(var.into()), | ||
attr: ast::Identifier::new("clear".to_string(), TextRange::default()), | ||
ctx: ast::ExprContext::Load, | ||
range: TextRange::default(), | ||
}; | ||
// Make it into a call `var.clear()` | ||
let call = ast::ExprCall { | ||
func: Box::new(attr.into()), | ||
arguments: ast::Arguments { | ||
args: vec![], | ||
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()) | ||
} | ||
|
||
fn match_full_slice<'a>(checker: &mut Checker, expr: &'a Expr) -> Option<&'a str> { | ||
// Check that it is del expr[...] | ||
let Expr::Subscript(subscript) = expr else { | ||
return None; | ||
}; | ||
|
||
// Check that it is del expr[:] | ||
let Expr::Slice(ast::ExprSlice { | ||
lower: None, | ||
upper: None, | ||
step: None, | ||
.. | ||
}) = subscript.slice.as_ref() | ||
else { | ||
return None; | ||
}; | ||
|
||
// Check that it is del var[:] | ||
let Expr::Name(ast::ExprName { id: name, .. }) = subscript.value.as_ref() else { | ||
return None; | ||
}; | ||
|
||
// Let's find definition for var | ||
let scope = checker.semantic().current_scope(); | ||
let bindings: Vec<&Binding> = scope | ||
.get_all(name) | ||
.map(|binding_id| checker.semantic().binding(binding_id)) | ||
.collect(); | ||
|
||
// NOTE: Maybe it is too strict of a limitation, but it seems reasonable. | ||
if bindings.len() != 1 { | ||
return None; | ||
} | ||
|
||
let binding = bindings[0]; | ||
// It should only apply to variables that are known to be lists or dicts. | ||
if binding.source.is_none() | ||
|| !(is_dict(checker, binding, name) || is_list(checker, binding, name)) | ||
{ | ||
return None; | ||
} | ||
|
||
// Name is needed for the fix suggestion. | ||
Some(name) | ||
} |
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,3 +1,5 @@ | ||
pub(crate) use delete_full_slice::*; | ||
pub(crate) use repeated_append::*; | ||
|
||
mod delete_full_slice; | ||
mod repeated_append; |
Oops, something went wrong.