mirror of https://github.com/astral-sh/ruff
[refurb] Implement `delete-full-slice` rule (`FURB131`) (#6897)
## Summary This PR is a continuation of #6702 and me replicating `refurb` rules (#1348). It adds support for [FURB131](https://github.com/dosisod/refurb/blob/master/refurb/checks/builtin/no_del.py. ## Test Plan I included a new test + checked that all other tests pass.
This commit is contained in:
parent
3200015c06
commit
c448b4086a
|
|
@ -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[:]
|
||||||
|
|
@ -1459,7 +1459,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Stmt::Delete(ast::StmtDelete { targets, range: _ }) => {
|
Stmt::Delete(delete @ ast::StmtDelete { targets, range: _ }) => {
|
||||||
if checker.enabled(Rule::GlobalStatement) {
|
if checker.enabled(Rule::GlobalStatement) {
|
||||||
for target in targets {
|
for target in targets {
|
||||||
if let Expr::Name(ast::ExprName { id, .. }) = target {
|
if let Expr::Name(ast::ExprName { id, .. }) = target {
|
||||||
|
|
@ -1467,6 +1467,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
if checker.enabled(Rule::DeleteFullSlice) {
|
||||||
|
refurb::rules::delete_full_slice(checker, delete);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
Stmt::Expr(ast::StmtExpr { value, range: _ }) => {
|
Stmt::Expr(ast::StmtExpr { value, range: _ }) => {
|
||||||
if checker.enabled(Rule::UselessComparison) {
|
if checker.enabled(Rule::UselessComparison) {
|
||||||
|
|
|
||||||
|
|
@ -867,7 +867,6 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
|
||||||
|
|
||||||
// refurb
|
// refurb
|
||||||
(Refurb, "113") => (RuleGroup::Nursery, rules::refurb::rules::RepeatedAppend),
|
(Refurb, "113") => (RuleGroup::Nursery, rules::refurb::rules::RepeatedAppend),
|
||||||
|
(Refurb, "131") => (RuleGroup::Nursery, rules::refurb::rules::DeleteFullSlice),
|
||||||
_ => return None,
|
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,184 @@
|
||||||
|
use ast::{ParameterWithDefault, Parameters};
|
||||||
|
use ruff_python_ast::helpers::map_subscript;
|
||||||
|
use ruff_python_ast::{self as ast, Expr, Stmt};
|
||||||
|
use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType};
|
||||||
|
use ruff_python_semantic::{Binding, BindingKind, SemanticModel};
|
||||||
|
|
||||||
|
/// Abstraction for a type checker, conservatively checks for the intended type(s).
|
||||||
|
trait TypeChecker {
|
||||||
|
/// Check annotation expression to match the intended type(s).
|
||||||
|
fn match_annotation(annotation: &Expr, semantic: &SemanticModel) -> bool;
|
||||||
|
/// Check initializer expression to match the intended type(s).
|
||||||
|
fn match_initializer(initializer: &Expr, semantic: &SemanticModel) -> bool;
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Check if the type checker accepts the given binding with the given name.
|
||||||
|
///
|
||||||
|
/// NOTE: this function doesn't perform more serious type inference, so it won't be able
|
||||||
|
/// to understand if the value gets initialized from a call to a function always returning
|
||||||
|
/// lists. This also implies no interfile analysis.
|
||||||
|
fn check_type<T: TypeChecker>(binding: &Binding, name: &str, semantic: &SemanticModel) -> bool {
|
||||||
|
match binding.kind {
|
||||||
|
BindingKind::Assignment => match binding.statement(semantic) {
|
||||||
|
// ```python
|
||||||
|
// x = init_expr
|
||||||
|
// ```
|
||||||
|
//
|
||||||
|
// The type checker might know how to infer the type based on `init_expr`.
|
||||||
|
Some(Stmt::Assign(ast::StmtAssign { value, .. })) => {
|
||||||
|
T::match_initializer(value.as_ref(), semantic)
|
||||||
|
}
|
||||||
|
|
||||||
|
// ```python
|
||||||
|
// x: annotation = some_expr
|
||||||
|
// ```
|
||||||
|
//
|
||||||
|
// In this situation, we check only the annotation.
|
||||||
|
Some(Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. })) => {
|
||||||
|
T::match_annotation(annotation.as_ref(), semantic)
|
||||||
|
}
|
||||||
|
_ => false,
|
||||||
|
},
|
||||||
|
|
||||||
|
BindingKind::Argument => match binding.statement(semantic) {
|
||||||
|
// ```python
|
||||||
|
// def foo(x: annotation):
|
||||||
|
// ...
|
||||||
|
// ```
|
||||||
|
//
|
||||||
|
// We trust the annotation and see if the type checker matches the annotation.
|
||||||
|
Some(Stmt::FunctionDef(ast::StmtFunctionDef { parameters, .. })) => {
|
||||||
|
// TODO(charlie): Store a pointer to the argument in the binding.
|
||||||
|
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(annotation.as_ref(), semantic)
|
||||||
|
}
|
||||||
|
_ => false,
|
||||||
|
},
|
||||||
|
|
||||||
|
BindingKind::Annotation => match binding.statement(semantic) {
|
||||||
|
// ```python
|
||||||
|
// x: annotation
|
||||||
|
// ```
|
||||||
|
//
|
||||||
|
// It's a typed declaration, type annotation is the only source of information.
|
||||||
|
Some(Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. })) => {
|
||||||
|
T::match_annotation(annotation.as_ref(), semantic)
|
||||||
|
}
|
||||||
|
_ => false,
|
||||||
|
},
|
||||||
|
|
||||||
|
_ => false,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Type checker for builtin types.
|
||||||
|
trait BuiltinTypeChecker {
|
||||||
|
/// Builtin type name.
|
||||||
|
const BUILTIN_TYPE_NAME: &'static str;
|
||||||
|
/// Type name as found in the `Typing` module.
|
||||||
|
const TYPING_NAME: &'static str;
|
||||||
|
/// [`PythonType`] associated with the intended type.
|
||||||
|
const EXPR_TYPE: PythonType;
|
||||||
|
|
||||||
|
/// Check annotation expression to match the intended type.
|
||||||
|
fn match_annotation(annotation: &Expr, semantic: &SemanticModel) -> bool {
|
||||||
|
let value = map_subscript(annotation);
|
||||||
|
Self::match_builtin_type(value, semantic)
|
||||||
|
|| semantic.match_typing_expr(value, Self::TYPING_NAME)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Check initializer expression to match the intended type.
|
||||||
|
fn match_initializer(initializer: &Expr, semantic: &SemanticModel) -> bool {
|
||||||
|
Self::match_expr_type(initializer) || Self::match_builtin_constructor(initializer, semantic)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Check if the type can be inferred from the given expression.
|
||||||
|
fn match_expr_type(initializer: &Expr) -> bool {
|
||||||
|
let init_type: ResolvedPythonType = initializer.into();
|
||||||
|
match init_type {
|
||||||
|
ResolvedPythonType::Atom(atom) => atom == Self::EXPR_TYPE,
|
||||||
|
_ => false,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Check if the given expression corresponds to a constructor call of the builtin type.
|
||||||
|
fn match_builtin_constructor(initializer: &Expr, semantic: &SemanticModel) -> bool {
|
||||||
|
let Expr::Call(ast::ExprCall { func, .. }) = initializer else {
|
||||||
|
return false;
|
||||||
|
};
|
||||||
|
Self::match_builtin_type(func.as_ref(), semantic)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Check if the given expression names the builtin type.
|
||||||
|
fn match_builtin_type(type_expr: &Expr, semantic: &SemanticModel) -> bool {
|
||||||
|
let Expr::Name(ast::ExprName { id, .. }) = type_expr else {
|
||||||
|
return false;
|
||||||
|
};
|
||||||
|
id == Self::BUILTIN_TYPE_NAME && semantic.is_builtin(Self::BUILTIN_TYPE_NAME)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
impl<T: BuiltinTypeChecker> TypeChecker for T {
|
||||||
|
fn match_annotation(annotation: &Expr, semantic: &SemanticModel) -> bool {
|
||||||
|
<Self as BuiltinTypeChecker>::match_annotation(annotation, semantic)
|
||||||
|
}
|
||||||
|
|
||||||
|
fn match_initializer(initializer: &Expr, semantic: &SemanticModel) -> bool {
|
||||||
|
<Self as BuiltinTypeChecker>::match_initializer(initializer, semantic)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
struct ListChecker;
|
||||||
|
|
||||||
|
impl BuiltinTypeChecker for ListChecker {
|
||||||
|
const BUILTIN_TYPE_NAME: &'static str = "list";
|
||||||
|
const TYPING_NAME: &'static str = "List";
|
||||||
|
const EXPR_TYPE: PythonType = PythonType::List;
|
||||||
|
}
|
||||||
|
|
||||||
|
struct DictChecker;
|
||||||
|
|
||||||
|
impl BuiltinTypeChecker for DictChecker {
|
||||||
|
const BUILTIN_TYPE_NAME: &'static str = "dict";
|
||||||
|
const TYPING_NAME: &'static str = "Dict";
|
||||||
|
const EXPR_TYPE: PythonType = PythonType::Dict;
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Test whether the given binding (and the given name) can be considered a list.
|
||||||
|
/// For this, we check what value might be associated with it through it's initialization and
|
||||||
|
/// what annotation it has (we consider `list` and `typing.List`).
|
||||||
|
pub(super) fn is_list<'a>(binding: &'a Binding, name: &str, semantic: &'a SemanticModel) -> bool {
|
||||||
|
check_type::<ListChecker>(binding, name, semantic)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Test whether the given binding (and the given name) can be considered a dictionary.
|
||||||
|
/// For this, we check what value might be associated with it through it's initialization and
|
||||||
|
/// what annotation it has (we consider `dict` and `typing.Dict`).
|
||||||
|
pub(super) fn is_dict<'a>(binding: &'a Binding, name: &str, semantic: &'a SemanticModel) -> bool {
|
||||||
|
check_type::<DictChecker>(binding, name, semantic)
|
||||||
|
}
|
||||||
|
|
||||||
|
#[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)
|
||||||
|
}
|
||||||
|
|
@ -1,4 +1,6 @@
|
||||||
//! Rules from [refurb](https://pypi.org/project/refurb/)/
|
//! Rules from [refurb](https://pypi.org/project/refurb/)/
|
||||||
|
|
||||||
|
mod helpers;
|
||||||
pub(crate) mod rules;
|
pub(crate) mod rules;
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
|
|
@ -13,6 +15,7 @@ mod tests {
|
||||||
use crate::{assert_messages, settings};
|
use crate::{assert_messages, settings};
|
||||||
|
|
||||||
#[test_case(Rule::RepeatedAppend, Path::new("FURB113.py"))]
|
#[test_case(Rule::RepeatedAppend, Path::new("FURB113.py"))]
|
||||||
|
#[test_case(Rule::DeleteFullSlice, Path::new("FURB131.py"))]
|
||||||
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
|
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
|
||||||
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
|
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
|
||||||
let diagnostics = test_path(
|
let diagnostics = test_path(
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,157 @@
|
||||||
|
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, SemanticModel};
|
||||||
|
use ruff_text_size::{Ranged, TextRange};
|
||||||
|
|
||||||
|
use crate::checkers::ast::Checker;
|
||||||
|
use crate::registry::AsRule;
|
||||||
|
use crate::rules::refurb::helpers::{is_dict, is_list};
|
||||||
|
|
||||||
|
/// ## What it does
|
||||||
|
/// Checks for `del` statements that delete the entire slice of a list or
|
||||||
|
/// dictionary.
|
||||||
|
///
|
||||||
|
/// ## Why is this bad?
|
||||||
|
/// It's 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()
|
||||||
|
/// ```
|
||||||
|
///
|
||||||
|
/// ## References
|
||||||
|
/// - [Python documentation: Mutable Sequence Types](https://docs.python.org/3/library/stdtypes.html?highlight=list#mutable-sequence-types)
|
||||||
|
/// - [Python documentation: `dict.clear()`](https://docs.python.org/3/library/stdtypes.html?highlight=list#dict.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 a 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) {
|
||||||
|
for target in &delete.targets {
|
||||||
|
let Some(name) = match_full_slice(target, checker.semantic()) else {
|
||||||
|
continue;
|
||||||
|
};
|
||||||
|
|
||||||
|
let mut diagnostic = Diagnostic::new(DeleteFullSlice, delete.range);
|
||||||
|
|
||||||
|
// Fix is only supported for single-target deletions.
|
||||||
|
if checker.patch(diagnostic.kind.rule()) && delete.targets.len() == 1 {
|
||||||
|
let replacement = make_suggestion(name, checker.generator());
|
||||||
|
diagnostic.set_fix(Fix::suggested(Edit::replacement(
|
||||||
|
replacement,
|
||||||
|
delete.start(),
|
||||||
|
delete.end(),
|
||||||
|
)));
|
||||||
|
}
|
||||||
|
|
||||||
|
checker.diagnostics.push(diagnostic);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Match `del expr[:]` where `expr` is a list or a dict.
|
||||||
|
fn match_full_slice<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a str> {
|
||||||
|
// Check that it is `del expr[...]`.
|
||||||
|
let subscript = expr.as_subscript_expr()?;
|
||||||
|
|
||||||
|
// Check that it is` `del expr[:]`.
|
||||||
|
if !matches!(
|
||||||
|
subscript.slice.as_ref(),
|
||||||
|
Expr::Slice(ast::ExprSlice {
|
||||||
|
lower: None,
|
||||||
|
upper: None,
|
||||||
|
step: None,
|
||||||
|
range: _,
|
||||||
|
})
|
||||||
|
) {
|
||||||
|
return None;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Check that it is del var[:]
|
||||||
|
let ast::ExprName { id: name, .. } = subscript.value.as_name_expr()?;
|
||||||
|
|
||||||
|
// Let's find definition for var
|
||||||
|
let scope = semantic.current_scope();
|
||||||
|
let bindings: Vec<&Binding> = scope
|
||||||
|
.get_all(name)
|
||||||
|
.map(|binding_id| semantic.binding(binding_id))
|
||||||
|
.collect();
|
||||||
|
|
||||||
|
// NOTE: Maybe it is too strict of a limitation, but it seems reasonable.
|
||||||
|
let [binding] = bindings.as_slice() else {
|
||||||
|
return None;
|
||||||
|
};
|
||||||
|
|
||||||
|
// It should only apply to variables that are known to be lists or dicts.
|
||||||
|
if binding.source.is_none()
|
||||||
|
|| !(is_dict(binding, name, semantic) || is_list(binding, name, semantic))
|
||||||
|
{
|
||||||
|
return None;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Name is needed for the fix suggestion.
|
||||||
|
Some(name)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Make fix suggestion for the given name, ie `name.clear()`.
|
||||||
|
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())
|
||||||
|
}
|
||||||
|
|
@ -1,3 +1,6 @@
|
||||||
|
pub(crate) use delete_full_slice::*;
|
||||||
pub(crate) use repeated_append::*;
|
pub(crate) use repeated_append::*;
|
||||||
|
|
||||||
|
mod delete_full_slice;
|
||||||
|
|
||||||
mod repeated_append;
|
mod repeated_append;
|
||||||
|
|
|
||||||
|
|
@ -1,18 +1,17 @@
|
||||||
use rustc_hash::FxHashMap;
|
use rustc_hash::FxHashMap;
|
||||||
|
|
||||||
use ast::{traversal, ParameterWithDefault, Parameters};
|
use ast::traversal;
|
||||||
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
|
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
|
||||||
use ruff_macros::{derive_message_formats, violation};
|
use ruff_macros::{derive_message_formats, violation};
|
||||||
use ruff_python_ast::helpers::map_subscript;
|
|
||||||
use ruff_python_ast::{self as ast, Expr, Stmt};
|
use ruff_python_ast::{self as ast, Expr, Stmt};
|
||||||
use ruff_python_codegen::Generator;
|
use ruff_python_codegen::Generator;
|
||||||
use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType};
|
use ruff_python_semantic::{Binding, BindingId, DefinitionId, SemanticModel};
|
||||||
use ruff_python_semantic::{Binding, BindingId, BindingKind, DefinitionId, SemanticModel};
|
|
||||||
use ruff_text_size::{Ranged, TextRange};
|
use ruff_text_size::{Ranged, TextRange};
|
||||||
|
|
||||||
use crate::autofix::snippet::SourceCodeSnippet;
|
use crate::autofix::snippet::SourceCodeSnippet;
|
||||||
use crate::checkers::ast::Checker;
|
use crate::checkers::ast::Checker;
|
||||||
use crate::registry::AsRule;
|
use crate::registry::AsRule;
|
||||||
|
use crate::rules::refurb::helpers::is_list;
|
||||||
|
|
||||||
/// ## What it does
|
/// ## What it does
|
||||||
/// Checks for consecutive calls to `append`.
|
/// Checks for consecutive calls to `append`.
|
||||||
|
|
@ -255,6 +254,64 @@ fn get_or_add<'a, 'b>(
|
||||||
group
|
group
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Matches that the given statement is a call to `append` on a list variable.
|
||||||
|
fn match_append<'a>(semantic: &'a SemanticModel, stmt: &'a Stmt) -> Option<Append<'a>> {
|
||||||
|
let Stmt::Expr(ast::StmtExpr { value, .. }) = stmt else {
|
||||||
|
return None;
|
||||||
|
};
|
||||||
|
|
||||||
|
let Expr::Call(ast::ExprCall {
|
||||||
|
func, arguments, ..
|
||||||
|
}) = value.as_ref()
|
||||||
|
else {
|
||||||
|
return None;
|
||||||
|
};
|
||||||
|
|
||||||
|
// `append` should have just one argument, an element to be added.
|
||||||
|
let [argument] = arguments.args.as_slice() else {
|
||||||
|
return None;
|
||||||
|
};
|
||||||
|
|
||||||
|
// The called function should be an attribute, ie `value.attr`.
|
||||||
|
let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else {
|
||||||
|
return None;
|
||||||
|
};
|
||||||
|
|
||||||
|
// `attr` should be `append` and it shouldn't have any keyword arguments.
|
||||||
|
if attr != "append" || !arguments.keywords.is_empty() {
|
||||||
|
return None;
|
||||||
|
}
|
||||||
|
|
||||||
|
// We match only variable references, i.e. `value` should be a name expression.
|
||||||
|
let Expr::Name(receiver @ ast::ExprName { id: name, .. }) = value.as_ref() else {
|
||||||
|
return None;
|
||||||
|
};
|
||||||
|
|
||||||
|
// Now we need to find what is this variable bound to...
|
||||||
|
let scope = semantic.current_scope();
|
||||||
|
let bindings: Vec<BindingId> = scope.get_all(name).collect();
|
||||||
|
|
||||||
|
// Maybe it is too strict of a limitation, but it seems reasonable.
|
||||||
|
let [binding_id] = bindings.as_slice() else {
|
||||||
|
return None;
|
||||||
|
};
|
||||||
|
|
||||||
|
let binding = semantic.binding(*binding_id);
|
||||||
|
|
||||||
|
// ...and whether this something is a list.
|
||||||
|
if binding.source.is_none() || !is_list(binding, name, semantic) {
|
||||||
|
return None;
|
||||||
|
}
|
||||||
|
|
||||||
|
Some(Append {
|
||||||
|
receiver,
|
||||||
|
binding_id: *binding_id,
|
||||||
|
binding,
|
||||||
|
stmt,
|
||||||
|
argument,
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
/// Make fix suggestion for the given group of appends.
|
/// Make fix suggestion for the given group of appends.
|
||||||
fn make_suggestion(group: &AppendGroup, generator: Generator) -> String {
|
fn make_suggestion(group: &AppendGroup, generator: Generator) -> String {
|
||||||
let appends = &group.appends;
|
let appends = &group.appends;
|
||||||
|
|
@ -304,139 +361,3 @@ fn make_suggestion(group: &AppendGroup, generator: Generator) -> String {
|
||||||
};
|
};
|
||||||
generator.stmt(&stmt.into())
|
generator.stmt(&stmt.into())
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Matches that the given statement is a call to `append` on a list variable.
|
|
||||||
fn match_append<'a>(semantic: &'a SemanticModel, stmt: &'a Stmt) -> Option<Append<'a>> {
|
|
||||||
let Stmt::Expr(ast::StmtExpr { value, .. }) = stmt else {
|
|
||||||
return None;
|
|
||||||
};
|
|
||||||
|
|
||||||
let Expr::Call(ast::ExprCall {
|
|
||||||
func, arguments, ..
|
|
||||||
}) = value.as_ref()
|
|
||||||
else {
|
|
||||||
return None;
|
|
||||||
};
|
|
||||||
|
|
||||||
// `append` should have just one argument, an element to be added.
|
|
||||||
let [argument] = arguments.args.as_slice() else {
|
|
||||||
return None;
|
|
||||||
};
|
|
||||||
|
|
||||||
// The called function should be an attribute, ie `value.attr`.
|
|
||||||
let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else {
|
|
||||||
return None;
|
|
||||||
};
|
|
||||||
|
|
||||||
// `attr` should be `append` and it shouldn't have any keyword arguments.
|
|
||||||
if attr != "append" || !arguments.keywords.is_empty() {
|
|
||||||
return None;
|
|
||||||
}
|
|
||||||
|
|
||||||
// We match only variable references, i.e. `value` should be a name expression.
|
|
||||||
let Expr::Name(receiver @ ast::ExprName { id: name, .. }) = value.as_ref() else {
|
|
||||||
return None;
|
|
||||||
};
|
|
||||||
|
|
||||||
// Now we need to find what is this variable bound to...
|
|
||||||
let scope = semantic.current_scope();
|
|
||||||
let bindings: Vec<BindingId> = scope.get_all(name).collect();
|
|
||||||
|
|
||||||
// Maybe it is too strict of a limitation, but it seems reasonable.
|
|
||||||
let [binding_id] = bindings.as_slice() else {
|
|
||||||
return None;
|
|
||||||
};
|
|
||||||
|
|
||||||
let binding = semantic.binding(*binding_id);
|
|
||||||
|
|
||||||
// ...and whether this something is a list.
|
|
||||||
if binding.source.is_none() || !is_list(semantic, binding, name) {
|
|
||||||
return None;
|
|
||||||
}
|
|
||||||
|
|
||||||
Some(Append {
|
|
||||||
receiver,
|
|
||||||
binding_id: *binding_id,
|
|
||||||
binding,
|
|
||||||
stmt,
|
|
||||||
argument,
|
|
||||||
})
|
|
||||||
}
|
|
||||||
|
|
||||||
/// Test whether the given binding (and the given name) can be considered a list.
|
|
||||||
/// For this, we check what value might be associated with it through it's initialization and
|
|
||||||
/// what annotation it has (we consider `list` and `typing.List`).
|
|
||||||
///
|
|
||||||
/// NOTE: this function doesn't perform more serious type inference, so it won't be able
|
|
||||||
/// to understand if the value gets initialized from a call to a function always returning
|
|
||||||
/// lists. This also implies no interfile analysis.
|
|
||||||
fn is_list<'a>(semantic: &'a SemanticModel, binding: &'a Binding, name: &str) -> bool {
|
|
||||||
match binding.kind {
|
|
||||||
BindingKind::Assignment => match binding.statement(semantic) {
|
|
||||||
Some(Stmt::Assign(ast::StmtAssign { value, .. })) => {
|
|
||||||
let value_type: ResolvedPythonType = value.as_ref().into();
|
|
||||||
let ResolvedPythonType::Atom(candidate) = value_type else {
|
|
||||||
return false;
|
|
||||||
};
|
|
||||||
matches!(candidate, PythonType::List)
|
|
||||||
}
|
|
||||||
Some(Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. })) => {
|
|
||||||
is_list_annotation(semantic, annotation.as_ref())
|
|
||||||
}
|
|
||||||
_ => false,
|
|
||||||
},
|
|
||||||
BindingKind::Argument => match binding.statement(semantic) {
|
|
||||||
Some(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;
|
|
||||||
};
|
|
||||||
is_list_annotation(semantic, annotation.as_ref())
|
|
||||||
}
|
|
||||||
_ => false,
|
|
||||||
},
|
|
||||||
BindingKind::Annotation => match binding.statement(semantic) {
|
|
||||||
Some(Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. })) => {
|
|
||||||
is_list_annotation(semantic, annotation.as_ref())
|
|
||||||
}
|
|
||||||
_ => false,
|
|
||||||
},
|
|
||||||
_ => false,
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
#[inline]
|
|
||||||
fn is_list_annotation(semantic: &SemanticModel, annotation: &Expr) -> bool {
|
|
||||||
let value = map_subscript(annotation);
|
|
||||||
match_builtin_list_type(semantic, value) || semantic.match_typing_expr(value, "List")
|
|
||||||
}
|
|
||||||
|
|
||||||
#[inline]
|
|
||||||
fn match_builtin_list_type(semantic: &SemanticModel, type_expr: &Expr) -> bool {
|
|
||||||
let Expr::Name(ast::ExprName { id, .. }) = type_expr else {
|
|
||||||
return false;
|
|
||||||
};
|
|
||||||
id == "list" && semantic.is_builtin("list")
|
|
||||||
}
|
|
||||||
|
|
||||||
#[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)
|
|
||||||
}
|
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,132 @@
|
||||||
|
---
|
||||||
|
source: crates/ruff/src/rules/refurb/mod.rs
|
||||||
|
---
|
||||||
|
FURB131.py:11:1: FURB131 [*] Prefer `clear` over deleting a full slice
|
||||||
|
|
|
||||||
|
10 | # FURB131
|
||||||
|
11 | del nums[:]
|
||||||
|
| ^^^^^^^^^^^ FURB131
|
||||||
|
|
|
||||||
|
= help: Replace with `clear()`
|
||||||
|
|
||||||
|
ℹ Suggested fix
|
||||||
|
8 8 | # these should match
|
||||||
|
9 9 |
|
||||||
|
10 10 | # FURB131
|
||||||
|
11 |-del nums[:]
|
||||||
|
11 |+nums.clear()
|
||||||
|
12 12 |
|
||||||
|
13 13 |
|
||||||
|
14 14 | # FURB131
|
||||||
|
|
||||||
|
FURB131.py:15:1: FURB131 [*] Prefer `clear` over deleting a full slice
|
||||||
|
|
|
||||||
|
14 | # FURB131
|
||||||
|
15 | del names[:]
|
||||||
|
| ^^^^^^^^^^^^ FURB131
|
||||||
|
|
|
||||||
|
= help: Replace with `clear()`
|
||||||
|
|
||||||
|
ℹ Suggested fix
|
||||||
|
12 12 |
|
||||||
|
13 13 |
|
||||||
|
14 14 | # FURB131
|
||||||
|
15 |-del names[:]
|
||||||
|
15 |+names.clear()
|
||||||
|
16 16 |
|
||||||
|
17 17 |
|
||||||
|
18 18 | # FURB131
|
||||||
|
|
||||||
|
FURB131.py:19:1: FURB131 Prefer `clear` over deleting a full slice
|
||||||
|
|
|
||||||
|
18 | # FURB131
|
||||||
|
19 | del x, nums[:]
|
||||||
|
| ^^^^^^^^^^^^^^ FURB131
|
||||||
|
|
|
||||||
|
= help: Replace with `clear()`
|
||||||
|
|
||||||
|
FURB131.py:23:1: FURB131 Prefer `clear` over deleting a full slice
|
||||||
|
|
|
||||||
|
22 | # FURB131
|
||||||
|
23 | del y, names[:], x
|
||||||
|
| ^^^^^^^^^^^^^^^^^^ FURB131
|
||||||
|
|
|
||||||
|
= help: Replace with `clear()`
|
||||||
|
|
||||||
|
FURB131.py:28:5: FURB131 [*] Prefer `clear` over deleting a full slice
|
||||||
|
|
|
||||||
|
26 | def yes_one(x: list[int]):
|
||||||
|
27 | # FURB131
|
||||||
|
28 | del x[:]
|
||||||
|
| ^^^^^^^^ FURB131
|
||||||
|
|
|
||||||
|
= help: Replace with `clear()`
|
||||||
|
|
||||||
|
ℹ Suggested fix
|
||||||
|
25 25 |
|
||||||
|
26 26 | def yes_one(x: list[int]):
|
||||||
|
27 27 | # FURB131
|
||||||
|
28 |- del x[:]
|
||||||
|
28 |+ x.clear()
|
||||||
|
29 29 |
|
||||||
|
30 30 |
|
||||||
|
31 31 | def yes_two(x: dict[int, str]):
|
||||||
|
|
||||||
|
FURB131.py:33:5: FURB131 [*] Prefer `clear` over deleting a full slice
|
||||||
|
|
|
||||||
|
31 | def yes_two(x: dict[int, str]):
|
||||||
|
32 | # FURB131
|
||||||
|
33 | del x[:]
|
||||||
|
| ^^^^^^^^ FURB131
|
||||||
|
|
|
||||||
|
= help: Replace with `clear()`
|
||||||
|
|
||||||
|
ℹ Suggested fix
|
||||||
|
30 30 |
|
||||||
|
31 31 | def yes_two(x: dict[int, str]):
|
||||||
|
32 32 | # FURB131
|
||||||
|
33 |- del x[:]
|
||||||
|
33 |+ x.clear()
|
||||||
|
34 34 |
|
||||||
|
35 35 |
|
||||||
|
36 36 | def yes_three(x: List[int]):
|
||||||
|
|
||||||
|
FURB131.py:38:5: FURB131 [*] Prefer `clear` over deleting a full slice
|
||||||
|
|
|
||||||
|
36 | def yes_three(x: List[int]):
|
||||||
|
37 | # FURB131
|
||||||
|
38 | del x[:]
|
||||||
|
| ^^^^^^^^ FURB131
|
||||||
|
|
|
||||||
|
= help: Replace with `clear()`
|
||||||
|
|
||||||
|
ℹ Suggested fix
|
||||||
|
35 35 |
|
||||||
|
36 36 | def yes_three(x: List[int]):
|
||||||
|
37 37 | # FURB131
|
||||||
|
38 |- del x[:]
|
||||||
|
38 |+ x.clear()
|
||||||
|
39 39 |
|
||||||
|
40 40 |
|
||||||
|
41 41 | def yes_four(x: Dict[int, str]):
|
||||||
|
|
||||||
|
FURB131.py:43:5: FURB131 [*] Prefer `clear` over deleting a full slice
|
||||||
|
|
|
||||||
|
41 | def yes_four(x: Dict[int, str]):
|
||||||
|
42 | # FURB131
|
||||||
|
43 | del x[:]
|
||||||
|
| ^^^^^^^^ FURB131
|
||||||
|
|
|
||||||
|
= help: Replace with `clear()`
|
||||||
|
|
||||||
|
ℹ Suggested fix
|
||||||
|
40 40 |
|
||||||
|
41 41 | def yes_four(x: Dict[int, str]):
|
||||||
|
42 42 | # FURB131
|
||||||
|
43 |- del x[:]
|
||||||
|
43 |+ x.clear()
|
||||||
|
44 44 |
|
||||||
|
45 45 |
|
||||||
|
46 46 | # these should not
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -2045,6 +2045,7 @@
|
||||||
"FLY002",
|
"FLY002",
|
||||||
"FURB",
|
"FURB",
|
||||||
"FURB113",
|
"FURB113",
|
||||||
|
"FURB131",
|
||||||
"G",
|
"G",
|
||||||
"G0",
|
"G0",
|
||||||
"G00",
|
"G00",
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue