Audit remove_argument usages to use end-of-function (#5480)

## Summary This PR applies the fix in #5478 to a variety of other call-sites, and fixes some other range hygienic stuff in the rules that were modified.
astral-sh · Jul 3, 2023 · d2450c2 · d2450c2
1 parent 1e4b889
commit d2450c2
Show file tree

Hide file tree

Showing 13 changed files with 247 additions and 317 deletions.
diff --git a/crates/ruff/resources/test/fixtures/pandas_vet/PD002.py b/crates/ruff/resources/test/fixtures/pandas_vet/PD002.py
@@ -4,7 +4,9 @@
 
 x.drop(["a"], axis=1, inplace=True)
 
-x.drop(["a"], axis=1, inplace=True)
+x.y.drop(["a"], axis=1, inplace=True)
+
+x["y"].drop(["a"], axis=1, inplace=True)
 
 x.drop(
     inplace=True,
@@ -23,6 +25,7 @@
 x.drop(["a"], axis=1, inplace=True, **kwargs)
 f(x.drop(["a"], axis=1, inplace=True))
 
-x.apply(lambda x: x.sort_values('a', inplace=True))
+x.apply(lambda x: x.sort_values("a", inplace=True))
 import torch
-torch.m.ReLU(inplace=True) # safe because this isn't a pandas call
+
+torch.m.ReLU(inplace=True)  # safe because this isn't a pandas call
diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs
@@ -668,21 +668,17 @@ where
                 }
                 if !self.is_stub {
                     if self.enabled(Rule::DjangoModelWithoutDunderStr) {
-                        if let Some(diagnostic) =
-                            flake8_django::rules::model_without_dunder_str(self, bases, body, stmt)
-                        {
-                            self.diagnostics.push(diagnostic);
-                        }
+                        flake8_django::rules::model_without_dunder_str(self, class_def);
                     }
                 }
                 if self.enabled(Rule::GlobalStatement) {
                     pylint::rules::global_statement(self, name);
                 }
                 if self.enabled(Rule::UselessObjectInheritance) {
-                    pyupgrade::rules::useless_object_inheritance(self, class_def, stmt);
+                    pyupgrade::rules::useless_object_inheritance(self, class_def);
                 }
                 if self.enabled(Rule::UnnecessaryClassParentheses) {
-                    pyupgrade::rules::unnecessary_class_parentheses(self, class_def, stmt);
+                    pyupgrade::rules::unnecessary_class_parentheses(self, class_def);
                 }
                 if self.enabled(Rule::AmbiguousClassName) {
                     if let Some(diagnostic) =
@@ -2756,17 +2752,12 @@ where
                     flake8_debugger::rules::debugger_call(self, expr, func);
                 }
                 if self.enabled(Rule::PandasUseOfInplaceArgument) {
-                    self.diagnostics.extend(
-                        pandas_vet::rules::inplace_argument(self, expr, func, args, keywords)
-                            .into_iter(),
-                    );
+                    pandas_vet::rules::inplace_argument(self, expr, func, args, keywords);
                 }
                 pandas_vet::rules::call(self, func);
 
                 if self.enabled(Rule::PandasUseOfPdMerge) {
-                    if let Some(diagnostic) = pandas_vet::rules::use_of_pd_merge(func) {
-                        self.diagnostics.push(diagnostic);
-                    };
+                    pandas_vet::rules::use_of_pd_merge(self, func);
                 }
                 if self.enabled(Rule::CallDatetimeWithoutTzinfo) {
                     flake8_datetimez::rules::call_datetime_without_tzinfo(

diff --git a/crates/ruff/src/rules/flake8_django/rules/model_without_dunder_str.rs b/crates/ruff/src/rules/flake8_django/rules/model_without_dunder_str.rs
@@ -52,21 +52,20 @@ impl Violation for DjangoModelWithoutDunderStr {
 
 /// DJ008
 pub(crate) fn model_without_dunder_str(
-    checker: &Checker,
-    bases: &[Expr],
-    body: &[Stmt],
-    class_location: &Stmt,
-) -> Option<Diagnostic> {
+    checker: &mut Checker,
+    ast::StmtClassDef {
+        name, bases, body, ..
+    }: &ast::StmtClassDef,
+) {
     if !is_non_abstract_model(bases, body, checker.semantic()) {
-        return None;
+        return;
     }
-    if !has_dunder_method(body) {
-        return Some(Diagnostic::new(
-            DjangoModelWithoutDunderStr,
-            class_location.range(),
-        ));
+    if has_dunder_method(body) {
+        return;
     }
-    None
+    checker
+        .diagnostics
+        .push(Diagnostic::new(DjangoModelWithoutDunderStr, name.range()));
 }
 
 fn has_dunder_method(body: &[Stmt]) -> bool {

diff --git a/.../src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ008_DJ008.py.snap b/.../src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ008_DJ008.py.snap
@@ -1,58 +1,26 @@
 ---
 source: crates/ruff/src/rules/flake8_django/mod.rs
 ---
-DJ008.py:6:1: DJ008 Model does not define `__str__` method
-   |
- 5 |   # Models without __str__
- 6 | / class TestModel1(models.Model):
- 7 | |     new_field = models.CharField(max_length=10)
- 8 | | 
- 9 | |     class Meta:
-10 | |         verbose_name = "test model"
-11 | |         verbose_name_plural = "test models"
-12 | | 
-13 | |     @property
-14 | |     def my_brand_new_property(self):
-15 | |         return 1
-16 | | 
-17 | |     def my_beautiful_method(self):
-18 | |         return 2
-   | |________________^ DJ008
-   |
+DJ008.py:6:7: DJ008 Model does not define `__str__` method
+  |
+5 | # Models without __str__
+6 | class TestModel1(models.Model):
+  |       ^^^^^^^^^^ DJ008
+7 |     new_field = models.CharField(max_length=10)
+  |
 
-DJ008.py:21:1: DJ008 Model does not define `__str__` method
+DJ008.py:21:7: DJ008 Model does not define `__str__` method
    |
-21 | / class TestModel2(Model):
-22 | |     new_field = models.CharField(max_length=10)
-23 | | 
-24 | |     class Meta:
-25 | |         verbose_name = "test model"
-26 | |         verbose_name_plural = "test models"
-27 | | 
-28 | |     @property
-29 | |     def my_brand_new_property(self):
-30 | |         return 1
-31 | | 
-32 | |     def my_beautiful_method(self):
-33 | |         return 2
-   | |________________^ DJ008
+21 | class TestModel2(Model):
+   |       ^^^^^^^^^^ DJ008
+22 |     new_field = models.CharField(max_length=10)
    |
 
-DJ008.py:36:1: DJ008 Model does not define `__str__` method
+DJ008.py:36:7: DJ008 Model does not define `__str__` method
    |
-36 | / class TestModel3(Model):
-37 | |     new_field = models.CharField(max_length=10)
-38 | | 
-39 | |     class Meta:
-40 | |         abstract = False
-41 | | 
-42 | |     @property
-43 | |     def my_brand_new_property(self):
-44 | |         return 1
-45 | | 
-46 | |     def my_beautiful_method(self):
-47 | |         return 2
-   | |________________^ DJ008
+36 | class TestModel3(Model):
+   |       ^^^^^^^^^^ DJ008
+37 |     new_field = models.CharField(max_length=10)
    |
 
 
diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/fixture.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/fixture.rs
@@ -1,17 +1,15 @@
 use std::fmt;
 
-use anyhow::Result;
-use ruff_text_size::{TextLen, TextRange, TextSize};
+use ruff_text_size::{TextLen, TextRange};
 use rustpython_parser::ast::Decorator;
-use rustpython_parser::ast::{self, ArgWithDefault, Arguments, Expr, Keyword, Ranged, Stmt};
+use rustpython_parser::ast::{self, ArgWithDefault, Arguments, Expr, Ranged, Stmt};
 
 use ruff_diagnostics::{AlwaysAutofixableViolation, Violation};
 use ruff_diagnostics::{Diagnostic, Edit, Fix};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::call_path::collect_call_path;
 use ruff_python_ast::helpers::collect_arg_names;
 use ruff_python_ast::identifier::Identifier;
-use ruff_python_ast::source_code::Locator;
 use ruff_python_ast::visitor;
 use ruff_python_ast::visitor::Visitor;
 use ruff_python_semantic::analyze::visibility::is_abstract;
@@ -25,21 +23,6 @@ use super::helpers::{
     get_mark_decorators, is_pytest_fixture, is_pytest_yield_fixture, keyword_is_literal,
 };
 
-#[derive(Debug, PartialEq, Eq)]
-pub(crate) enum Parentheses {
-    None,
-    Empty,
-}
-
-impl fmt::Display for Parentheses {
-    fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
-        match self {
-            Parentheses::None => fmt.write_str(""),
-            Parentheses::Empty => fmt.write_str("()"),
-        }
-    }
-}
-
 #[violation]
 pub struct PytestFixtureIncorrectParenthesesStyle {
     expected: Parentheses,
@@ -196,8 +179,23 @@ impl AlwaysAutofixableViolation for PytestUnnecessaryAsyncioMarkOnFixture {
     }
 }
 
-#[derive(Default)]
+#[derive(Debug, PartialEq, Eq)]
+enum Parentheses {
+    None,
+    Empty,
+}
+
+impl fmt::Display for Parentheses {
+    fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            Parentheses::None => fmt.write_str(""),
+            Parentheses::Empty => fmt.write_str("()"),
+        }
+    }
+}
+
 /// Visitor that skips functions
+#[derive(Debug, Default)]
 struct SkipFunctionsVisitor<'a> {
     has_return_with_value: bool,
     has_yield_from: bool,
@@ -245,7 +243,7 @@ where
     }
 }
 
-fn get_fixture_decorator<'a>(
+fn fixture_decorator<'a>(
     decorators: &'a [Decorator],
     semantic: &SemanticModel,
 ) -> Option<&'a Decorator> {
@@ -271,16 +269,6 @@ fn pytest_fixture_parentheses(
     checker.diagnostics.push(diagnostic);
 }
 
-pub(crate) fn fix_extraneous_scope_function(
-    locator: &Locator,
-    stmt_at: TextSize,
-    expr_range: TextRange,
-    args: &[Expr],
-    keywords: &[Keyword],
-) -> Result<Edit> {
-    remove_argument(locator, stmt_at, expr_range, args, keywords, false)
-}
-
 /// PT001, PT002, PT003
 fn check_fixture_decorator(checker: &mut Checker, func_name: &str, decorator: &Decorator) {
     match &decorator.expression {
@@ -290,28 +278,31 @@ fn check_fixture_decorator(checker: &mut Checker, func_name: &str, decorator: &D
             keywords,
             range: _,
         }) => {
-            if checker.enabled(Rule::PytestFixtureIncorrectParenthesesStyle)
-                && !checker.settings.flake8_pytest_style.fixture_parentheses
-                && args.is_empty()
-                && keywords.is_empty()
-            {
-                let fix = Fix::automatic(Edit::deletion(func.end(), decorator.end()));
-                pytest_fixture_parentheses(
-                    checker,
-                    decorator,
-                    fix,
-                    Parentheses::None,
-                    Parentheses::Empty,
-                );
+            if checker.enabled(Rule::PytestFixtureIncorrectParenthesesStyle) {
+                if !checker.settings.flake8_pytest_style.fixture_parentheses
+                    && args.is_empty()
+                    && keywords.is_empty()
+                {
+                    let fix = Fix::automatic(Edit::deletion(func.end(), decorator.end()));
+                    pytest_fixture_parentheses(
+                        checker,
+                        decorator,
+                        fix,
+                        Parentheses::None,
+                        Parentheses::Empty,
+                    );
+                }
             }
 
-            if checker.enabled(Rule::PytestFixturePositionalArgs) && !args.is_empty() {
-                checker.diagnostics.push(Diagnostic::new(
-                    PytestFixturePositionalArgs {
-                        function: func_name.to_string(),
-                    },
-                    decorator.range(),
-                ));
+            if checker.enabled(Rule::PytestFixturePositionalArgs) {
+                if !args.is_empty() {
+                    checker.diagnostics.push(Diagnostic::new(
+                        PytestFixturePositionalArgs {
+                            function: func_name.to_string(),
+                        },
+                        decorator.range(),
+                    ));
+                }
             }
 
             if checker.enabled(Rule::PytestExtraneousScopeFunction) {
@@ -324,16 +315,16 @@ fn check_fixture_decorator(checker: &mut Checker, func_name: &str, decorator: &D
                         let mut diagnostic =
                             Diagnostic::new(PytestExtraneousScopeFunction, scope_keyword.range());
                         if checker.patch(diagnostic.kind.rule()) {
-                            let expr_range = diagnostic.range();
-                            #[allow(deprecated)]
-                            diagnostic.try_set_fix_from_edit(|| {
-                                fix_extraneous_scope_function(
+                            diagnostic.try_set_fix(|| {
+                                remove_argument(
                                     checker.locator,
-                                    decorator.start(),
-                                    expr_range,
+                                    func.end(),
+                                    scope_keyword.range,
                                     args,
                                     keywords,
+                                    false,
                                 )
+                                .map(Fix::suggested)
                             });
                         }
                         checker.diagnostics.push(diagnostic);
@@ -342,20 +333,20 @@ fn check_fixture_decorator(checker: &mut Checker, func_name: &str, decorator: &D
             }
         }
         _ => {
-            if checker.enabled(Rule::PytestFixtureIncorrectParenthesesStyle)
-                && checker.settings.flake8_pytest_style.fixture_parentheses
-            {
-                let fix = Fix::automatic(Edit::insertion(
-                    Parentheses::Empty.to_string(),
-                    decorator.end(),
-                ));
-                pytest_fixture_parentheses(
-                    checker,
-                    decorator,
-                    fix,
-                    Parentheses::Empty,
-                    Parentheses::None,
-                );
+            if checker.enabled(Rule::PytestFixtureIncorrectParenthesesStyle) {
+                if checker.settings.flake8_pytest_style.fixture_parentheses {
+                    let fix = Fix::automatic(Edit::insertion(
+                        Parentheses::Empty.to_string(),
+                        decorator.end(),
+                    ));
+                    pytest_fixture_parentheses(
+                        checker,
+                        decorator,
+                        fix,
+                        Parentheses::Empty,
+                        Parentheses::None,
+                    );
+                }
             }
         }
     }
@@ -511,7 +502,7 @@ pub(crate) fn fixture(
     decorators: &[Decorator],
     body: &[Stmt],
 ) {
-    let decorator = get_fixture_decorator(decorators, checker.semantic());
+    let decorator = fixture_decorator(decorators, checker.semantic());
     if let Some(decorator) = decorator {
         if checker.enabled(Rule::PytestFixtureIncorrectParenthesesStyle)
             || checker.enabled(Rule::PytestFixturePositionalArgs)