Skip to content

Commit

Permalink
[refurb] Implement delete-full-slice rule (FURB131) (#6897)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
SavchenkoValeriy committed Aug 29, 2023
1 parent 3200015 commit c448b40
Show file tree
Hide file tree
Showing 10 changed files with 597 additions and 130 deletions.
64 changes: 64 additions & 0 deletions crates/ruff/resources/test/fixtures/refurb/FURB131.py
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[:]
5 changes: 4 additions & 1 deletion crates/ruff/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1459,14 +1459,17 @@ 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) {
for target in targets {
if let Expr::Name(ast::ExprName { id, .. }) = target {
pylint::rules::global_statement(checker, id);
}
}
}
if checker.enabled(Rule::DeleteFullSlice) {
refurb::rules::delete_full_slice(checker, delete);
}
}
Stmt::Expr(ast::StmtExpr { value, range: _ }) => {
if checker.enabled(Rule::UselessComparison) {
Expand Down
3 changes: 1 addition & 2 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -867,7 +867,6 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {

// refurb
(Refurb, "113") => (RuleGroup::Nursery, rules::refurb::rules::RepeatedAppend),

_ => return None,
(Refurb, "131") => (RuleGroup::Nursery, rules::refurb::rules::DeleteFullSlice),
})
}
184 changes: 184 additions & 0 deletions crates/ruff/src/rules/refurb/helpers.rs
Original file line number Diff line number Diff line change
@@ -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(&parameters.args, name)
.or_else(|| find_parameter_by_name_impl(&parameters.posonlyargs, name))
.or_else(|| find_parameter_by_name_impl(&parameters.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)
}
3 changes: 3 additions & 0 deletions crates/ruff/src/rules/refurb/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
//! Rules from [refurb](https://pypi.org/project/refurb/)/

mod helpers;
pub(crate) mod rules;

#[cfg(test)]
Expand All @@ -13,6 +15,7 @@ mod tests {
use crate::{assert_messages, settings};

#[test_case(Rule::RepeatedAppend, Path::new("FURB113.py"))]
#[test_case(Rule::DeleteFullSlice, Path::new("FURB131.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
Loading

0 comments on commit c448b40

Please sign in to comment.