From b3879212c69e3aaf8fc3f32f1825c0fc032ce86f Mon Sep 17 00:00:00 2001 From: Valeriy Savchenko Date: Sun, 20 Aug 2023 13:32:00 +0100 Subject: [PATCH 1/6] [refurb] Implement `repeated-append` rule (`FURB113`) --- .../resources/test/fixtures/refurb/FURB113.py | 161 +++++++ .../src/checkers/ast/analyze/statement.rs | 5 +- crates/ruff/src/codes.rs | 3 + crates/ruff/src/registry.rs | 3 + crates/ruff/src/rules/mod.rs | 1 + crates/ruff/src/rules/refurb/mod.rs | 25 + crates/ruff/src/rules/refurb/rules/mod.rs | 3 + .../src/rules/refurb/rules/repeated_append.rs | 452 ++++++++++++++++++ ...es__refurb__tests__FURB113_FURB113.py.snap | 313 ++++++++++++ crates/ruff_python_ast/src/traversal.rs | 1 + ruff.schema.json | 4 + 11 files changed, 970 insertions(+), 1 deletion(-) create mode 100644 crates/ruff/resources/test/fixtures/refurb/FURB113.py create mode 100644 crates/ruff/src/rules/refurb/mod.rs create mode 100644 crates/ruff/src/rules/refurb/rules/mod.rs create mode 100644 crates/ruff/src/rules/refurb/rules/repeated_append.rs create mode 100644 crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap diff --git a/crates/ruff/resources/test/fixtures/refurb/FURB113.py b/crates/ruff/resources/test/fixtures/refurb/FURB113.py new file mode 100644 index 0000000000000..1377215045f1e --- /dev/null +++ b/crates/ruff/resources/test/fixtures/refurb/FURB113.py @@ -0,0 +1,161 @@ +from typing import List + + +def foo(): + pass + +nums = [] +nums2 = [] +nums3: list[int] = foo() +nums4: List[int] + +class C: + def append(self, x): + pass + + +# these will match + + +# FURB113 +nums.append(1) +nums.append(2) +pass + + +# FURB113 +nums3.append(1) +nums3.append(2) +pass + + +# FURB113 +nums4.append(1) +nums4.append(2) +pass + + +# FURB113 +nums.append(1) +nums2.append(1) +nums.append(2) +nums.append(3) +pass + + +# FURB113 +nums.append(1) +nums2.append(1) +nums.append(2) +# FURB113 +nums3.append(1) +nums.append(3) +# FURB113 +nums4.append(1) +nums4.append(2) +nums3.append(2) +pass + +# FURB113 +nums.append(1) +nums.append(2) +nums.append(3) + + +if True: + # FURB113 + nums.append(1) + nums.append(2) + + +if True: + # FURB113 + nums.append(1) + nums.append(2) + pass + + +if True: + # FURB113 + nums.append(1) + nums2.append(1) + nums.append(2) + nums.append(3) + + +def yes_one(x: list[int]): + # FURB113 + x.append(1) + x.append(2) + + +def yes_two(x: List[int]): + # FURB113 + x.append(1) + x.append(2) + + +def yes_three(*, x: list[int]): + # FURB113 + x.append(1) + x.append(2) + + +def yes_four(x: list[int], /): + # FURB113 + x.append(1) + x.append(2) + + +def yes_five(x: list[int], y: list[int]): + # FURB113 + x.append(1) + x.append(2) + y.append(1) + x.append(3) + + +# these will not + +nums.append(1) +pass +nums.append(2) + + +if True: + nums.append(1) + pass + nums.append(2) + + +nums.append(1) +pass + + +nums.append(1) +nums2.append(2) + + +nums.copy() +nums.copy() + + +c = C() +c.append(1) +c.append(2) + + +def not_one(x): + x.append(1) + x.append(2) + + +def not_two(x: C): + x.append(1) + x.append(2) + + +# redefining a list variable with a new type shouldn't confuse ruff +nums2 = C() +nums2.append(1) +nums2.append(2) diff --git a/crates/ruff/src/checkers/ast/analyze/statement.rs b/crates/ruff/src/checkers/ast/analyze/statement.rs index 892653835c37e..da2211c62cca3 100644 --- a/crates/ruff/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff/src/checkers/ast/analyze/statement.rs @@ -14,7 +14,7 @@ use crate::rules::{ flake8_django, flake8_errmsg, flake8_import_conventions, flake8_pie, flake8_pyi, flake8_pytest_style, flake8_raise, flake8_return, flake8_simplify, flake8_slots, flake8_tidy_imports, flake8_type_checking, mccabe, pandas_vet, pep8_naming, perflint, - pycodestyle, pyflakes, pygrep_hooks, pylint, pyupgrade, ruff, tryceratops, + pycodestyle, pyflakes, pygrep_hooks, pylint, pyupgrade, refurb, ruff, tryceratops, }; use crate::settings::types::PythonVersion; @@ -1485,6 +1485,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::AsyncioDanglingTask) { ruff::rules::asyncio_dangling_task(checker, value); } + if checker.enabled(Rule::RepeatedAppend) { + refurb::rules::repeated_append(checker, stmt); + } } _ => {} } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index db1ab2831c04b..d7e61976842ce 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -865,6 +865,9 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Slots, "001") => (RuleGroup::Unspecified, rules::flake8_slots::rules::NoSlotsInTupleSubclass), (Flake8Slots, "002") => (RuleGroup::Unspecified, rules::flake8_slots::rules::NoSlotsInNamedtupleSubclass), + // refurb + (Refurb, "113") => (RuleGroup::Unspecified, rules::refurb::rules::RepeatedAppend), + _ => return None, }) } diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index dcede096b1482..23b54dc9b25fd 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -196,6 +196,9 @@ pub enum Linter { /// [Perflint](https://pypi.org/project/perflint/) #[prefix = "PERF"] Perflint, + /// [refurb](https://pypi.org/project/refurb/) + #[prefix = "FURB"] + Refurb, /// Ruff-specific rules #[prefix = "RUF"] Ruff, diff --git a/crates/ruff/src/rules/mod.rs b/crates/ruff/src/rules/mod.rs index e8fd430cb0212..225acb00e9e77 100644 --- a/crates/ruff/src/rules/mod.rs +++ b/crates/ruff/src/rules/mod.rs @@ -52,5 +52,6 @@ pub mod pyflakes; pub mod pygrep_hooks; pub mod pylint; pub mod pyupgrade; +pub mod refurb; pub mod ruff; pub mod tryceratops; diff --git a/crates/ruff/src/rules/refurb/mod.rs b/crates/ruff/src/rules/refurb/mod.rs new file mode 100644 index 0000000000000..0a3584bc5f331 --- /dev/null +++ b/crates/ruff/src/rules/refurb/mod.rs @@ -0,0 +1,25 @@ +//! Rules from [refurb](https://pypi.org/project/refurb/)/ +pub(crate) mod rules; + +#[cfg(test)] +mod tests { + use std::path::Path; + + use anyhow::Result; + use test_case::test_case; + + use crate::registry::Rule; + use crate::test::test_path; + use crate::{assert_messages, settings}; + + #[test_case(Rule::RepeatedAppend, Path::new("FURB113.py"))] + fn rules(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); + let diagnostics = test_path( + Path::new("refurb").join(path).as_path(), + &settings::Settings::for_rule(rule_code), + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } +} diff --git a/crates/ruff/src/rules/refurb/rules/mod.rs b/crates/ruff/src/rules/refurb/rules/mod.rs new file mode 100644 index 0000000000000..b4b2317a3c838 --- /dev/null +++ b/crates/ruff/src/rules/refurb/rules/mod.rs @@ -0,0 +1,3 @@ +pub(crate) use repeated_append::*; + +mod repeated_append; diff --git a/crates/ruff/src/rules/refurb/rules/repeated_append.rs b/crates/ruff/src/rules/refurb/rules/repeated_append.rs new file mode 100644 index 0000000000000..9c4a410823720 --- /dev/null +++ b/crates/ruff/src/rules/refurb/rules/repeated_append.rs @@ -0,0 +1,452 @@ +use std::collections::HashMap; + +use ast::{traversal, ParameterWithDefault, Parameters, Ranged}; +use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Expr, ExprName, Stmt}; +use ruff_python_codegen::Generator; +use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType}; +use ruff_python_semantic::{ + Binding, BindingId, BindingKind, Definition, DefinitionId, SemanticModel, +}; +use ruff_text_size::TextRange; + +use crate::autofix::snippet::SourceCodeSnippet; +use crate::checkers::ast::Checker; +use crate::registry::AsRule; + +/// ## What it does +/// Checks for consecutive calls to `append`. +/// +/// ## Why is this bad? +/// Consecutive calls to `append` can be less efficient than using `extend` because each +/// `append` re-sizes the list individually, whereas `extend` re-sizes it once for all elements. +/// +/// ## Example +/// ```python +/// nums = [1, 2, 3] +/// +/// nums.append(4) +/// nums.append(5) +/// nums.append(6) +/// ``` +/// +/// Use instead: +/// ```python +/// nums = [1, 2, 3] +/// +/// nums.extend((4, 5, 6)) +/// ``` +/// +/// ## References +/// - [Python documentation: More on Lists](https://docs.python.org/3/tutorial/datastructures.html#more-on-lists) +#[violation] +pub struct RepeatedAppend { + name: String, + replacement: SourceCodeSnippet, +} + +impl RepeatedAppend { + fn suggestion(&self) -> String { + let name = &self.name; + self.replacement.full_display().map_or( + format!("{name}.extend(...)"), + std::string::ToString::to_string, + ) + } +} + +impl Violation for RepeatedAppend { + const AUTOFIX: AutofixKind = AutofixKind::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + let name = &self.name; + let suggestion = self.suggestion(); + format!("Use `{suggestion}` instead of repeatedly calling `{name}.append()`") + } + + fn autofix_title(&self) -> Option { + let suggestion = self.suggestion(); + Some(format!("Replace with `{suggestion}`")) + } +} + +pub(crate) fn repeated_append(checker: &mut Checker, stmt: &Stmt) { + let appends = match_consecutive_appends(checker.semantic(), stmt); + + // No need to proceed if we have less than 1 append to work with. + if appends.len() <= 1 { + return; + } + + let groups = group_appends(&appends); + + // group borrows from checker, so we can't directly push into checker.diagnostics + let mut diagnostics: Vec = vec![]; + diagnostics.reserve_exact(groups.len()); + + for group in groups { + // Groups with just one element are fine, and shouldn't be replaced by `extend`. + if group.appends.len() <= 1 { + continue; + } + + let replacement = make_suggestion(&group, checker.generator()); + + let mut diagnostic = Diagnostic::new( + RepeatedAppend { + name: group.name().to_string(), + replacement: SourceCodeSnippet::new(replacement.to_string()), + }, + group.range(), + ); + + // We only suggest fix when all appends in a group are clumped together. + if checker.patch(diagnostic.kind.rule()) && group.is_consecutive { + diagnostic.set_fix(Fix::suggested(Edit::replacement( + replacement, + group.start(), + group.end(), + ))); + } + + diagnostics.push(diagnostic); + } + checker.diagnostics.extend(diagnostics); +} + +#[derive(Debug, Clone)] +struct Append<'a> { + /// Receiver of the `append` call (aka `self` argument). + receiver: &'a ExprName, + /// [`BindingId`] that the receiver references. + binding_id: BindingId, + /// [`Binding`] that the receiver references. + binding: &'a Binding<'a>, + /// [`Expr`] serving as a sole argument to `append`. + argument: &'a Expr, + /// The statement containing the `append` call. + stmt: &'a Stmt, +} + +#[derive(Debug)] +struct AppendGroup<'a> { + /// A sequence of `appends` connected to the same binding. + appends: Vec>, + /// `true` when all appends in the group follow one another and don't have other statements in + /// between. It is much easier to make fix suggestions for consecutive groups. + is_consecutive: bool, +} + +impl<'a> AppendGroup<'a> { + fn name(&self) -> &'a str { + assert!(!self.appends.is_empty()); + &self.appends.first().unwrap().receiver.id + } +} + +impl<'a> Ranged for AppendGroup<'a> { + fn range(&self) -> TextRange { + assert!(!self.appends.is_empty()); + TextRange::new( + self.appends.first().unwrap().stmt.start(), + self.appends.last().unwrap().stmt.end(), + ) + } +} + +/// Match consecutive calls to `append` on list variables starting from the given statement. +fn match_consecutive_appends<'a>(semantic: &'a SemanticModel, stmt: &'a Stmt) -> Vec> { + // In order to match consecutive statements, we need to go to the tree ancestor of the + // given statement, find its position there, and match all 'appends' from there. + let empty: Vec> = vec![]; + + let siblings: &'a [Stmt] = if semantic.at_top_level() { + // If the statement is at the top level, we should go to the parent module. + // Module is available in the definitions list. + let Definition::Module(ref module_def) = semantic.definitions[DefinitionId::module()] + else { + return empty; + }; + module_def.python_ast + } else { + // Otherwise, go to the parent, and take its body as a sequence of siblings. + let Some(suite) = semantic + .current_statement_parent() + .and_then(|parent| traversal::suite(stmt, parent)) + else { + return empty; + }; + suite + }; + + let Some(stmt_index) = siblings.iter().position(|x| x == stmt) else { + return empty; + }; + + // Match the current statement, to see if it's an append. + let Some(append) = match_append(semantic, stmt) else { + return empty; + }; + + // We shouldn't repeat the same work for many 'appends' that go in a row. Let's check + // that this statement is at the beginning of such a group. + if stmt_index != 0 && match_append(semantic, &siblings[stmt_index - 1]).is_some() { + return empty; + } + + // Starting from the next statement, let's match all appends and make a vector. + std::iter::once(append) + .chain( + siblings + .iter() + // Skip the current statement, since we already tested it... + .skip(stmt_index + 1) + .map_while(|next| match_append(semantic, next)), + ) + .collect() +} + +/// Group the given appends by the associated bindings. +fn group_appends<'a>(appends: &[Append<'a>]) -> Vec> { + // We want to go over the given list of appends and group the by receivers. + let mut map: HashMap> = HashMap::new(); + let mut iter = appends.iter(); + let mut last_binding = { + let first_append = iter.next().unwrap(); + let _ = get_or_add(&mut map, first_append); + first_append.binding_id + }; + + for append in iter { + let group = get_or_add(&mut map, append); + if append.binding_id != last_binding { + // If the group is not brand new, and the previous group was different, + // we should mark it as "non-consecutive". + // + // We are catching the following situation: + // ```python + // a.append(1) + // a.append(2) + // b.append(1) + // a.append(3) # <- we are currently here + // ``` + // + // So, `a` != `b` and group for `a` already contains appends 1 and 2. + // It is only possible if this group got interrupted by at least one + // other group and, thus, it is non-consecutive. + if group.appends.len() > 1 { + group.is_consecutive = false; + } + + last_binding = append.binding_id; + } + } + + map.into_values().collect() +} + +#[inline] +fn get_or_add<'a, 'b>( + map: &'b mut HashMap>, + append: &Append<'a>, +) -> &'b mut AppendGroup<'a> { + let group = map.entry(append.binding_id).or_insert(AppendGroup { + appends: vec![], + is_consecutive: true, + }); + group.appends.push(append.clone()); + group +} + +/// Make fix suggestion for the given group of appends. +fn make_suggestion(group: &AppendGroup, generator: Generator) -> String { + let appends = &group.appends; + + assert!(!appends.is_empty()); + let first = appends.first().unwrap(); + + assert!(appends + .iter() + .all(|append| append.binding.source == first.binding.source)); + + // Here we construct `var.extend((elt1, elt2, ..., eltN)) + // + // Each eltK comes from an individual `var.append(eltK)`. + let elts: Vec = appends + .iter() + .map(|append| append.argument.clone()) + .collect(); + // Join all elements into a tuple: `(elt1, elt2, ..., eltN)` + let tuple = ast::ExprTuple { + elts, + ctx: ast::ExprContext::Load, + range: TextRange::default(), + }; + // Make `var.extend`. + // NOTE: receiver is the same for all appends and that's why we can take the first. + let attr = ast::ExprAttribute { + value: Box::new(first.receiver.clone().into()), + attr: ast::Identifier::new("extend".to_string(), TextRange::default()), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + }; + // Make the actual call `var.extend((elt1, elt2, ..., eltN))` + let call = ast::ExprCall { + func: Box::new(attr.into()), + arguments: ast::Arguments { + args: vec![tuple.into()], + 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()) +} + +/// 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> { + 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 = 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 { + assert!(binding.source.is_some()); + let stmt = 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; + }; + matches!(candidate, PythonType::List) + } + Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. }) => { + is_list_annotation(semantic, 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; + }; + is_list_annotation(semantic, annotation.as_ref()) + } + _ => false, + }, + BindingKind::Annotation => match stmt { + 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 Expr::Subscript(ast::ExprSubscript { value, .. }) = annotation else { + return false; + }; + 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) +} diff --git a/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap new file mode 100644 index 0000000000000..1518a6ed261a5 --- /dev/null +++ b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap @@ -0,0 +1,313 @@ +--- +source: crates/ruff/src/rules/refurb/mod.rs +--- +FURB113.py:21:1: FURB113 [*] Use `nums.extend((1, 2))` instead of repeatedly calling `nums.append()` + | +20 | # FURB113 +21 | / nums.append(1) +22 | | nums.append(2) + | |______________^ FURB113 +23 | pass + | + = help: Replace with `nums.extend((1, 2))` + +ℹ Suggested fix +18 18 | +19 19 | +20 20 | # FURB113 +21 |-nums.append(1) +22 |-nums.append(2) + 21 |+nums.extend((1, 2)) +23 22 | pass +24 23 | +25 24 | + +FURB113.py:27:1: FURB113 [*] Use `nums3.extend((1, 2))` instead of repeatedly calling `nums3.append()` + | +26 | # FURB113 +27 | / nums3.append(1) +28 | | nums3.append(2) + | |_______________^ FURB113 +29 | pass + | + = help: Replace with `nums3.extend((1, 2))` + +ℹ Suggested fix +24 24 | +25 25 | +26 26 | # FURB113 +27 |-nums3.append(1) +28 |-nums3.append(2) + 27 |+nums3.extend((1, 2)) +29 28 | pass +30 29 | +31 30 | + +FURB113.py:33:1: FURB113 [*] Use `nums4.extend((1, 2))` instead of repeatedly calling `nums4.append()` + | +32 | # FURB113 +33 | / nums4.append(1) +34 | | nums4.append(2) + | |_______________^ FURB113 +35 | pass + | + = help: Replace with `nums4.extend((1, 2))` + +ℹ Suggested fix +30 30 | +31 31 | +32 32 | # FURB113 +33 |-nums4.append(1) +34 |-nums4.append(2) + 33 |+nums4.extend((1, 2)) +35 34 | pass +36 35 | +37 36 | + +FURB113.py:39:1: FURB113 Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` + | +38 | # FURB113 +39 | / nums.append(1) +40 | | nums2.append(1) +41 | | nums.append(2) +42 | | nums.append(3) + | |______________^ FURB113 +43 | pass + | + = help: Replace with `nums.extend((1, 2, 3))` + +FURB113.py:47:1: FURB113 Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` + | +46 | # FURB113 +47 | / nums.append(1) +48 | | nums2.append(1) +49 | | nums.append(2) +50 | | # FURB113 +51 | | nums3.append(1) +52 | | nums.append(3) + | |______________^ FURB113 +53 | # FURB113 +54 | nums4.append(1) + | + = help: Replace with `nums.extend((1, 2, 3))` + +FURB113.py:51:1: FURB113 Use `nums3.extend((1, 2))` instead of repeatedly calling `nums3.append()` + | +49 | nums.append(2) +50 | # FURB113 +51 | / nums3.append(1) +52 | | nums.append(3) +53 | | # FURB113 +54 | | nums4.append(1) +55 | | nums4.append(2) +56 | | nums3.append(2) + | |_______________^ FURB113 +57 | pass + | + = help: Replace with `nums3.extend((1, 2))` + +FURB113.py:54:1: FURB113 [*] Use `nums4.extend((1, 2))` instead of repeatedly calling `nums4.append()` + | +52 | nums.append(3) +53 | # FURB113 +54 | / nums4.append(1) +55 | | nums4.append(2) + | |_______________^ FURB113 +56 | nums3.append(2) +57 | pass + | + = help: Replace with `nums4.extend((1, 2))` + +ℹ Suggested fix +51 51 | nums3.append(1) +52 52 | nums.append(3) +53 53 | # FURB113 +54 |-nums4.append(1) +55 |-nums4.append(2) + 54 |+nums4.extend((1, 2)) +56 55 | nums3.append(2) +57 56 | pass +58 57 | + +FURB113.py:60:1: FURB113 [*] Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` + | +59 | # FURB113 +60 | / nums.append(1) +61 | | nums.append(2) +62 | | nums.append(3) + | |______________^ FURB113 + | + = help: Replace with `nums.extend((1, 2, 3))` + +ℹ Suggested fix +57 57 | pass +58 58 | +59 59 | # FURB113 +60 |-nums.append(1) +61 |-nums.append(2) +62 |-nums.append(3) + 60 |+nums.extend((1, 2, 3)) +63 61 | +64 62 | +65 63 | if True: + +FURB113.py:67:5: FURB113 [*] Use `nums.extend((1, 2))` instead of repeatedly calling `nums.append()` + | +65 | if True: +66 | # FURB113 +67 | nums.append(1) + | _____^ +68 | | nums.append(2) + | |__________________^ FURB113 + | + = help: Replace with `nums.extend((1, 2))` + +ℹ Suggested fix +64 64 | +65 65 | if True: +66 66 | # FURB113 +67 |- nums.append(1) +68 |- nums.append(2) + 67 |+ nums.extend((1, 2)) +69 68 | +70 69 | +71 70 | if True: + +FURB113.py:73:5: FURB113 [*] Use `nums.extend((1, 2))` instead of repeatedly calling `nums.append()` + | +71 | if True: +72 | # FURB113 +73 | nums.append(1) + | _____^ +74 | | nums.append(2) + | |__________________^ FURB113 +75 | pass + | + = help: Replace with `nums.extend((1, 2))` + +ℹ Suggested fix +70 70 | +71 71 | if True: +72 72 | # FURB113 +73 |- nums.append(1) +74 |- nums.append(2) + 73 |+ nums.extend((1, 2)) +75 74 | pass +76 75 | +77 76 | + +FURB113.py:80:5: FURB113 Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` + | +78 | if True: +79 | # FURB113 +80 | nums.append(1) + | _____^ +81 | | nums2.append(1) +82 | | nums.append(2) +83 | | nums.append(3) + | |__________________^ FURB113 + | + = help: Replace with `nums.extend((1, 2, 3))` + +FURB113.py:88:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` + | +86 | def yes_one(x: list[int]): +87 | # FURB113 +88 | x.append(1) + | _____^ +89 | | x.append(2) + | |_______________^ FURB113 + | + = help: Replace with `x.extend((1, 2))` + +ℹ Suggested fix +85 85 | +86 86 | def yes_one(x: list[int]): +87 87 | # FURB113 +88 |- x.append(1) +89 |- x.append(2) + 88 |+ x.extend((1, 2)) +90 89 | +91 90 | +92 91 | def yes_two(x: List[int]): + +FURB113.py:94:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` + | +92 | def yes_two(x: List[int]): +93 | # FURB113 +94 | x.append(1) + | _____^ +95 | | x.append(2) + | |_______________^ FURB113 + | + = help: Replace with `x.extend((1, 2))` + +ℹ Suggested fix +91 91 | +92 92 | def yes_two(x: List[int]): +93 93 | # FURB113 +94 |- x.append(1) +95 |- x.append(2) + 94 |+ x.extend((1, 2)) +96 95 | +97 96 | +98 97 | def yes_three(*, x: list[int]): + +FURB113.py:100:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` + | + 98 | def yes_three(*, x: list[int]): + 99 | # FURB113 +100 | x.append(1) + | _____^ +101 | | x.append(2) + | |_______________^ FURB113 + | + = help: Replace with `x.extend((1, 2))` + +ℹ Suggested fix +97 97 | +98 98 | def yes_three(*, x: list[int]): +99 99 | # FURB113 +100 |- x.append(1) +101 |- x.append(2) + 100 |+ x.extend((1, 2)) +102 101 | +103 102 | +104 103 | def yes_four(x: list[int], /): + +FURB113.py:106:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` + | +104 | def yes_four(x: list[int], /): +105 | # FURB113 +106 | x.append(1) + | _____^ +107 | | x.append(2) + | |_______________^ FURB113 + | + = help: Replace with `x.extend((1, 2))` + +ℹ Suggested fix +103 103 | +104 104 | def yes_four(x: list[int], /): +105 105 | # FURB113 +106 |- x.append(1) +107 |- x.append(2) + 106 |+ x.extend((1, 2)) +108 107 | +109 108 | +110 109 | def yes_five(x: list[int], y: list[int]): + +FURB113.py:112:5: FURB113 Use `x.extend((1, 2, 3))` instead of repeatedly calling `x.append()` + | +110 | def yes_five(x: list[int], y: list[int]): +111 | # FURB113 +112 | x.append(1) + | _____^ +113 | | x.append(2) +114 | | y.append(1) +115 | | x.append(3) + | |_______________^ FURB113 + | + = help: Replace with `x.extend((1, 2, 3))` + + diff --git a/crates/ruff_python_ast/src/traversal.rs b/crates/ruff_python_ast/src/traversal.rs index d89e29484b2ae..1e050cfa94b1e 100644 --- a/crates/ruff_python_ast/src/traversal.rs +++ b/crates/ruff_python_ast/src/traversal.rs @@ -3,6 +3,7 @@ use crate::{self as ast, ExceptHandler, Stmt, Suite}; /// Given a [`Stmt`] and its parent, return the [`Suite`] that contains the [`Stmt`]. pub fn suite<'a>(stmt: &'a Stmt, parent: &'a Stmt) -> Option<&'a Suite> { + // TODO: refactor this to work without a parent, ie when `stmt` is at the top level match parent { Stmt::FunctionDef(ast::StmtFunctionDef { body, .. }) => Some(body), Stmt::ClassDef(ast::StmtClassDef { body, .. }) => Some(body), diff --git a/ruff.schema.json b/ruff.schema.json index beb32b04ede19..53f8fdb187f62 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2043,6 +2043,10 @@ "FLY0", "FLY00", "FLY002", + "FURB", + "FURB1", + "FURB11", + "FURB113", "G", "G0", "G00", From 6ac30c6a9495de88fd3a75801c6b12785d5de0ec Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 28 Aug 2023 18:08:40 -0400 Subject: [PATCH 2/6] Minor tweaks --- .../resources/test/fixtures/refurb/FURB113.py | 12 +- .../src/rules/refurb/rules/repeated_append.rs | 181 ++++---- ...es__refurb__tests__FURB113_FURB113.py.snap | 398 +++++++++--------- crates/ruff_python_semantic/src/definition.rs | 2 +- 4 files changed, 295 insertions(+), 298 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/refurb/FURB113.py b/crates/ruff/resources/test/fixtures/refurb/FURB113.py index 1377215045f1e..f89382f5854b4 100644 --- a/crates/ruff/resources/test/fixtures/refurb/FURB113.py +++ b/crates/ruff/resources/test/fixtures/refurb/FURB113.py @@ -1,20 +1,22 @@ from typing import List -def foo(): +def func(): pass + nums = [] nums2 = [] -nums3: list[int] = foo() +nums3: list[int] = func() nums4: List[int] + class C: def append(self, x): pass -# these will match +# Errors. # FURB113 @@ -115,7 +117,7 @@ def yes_five(x: list[int], y: list[int]): x.append(3) -# these will not +# Non-errors. nums.append(1) pass @@ -155,7 +157,7 @@ def not_two(x: C): x.append(2) -# redefining a list variable with a new type shouldn't confuse ruff +# redefining a list variable with a new type shouldn't confuse ruff. nums2 = C() nums2.append(1) nums2.append(2) diff --git a/crates/ruff/src/rules/refurb/rules/repeated_append.rs b/crates/ruff/src/rules/refurb/rules/repeated_append.rs index 9c4a410823720..8e2a459e32ac3 100644 --- a/crates/ruff/src/rules/refurb/rules/repeated_append.rs +++ b/crates/ruff/src/rules/refurb/rules/repeated_append.rs @@ -1,15 +1,13 @@ -use std::collections::HashMap; +use rustc_hash::FxHashMap; -use ast::{traversal, ParameterWithDefault, Parameters, Ranged}; +use ast::{traversal, ParameterWithDefault, Parameters}; use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{self as ast, Expr, ExprName, Stmt}; +use ruff_python_ast::{self as ast, Expr, Stmt}; use ruff_python_codegen::Generator; use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType}; -use ruff_python_semantic::{ - Binding, BindingId, BindingKind, Definition, DefinitionId, SemanticModel, -}; -use ruff_text_size::TextRange; +use ruff_python_semantic::{Binding, BindingId, BindingKind, DefinitionId, SemanticModel}; +use ruff_text_size::{Ranged, TextRange}; use crate::autofix::snippet::SourceCodeSnippet; use crate::checkers::ast::Checker; @@ -19,8 +17,9 @@ use crate::registry::AsRule; /// Checks for consecutive calls to `append`. /// /// ## Why is this bad? -/// Consecutive calls to `append` can be less efficient than using `extend` because each -/// `append` re-sizes the list individually, whereas `extend` re-sizes it once for all elements. +/// Consecutive calls to `append` can be less efficient than batching them into +/// a single `extend`. Each `append` resizes the list individually, whereas an +/// `extend` can resize the list once for all elements. /// /// ## Example /// ```python @@ -49,10 +48,9 @@ pub struct RepeatedAppend { impl RepeatedAppend { fn suggestion(&self) -> String { let name = &self.name; - self.replacement.full_display().map_or( - format!("{name}.extend(...)"), - std::string::ToString::to_string, - ) + self.replacement + .full_display() + .map_or(format!("{name}.extend(...)"), ToString::to_string) } } @@ -72,54 +70,57 @@ impl Violation for RepeatedAppend { } } +/// FURB113 pub(crate) fn repeated_append(checker: &mut Checker, stmt: &Stmt) { - let appends = match_consecutive_appends(checker.semantic(), stmt); + let Some(appends) = match_consecutive_appends(checker.semantic(), stmt) else { + return; + }; - // No need to proceed if we have less than 1 append to work with. + // No need to proceed if we have less than 1 `append` to work with. if appends.len() <= 1 { return; } - let groups = group_appends(&appends); - // group borrows from checker, so we can't directly push into checker.diagnostics - let mut diagnostics: Vec = vec![]; - diagnostics.reserve_exact(groups.len()); + let diagnostics: Vec = group_appends(appends) + .iter() + .filter_map(|group| { + // Groups with just one element are fine, and shouldn't be replaced by `extend`. + if group.appends.len() <= 1 { + return None; + } - for group in groups { - // Groups with just one element are fine, and shouldn't be replaced by `extend`. - if group.appends.len() <= 1 { - continue; - } + let replacement = make_suggestion(group, checker.generator()); + + let mut diagnostic = Diagnostic::new( + RepeatedAppend { + name: group.name().to_string(), + replacement: SourceCodeSnippet::new(replacement.clone()), + }, + group.range(), + ); + + // We only suggest a fix when all appends in a group are clumped together. If they're + // non-consecutive, fixing them is much more difficult. + if checker.patch(diagnostic.kind.rule()) && group.is_consecutive { + diagnostic.set_fix(Fix::suggested(Edit::replacement( + replacement, + group.start(), + group.end(), + ))); + } - let replacement = make_suggestion(&group, checker.generator()); - - let mut diagnostic = Diagnostic::new( - RepeatedAppend { - name: group.name().to_string(), - replacement: SourceCodeSnippet::new(replacement.to_string()), - }, - group.range(), - ); - - // We only suggest fix when all appends in a group are clumped together. - if checker.patch(diagnostic.kind.rule()) && group.is_consecutive { - diagnostic.set_fix(Fix::suggested(Edit::replacement( - replacement, - group.start(), - group.end(), - ))); - } + Some(diagnostic) + }) + .collect(); - diagnostics.push(diagnostic); - } checker.diagnostics.extend(diagnostics); } #[derive(Debug, Clone)] struct Append<'a> { /// Receiver of the `append` call (aka `self` argument). - receiver: &'a ExprName, + receiver: &'a ast::ExprName, /// [`BindingId`] that the receiver references. binding_id: BindingId, /// [`Binding`] that the receiver references. @@ -157,71 +158,64 @@ impl<'a> Ranged for AppendGroup<'a> { } /// Match consecutive calls to `append` on list variables starting from the given statement. -fn match_consecutive_appends<'a>(semantic: &'a SemanticModel, stmt: &'a Stmt) -> Vec> { +fn match_consecutive_appends<'a>( + semantic: &'a SemanticModel, + stmt: &'a Stmt, +) -> Option>> { + // Match the current statement, to see if it's an append. + let append = match_append(semantic, stmt)?; + // In order to match consecutive statements, we need to go to the tree ancestor of the // given statement, find its position there, and match all 'appends' from there. - let empty: Vec> = vec![]; - let siblings: &'a [Stmt] = if semantic.at_top_level() { // If the statement is at the top level, we should go to the parent module. // Module is available in the definitions list. - let Definition::Module(ref module_def) = semantic.definitions[DefinitionId::module()] - else { - return empty; - }; - module_def.python_ast + let module = semantic.definitions[DefinitionId::module()].as_module()?; + module.python_ast } else { // Otherwise, go to the parent, and take its body as a sequence of siblings. - let Some(suite) = semantic + semantic .current_statement_parent() - .and_then(|parent| traversal::suite(stmt, parent)) - else { - return empty; - }; - suite + .and_then(|parent| traversal::suite(stmt, parent))? }; - let Some(stmt_index) = siblings.iter().position(|x| x == stmt) else { - return empty; - }; - - // Match the current statement, to see if it's an append. - let Some(append) = match_append(semantic, stmt) else { - return empty; - }; + let stmt_index = siblings.iter().position(|sibling| sibling == stmt)?; // We shouldn't repeat the same work for many 'appends' that go in a row. Let's check // that this statement is at the beginning of such a group. if stmt_index != 0 && match_append(semantic, &siblings[stmt_index - 1]).is_some() { - return empty; + return None; } // Starting from the next statement, let's match all appends and make a vector. - std::iter::once(append) - .chain( - siblings - .iter() - // Skip the current statement, since we already tested it... - .skip(stmt_index + 1) - .map_while(|next| match_append(semantic, next)), - ) - .collect() + Some( + std::iter::once(append) + .chain( + siblings + .iter() + .skip(stmt_index + 1) + .map_while(|sibling| match_append(semantic, sibling)), + ) + .collect(), + ) } /// Group the given appends by the associated bindings. -fn group_appends<'a>(appends: &[Append<'a>]) -> Vec> { +fn group_appends<'a>(appends: Vec>) -> Vec> { // We want to go over the given list of appends and group the by receivers. - let mut map: HashMap> = HashMap::new(); - let mut iter = appends.iter(); + let mut map: FxHashMap> = FxHashMap::default(); + let mut iter = appends.into_iter(); let mut last_binding = { let first_append = iter.next().unwrap(); + let binding_id = first_append.binding_id; let _ = get_or_add(&mut map, first_append); - first_append.binding_id + binding_id }; for append in iter { + let binding_id = append.binding_id; let group = get_or_add(&mut map, append); - if append.binding_id != last_binding { + if binding_id != last_binding { // If the group is not brand new, and the previous group was different, // we should mark it as "non-consecutive". // @@ -240,7 +234,7 @@ fn group_appends<'a>(appends: &[Append<'a>]) -> Vec> { group.is_consecutive = false; } - last_binding = append.binding_id; + last_binding = binding_id; } } @@ -249,14 +243,14 @@ fn group_appends<'a>(appends: &[Append<'a>]) -> Vec> { #[inline] fn get_or_add<'a, 'b>( - map: &'b mut HashMap>, - append: &Append<'a>, + map: &'b mut FxHashMap>, + append: Append<'a>, ) -> &'b mut AppendGroup<'a> { let group = map.entry(append.binding_id).or_insert(AppendGroup { appends: vec![], is_consecutive: true, }); - group.appends.push(append.clone()); + group.appends.push(append); group } @@ -274,7 +268,7 @@ fn make_suggestion(group: &AppendGroup, generator: Generator) -> String { // Here we construct `var.extend((elt1, elt2, ..., eltN)) // // Each eltK comes from an individual `var.append(eltK)`. - let elts: Vec = appends + let elts: Vec = appends .iter() .map(|append| append.argument.clone()) .collect(); @@ -323,17 +317,17 @@ fn match_append<'a>(semantic: &'a SemanticModel, stmt: &'a Stmt) -> Option(semantic: &'a SemanticModel, stmt: &'a Stmt) -> Option(semantic: &'a SemanticModel, binding: &'a Binding, name: &str) -> bool { - assert!(binding.source.is_some()); - let stmt = semantic.statement(binding.source.unwrap()); - + let Some(statement_id) = binding.source else { + return false; + }; + let stmt = semantic.statement(statement_id); match binding.kind { BindingKind::Assignment => match stmt { Stmt::Assign(ast::StmtAssign { value, .. }) => { diff --git a/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap index 1518a6ed261a5..e2b12099ab7c4 100644 --- a/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap +++ b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap @@ -1,311 +1,311 @@ --- source: crates/ruff/src/rules/refurb/mod.rs --- -FURB113.py:21:1: FURB113 [*] Use `nums.extend((1, 2))` instead of repeatedly calling `nums.append()` +FURB113.py:23:1: FURB113 [*] Use `nums.extend((1, 2))` instead of repeatedly calling `nums.append()` | -20 | # FURB113 -21 | / nums.append(1) -22 | | nums.append(2) +22 | # FURB113 +23 | / nums.append(1) +24 | | nums.append(2) | |______________^ FURB113 -23 | pass +25 | pass | = help: Replace with `nums.extend((1, 2))` ℹ Suggested fix -18 18 | -19 19 | -20 20 | # FURB113 -21 |-nums.append(1) -22 |-nums.append(2) - 21 |+nums.extend((1, 2)) -23 22 | pass -24 23 | -25 24 | +20 20 | +21 21 | +22 22 | # FURB113 +23 |-nums.append(1) +24 |-nums.append(2) + 23 |+nums.extend((1, 2)) +25 24 | pass +26 25 | +27 26 | -FURB113.py:27:1: FURB113 [*] Use `nums3.extend((1, 2))` instead of repeatedly calling `nums3.append()` +FURB113.py:29:1: FURB113 [*] Use `nums3.extend((1, 2))` instead of repeatedly calling `nums3.append()` | -26 | # FURB113 -27 | / nums3.append(1) -28 | | nums3.append(2) +28 | # FURB113 +29 | / nums3.append(1) +30 | | nums3.append(2) | |_______________^ FURB113 -29 | pass +31 | pass | = help: Replace with `nums3.extend((1, 2))` ℹ Suggested fix -24 24 | -25 25 | -26 26 | # FURB113 -27 |-nums3.append(1) -28 |-nums3.append(2) - 27 |+nums3.extend((1, 2)) -29 28 | pass -30 29 | -31 30 | +26 26 | +27 27 | +28 28 | # FURB113 +29 |-nums3.append(1) +30 |-nums3.append(2) + 29 |+nums3.extend((1, 2)) +31 30 | pass +32 31 | +33 32 | -FURB113.py:33:1: FURB113 [*] Use `nums4.extend((1, 2))` instead of repeatedly calling `nums4.append()` +FURB113.py:35:1: FURB113 [*] Use `nums4.extend((1, 2))` instead of repeatedly calling `nums4.append()` | -32 | # FURB113 -33 | / nums4.append(1) -34 | | nums4.append(2) +34 | # FURB113 +35 | / nums4.append(1) +36 | | nums4.append(2) | |_______________^ FURB113 -35 | pass +37 | pass | = help: Replace with `nums4.extend((1, 2))` ℹ Suggested fix -30 30 | -31 31 | -32 32 | # FURB113 -33 |-nums4.append(1) -34 |-nums4.append(2) - 33 |+nums4.extend((1, 2)) -35 34 | pass -36 35 | -37 36 | +32 32 | +33 33 | +34 34 | # FURB113 +35 |-nums4.append(1) +36 |-nums4.append(2) + 35 |+nums4.extend((1, 2)) +37 36 | pass +38 37 | +39 38 | -FURB113.py:39:1: FURB113 Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` +FURB113.py:41:1: FURB113 Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` | -38 | # FURB113 -39 | / nums.append(1) -40 | | nums2.append(1) -41 | | nums.append(2) -42 | | nums.append(3) +40 | # FURB113 +41 | / nums.append(1) +42 | | nums2.append(1) +43 | | nums.append(2) +44 | | nums.append(3) | |______________^ FURB113 -43 | pass +45 | pass | = help: Replace with `nums.extend((1, 2, 3))` -FURB113.py:47:1: FURB113 Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` +FURB113.py:49:1: FURB113 Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` | -46 | # FURB113 -47 | / nums.append(1) -48 | | nums2.append(1) -49 | | nums.append(2) -50 | | # FURB113 -51 | | nums3.append(1) -52 | | nums.append(3) +48 | # FURB113 +49 | / nums.append(1) +50 | | nums2.append(1) +51 | | nums.append(2) +52 | | # FURB113 +53 | | nums3.append(1) +54 | | nums.append(3) | |______________^ FURB113 -53 | # FURB113 -54 | nums4.append(1) +55 | # FURB113 +56 | nums4.append(1) | = help: Replace with `nums.extend((1, 2, 3))` -FURB113.py:51:1: FURB113 Use `nums3.extend((1, 2))` instead of repeatedly calling `nums3.append()` +FURB113.py:53:1: FURB113 Use `nums3.extend((1, 2))` instead of repeatedly calling `nums3.append()` | -49 | nums.append(2) -50 | # FURB113 -51 | / nums3.append(1) -52 | | nums.append(3) -53 | | # FURB113 -54 | | nums4.append(1) -55 | | nums4.append(2) -56 | | nums3.append(2) +51 | nums.append(2) +52 | # FURB113 +53 | / nums3.append(1) +54 | | nums.append(3) +55 | | # FURB113 +56 | | nums4.append(1) +57 | | nums4.append(2) +58 | | nums3.append(2) | |_______________^ FURB113 -57 | pass +59 | pass | = help: Replace with `nums3.extend((1, 2))` -FURB113.py:54:1: FURB113 [*] Use `nums4.extend((1, 2))` instead of repeatedly calling `nums4.append()` +FURB113.py:56:1: FURB113 [*] Use `nums4.extend((1, 2))` instead of repeatedly calling `nums4.append()` | -52 | nums.append(3) -53 | # FURB113 -54 | / nums4.append(1) -55 | | nums4.append(2) +54 | nums.append(3) +55 | # FURB113 +56 | / nums4.append(1) +57 | | nums4.append(2) | |_______________^ FURB113 -56 | nums3.append(2) -57 | pass +58 | nums3.append(2) +59 | pass | = help: Replace with `nums4.extend((1, 2))` ℹ Suggested fix -51 51 | nums3.append(1) -52 52 | nums.append(3) -53 53 | # FURB113 -54 |-nums4.append(1) -55 |-nums4.append(2) - 54 |+nums4.extend((1, 2)) -56 55 | nums3.append(2) -57 56 | pass -58 57 | +53 53 | nums3.append(1) +54 54 | nums.append(3) +55 55 | # FURB113 +56 |-nums4.append(1) +57 |-nums4.append(2) + 56 |+nums4.extend((1, 2)) +58 57 | nums3.append(2) +59 58 | pass +60 59 | -FURB113.py:60:1: FURB113 [*] Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` +FURB113.py:62:1: FURB113 [*] Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` | -59 | # FURB113 -60 | / nums.append(1) -61 | | nums.append(2) -62 | | nums.append(3) +61 | # FURB113 +62 | / nums.append(1) +63 | | nums.append(2) +64 | | nums.append(3) | |______________^ FURB113 | = help: Replace with `nums.extend((1, 2, 3))` ℹ Suggested fix -57 57 | pass -58 58 | -59 59 | # FURB113 -60 |-nums.append(1) -61 |-nums.append(2) -62 |-nums.append(3) - 60 |+nums.extend((1, 2, 3)) -63 61 | -64 62 | -65 63 | if True: +59 59 | pass +60 60 | +61 61 | # FURB113 +62 |-nums.append(1) +63 |-nums.append(2) +64 |-nums.append(3) + 62 |+nums.extend((1, 2, 3)) +65 63 | +66 64 | +67 65 | if True: -FURB113.py:67:5: FURB113 [*] Use `nums.extend((1, 2))` instead of repeatedly calling `nums.append()` +FURB113.py:69:5: FURB113 [*] Use `nums.extend((1, 2))` instead of repeatedly calling `nums.append()` | -65 | if True: -66 | # FURB113 -67 | nums.append(1) +67 | if True: +68 | # FURB113 +69 | nums.append(1) | _____^ -68 | | nums.append(2) +70 | | nums.append(2) | |__________________^ FURB113 | = help: Replace with `nums.extend((1, 2))` ℹ Suggested fix -64 64 | -65 65 | if True: -66 66 | # FURB113 -67 |- nums.append(1) -68 |- nums.append(2) - 67 |+ nums.extend((1, 2)) -69 68 | -70 69 | -71 70 | if True: +66 66 | +67 67 | if True: +68 68 | # FURB113 +69 |- nums.append(1) +70 |- nums.append(2) + 69 |+ nums.extend((1, 2)) +71 70 | +72 71 | +73 72 | if True: -FURB113.py:73:5: FURB113 [*] Use `nums.extend((1, 2))` instead of repeatedly calling `nums.append()` +FURB113.py:75:5: FURB113 [*] Use `nums.extend((1, 2))` instead of repeatedly calling `nums.append()` | -71 | if True: -72 | # FURB113 -73 | nums.append(1) +73 | if True: +74 | # FURB113 +75 | nums.append(1) | _____^ -74 | | nums.append(2) +76 | | nums.append(2) | |__________________^ FURB113 -75 | pass +77 | pass | = help: Replace with `nums.extend((1, 2))` ℹ Suggested fix -70 70 | -71 71 | if True: -72 72 | # FURB113 -73 |- nums.append(1) -74 |- nums.append(2) - 73 |+ nums.extend((1, 2)) -75 74 | pass -76 75 | -77 76 | +72 72 | +73 73 | if True: +74 74 | # FURB113 +75 |- nums.append(1) +76 |- nums.append(2) + 75 |+ nums.extend((1, 2)) +77 76 | pass +78 77 | +79 78 | -FURB113.py:80:5: FURB113 Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` +FURB113.py:82:5: FURB113 Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` | -78 | if True: -79 | # FURB113 -80 | nums.append(1) +80 | if True: +81 | # FURB113 +82 | nums.append(1) | _____^ -81 | | nums2.append(1) -82 | | nums.append(2) -83 | | nums.append(3) +83 | | nums2.append(1) +84 | | nums.append(2) +85 | | nums.append(3) | |__________________^ FURB113 | = help: Replace with `nums.extend((1, 2, 3))` -FURB113.py:88:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` +FURB113.py:90:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` | -86 | def yes_one(x: list[int]): -87 | # FURB113 -88 | x.append(1) +88 | def yes_one(x: list[int]): +89 | # FURB113 +90 | x.append(1) | _____^ -89 | | x.append(2) +91 | | x.append(2) | |_______________^ FURB113 | = help: Replace with `x.extend((1, 2))` ℹ Suggested fix -85 85 | -86 86 | def yes_one(x: list[int]): -87 87 | # FURB113 -88 |- x.append(1) -89 |- x.append(2) - 88 |+ x.extend((1, 2)) -90 89 | -91 90 | -92 91 | def yes_two(x: List[int]): +87 87 | +88 88 | def yes_one(x: list[int]): +89 89 | # FURB113 +90 |- x.append(1) +91 |- x.append(2) + 90 |+ x.extend((1, 2)) +92 91 | +93 92 | +94 93 | def yes_two(x: List[int]): -FURB113.py:94:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` +FURB113.py:96:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` | -92 | def yes_two(x: List[int]): -93 | # FURB113 -94 | x.append(1) +94 | def yes_two(x: List[int]): +95 | # FURB113 +96 | x.append(1) | _____^ -95 | | x.append(2) +97 | | x.append(2) | |_______________^ FURB113 | = help: Replace with `x.extend((1, 2))` ℹ Suggested fix -91 91 | -92 92 | def yes_two(x: List[int]): -93 93 | # FURB113 -94 |- x.append(1) -95 |- x.append(2) - 94 |+ x.extend((1, 2)) -96 95 | -97 96 | -98 97 | def yes_three(*, x: list[int]): +93 93 | +94 94 | def yes_two(x: List[int]): +95 95 | # FURB113 +96 |- x.append(1) +97 |- x.append(2) + 96 |+ x.extend((1, 2)) +98 97 | +99 98 | +100 99 | def yes_three(*, x: list[int]): -FURB113.py:100:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` +FURB113.py:102:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` | - 98 | def yes_three(*, x: list[int]): - 99 | # FURB113 -100 | x.append(1) +100 | def yes_three(*, x: list[int]): +101 | # FURB113 +102 | x.append(1) | _____^ -101 | | x.append(2) +103 | | x.append(2) | |_______________^ FURB113 | = help: Replace with `x.extend((1, 2))` ℹ Suggested fix -97 97 | -98 98 | def yes_three(*, x: list[int]): -99 99 | # FURB113 -100 |- x.append(1) -101 |- x.append(2) - 100 |+ x.extend((1, 2)) -102 101 | -103 102 | -104 103 | def yes_four(x: list[int], /): +99 99 | +100 100 | def yes_three(*, x: list[int]): +101 101 | # FURB113 +102 |- x.append(1) +103 |- x.append(2) + 102 |+ x.extend((1, 2)) +104 103 | +105 104 | +106 105 | def yes_four(x: list[int], /): -FURB113.py:106:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` +FURB113.py:108:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` | -104 | def yes_four(x: list[int], /): -105 | # FURB113 -106 | x.append(1) +106 | def yes_four(x: list[int], /): +107 | # FURB113 +108 | x.append(1) | _____^ -107 | | x.append(2) +109 | | x.append(2) | |_______________^ FURB113 | = help: Replace with `x.extend((1, 2))` ℹ Suggested fix -103 103 | -104 104 | def yes_four(x: list[int], /): -105 105 | # FURB113 -106 |- x.append(1) -107 |- x.append(2) - 106 |+ x.extend((1, 2)) -108 107 | -109 108 | -110 109 | def yes_five(x: list[int], y: list[int]): +105 105 | +106 106 | def yes_four(x: list[int], /): +107 107 | # FURB113 +108 |- x.append(1) +109 |- x.append(2) + 108 |+ x.extend((1, 2)) +110 109 | +111 110 | +112 111 | def yes_five(x: list[int], y: list[int]): -FURB113.py:112:5: FURB113 Use `x.extend((1, 2, 3))` instead of repeatedly calling `x.append()` +FURB113.py:114:5: FURB113 Use `x.extend((1, 2, 3))` instead of repeatedly calling `x.append()` | -110 | def yes_five(x: list[int], y: list[int]): -111 | # FURB113 -112 | x.append(1) +112 | def yes_five(x: list[int], y: list[int]): +113 | # FURB113 +114 | x.append(1) | _____^ -113 | | x.append(2) -114 | | y.append(1) -115 | | x.append(3) +115 | | x.append(2) +116 | | y.append(1) +117 | | x.append(3) | |_______________^ FURB113 | = help: Replace with `x.extend((1, 2, 3))` diff --git a/crates/ruff_python_semantic/src/definition.rs b/crates/ruff_python_semantic/src/definition.rs index 1ea933a5c226b..67a675aef4fec 100644 --- a/crates/ruff_python_semantic/src/definition.rs +++ b/crates/ruff_python_semantic/src/definition.rs @@ -117,7 +117,7 @@ impl Ranged for Member<'_> { } /// A definition within a Python program. -#[derive(Debug)] +#[derive(Debug, is_macro::Is)] pub enum Definition<'a> { Module(Module<'a>), Member(Member<'a>), From 0e15d3915d5e7e60fff777a9ff6f7c4264025143 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 28 Aug 2023 18:12:33 -0400 Subject: [PATCH 3/6] Remove some explicit lifetimes --- .../ruff/src/rules/refurb/rules/repeated_append.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/ruff/src/rules/refurb/rules/repeated_append.rs b/crates/ruff/src/rules/refurb/rules/repeated_append.rs index 8e2a459e32ac3..ee79fb8440d6b 100644 --- a/crates/ruff/src/rules/refurb/rules/repeated_append.rs +++ b/crates/ruff/src/rules/refurb/rules/repeated_append.rs @@ -140,14 +140,14 @@ struct AppendGroup<'a> { is_consecutive: bool, } -impl<'a> AppendGroup<'a> { - fn name(&self) -> &'a str { +impl AppendGroup<'_> { + fn name(&self) -> &str { assert!(!self.appends.is_empty()); &self.appends.first().unwrap().receiver.id } } -impl<'a> Ranged for AppendGroup<'a> { +impl Ranged for AppendGroup<'_> { fn range(&self) -> TextRange { assert!(!self.appends.is_empty()); TextRange::new( @@ -167,7 +167,7 @@ fn match_consecutive_appends<'a>( // In order to match consecutive statements, we need to go to the tree ancestor of the // given statement, find its position there, and match all 'appends' from there. - let siblings: &'a [Stmt] = if semantic.at_top_level() { + let siblings: &[Stmt] = if semantic.at_top_level() { // If the statement is at the top level, we should go to the parent module. // Module is available in the definitions list. let module = semantic.definitions[DefinitionId::module()].as_module()?; @@ -201,9 +201,9 @@ fn match_consecutive_appends<'a>( } /// Group the given appends by the associated bindings. -fn group_appends<'a>(appends: Vec>) -> Vec> { +fn group_appends(appends: Vec>) -> Vec> { // We want to go over the given list of appends and group the by receivers. - let mut map: FxHashMap> = FxHashMap::default(); + let mut map: FxHashMap = FxHashMap::default(); let mut iter = appends.into_iter(); let mut last_binding = { let first_append = iter.next().unwrap(); From 8b911f56ebda072eee4596df6a9c84a0ebf5d7e4 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 28 Aug 2023 18:15:37 -0400 Subject: [PATCH 4/6] Allow list annotation without subscript --- .../resources/test/fixtures/refurb/FURB113.py | 6 +++++ .../src/rules/refurb/rules/repeated_append.rs | 5 ++--- ...es__refurb__tests__FURB113_FURB113.py.snap | 22 +++++++++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/refurb/FURB113.py b/crates/ruff/resources/test/fixtures/refurb/FURB113.py index f89382f5854b4..48b0283f67df6 100644 --- a/crates/ruff/resources/test/fixtures/refurb/FURB113.py +++ b/crates/ruff/resources/test/fixtures/refurb/FURB113.py @@ -117,6 +117,12 @@ def yes_five(x: list[int], y: list[int]): x.append(3) +def yes_six(x: list): + # FURB113 + x.append(1) + x.append(2) + + # Non-errors. nums.append(1) diff --git a/crates/ruff/src/rules/refurb/rules/repeated_append.rs b/crates/ruff/src/rules/refurb/rules/repeated_append.rs index ee79fb8440d6b..3167326e6b404 100644 --- a/crates/ruff/src/rules/refurb/rules/repeated_append.rs +++ b/crates/ruff/src/rules/refurb/rules/repeated_append.rs @@ -3,6 +3,7 @@ use rustc_hash::FxHashMap; use ast::{traversal, ParameterWithDefault, Parameters}; use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, 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_codegen::Generator; use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType}; @@ -412,9 +413,7 @@ fn is_list<'a>(semantic: &'a SemanticModel, binding: &'a Binding, name: &str) -> #[inline] fn is_list_annotation(semantic: &SemanticModel, annotation: &Expr) -> bool { - let Expr::Subscript(ast::ExprSubscript { value, .. }) = annotation else { - return false; - }; + let value = map_subscript(annotation); match_builtin_list_type(semantic, value) || semantic.match_typing_expr(value, "List") } diff --git a/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap index e2b12099ab7c4..51024c918fbb1 100644 --- a/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap +++ b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap @@ -310,4 +310,26 @@ FURB113.py:114:5: FURB113 Use `x.extend((1, 2, 3))` instead of repeatedly callin | = help: Replace with `x.extend((1, 2, 3))` +FURB113.py:122:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` + | +120 | def yes_six(x: list): +121 | # FURB113 +122 | x.append(1) + | _____^ +123 | | x.append(2) + | |_______________^ FURB113 + | + = help: Replace with `x.extend((1, 2))` + +ℹ Suggested fix +119 119 | +120 120 | def yes_six(x: list): +121 121 | # FURB113 +122 |- x.append(1) +123 |- x.append(2) + 122 |+ x.extend((1, 2)) +124 123 | +125 124 | +126 125 | # Non-errors. + From 7b4517d0381c9936eac295dbc723baa65aede10c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 28 Aug 2023 18:18:24 -0400 Subject: [PATCH 5/6] Add Binding#statement --- .../src/rules/refurb/rules/repeated_append.rs | 18 ++++++--------- crates/ruff_python_semantic/src/binding.rs | 23 +++++++++++-------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/crates/ruff/src/rules/refurb/rules/repeated_append.rs b/crates/ruff/src/rules/refurb/rules/repeated_append.rs index 3167326e6b404..5ff7d4832590e 100644 --- a/crates/ruff/src/rules/refurb/rules/repeated_append.rs +++ b/crates/ruff/src/rules/refurb/rules/repeated_append.rs @@ -371,26 +371,22 @@ fn match_append<'a>(semantic: &'a SemanticModel, stmt: &'a Stmt) -> Option(semantic: &'a SemanticModel, binding: &'a Binding, name: &str) -> bool { - let Some(statement_id) = binding.source else { - return false; - }; - let stmt = semantic.statement(statement_id); match binding.kind { - BindingKind::Assignment => match stmt { - Stmt::Assign(ast::StmtAssign { value, .. }) => { + 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) } - Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. }) => { + Some(Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. })) => { is_list_annotation(semantic, annotation.as_ref()) } _ => false, }, - BindingKind::Argument => match stmt { - Stmt::FunctionDef(ast::StmtFunctionDef { parameters, .. }) => { + 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; }; @@ -401,8 +397,8 @@ fn is_list<'a>(semantic: &'a SemanticModel, binding: &'a Binding, name: &str) -> } _ => false, }, - BindingKind::Annotation => match stmt { - Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. }) => { + BindingKind::Annotation => match binding.statement(semantic) { + Some(Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. })) => { is_list_annotation(semantic, annotation.as_ref()) } _ => false, diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index e357bc7f3ea2c..dba1616e086d2 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -5,6 +5,7 @@ use bitflags::bitflags; use ruff_index::{newtype_index, IndexSlice, IndexVec}; use ruff_python_ast::call_path::format_call_path; +use ruff_python_ast::Stmt; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange}; @@ -181,17 +182,21 @@ impl<'a> Binding<'a> { locator.slice(self.range) } + /// Returns the statement in which the binding was defined. + pub fn statement<'b>(&self, semantic: &'b SemanticModel) -> Option<&'b Stmt> { + self.source + .map(|statement_id| semantic.statement(statement_id)) + } + /// Returns the range of the binding's parent. pub fn parent_range(&self, semantic: &SemanticModel) -> Option { - self.source - .map(|id| semantic.statement(id)) - .and_then(|parent| { - if parent.is_import_from_stmt() { - Some(parent.range()) - } else { - None - } - }) + self.statement(semantic).and_then(|parent| { + if parent.is_import_from_stmt() { + Some(parent.range()) + } else { + None + } + }) } pub fn as_any_import(&'a self) -> Option> { From 2c0d731db40c42c5c91d104db0e2d29ebd4198b9 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 28 Aug 2023 18:40:38 -0400 Subject: [PATCH 6/6] Make nursery --- crates/ruff/src/codes.rs | 2 +- ruff.schema.json | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index d7e61976842ce..e6be8932bfe82 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -866,7 +866,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Slots, "002") => (RuleGroup::Unspecified, rules::flake8_slots::rules::NoSlotsInNamedtupleSubclass), // refurb - (Refurb, "113") => (RuleGroup::Unspecified, rules::refurb::rules::RepeatedAppend), + (Refurb, "113") => (RuleGroup::Nursery, rules::refurb::rules::RepeatedAppend), _ => return None, }) diff --git a/ruff.schema.json b/ruff.schema.json index 9b5c15a4de6d5..31e750a826c0b 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2044,8 +2044,6 @@ "FLY00", "FLY002", "FURB", - "FURB1", - "FURB11", "FURB113", "G", "G0",