From 6394456c773c67518a45d26575f388f05f7f3a7b Mon Sep 17 00:00:00 2001 From: harupy Date: Sat, 19 Aug 2023 14:46:21 +0900 Subject: [PATCH 1/6] Implement PIE808 --- .../test/fixtures/flake8_pie/PIE808.py | 6 ++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/flake8_pie/mod.rs | 1 + crates/ruff/src/rules/flake8_pie/rules/mod.rs | 2 + .../rules/unnecessary_range_start.rs | 78 +++++++++++++++++++ ...__flake8_pie__tests__PIE808_PIE808.py.snap | 20 +++++ ruff.schema.json | 1 + 8 files changed, 112 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/flake8_pie/PIE808.py create mode 100644 crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs create mode 100644 crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_pie/PIE808.py b/crates/ruff/resources/test/fixtures/flake8_pie/PIE808.py new file mode 100644 index 0000000000000..c7288280c49e6 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pie/PIE808.py @@ -0,0 +1,6 @@ +range(0, 10) + +range(x, 10) +range(-15, 10) +range(10) +range(0, 10, x) diff --git a/crates/ruff/src/checkers/ast/analyze/expression.rs b/crates/ruff/src/checkers/ast/analyze/expression.rs index 14278acc53fa9..94f10560dcec7 100644 --- a/crates/ruff/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff/src/checkers/ast/analyze/expression.rs @@ -531,6 +531,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryDictKwargs) { flake8_pie::rules::unnecessary_dict_kwargs(checker, expr, keywords); } + if checker.enabled(Rule::UnnecessaryRangeStart) { + flake8_pie::rules::unnecessary_range_start(checker, call); + } if checker.enabled(Rule::ExecBuiltin) { flake8_bandit::rules::exec_used(checker, func); } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index d7f115bb29065..a94ccc69fe8b4 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -707,6 +707,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Pie, "800") => (RuleGroup::Unspecified, rules::flake8_pie::rules::UnnecessarySpread), (Flake8Pie, "804") => (RuleGroup::Unspecified, rules::flake8_pie::rules::UnnecessaryDictKwargs), (Flake8Pie, "807") => (RuleGroup::Unspecified, rules::flake8_pie::rules::ReimplementedListBuiltin), + (Flake8Pie, "808") => (RuleGroup::Unspecified, rules::flake8_pie::rules::UnnecessaryRangeStart), (Flake8Pie, "810") => (RuleGroup::Unspecified, rules::flake8_pie::rules::MultipleStartsEndsWith), // flake8-commas diff --git a/crates/ruff/src/rules/flake8_pie/mod.rs b/crates/ruff/src/rules/flake8_pie/mod.rs index a1cfb8349c99a..d31962dc28c3a 100644 --- a/crates/ruff/src/rules/flake8_pie/mod.rs +++ b/crates/ruff/src/rules/flake8_pie/mod.rs @@ -15,6 +15,7 @@ mod tests { #[test_case(Rule::DuplicateClassFieldDefinition, Path::new("PIE794.py"))] #[test_case(Rule::UnnecessaryDictKwargs, Path::new("PIE804.py"))] #[test_case(Rule::MultipleStartsEndsWith, Path::new("PIE810.py"))] + #[test_case(Rule::UnnecessaryRangeStart, Path::new("PIE808.py"))] #[test_case(Rule::UnnecessaryPass, Path::new("PIE790.py"))] #[test_case(Rule::UnnecessarySpread, Path::new("PIE800.py"))] #[test_case(Rule::ReimplementedListBuiltin, Path::new("PIE807.py"))] diff --git a/crates/ruff/src/rules/flake8_pie/rules/mod.rs b/crates/ruff/src/rules/flake8_pie/rules/mod.rs index e86cd7a19dc51..79012144acf6d 100644 --- a/crates/ruff/src/rules/flake8_pie/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_pie/rules/mod.rs @@ -4,6 +4,7 @@ pub(crate) use no_unnecessary_pass::*; pub(crate) use non_unique_enums::*; pub(crate) use reimplemented_list_builtin::*; pub(crate) use unnecessary_dict_kwargs::*; +pub(crate) use unnecessary_range_start::*; pub(crate) use unnecessary_spread::*; mod duplicate_class_field_definition; @@ -12,4 +13,5 @@ mod no_unnecessary_pass; mod non_unique_enums; mod reimplemented_list_builtin; mod unnecessary_dict_kwargs; +mod unnecessary_range_start; mod unnecessary_spread; diff --git a/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs b/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs new file mode 100644 index 0000000000000..65b6f7c467ccd --- /dev/null +++ b/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs @@ -0,0 +1,78 @@ +use num_bigint::BigInt; +use ruff_python_ast::{self as ast, Constant, Expr, Ranged}; + +use ruff_diagnostics::Diagnostic; +use ruff_diagnostics::{AlwaysAutofixableViolation, Fix}; +use ruff_macros::{derive_message_formats, violation}; + +use crate::autofix::edits::{remove_argument, Parentheses}; +use crate::checkers::ast::Checker; +use crate::registry::AsRule; + +/// ## What it does +/// Checks for `range` calls with an unnecessary `start` argument. +/// +/// ## Why is this bad? +/// `range(0, x)` is equivalent to `range(x)`. The `start` argument is unnecessary. +/// +/// ## Example +/// ```python +/// range(0, 3) +/// ``` +/// +/// Use instead: +/// ```python +/// range(3) +/// ``` +#[violation] +pub struct UnnecessaryRangeStart {} + +impl AlwaysAutofixableViolation for UnnecessaryRangeStart { + #[derive_message_formats] + fn message(&self) -> String { + format!("`range(0, ...)` is equivalent to `range(...)`") + } + + fn autofix_title(&self) -> String { + format!("Remove the `start` argument") + } +} + +/// PIE808 +pub(crate) fn unnecessary_range_start(checker: &mut Checker, call: &ast::ExprCall) { + if !checker.semantic().is_builtin("range") { + return; + }; + let ast::ExprCall { + func, arguments, .. + } = call; + let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { + return + }; + if id != "range" { + return; + }; + let [start, _end] = &arguments.args[..] else { + return + }; + let Expr::Constant(ast::ExprConstant { value: Constant::Int(value), .. }) = start else { + return + }; + if *value != BigInt::from(0) { + return; + }; + let mut diagnostic = Diagnostic::new(UnnecessaryRangeStart {}, start.range()); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.try_set_fix(|| { + remove_argument( + &start, + arguments, + Parentheses::Remove, + checker.locator(), + checker.source_type, + ) + .map(Fix::automatic) + }); + } + checker.diagnostics.push(diagnostic); +} diff --git a/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap b/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap new file mode 100644 index 0000000000000..3159e6a79dfb1 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap @@ -0,0 +1,20 @@ +--- +source: crates/ruff/src/rules/flake8_pie/mod.rs +--- +PIE808.py:1:7: PIE808 [*] `range(0, ...)` is equivalent to `range(...)` + | +1 | range(0, 10) + | ^ PIE808 +2 | +3 | range(x, 10) + | + = help: Remove the first argument + +ℹ Fix +1 |-range(0, 10) + 1 |+range(10) +2 2 | +3 3 | range(x, 10) +4 4 | range(-15, 10) + + diff --git a/ruff.schema.json b/ruff.schema.json index 8afd482e95186..faef882ebdb8d 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2159,6 +2159,7 @@ "PIE800", "PIE804", "PIE807", + "PIE808", "PIE81", "PIE810", "PL", From 95a0d2ece6666529e3e460170e9a59eb44117fb4 Mon Sep 17 00:00:00 2001 From: harupy Date: Sat, 19 Aug 2023 14:51:55 +0900 Subject: [PATCH 2/6] Fix message --- .../src/rules/flake8_pie/rules/unnecessary_range_start.rs | 4 ++-- .../ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs b/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs index 65b6f7c467ccd..359e4a901d84e 100644 --- a/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs +++ b/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs @@ -30,11 +30,11 @@ pub struct UnnecessaryRangeStart {} impl AlwaysAutofixableViolation for UnnecessaryRangeStart { #[derive_message_formats] fn message(&self) -> String { - format!("`range(0, ...)` is equivalent to `range(...)`") + format!("Unnecessary `start` argument in `range`") } fn autofix_title(&self) -> String { - format!("Remove the `start` argument") + format!("Remove `start` argument") } } diff --git a/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap b/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap index 3159e6a79dfb1..a10daaee2e1ce 100644 --- a/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap +++ b/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap @@ -1,14 +1,14 @@ --- source: crates/ruff/src/rules/flake8_pie/mod.rs --- -PIE808.py:1:7: PIE808 [*] `range(0, ...)` is equivalent to `range(...)` +PIE808.py:1:7: PIE808 [*] Unnecessary `start` argument in `range` | 1 | range(0, 10) | ^ PIE808 2 | 3 | range(x, 10) | - = help: Remove the first argument + = help: Remove `start` argument ℹ Fix 1 |-range(0, 10) From 409ae7a7f1c153f1866b1b5f6ccecddc1d3a3b66 Mon Sep 17 00:00:00 2001 From: harupy Date: Sat, 19 Aug 2023 15:23:58 +0900 Subject: [PATCH 3/6] Fix clippy error --- .../ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs b/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs index 359e4a901d84e..60e076f2b7a96 100644 --- a/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs +++ b/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs @@ -25,7 +25,7 @@ use crate::registry::AsRule; /// range(3) /// ``` #[violation] -pub struct UnnecessaryRangeStart {} +pub struct UnnecessaryRangeStart; impl AlwaysAutofixableViolation for UnnecessaryRangeStart { #[derive_message_formats] From 28edaa95c162f2fd2f834edfd924deb011847ab5 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 19 Aug 2023 17:44:45 -0400 Subject: [PATCH 4/6] Add docs --- .../src/rules/flake8_pie/rules/unnecessary_range_start.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs b/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs index 60e076f2b7a96..4c1023ef2594f 100644 --- a/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs +++ b/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs @@ -13,7 +13,9 @@ use crate::registry::AsRule; /// Checks for `range` calls with an unnecessary `start` argument. /// /// ## Why is this bad? -/// `range(0, x)` is equivalent to `range(x)`. The `start` argument is unnecessary. +/// `range(0, x)` is equivalent to `range(x)`, as `0` is the default value for +/// the `start` argument. Omitting the `start` argument makes the code more +/// concise and idiomatic. /// /// ## Example /// ```python @@ -24,6 +26,9 @@ use crate::registry::AsRule; /// ```python /// range(3) /// ``` +/// +/// ## References +/// - [Python documentation: `range`](https://docs.python.org/3/library/stdtypes.html#range) #[violation] pub struct UnnecessaryRangeStart; From 76fa8b98fb770936afb220e00daa6ea293c5b28e Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 19 Aug 2023 17:50:08 -0400 Subject: [PATCH 5/6] Add more tests --- .../test/fixtures/flake8_pie/PIE808.py | 7 ++++ .../rules/unnecessary_range_start.rs | 32 ++++++++++++------- ...__flake8_pie__tests__PIE808_PIE808.py.snap | 20 ++++++------ 3 files changed, 38 insertions(+), 21 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_pie/PIE808.py b/crates/ruff/resources/test/fixtures/flake8_pie/PIE808.py index c7288280c49e6..6765a20137f6b 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pie/PIE808.py +++ b/crates/ruff/resources/test/fixtures/flake8_pie/PIE808.py @@ -1,6 +1,13 @@ +# PIE808 range(0, 10) +# OK range(x, 10) range(-15, 10) range(10) +range(0) range(0, 10, x) +range(0, 10, 1) +range(0, 10, step=1) +range(start=0, stop=10) +range(0, stop=10) diff --git a/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs b/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs index 4c1023ef2594f..0740f2c12ff12 100644 --- a/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs +++ b/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs @@ -1,9 +1,9 @@ use num_bigint::BigInt; -use ruff_python_ast::{self as ast, Constant, Expr, Ranged}; use ruff_diagnostics::Diagnostic; use ruff_diagnostics::{AlwaysAutofixableViolation, Fix}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Constant, Expr, Ranged}; use crate::autofix::edits::{remove_argument, Parentheses}; use crate::checkers::ast::Checker; @@ -45,34 +45,42 @@ impl AlwaysAutofixableViolation for UnnecessaryRangeStart { /// PIE808 pub(crate) fn unnecessary_range_start(checker: &mut Checker, call: &ast::ExprCall) { - if !checker.semantic().is_builtin("range") { - return; - }; - let ast::ExprCall { - func, arguments, .. - } = call; - let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { + // Verify that the call is to the `range` builtin. + let Expr::Name(ast::ExprName { id, .. }) = call.func.as_ref() else { return }; if id != "range" { return; }; - let [start, _end] = &arguments.args[..] else { + if !checker.semantic().is_builtin("range") { + return; + }; + + // `range` doesn't accept keyword arguments. + if !call.arguments.keywords.is_empty() { + return; + } + + // Verify that the call has exactly two arguments (no `step`). + let [start, _] = call.arguments.args.as_slice() else { return }; + + // Verify that the `start` argument is the literal `0`. let Expr::Constant(ast::ExprConstant { value: Constant::Int(value), .. }) = start else { return }; if *value != BigInt::from(0) { return; }; - let mut diagnostic = Diagnostic::new(UnnecessaryRangeStart {}, start.range()); + + let mut diagnostic = Diagnostic::new(UnnecessaryRangeStart, start.range()); if checker.patch(diagnostic.kind.rule()) { diagnostic.try_set_fix(|| { remove_argument( &start, - arguments, - Parentheses::Remove, + &call.arguments, + Parentheses::Preserve, checker.locator(), checker.source_type, ) diff --git a/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap b/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap index a10daaee2e1ce..0215ece47a8cb 100644 --- a/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap +++ b/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE808_PIE808.py.snap @@ -1,20 +1,22 @@ --- source: crates/ruff/src/rules/flake8_pie/mod.rs --- -PIE808.py:1:7: PIE808 [*] Unnecessary `start` argument in `range` +PIE808.py:2:7: PIE808 [*] Unnecessary `start` argument in `range` | -1 | range(0, 10) +1 | # PIE808 +2 | range(0, 10) | ^ PIE808 -2 | -3 | range(x, 10) +3 | +4 | # OK | = help: Remove `start` argument ℹ Fix -1 |-range(0, 10) - 1 |+range(10) -2 2 | -3 3 | range(x, 10) -4 4 | range(-15, 10) +1 1 | # PIE808 +2 |-range(0, 10) + 2 |+range(10) +3 3 | +4 4 | # OK +5 5 | range(x, 10) From a1a94bbd48f29eb3ccb1dd7c914143cedad37266 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 19 Aug 2023 17:51:26 -0400 Subject: [PATCH 6/6] Run cargo fmt --- .../flake8_pie/rules/unnecessary_range_start.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs b/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs index 0740f2c12ff12..07067d6f75bb4 100644 --- a/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs +++ b/crates/ruff/src/rules/flake8_pie/rules/unnecessary_range_start.rs @@ -47,7 +47,7 @@ impl AlwaysAutofixableViolation for UnnecessaryRangeStart { pub(crate) fn unnecessary_range_start(checker: &mut Checker, call: &ast::ExprCall) { // Verify that the call is to the `range` builtin. let Expr::Name(ast::ExprName { id, .. }) = call.func.as_ref() else { - return + return; }; if id != "range" { return; @@ -63,12 +63,16 @@ pub(crate) fn unnecessary_range_start(checker: &mut Checker, call: &ast::ExprCal // Verify that the call has exactly two arguments (no `step`). let [start, _] = call.arguments.args.as_slice() else { - return + return; }; // Verify that the `start` argument is the literal `0`. - let Expr::Constant(ast::ExprConstant { value: Constant::Int(value), .. }) = start else { - return + let Expr::Constant(ast::ExprConstant { + value: Constant::Int(value), + .. + }) = start + else { + return; }; if *value != BigInt::from(0) { return;