Merge branch 'astral-sh:main' into pytlint/singledispatch-method-E1519

crates/ruff_linter/resources/test/fixtures/pylint/E1519.py

@@ -1,8 +1,21 @@
-from functools import singledispatch
+from functools import singledispatch, singledispatchmethod
 
 
+@singledispatch
+def convert_position(position):
+    pass
+
 class Board:
     @singledispatch  # [singledispatch-method]
     @classmethod
     def convert_position(cls, position):
         pass
+
+
+    @singledispatch  # [singledispatch-method]
+    def get_position(self):
+        pass
+
+    @singledispatchmethod
+    def move(self, position):
+        pass

crates/ruff_linter/resources/test/fixtures/ruff/RUF015.py

@@ -57,6 +57,16 @@
 list(zip(x, y))[0]
 [*zip(x, y)][0]
 
+# RUF015 (pop)
+list(x).pop(0)
+[i for i in x].pop(0)
+list(i for i in x).pop(0)
+
+# OK
+list(x).pop(1)
+list(x).remove(0)
+list(x).remove(1)
+
 
 def test():
     zip = list  # Overwrite the builtin zip

crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs

@@ -392,7 +392,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
             }
 
             if checker.enabled(Rule::SingleDispatchMethod) {
-                pylint::rules::single_dispatch_method(checker, scope_id, &mut diagnostics);
+                pylint::rules::single_dispatch_method(checker, scope, &mut diagnostics);
             }
         }
     }

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

@@ -116,7 +116,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
                 flake8_simplify::rules::use_capital_environment_variables(checker, expr);
             }
             if checker.enabled(Rule::UnnecessaryIterableAllocationForFirstElement) {
-                ruff::rules::unnecessary_iterable_allocation_for_first_element(checker, subscript);
+                ruff::rules::unnecessary_iterable_allocation_for_first_element(checker, expr);
             }
             if checker.enabled(Rule::InvalidIndexType) {
                 ruff::rules::invalid_index_type(checker, subscript);
@@ -965,6 +965,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
             if checker.enabled(Rule::DefaultFactoryKwarg) {
                 ruff::rules::default_factory_kwarg(checker, call);
             }
+            if checker.enabled(Rule::UnnecessaryIterableAllocationForFirstElement) {
+                ruff::rules::unnecessary_iterable_allocation_for_first_element(checker, expr);
+            }
         }
         Expr::Dict(dict) => {
             if checker.any_enabled(&[

crates/ruff_linter/src/rules/pylint/rules/single_dispatch_method.rs

@@ -1,11 +1,16 @@
 use ruff_diagnostics::{Diagnostic, FixAvailability, Violation};
 use ruff_macros::{derive_message_formats, violation};
-use ruff_python_semantic::ScopeId;
-
-use ruff_source_file::SourceRow;
+use ruff_python_ast::{self as ast, ParameterWithDefault};
+use ruff_python_semantic::{analyze::function_type, Scope};
+use ruff_text_size::Ranged;
 
 use crate::checkers::ast::Checker;
 
+/// ## What it does
+/// Checks for singledispatch decorators on class methods.
+///
+/// ## Why is this bad?
+/// Single dispatch must happen on the type of first non self argument
 #[violation]
 pub struct SingleDispatchMethod {}
 
@@ -14,9 +19,53 @@ impl Violation for SingleDispatchMethod {
 
     #[derive_message_formats]
     fn message(&self) -> String {
-        format!("")
+        format!("singledispatch decorator should not be used with methods, use singledispatchmethod instead.")
     }
 }
 
 /// E1519
-pub(crate) fn single_dispatch_method(checker: &Checker, diagnostics: &mut Vec<Diagnostic>) {}
+pub(crate) fn single_dispatch_method(
+    checker: &Checker,
+    scope: &Scope,
+    diagnostics: &mut Vec<Diagnostic>,
+) {
+    let Some(func) = scope.kind.as_function() else {
+        return;
+    };
+
+    let ast::StmtFunctionDef {
+        name,
+        decorator_list,
+        ..
+    } = func;
+
+    let Some(parent) = &checker.semantic().first_non_type_parent_scope(scope) else {
+        return;
+    };
+
+    if !matches!(
+        function_type::classify(
+            name,
+            decorator_list,
+            parent,
+            checker.semantic(),
+            &checker.settings.pep8_naming.classmethod_decorators,
+            &checker.settings.pep8_naming.staticmethod_decorators,
+        ),
+        function_type::FunctionType::Method | function_type::FunctionType::ClassMethod
+    ) {
+        return;
+    }
+
+    for decorator in decorator_list {
+        if checker
+            .semantic()
+            .resolve_call_path(&decorator.expression)
+            .is_some_and(|call_path| {
+                matches!(call_path.as_slice(), ["functools", "singledispatch"])
+            })
+        {
+            diagnostics.push(Diagnostic::new(SingleDispatchMethod {}, decorator.range()));
+        }
+    }
+}

crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1519_E1519.py.snap

@@ -0,0 +1,19 @@
+---
+source: crates/ruff_linter/src/rules/pylint/mod.rs
+---
+E1519.py:9:5: PLE1519 singledispatch decorator should not be used with methods, use singledispatchmethod instead.
+   |
+ 8 | class Board:
+ 9 |     @singledispatch  # [singledispatch-method]
+   |     ^^^^^^^^^^^^^^^ PLE1519
+10 |     @classmethod
+11 |     def convert_position(cls, position):
+   |
+
+E1519.py:15:5: PLE1519 singledispatch decorator should not be used with methods, use singledispatchmethod instead.
+   |
+15 |     @singledispatch  # [singledispatch-method]
+   |     ^^^^^^^^^^^^^^^ PLE1519
+16 |     def get_position(self):
+17 |         pass
+   |

crates/ruff_linter/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs

@@ -11,14 +11,21 @@ use crate::checkers::ast::Checker;
 use crate::fix::snippet::SourceCodeSnippet;
 
 /// ## What it does
-/// Checks for uses of `list(...)[0]` that can be replaced with
-/// `next(iter(...))`.
+/// Checks the following constructs, all of which can be replaced by
+/// `next(iter(...))`:
+///
+/// - `list(...)[0]`
+/// - `tuple(...)[0]`
+/// - `list(i for i in ...)[0]`
+/// - `[i for i in ...][0]`
+/// - `list(...).pop(0)`
 ///
 /// ## Why is this bad?
-/// Calling `list(...)` will create a new list of the entire collection, which
-/// can be very expensive for large collections. If you only need the first
-/// element of the collection, you can use `next(...)` or `next(iter(...)` to
-/// lazily fetch the first element.
+/// Calling e.g. `list(...)` will create a new list of the entire collection,
+/// which can be very expensive for large collections. If you only need the
+/// first element of the collection, you can use `next(...)` or
+/// `next(iter(...)` to lazily fetch the first element. The same is true for
+/// the other constructs.
 ///
 /// ## Example
 /// ```python
@@ -33,14 +40,16 @@ use crate::fix::snippet::SourceCodeSnippet;
 /// ```
 ///
 /// ## Fix safety
-/// This rule's fix is marked as unsafe, as migrating from `list(...)[0]` to
-/// `next(iter(...))` can change the behavior of your program in two ways:
+/// This rule's fix is marked as unsafe, as migrating from (e.g.) `list(...)[0]`
+/// to `next(iter(...))` can change the behavior of your program in two ways:
 ///
-/// 1. First, `list(...)` will eagerly evaluate the entire collection, while
-///    `next(iter(...))` will only evaluate the first element. As such, any
-///    side effects that occur during iteration will be delayed.
-/// 2. Second, `list(...)[0]` will raise `IndexError` if the collection is
-///    empty, while `next(iter(...))` will raise `StopIteration`.
+/// 1. First, all above mentioned constructs will eagerly evaluate the entire
+///    collection, while `next(iter(...))` will only evaluate the first
+///    element. As such, any side effects that occur during iteration will be
+///    delayed.
+/// 2. Second, accessing members of a collection via square bracket notation
+///    `[0]` of the `pop()` function will raise `IndexError` if the collection
+///    is empty, while `next(iter(...))` will raise `StopIteration`.
 ///
 /// ## References
 /// - [Iterators and Iterables in Python: Run Efficient Iterations](https://realpython.com/python-iterators-iterables/#when-to-use-an-iterator-in-python)
@@ -67,18 +76,39 @@ impl AlwaysFixableViolation for UnnecessaryIterableAllocationForFirstElement {
 /// RUF015
 pub(crate) fn unnecessary_iterable_allocation_for_first_element(
     checker: &mut Checker,
-    subscript: &ast::ExprSubscript,
+    expr: &Expr,
 ) {
-    let ast::ExprSubscript {
-        value,
-        slice,
-        range,
-        ..
-    } = subscript;
-
-    if !is_head_slice(slice) {
-        return;
-    }
+    let value = match expr {
+        // Ex) `list(x)[0]`
+        Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => {
+            if !is_zero(slice) {
+                return;
+            }
+            value
+        }
+        // Ex) `list(x).pop(0)`
+        Expr::Call(ast::ExprCall {
+            func, arguments, ..
+        }) => {
+            if !arguments.keywords.is_empty() {
+                return;
+            }
+            let [arg] = arguments.args.as_ref() else {
+                return;
+            };
+            if !is_zero(arg) {
+                return;
+            }
+            let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else {
+                return;
+            };
+            if !matches!(attr.as_str(), "pop") {
+                return;
+            }
+            value
+        }
+        _ => return,
+    };
 
     let Some(target) = match_iteration_target(value, checker.semantic()) else {
         return;
@@ -94,19 +124,19 @@ pub(crate) fn unnecessary_iterable_allocation_for_first_element(
         UnnecessaryIterableAllocationForFirstElement {
             iterable: SourceCodeSnippet::new(iterable.to_string()),
         },
-        *range,
+        expr.range(),
     );
 
     diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
         format!("next({iterable})"),
-        *range,
+        expr.range(),
     )));
 
     checker.diagnostics.push(diagnostic);
 }
 
 /// Check that the slice [`Expr`] is a slice of the first element (e.g., `x[0]`).
-fn is_head_slice(expr: &Expr) -> bool {
+fn is_zero(expr: &Expr) -> bool {
     matches!(
         expr,
         Expr::NumberLiteral(ast::ExprNumberLiteral {