From 26d53c56a2409973e2305023fc34b2a6641e4b09 Mon Sep 17 00:00:00 2001 From: Valeriy Savchenko Date: Mon, 28 Aug 2023 23:51:59 +0100 Subject: [PATCH] [refurb] Implement `repeated-append` rule (`FURB113`) (#6702) ## Summary As an initial effort with replicating `refurb` rules (#1348 ), this PR adds support for [FURB113](https://github.com/dosisod/refurb/blob/master/refurb/checks/builtin/list_extend.py) and adds a new category of checks. ## Test Plan I included a new test + checked that all other tests pass. --- .../resources/test/fixtures/refurb/FURB113.py | 169 +++++++ .../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 | 442 ++++++++++++++++++ ...es__refurb__tests__FURB113_FURB113.py.snap | 335 +++++++++++++ crates/ruff_python_ast/src/traversal.rs | 1 + crates/ruff_python_semantic/src/binding.rs | 23 +- crates/ruff_python_semantic/src/definition.rs | 2 +- ruff.schema.json | 2 + 13 files changed, 1003 insertions(+), 11 deletions(-) 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..48b0283f67df6 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/refurb/FURB113.py @@ -0,0 +1,169 @@ +from typing import List + + +def func(): + pass + + +nums = [] +nums2 = [] +nums3: list[int] = func() +nums4: List[int] + + +class C: + def append(self, x): + pass + + +# Errors. + + +# 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) + + +def yes_six(x: list): + # FURB113 + x.append(1) + x.append(2) + + +# Non-errors. + +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 ed68db699437c..3995fecc0bb9e 100644 --- a/crates/ruff/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff/src/checkers/ast/analyze/statement.rs @@ -13,7 +13,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; @@ -1484,6 +1484,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..e6be8932bfe82 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::Nursery, 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..5ff7d4832590e --- /dev/null +++ b/crates/ruff/src/rules/refurb/rules/repeated_append.rs @@ -0,0 +1,442 @@ +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}; +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; +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 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 +/// 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(...)"), 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}`")) + } +} + +/// FURB113 +pub(crate) fn repeated_append(checker: &mut Checker, stmt: &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. + if appends.len() <= 1 { + return; + } + + // group borrows from checker, so we can't directly push into checker.diagnostics + 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; + } + + 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(), + ))); + } + + Some(diagnostic) + }) + .collect(); + + checker.diagnostics.extend(diagnostics); +} + +#[derive(Debug, Clone)] +struct Append<'a> { + /// Receiver of the `append` call (aka `self` argument). + receiver: &'a ast::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 AppendGroup<'_> { + fn name(&self) -> &str { + assert!(!self.appends.is_empty()); + &self.appends.first().unwrap().receiver.id + } +} + +impl Ranged for AppendGroup<'_> { + 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, +) -> 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 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()?; + module.python_ast + } else { + // Otherwise, go to the parent, and take its body as a sequence of siblings. + semantic + .current_statement_parent() + .and_then(|parent| traversal::suite(stmt, parent))? + }; + + 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 None; + } + + // Starting from the next statement, let's match all appends and make a vector. + 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(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 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); + binding_id + }; + + for append in iter { + let binding_id = append.binding_id; + let group = get_or_add(&mut map, append); + 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". + // + // 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 = binding_id; + } + } + + map.into_values().collect() +} + +#[inline] +fn get_or_add<'a, 'b>( + 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); + 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 { + 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) +} 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..51024c918fbb1 --- /dev/null +++ b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB113_FURB113.py.snap @@ -0,0 +1,335 @@ +--- +source: crates/ruff/src/rules/refurb/mod.rs +--- +FURB113.py:23:1: FURB113 [*] Use `nums.extend((1, 2))` instead of repeatedly calling `nums.append()` + | +22 | # FURB113 +23 | / nums.append(1) +24 | | nums.append(2) + | |______________^ FURB113 +25 | pass + | + = help: Replace with `nums.extend((1, 2))` + +ℹ Suggested fix +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:29:1: FURB113 [*] Use `nums3.extend((1, 2))` instead of repeatedly calling `nums3.append()` + | +28 | # FURB113 +29 | / nums3.append(1) +30 | | nums3.append(2) + | |_______________^ FURB113 +31 | pass + | + = help: Replace with `nums3.extend((1, 2))` + +ℹ Suggested fix +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:35:1: FURB113 [*] Use `nums4.extend((1, 2))` instead of repeatedly calling `nums4.append()` + | +34 | # FURB113 +35 | / nums4.append(1) +36 | | nums4.append(2) + | |_______________^ FURB113 +37 | pass + | + = help: Replace with `nums4.extend((1, 2))` + +ℹ Suggested fix +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:41:1: FURB113 Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` + | +40 | # FURB113 +41 | / nums.append(1) +42 | | nums2.append(1) +43 | | nums.append(2) +44 | | nums.append(3) + | |______________^ FURB113 +45 | pass + | + = help: Replace with `nums.extend((1, 2, 3))` + +FURB113.py:49:1: FURB113 Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` + | +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 +55 | # FURB113 +56 | nums4.append(1) + | + = help: Replace with `nums.extend((1, 2, 3))` + +FURB113.py:53:1: FURB113 Use `nums3.extend((1, 2))` instead of repeatedly calling `nums3.append()` + | +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 +59 | pass + | + = help: Replace with `nums3.extend((1, 2))` + +FURB113.py:56:1: FURB113 [*] Use `nums4.extend((1, 2))` instead of repeatedly calling `nums4.append()` + | +54 | nums.append(3) +55 | # FURB113 +56 | / nums4.append(1) +57 | | nums4.append(2) + | |_______________^ FURB113 +58 | nums3.append(2) +59 | pass + | + = help: Replace with `nums4.extend((1, 2))` + +ℹ Suggested fix +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:62:1: FURB113 [*] Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` + | +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 +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:69:5: FURB113 [*] Use `nums.extend((1, 2))` instead of repeatedly calling `nums.append()` + | +67 | if True: +68 | # FURB113 +69 | nums.append(1) + | _____^ +70 | | nums.append(2) + | |__________________^ FURB113 + | + = help: Replace with `nums.extend((1, 2))` + +ℹ Suggested fix +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:75:5: FURB113 [*] Use `nums.extend((1, 2))` instead of repeatedly calling `nums.append()` + | +73 | if True: +74 | # FURB113 +75 | nums.append(1) + | _____^ +76 | | nums.append(2) + | |__________________^ FURB113 +77 | pass + | + = help: Replace with `nums.extend((1, 2))` + +ℹ Suggested fix +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:82:5: FURB113 Use `nums.extend((1, 2, 3))` instead of repeatedly calling `nums.append()` + | +80 | if True: +81 | # FURB113 +82 | nums.append(1) + | _____^ +83 | | nums2.append(1) +84 | | nums.append(2) +85 | | nums.append(3) + | |__________________^ FURB113 + | + = help: Replace with `nums.extend((1, 2, 3))` + +FURB113.py:90:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` + | +88 | def yes_one(x: list[int]): +89 | # FURB113 +90 | x.append(1) + | _____^ +91 | | x.append(2) + | |_______________^ FURB113 + | + = help: Replace with `x.extend((1, 2))` + +ℹ Suggested fix +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:96:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` + | +94 | def yes_two(x: List[int]): +95 | # FURB113 +96 | x.append(1) + | _____^ +97 | | x.append(2) + | |_______________^ FURB113 + | + = help: Replace with `x.extend((1, 2))` + +ℹ Suggested fix +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:102:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` + | +100 | def yes_three(*, x: list[int]): +101 | # FURB113 +102 | x.append(1) + | _____^ +103 | | x.append(2) + | |_______________^ FURB113 + | + = help: Replace with `x.extend((1, 2))` + +ℹ Suggested fix +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:108:5: FURB113 [*] Use `x.extend((1, 2))` instead of repeatedly calling `x.append()` + | +106 | def yes_four(x: list[int], /): +107 | # FURB113 +108 | x.append(1) + | _____^ +109 | | x.append(2) + | |_______________^ FURB113 + | + = help: Replace with `x.extend((1, 2))` + +ℹ Suggested fix +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:114:5: FURB113 Use `x.extend((1, 2, 3))` instead of repeatedly calling `x.append()` + | +112 | def yes_five(x: list[int], y: list[int]): +113 | # FURB113 +114 | x.append(1) + | _____^ +115 | | x.append(2) +116 | | y.append(1) +117 | | x.append(3) + | |_______________^ FURB113 + | + = 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. + + 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/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> { 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>), diff --git a/ruff.schema.json b/ruff.schema.json index b6844e3c5f2da..31e750a826c0b 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2043,6 +2043,8 @@ "FLY0", "FLY00", "FLY002", + "FURB", + "FURB113", "G", "G0", "G00",