From 95d272b222b927551fa0c3e496c94ad069578952 Mon Sep 17 00:00:00 2001 From: Gilles Peiffer Date: Sat, 29 Jun 2024 16:00:19 +0200 Subject: [PATCH 1/4] [`pylint`] Add fix for `duplicate-bases` (`PLE0241`) --- .../test/fixtures/pylint/duplicate_bases.py | 50 +++++- .../src/rules/pylint/rules/duplicate_bases.rs | 66 +++++++- ...nt__tests__PLE0241_duplicate_bases.py.snap | 157 +++++++++++++++++- 3 files changed, 262 insertions(+), 11 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/duplicate_bases.py b/crates/ruff_linter/resources/test/fixtures/pylint/duplicate_bases.py index 491421ccf5eb2..e59b561ec70ac 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/duplicate_bases.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/duplicate_bases.py @@ -5,7 +5,55 @@ class A: ... -class B(A, A): +class B: + ... + + +# Duplicate base class is last. +class F1(A, A): + ... + + +class F2(A, A,): + ... + + +class F3( + A, + A +): + ... + + +class F4( + A, + A, +): + ... + + +# Duplicate base class is not last. +class G1(A, A, B): + ... + + +class G2(A, A, B,): + ... + + +class G3( + A, + A, + B +): + ... + + +class G4( + A, + A, + B, +): ... diff --git a/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs b/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs index a601e86900bd7..63781ceed6099 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs @@ -1,8 +1,10 @@ +use anyhow::{Context, Result}; use ruff_python_ast::{self as ast, Arguments, Expr}; use rustc_hash::{FxBuildHasher, FxHashSet}; -use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -42,11 +44,17 @@ pub struct DuplicateBases { } impl Violation for DuplicateBases { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { let DuplicateBases { base, class } = self; format!("Duplicate base `{base}` for class `{class}`") } + + fn fix_title(&self) -> Option { + Some(format!("Remove duplicate base")) + } } /// PLE0241 @@ -56,17 +64,67 @@ pub(crate) fn duplicate_bases(checker: &mut Checker, name: &str, arguments: Opti }; let mut seen: FxHashSet<&str> = FxHashSet::with_capacity_and_hasher(bases.len(), FxBuildHasher); - for base in bases.iter() { + let mut prev: Option<&Expr> = bases.iter().next(); + let len: usize = bases.iter().count(); + for (index, base) in bases.iter().enumerate() { if let Expr::Name(ast::ExprName { id, .. }) = base { if !seen.insert(id) { - checker.diagnostics.push(Diagnostic::new( + let mut diagnostic = Diagnostic::new( DuplicateBases { base: id.to_string(), class: name.to_string(), }, base.range(), - )); + ); + // diagnostic.set_fix(Fix::safe_edit(Edit::range_deletion(base.range()))); + diagnostic.try_set_fix(|| { + remove_base( + base, + prev.unwrap(), + index == len - 1, + checker.locator().contents(), + ) + .map(Fix::safe_edit) + }); + checker.diagnostics.push(diagnostic); } } + prev = Some(&base); + } +} + +/// Remove the base at the given index. +fn remove_base(base: &Expr, prev: &Expr, is_last: bool, source: &str) -> Result { + if !is_last { + // Case 1: the base class is _not_ the last one, so delete from the start of the + // expression to the end of the subsequent comma. + // Ex) Delete `A` in `class Foo(A, B)`. + let mut tokenizer = SimpleTokenizer::starts_at(base.end(), source); + + // Find the trailing comma. + tokenizer + .find(|token| token.kind == SimpleTokenKind::Comma) + .context("Unable to find trailing comma")?; + + // Find the next non-whitespace token. + let next = tokenizer + .find(|token| { + token.kind != SimpleTokenKind::Whitespace && token.kind != SimpleTokenKind::Newline + }) + .context("Unable to find next token")?; + + Ok(Edit::deletion(base.start(), next.start())) + } else { + // Case 2: the expression is the last node, but not the _only_ node, so delete from the + // start of the previous comma to the end of the expression. + // Ex) Delete `B` in `class Foo(A, B)`. + let mut tokenizer = SimpleTokenizer::starts_at(prev.end(), source); + + // Find the trailing comma. + let comma = tokenizer + .find(|token| token.kind == SimpleTokenKind::Comma) + .context("Unable to find trailing comma")?; + + Ok(Edit::deletion(comma.start(), base.end())) } } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0241_duplicate_bases.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0241_duplicate_bases.py.snap index ecacbd9b02217..f939250ceb6f3 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0241_duplicate_bases.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0241_duplicate_bases.py.snap @@ -1,11 +1,156 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -duplicate_bases.py:8:12: PLE0241 Duplicate base `A` for class `B` - | -8 | class B(A, A): - | ^ PLE0241 -9 | ... - | +duplicate_bases.py:13:13: PLE0241 [*] Duplicate base `A` for class `F1` + | +12 | # Duplicate base class is last. +13 | class F1(A, A): + | ^ PLE0241 +14 | ... + | + = help: Remove duplicate base +ℹ Safe fix +10 10 | +11 11 | +12 12 | # Duplicate base class is last. +13 |-class F1(A, A): + 13 |+class F1(A): +14 14 | ... +15 15 | +16 16 | +duplicate_bases.py:17:13: PLE0241 [*] Duplicate base `A` for class `F2` + | +17 | class F2(A, A,): + | ^ PLE0241 +18 | ... + | + = help: Remove duplicate base + +ℹ Safe fix +14 14 | ... +15 15 | +16 16 | +17 |-class F2(A, A,): + 17 |+class F2(A,): +18 18 | ... +19 19 | +20 20 | + +duplicate_bases.py:23:5: PLE0241 [*] Duplicate base `A` for class `F3` + | +21 | class F3( +22 | A, +23 | A + | ^ PLE0241 +24 | ): +25 | ... + | + = help: Remove duplicate base + +ℹ Safe fix +19 19 | +20 20 | +21 21 | class F3( +22 |- A, +23 22 | A +24 23 | ): +25 24 | ... + +duplicate_bases.py:30:5: PLE0241 [*] Duplicate base `A` for class `F4` + | +28 | class F4( +29 | A, +30 | A, + | ^ PLE0241 +31 | ): +32 | ... + | + = help: Remove duplicate base + +ℹ Safe fix +27 27 | +28 28 | class F4( +29 29 | A, +30 |- A, +31 30 | ): +32 31 | ... +33 32 | + +duplicate_bases.py:36:13: PLE0241 [*] Duplicate base `A` for class `G1` + | +35 | # Duplicate base class is not last. +36 | class G1(A, A, B): + | ^ PLE0241 +37 | ... + | + = help: Remove duplicate base + +ℹ Safe fix +33 33 | +34 34 | +35 35 | # Duplicate base class is not last. +36 |-class G1(A, A, B): + 36 |+class G1(A, B): +37 37 | ... +38 38 | +39 39 | + +duplicate_bases.py:40:13: PLE0241 [*] Duplicate base `A` for class `G2` + | +40 | class G2(A, A, B,): + | ^ PLE0241 +41 | ... + | + = help: Remove duplicate base + +ℹ Safe fix +37 37 | ... +38 38 | +39 39 | +40 |-class G2(A, A, B,): + 40 |+class G2(A, B,): +41 41 | ... +42 42 | +43 43 | + +duplicate_bases.py:46:5: PLE0241 [*] Duplicate base `A` for class `G3` + | +44 | class G3( +45 | A, +46 | A, + | ^ PLE0241 +47 | B +48 | ): + | + = help: Remove duplicate base + +ℹ Safe fix +43 43 | +44 44 | class G3( +45 45 | A, +46 |- A, +47 46 | B +48 47 | ): +49 48 | ... + +duplicate_bases.py:54:5: PLE0241 [*] Duplicate base `A` for class `G4` + | +52 | class G4( +53 | A, +54 | A, + | ^ PLE0241 +55 | B, +56 | ): + | + = help: Remove duplicate base + +ℹ Safe fix +51 51 | +52 52 | class G4( +53 53 | A, +54 |- A, +55 54 | B, +56 55 | ): +57 56 | ... From 82b5c8722e7caa874a2b2f952faadd66bce64a79 Mon Sep 17 00:00:00 2001 From: Gilles Peiffer Date: Sat, 29 Jun 2024 16:42:10 +0200 Subject: [PATCH 2/4] Use existing `remove_argument` in fix --- .../src/rules/pylint/rules/duplicate_bases.rs | 56 +++---------------- 1 file changed, 8 insertions(+), 48 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs b/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs index 63781ceed6099..66e97e6ac0f75 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs @@ -1,13 +1,12 @@ -use anyhow::{Context, Result}; use ruff_python_ast::{self as ast, Arguments, Expr}; use rustc_hash::{FxBuildHasher, FxHashSet}; -use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_diagnostics::{Diagnostic, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; +use crate::fix::edits::{remove_argument, Parentheses}; /// ## What it does /// Checks for duplicate base classes in class definitions. @@ -53,7 +52,7 @@ impl Violation for DuplicateBases { } fn fix_title(&self) -> Option { - Some(format!("Remove duplicate base")) + Some("Remove duplicate base".to_string()) } } @@ -64,9 +63,7 @@ pub(crate) fn duplicate_bases(checker: &mut Checker, name: &str, arguments: Opti }; let mut seen: FxHashSet<&str> = FxHashSet::with_capacity_and_hasher(bases.len(), FxBuildHasher); - let mut prev: Option<&Expr> = bases.iter().next(); - let len: usize = bases.iter().count(); - for (index, base) in bases.iter().enumerate() { + for base in bases.iter() { if let Expr::Name(ast::ExprName { id, .. }) = base { if !seen.insert(id) { let mut diagnostic = Diagnostic::new( @@ -76,12 +73,12 @@ pub(crate) fn duplicate_bases(checker: &mut Checker, name: &str, arguments: Opti }, base.range(), ); - // diagnostic.set_fix(Fix::safe_edit(Edit::range_deletion(base.range()))); + diagnostic.try_set_fix(|| { - remove_base( + remove_argument( base, - prev.unwrap(), - index == len - 1, + arguments.unwrap(), + Parentheses::Preserve, checker.locator().contents(), ) .map(Fix::safe_edit) @@ -89,42 +86,5 @@ pub(crate) fn duplicate_bases(checker: &mut Checker, name: &str, arguments: Opti checker.diagnostics.push(diagnostic); } } - prev = Some(&base); - } -} - -/// Remove the base at the given index. -fn remove_base(base: &Expr, prev: &Expr, is_last: bool, source: &str) -> Result { - if !is_last { - // Case 1: the base class is _not_ the last one, so delete from the start of the - // expression to the end of the subsequent comma. - // Ex) Delete `A` in `class Foo(A, B)`. - let mut tokenizer = SimpleTokenizer::starts_at(base.end(), source); - - // Find the trailing comma. - tokenizer - .find(|token| token.kind == SimpleTokenKind::Comma) - .context("Unable to find trailing comma")?; - - // Find the next non-whitespace token. - let next = tokenizer - .find(|token| { - token.kind != SimpleTokenKind::Whitespace && token.kind != SimpleTokenKind::Newline - }) - .context("Unable to find next token")?; - - Ok(Edit::deletion(base.start(), next.start())) - } else { - // Case 2: the expression is the last node, but not the _only_ node, so delete from the - // start of the previous comma to the end of the expression. - // Ex) Delete `B` in `class Foo(A, B)`. - let mut tokenizer = SimpleTokenizer::starts_at(prev.end(), source); - - // Find the trailing comma. - let comma = tokenizer - .find(|token| token.kind == SimpleTokenKind::Comma) - .context("Unable to find trailing comma")?; - - Ok(Edit::deletion(comma.start(), base.end())) } } From 28fad8475de1fe6b2a31cead93e460dec5197a32 Mon Sep 17 00:00:00 2001 From: Gilles Peiffer Date: Sat, 29 Jun 2024 16:48:07 +0200 Subject: [PATCH 3/4] Clarify `parentheses` parameter --- crates/ruff_linter/src/fix/edits.rs | 6 +++--- .../ruff_linter/src/rules/pylint/rules/duplicate_bases.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ruff_linter/src/fix/edits.rs b/crates/ruff_linter/src/fix/edits.rs index 0901a9f694a2f..6de9c25420ded 100644 --- a/crates/ruff_linter/src/fix/edits.rs +++ b/crates/ruff_linter/src/fix/edits.rs @@ -202,11 +202,11 @@ pub(crate) enum Parentheses { } /// Generic function to remove arguments or keyword arguments in function -/// calls and class definitions. (For classes `args` should be considered -/// `bases`) +/// calls and class definitions. (For classes, `args` should be considered +/// `bases`.) /// /// Supports the removal of parentheses when this is the only (kw)arg left. -/// For this behavior, set `remove_parentheses` to `true`. +/// For this behavior, set `parentheses` to `Parentheses::Remove`. pub(crate) fn remove_argument( argument: &T, arguments: &Arguments, diff --git a/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs b/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs index 66e97e6ac0f75..d92e87a3cd48b 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs @@ -78,7 +78,7 @@ pub(crate) fn duplicate_bases(checker: &mut Checker, name: &str, arguments: Opti remove_argument( base, arguments.unwrap(), - Parentheses::Preserve, + Parentheses::Remove, checker.locator().contents(), ) .map(Fix::safe_edit) From 25f7ee6673d479754ce229bc79e59ab7c80a2e26 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 29 Jun 2024 13:44:30 -0400 Subject: [PATCH 4/4] Avoid unwrap --- .../ruff_linter/src/rules/pylint/rules/duplicate_bases.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs b/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs index d92e87a3cd48b..890ca16cdc53d 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/duplicate_bases.rs @@ -58,9 +58,10 @@ impl Violation for DuplicateBases { /// PLE0241 pub(crate) fn duplicate_bases(checker: &mut Checker, name: &str, arguments: Option<&Arguments>) { - let Some(Arguments { args: bases, .. }) = arguments else { + let Some(arguments) = arguments else { return; }; + let bases = &arguments.args; let mut seen: FxHashSet<&str> = FxHashSet::with_capacity_and_hasher(bases.len(), FxBuildHasher); for base in bases.iter() { @@ -73,11 +74,10 @@ pub(crate) fn duplicate_bases(checker: &mut Checker, name: &str, arguments: Opti }, base.range(), ); - diagnostic.try_set_fix(|| { remove_argument( base, - arguments.unwrap(), + arguments, Parentheses::Remove, checker.locator().contents(), )