From 3238a9b8baa67e850576ae7dbc22eb073b4fa8ec Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Wed, 25 Sep 2024 19:02:13 -0500 Subject: [PATCH] Fix handling of slices in tuples for FURB118, e.g., `x[:, 1]` There was already handling for the singleton `x[:]` case but not the tuple case.\nCloses https://github.com/astral-sh/ruff/issues/13508 --- .../resources/test/fixtures/refurb/FURB118.py | 10 ++ .../refurb/rules/reimplemented_operator.rs | 14 ++- ...es__refurb__tests__FURB118_FURB118.py.snap | 99 +++++++++++++++++++ 3 files changed, 122 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py index 562efacf852302..52c8c0ac205d5f 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py @@ -83,3 +83,13 @@ class Class: @staticmethod def add(x, y): return x + y + +# See https://github.com/astral-sh/ruff/issues/13508 +op_itemgetter = lambda x: x[:, 1] +op_itemgetter = lambda x: x[1, :] + +# With a slice, trivia is dropped +op_itemgetter = lambda x: x[1, :] + +# Without a slice, trivia is retained +op_itemgetter = lambda x: x[1, 2] diff --git a/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs b/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs index 8d550e22d18cbe..8cd14f625be14f 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs @@ -3,6 +3,7 @@ use std::fmt::{Debug, Display, Formatter}; use anyhow::Result; +use itertools::Itertools; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::any_over_expr; @@ -213,7 +214,18 @@ fn subscript_slice_to_string<'a>(expr: &Expr, locator: &Locator<'a>) -> Cow<'a, if let Expr::Slice(expr_slice) = expr { Cow::Owned(slice_expr_to_slice_call(expr_slice, locator)) } else if let Expr::Tuple(tuple) = expr { - if tuple.parenthesized { + if locator.slice(tuple).contains(':') { + // We cannot perform a trivial replacement if there's a `:` in the expression + let inner = tuple + .iter() + .map(|expr| match expr { + Expr::Slice(expr) => Cow::Owned(slice_expr_to_slice_call(expr, locator)), + _ => Cow::Borrowed(locator.slice(expr)), + }) + .join(", "); + + Cow::Owned(format!("({inner})")) + } else if tuple.parenthesized { Cow::Borrowed(locator.slice(expr)) } else { Cow::Owned(format!("({})", locator.slice(tuple))) diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB118_FURB118.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB118_FURB118.py.snap index fdfe0dbfbe8187..14a15778699083 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB118_FURB118.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB118_FURB118.py.snap @@ -847,3 +847,102 @@ FURB118.py:42:5: FURB118 Use `operator.add` instead of defining a function 43 | return x + y | = help: Replace with `operator.add` + +FURB118.py:88:17: FURB118 [*] Use `operator.itemgetter((slice(None), 1))` instead of defining a lambda + | +87 | # See https://github.com/astral-sh/ruff/issues/13508 +88 | op_itemgetter = lambda x: x[:, 1] + | ^^^^^^^^^^^^^^^^^ FURB118 +89 | op_itemgetter = lambda x: x[1, :] + | + = help: Replace with `operator.itemgetter((slice(None), 1))` + +ℹ Safe fix +1 1 | # Errors. + 2 |+import operator +2 3 | op_bitnot = lambda x: ~x +3 4 | op_not = lambda x: not x +4 5 | op_pos = lambda x: +x +-------------------------------------------------------------------------------- +85 86 | return x + y +86 87 | +87 88 | # See https://github.com/astral-sh/ruff/issues/13508 +88 |-op_itemgetter = lambda x: x[:, 1] + 89 |+op_itemgetter = operator.itemgetter((slice(None), 1)) +89 90 | op_itemgetter = lambda x: x[1, :] +90 91 | +91 92 | # With a slice, trivia is dropped + +FURB118.py:89:17: FURB118 [*] Use `operator.itemgetter((1, slice(None)))` instead of defining a lambda + | +87 | # See https://github.com/astral-sh/ruff/issues/13508 +88 | op_itemgetter = lambda x: x[:, 1] +89 | op_itemgetter = lambda x: x[1, :] + | ^^^^^^^^^^^^^^^^^ FURB118 +90 | +91 | # With a slice, trivia is dropped + | + = help: Replace with `operator.itemgetter((1, slice(None)))` + +ℹ Safe fix +1 1 | # Errors. + 2 |+import operator +2 3 | op_bitnot = lambda x: ~x +3 4 | op_not = lambda x: not x +4 5 | op_pos = lambda x: +x +-------------------------------------------------------------------------------- +86 87 | +87 88 | # See https://github.com/astral-sh/ruff/issues/13508 +88 89 | op_itemgetter = lambda x: x[:, 1] +89 |-op_itemgetter = lambda x: x[1, :] + 90 |+op_itemgetter = operator.itemgetter((1, slice(None))) +90 91 | +91 92 | # With a slice, trivia is dropped +92 93 | op_itemgetter = lambda x: x[1, :] + +FURB118.py:92:17: FURB118 [*] Use `operator.itemgetter((1, slice(None)))` instead of defining a lambda + | +91 | # With a slice, trivia is dropped +92 | op_itemgetter = lambda x: x[1, :] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB118 +93 | +94 | # Without a slice, trivia is retained + | + = help: Replace with `operator.itemgetter((1, slice(None)))` + +ℹ Safe fix +1 1 | # Errors. + 2 |+import operator +2 3 | op_bitnot = lambda x: ~x +3 4 | op_not = lambda x: not x +4 5 | op_pos = lambda x: +x +-------------------------------------------------------------------------------- +89 90 | op_itemgetter = lambda x: x[1, :] +90 91 | +91 92 | # With a slice, trivia is dropped +92 |-op_itemgetter = lambda x: x[1, :] + 93 |+op_itemgetter = operator.itemgetter((1, slice(None))) +93 94 | +94 95 | # Without a slice, trivia is retained +95 96 | op_itemgetter = lambda x: x[1, 2] + +FURB118.py:95:17: FURB118 [*] Use `operator.itemgetter((1, 2))` instead of defining a lambda + | +94 | # Without a slice, trivia is retained +95 | op_itemgetter = lambda x: x[1, 2] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB118 + | + = help: Replace with `operator.itemgetter((1, 2))` + +ℹ Safe fix +1 1 | # Errors. + 2 |+import operator +2 3 | op_bitnot = lambda x: ~x +3 4 | op_not = lambda x: not x +4 5 | op_pos = lambda x: +x +-------------------------------------------------------------------------------- +92 93 | op_itemgetter = lambda x: x[1, :] +93 94 | +94 95 | # Without a slice, trivia is retained +95 |-op_itemgetter = lambda x: x[1, 2] + 96 |+op_itemgetter = operator.itemgetter((1, 2))