From 8a311a9b2be00fba131fc347715ae4c3fbd959d2 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 19 Sep 2023 15:59:52 -0400 Subject: [PATCH] Allow fix with start --- .../resources/test/fixtures/refurb/FURB148.py | 12 + .../refurb/rules/unnecessary_enumerate.rs | 98 +++---- ...es__refurb__tests__FURB148_FURB148.py.snap | 256 +++++++++++------- 3 files changed, 211 insertions(+), 155 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/refurb/FURB148.py b/crates/ruff/resources/test/fixtures/refurb/FURB148.py index 0c6b40137c8b1c..75f2c8a9006033 100644 --- a/crates/ruff/resources/test/fixtures/refurb/FURB148.py +++ b/crates/ruff/resources/test/fixtures/refurb/FURB148.py @@ -16,6 +16,12 @@ for index, _ in enumerate(books, 1): print(index) +for index, _ in enumerate(books, start=x): + print(book) + +for index, _ in enumerate(books, x): + print(book) + for _, book in enumerate(books): print(book) @@ -31,6 +37,12 @@ for _, book in enumerate(books, 1): print(book) +for _, book in enumerate(books, start=x): + print(book) + +for _, book in enumerate(books, x): + print(book) + for index, (_, _) in enumerate(books): print(index) diff --git a/crates/ruff/src/rules/refurb/rules/unnecessary_enumerate.rs b/crates/ruff/src/rules/refurb/rules/unnecessary_enumerate.rs index fd958569a654cc..80f0078def546d 100644 --- a/crates/ruff/src/rules/refurb/rules/unnecessary_enumerate.rs +++ b/crates/ruff/src/rules/refurb/rules/unnecessary_enumerate.rs @@ -1,6 +1,6 @@ use std::fmt; -use num_traits::ToPrimitive; +use num_traits::{ToPrimitive, Zero}; use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; @@ -130,20 +130,16 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF // The index is unused, so replace with `for value in sequence`. if checker.patch(diagnostic.kind.rule()) { - // Get the `start` argument, if it is a constant integer. - if start(arguments).map_or(true, |start| start == 0) { - let replace_iter = - Edit::range_replacement(sequence.into(), stmt_for.iter.range()); - let replace_target = Edit::range_replacement( - pad( - checker.locator().slice(value).to_string(), - stmt_for.target.range(), - checker.locator(), - ), + let replace_iter = Edit::range_replacement(sequence.into(), stmt_for.iter.range()); + let replace_target = Edit::range_replacement( + pad( + checker.locator().slice(value).to_string(), stmt_for.target.range(), - ); - diagnostic.set_fix(Fix::suggested_edits(replace_iter, [replace_target])); - } + checker.locator(), + ), + stmt_for.target.range(), + ); + diagnostic.set_fix(Fix::suggested_edits(replace_iter, [replace_target])); } checker.diagnostics.push(diagnostic); @@ -156,25 +152,38 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF }, func.range(), ); - if checker.patch(diagnostic.kind.rule()) { - // Get the `start` argument, if it is a constant integer. - let start = start(arguments); - - let replace_iter = Edit::range_replacement( - generate_range_len_call(sequence, start, checker.generator()), - stmt_for.iter.range(), - ); + if checker.patch(diagnostic.kind.rule()) + && checker.semantic().is_builtin("range") + && checker.semantic().is_builtin("len") + { + // If the `start` argument is set to something other than the `range` default, + // there's no clear fix. + if arguments.find_argument("start", 1).map_or(true, |start| { + let Expr::Constant(ast::ExprConstant { + value: Constant::Int(value), + .. + }) = start + else { + return false; + }; + value.is_zero() + }) { + let replace_iter = Edit::range_replacement( + generate_range_len_call(sequence, checker.generator()), + stmt_for.iter.range(), + ); - let replace_target = Edit::range_replacement( - pad( - checker.locator().slice(index).to_string(), + let replace_target = Edit::range_replacement( + pad( + checker.locator().slice(index).to_string(), + stmt_for.target.range(), + checker.locator(), + ), stmt_for.target.range(), - checker.locator(), - ), - stmt_for.target.range(), - ); + ); - diagnostic.set_fix(Fix::suggested_edits(replace_iter, [replace_target])); + diagnostic.set_fix(Fix::suggested_edits(replace_iter, [replace_target])); + } } checker.diagnostics.push(diagnostic); } @@ -198,24 +207,9 @@ impl fmt::Display for EnumerateSubset { } } -/// Returns the value of the `start` argument to `enumerate`, if it is a -/// constant integer. Otherwise, returns `None`. -fn start(arguments: &Arguments) -> Option { - let step_param = arguments.find_argument("start", 1)?; - if let Expr::Constant(ast::ExprConstant { - value: Constant::Int(value), - .. - }) = &step_param - { - value.to_u32() - } else { - None - } -} - /// Format a code snippet to call `range(len(name))`, where `name` is the given /// sequence name. -fn generate_range_len_call(name: &str, start: Option, generator: Generator) -> String { +fn generate_range_len_call(name: &str, generator: Generator) -> String { // Construct `name`. let var = ast::ExprName { id: name.to_string(), @@ -240,16 +234,6 @@ fn generate_range_len_call(name: &str, start: Option, generator: Generator) range: TextRange::default(), }; // Construct `range(len(name))`. - let range_args: Vec = match start { - Some(start) if start > 0 => { - let start_expr: Expr = Expr::Constant(ast::ExprConstant { - range: TextRange::default(), - value: Constant::Int(start.into()), - }); - vec![start_expr, len.into()] - } - _ => vec![len.into()], - }; let range = ast::ExprCall { func: Box::new( ast::ExprName { @@ -260,7 +244,7 @@ fn generate_range_len_call(name: &str, start: Option, generator: Generator) .into(), ), arguments: Arguments { - args: range_args, + args: vec![len.into()], keywords: vec![], range: TextRange::default(), }, diff --git a/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB148_FURB148.py.snap b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB148_FURB148.py.snap index 2244c3577d4a40..1c4f0e78f1e8ab 100644 --- a/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB148_FURB148.py.snap +++ b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB148_FURB148.py.snap @@ -60,7 +60,7 @@ FURB148.py:10:17: FURB148 [*] `enumerate` value is unused, use `for x in range(l 12 12 | 13 13 | for index, _ in enumerate(books, start=1): -FURB148.py:13:17: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead +FURB148.py:13:17: FURB148 `enumerate` value is unused, use `for x in range(len(y))` instead | 11 | print(index) 12 | @@ -70,17 +70,7 @@ FURB148.py:13:17: FURB148 [*] `enumerate` value is unused, use `for x in range(l | = help: Replace with `range(len(...))` -ℹ Suggested fix -10 10 | for index, _ in enumerate(books, 0): -11 11 | print(index) -12 12 | -13 |-for index, _ in enumerate(books, start=1): - 13 |+for index in range(1, len(books)): -14 14 | print(index) -15 15 | -16 16 | for index, _ in enumerate(books, 1): - -FURB148.py:16:17: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead +FURB148.py:16:17: FURB148 `enumerate` value is unused, use `for x in range(len(y))` instead | 14 | print(index) 15 | @@ -90,174 +80,244 @@ FURB148.py:16:17: FURB148 [*] `enumerate` value is unused, use `for x in range(l | = help: Replace with `range(len(...))` -ℹ Suggested fix -13 13 | for index, _ in enumerate(books, start=1): -14 14 | print(index) -15 15 | -16 |-for index, _ in enumerate(books, 1): - 16 |+for index in range(1, len(books)): -17 17 | print(index) -18 18 | -19 19 | for _, book in enumerate(books): - -FURB148.py:19:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead +FURB148.py:19:17: FURB148 `enumerate` value is unused, use `for x in range(len(y))` instead | 17 | print(index) 18 | -19 | for _, book in enumerate(books): - | ^^^^^^^^^ FURB148 +19 | for index, _ in enumerate(books, start=x): + | ^^^^^^^^^ FURB148 20 | print(book) | - = help: Remove `enumerate` - -ℹ Suggested fix -16 16 | for index, _ in enumerate(books, 1): -17 17 | print(index) -18 18 | -19 |-for _, book in enumerate(books): - 19 |+for book in books: -20 20 | print(book) -21 21 | -22 22 | for _, book in enumerate(books, start=0): + = help: Replace with `range(len(...))` -FURB148.py:22:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead +FURB148.py:22:17: FURB148 `enumerate` value is unused, use `for x in range(len(y))` instead | 20 | print(book) 21 | -22 | for _, book in enumerate(books, start=0): - | ^^^^^^^^^ FURB148 +22 | for index, _ in enumerate(books, x): + | ^^^^^^^^^ FURB148 23 | print(book) | - = help: Remove `enumerate` - -ℹ Suggested fix -19 19 | for _, book in enumerate(books): -20 20 | print(book) -21 21 | -22 |-for _, book in enumerate(books, start=0): - 22 |+for book in books: -23 23 | print(book) -24 24 | -25 25 | for _, book in enumerate(books, 0): + = help: Replace with `range(len(...))` FURB148.py:25:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead | 23 | print(book) 24 | -25 | for _, book in enumerate(books, 0): +25 | for _, book in enumerate(books): | ^^^^^^^^^ FURB148 26 | print(book) | = help: Remove `enumerate` ℹ Suggested fix -22 22 | for _, book in enumerate(books, start=0): +22 22 | for index, _ in enumerate(books, x): 23 23 | print(book) 24 24 | -25 |-for _, book in enumerate(books, 0): +25 |-for _, book in enumerate(books): 25 |+for book in books: 26 26 | print(book) 27 27 | -28 28 | for _, book in enumerate(books, start=1): +28 28 | for _, book in enumerate(books, start=0): -FURB148.py:28:16: FURB148 `enumerate` index is unused, use `for x in y` instead +FURB148.py:28:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead | 26 | print(book) 27 | -28 | for _, book in enumerate(books, start=1): +28 | for _, book in enumerate(books, start=0): | ^^^^^^^^^ FURB148 29 | print(book) | = help: Remove `enumerate` -FURB148.py:31:16: FURB148 `enumerate` index is unused, use `for x in y` instead +ℹ Suggested fix +25 25 | for _, book in enumerate(books): +26 26 | print(book) +27 27 | +28 |-for _, book in enumerate(books, start=0): + 28 |+for book in books: +29 29 | print(book) +30 30 | +31 31 | for _, book in enumerate(books, 0): + +FURB148.py:31:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead | 29 | print(book) 30 | -31 | for _, book in enumerate(books, 1): +31 | for _, book in enumerate(books, 0): | ^^^^^^^^^ FURB148 32 | print(book) | = help: Remove `enumerate` -FURB148.py:34:22: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead +ℹ Suggested fix +28 28 | for _, book in enumerate(books, start=0): +29 29 | print(book) +30 30 | +31 |-for _, book in enumerate(books, 0): + 31 |+for book in books: +32 32 | print(book) +33 33 | +34 34 | for _, book in enumerate(books, start=1): + +FURB148.py:34:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead | 32 | print(book) 33 | -34 | for index, (_, _) in enumerate(books): - | ^^^^^^^^^ FURB148 -35 | print(index) +34 | for _, book in enumerate(books, start=1): + | ^^^^^^^^^ FURB148 +35 | print(book) | - = help: Replace with `range(len(...))` + = help: Remove `enumerate` ℹ Suggested fix -31 31 | for _, book in enumerate(books, 1): +31 31 | for _, book in enumerate(books, 0): 32 32 | print(book) 33 33 | -34 |-for index, (_, _) in enumerate(books): - 34 |+for index in range(len(books)): -35 35 | print(index) +34 |-for _, book in enumerate(books, start=1): + 34 |+for book in books: +35 35 | print(book) 36 36 | -37 37 | for (_, _), book in enumerate(books): +37 37 | for _, book in enumerate(books, 1): -FURB148.py:37:21: FURB148 [*] `enumerate` index is unused, use `for x in y` instead +FURB148.py:37:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead | -35 | print(index) +35 | print(book) 36 | -37 | for (_, _), book in enumerate(books): - | ^^^^^^^^^ FURB148 +37 | for _, book in enumerate(books, 1): + | ^^^^^^^^^ FURB148 38 | print(book) | = help: Remove `enumerate` ℹ Suggested fix -34 34 | for index, (_, _) in enumerate(books): -35 35 | print(index) +34 34 | for _, book in enumerate(books, start=1): +35 35 | print(book) 36 36 | -37 |-for (_, _), book in enumerate(books): +37 |-for _, book in enumerate(books, 1): 37 |+for book in books: 38 38 | print(book) 39 39 | -40 40 | for(index, _)in enumerate(books): +40 40 | for _, book in enumerate(books, start=x): -FURB148.py:40:17: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead +FURB148.py:40:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead | 38 | print(book) 39 | -40 | for(index, _)in enumerate(books): - | ^^^^^^^^^ FURB148 -41 | print(index) +40 | for _, book in enumerate(books, start=x): + | ^^^^^^^^^ FURB148 +41 | print(book) | - = help: Replace with `range(len(...))` + = help: Remove `enumerate` ℹ Suggested fix -37 37 | for (_, _), book in enumerate(books): +37 37 | for _, book in enumerate(books, 1): 38 38 | print(book) 39 39 | -40 |-for(index, _)in enumerate(books): - 40 |+for index in range(len(books)): -41 41 | print(index) +40 |-for _, book in enumerate(books, start=x): + 40 |+for book in books: +41 41 | print(book) 42 42 | -43 43 | for(index), _ in enumerate(books): +43 43 | for _, book in enumerate(books, x): -FURB148.py:43:18: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead +FURB148.py:43:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead | -41 | print(index) +41 | print(book) 42 | -43 | for(index), _ in enumerate(books): - | ^^^^^^^^^ FURB148 -44 | print(index) +43 | for _, book in enumerate(books, x): + | ^^^^^^^^^ FURB148 +44 | print(book) | - = help: Replace with `range(len(...))` + = help: Remove `enumerate` ℹ Suggested fix -40 40 | for(index, _)in enumerate(books): -41 41 | print(index) +40 40 | for _, book in enumerate(books, start=x): +41 41 | print(book) 42 42 | -43 |-for(index), _ in enumerate(books): - 43 |+for index in range(len(books)): -44 44 | print(index) +43 |-for _, book in enumerate(books, x): + 43 |+for book in books: +44 44 | print(book) +45 45 | +46 46 | for index, (_, _) in enumerate(books): + +FURB148.py:46:22: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead + | +44 | print(book) +45 | +46 | for index, (_, _) in enumerate(books): + | ^^^^^^^^^ FURB148 +47 | print(index) + | + = help: Replace with `range(len(...))` + +ℹ Suggested fix +43 43 | for _, book in enumerate(books, x): +44 44 | print(book) 45 45 | -46 46 | # OK +46 |-for index, (_, _) in enumerate(books): + 46 |+for index in range(len(books)): +47 47 | print(index) +48 48 | +49 49 | for (_, _), book in enumerate(books): + +FURB148.py:49:21: FURB148 [*] `enumerate` index is unused, use `for x in y` instead + | +47 | print(index) +48 | +49 | for (_, _), book in enumerate(books): + | ^^^^^^^^^ FURB148 +50 | print(book) + | + = help: Remove `enumerate` + +ℹ Suggested fix +46 46 | for index, (_, _) in enumerate(books): +47 47 | print(index) +48 48 | +49 |-for (_, _), book in enumerate(books): + 49 |+for book in books: +50 50 | print(book) +51 51 | +52 52 | for(index, _)in enumerate(books): + +FURB148.py:52:17: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead + | +50 | print(book) +51 | +52 | for(index, _)in enumerate(books): + | ^^^^^^^^^ FURB148 +53 | print(index) + | + = help: Replace with `range(len(...))` + +ℹ Suggested fix +49 49 | for (_, _), book in enumerate(books): +50 50 | print(book) +51 51 | +52 |-for(index, _)in enumerate(books): + 52 |+for index in range(len(books)): +53 53 | print(index) +54 54 | +55 55 | for(index), _ in enumerate(books): + +FURB148.py:55:18: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead + | +53 | print(index) +54 | +55 | for(index), _ in enumerate(books): + | ^^^^^^^^^ FURB148 +56 | print(index) + | + = help: Replace with `range(len(...))` + +ℹ Suggested fix +52 52 | for(index, _)in enumerate(books): +53 53 | print(index) +54 54 | +55 |-for(index), _ in enumerate(books): + 55 |+for index in range(len(books)): +56 56 | print(index) +57 57 | +58 58 | # OK