[perflint] Add PERF401 and PERF402 rules (#5298)

## Summary

Adds `PERF401` and `PERF402` mirroring `W8401` and `W8402` from
https://github.com/tonybaloney/perflint

Implementation is not super smart but should be at parity with upstream
implementation judging by:
c07391c176/perflint/comprehension_checker.py (L42-L73)

It essentially checks:

- If the body of a for-loop is just one statement
- If that statement is an `if` and the if-statement contains a call to
`append()` we flag `PERF401` and suggest a list comprehension
- If that statement is a plain call to `append()` or `insert()` we flag
`PERF402` and suggest `list()` or `list.copy()`

I've set the violation to only flag the first append call in a long
`if-else` statement for `PERF401`. Happy to change this to some other
location or make it multiple violations if that makes more sense.

## Test Plan

Fixtures were added with the relevant scenarios for both rules

## Issue Links

Refers: https://github.com/astral-sh/ruff/issues/4789
This commit is contained in:
qdegraaf 2023-07-03 06:03:09 +02:00 committed by GitHub
parent 0bff4ed4d3
commit 93b2bd7184
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
15 changed files with 248 additions and 11 deletions

View file

@ -0,0 +1,18 @@
def foo():
items = [1, 2, 3, 4]
result = []
for i in items:
if i % 2:
result.append(i) # PERF401
def foo():
items = [1,2,3,4]
result = []
for i in items:
if i % 2:
result.append(i) # PERF401
elif i % 2:
result.append(i) # PERF401
else:
result.append(i) # PERF401

View file

@ -0,0 +1,12 @@
def foo():
items = [1, 2, 3, 4]
result = []
for i in items:
result.append(i) # PERF402
def foo():
items = [1, 2, 3, 4]
result = []
for i in items:
result.insert(0, i) # PERF402

View file

@ -1501,6 +1501,12 @@ where
if self.enabled(Rule::IncorrectDictIterator) {
perflint::rules::incorrect_dict_iterator(self, target, iter);
}
if self.enabled(Rule::ManualListComprehension) {
perflint::rules::manual_list_comprehension(self, body);
}
if self.enabled(Rule::ManualListCopy) {
perflint::rules::manual_list_copy(self, body);
}
if self.enabled(Rule::UnnecessaryListCast) {
perflint::rules::unnecessary_list_cast(self, iter);
}

View file

@ -793,6 +793,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Perflint, "101") => (RuleGroup::Unspecified, rules::perflint::rules::UnnecessaryListCast),
(Perflint, "102") => (RuleGroup::Unspecified, rules::perflint::rules::IncorrectDictIterator),
(Perflint, "203") => (RuleGroup::Unspecified, rules::perflint::rules::TryExceptInLoop),
(Perflint, "401") => (RuleGroup::Unspecified, rules::perflint::rules::ManualListComprehension),
(Perflint, "402") => (RuleGroup::Unspecified, rules::perflint::rules::ManualListCopy),
// flake8-fixme
(Flake8Fixme, "001") => (RuleGroup::Unspecified, rules::flake8_fixme::rules::LineContainsFixme),

View file

@ -16,6 +16,8 @@ mod tests {
#[test_case(Rule::UnnecessaryListCast, Path::new("PERF101.py"))]
#[test_case(Rule::IncorrectDictIterator, Path::new("PERF102.py"))]
#[test_case(Rule::TryExceptInLoop, Path::new("PERF203.py"))]
#[test_case(Rule::ManualListComprehension, Path::new("PERF401.py"))]
#[test_case(Rule::ManualListCopy, Path::new("PERF402.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(

View file

@ -23,6 +23,10 @@ use crate::registry::AsRule;
/// avoid allocating tuples for every item in the dictionary. They also
/// communicate the intent of the code more clearly.
///
/// Note that, as with all `perflint` rules, this is only intended as a
/// micro-optimization, and will have a negligible impact on performance in
/// most cases.
///
/// ## Example
/// ```python
/// some_dict = {"a": 1, "b": 2}

View file

@ -0,0 +1,76 @@
use rustpython_parser::ast::{self, Expr, Stmt};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for `for` loops that can be replaced by a list comprehension.
///
/// ## Why is this bad?
/// When creating a filtered list from an existing list using a for-loop,
/// prefer a list comprehension. List comprehensions are more readable and
/// more performant.
///
/// Using the below as an example, the list comprehension is ~10% faster on
/// Python 3.11, and ~25% faster on Python 3.10.
///
/// Note that, as with all `perflint` rules, this is only intended as a
/// micro-optimization, and will have a negligible impact on performance in
/// most cases.
///
/// ## Example
/// ```python
/// original = list(range(10000))
/// filtered = []
/// for i in original:
/// if i % 2:
/// filtered.append(i)
/// ```
///
/// Use instead:
/// ```python
/// original = list(range(10000))
/// filtered = [x for x in original if x % 2]
/// ```
#[violation]
pub struct ManualListComprehension;
impl Violation for ManualListComprehension {
#[derive_message_formats]
fn message(&self) -> String {
format!("Use a list comprehension to create a new filtered list")
}
}
/// PERF401
pub(crate) fn manual_list_comprehension(checker: &mut Checker, body: &[Stmt]) {
let [stmt] = body else {
return;
};
let Stmt::If(ast::StmtIf { body, .. }) = stmt else {
return;
};
for stmt in body {
let Stmt::Expr(ast::StmtExpr { value, .. }) = stmt else {
continue;
};
let Expr::Call(ast::ExprCall { func, range, .. }) = value.as_ref() else {
continue;
};
let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func.as_ref() else {
continue;
};
if attr.as_str() == "append" {
checker
.diagnostics
.push(Diagnostic::new(ManualListComprehension, *range));
}
}
}

View file

@ -0,0 +1,69 @@
use rustpython_parser::ast::{self, Expr, Stmt};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for `for` loops that can be replaced by a making a copy of a list.
///
/// ## Why is this bad?
/// When creating a copy of an existing list using a for-loop, prefer
/// `list` or `list.copy` instead. Making a direct copy is more readable and
/// more performant.
///
/// Using the below as an example, the `list`-based copy is ~2x faster on
/// Python 3.11.
///
/// Note that, as with all `perflint` rules, this is only intended as a
/// micro-optimization, and will have a negligible impact on performance in
/// most cases.
///
/// ## Example
/// ```python
/// original = list(range(10000))
/// filtered = []
/// for i in original:
/// filtered.append(i)
/// ```
///
/// Use instead:
/// ```python
/// original = list(range(10000))
/// filtered = list(original)
/// ```
#[violation]
pub struct ManualListCopy;
impl Violation for ManualListCopy {
#[derive_message_formats]
fn message(&self) -> String {
format!("Use `list` or `list.copy` to create a copy of a list")
}
}
/// PERF402
pub(crate) fn manual_list_copy(checker: &mut Checker, body: &[Stmt]) {
let [stmt] = body else {
return;
};
let Stmt::Expr(ast::StmtExpr { value, .. }) = stmt else {
return;
};
let Expr::Call(ast::ExprCall { func, range, .. }) = value.as_ref() else {
return;
};
let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func.as_ref() else {
return;
};
if matches!(attr.as_str(), "append" | "insert") {
checker
.diagnostics
.push(Diagnostic::new(ManualListCopy, *range));
}
}

View file

@ -1,7 +1,11 @@
pub(crate) use incorrect_dict_iterator::*;
pub(crate) use manual_list_comprehension::*;
pub(crate) use manual_list_copy::*;
pub(crate) use try_except_in_loop::*;
pub(crate) use unnecessary_list_cast::*;
mod incorrect_dict_iterator;
mod manual_list_comprehension;
mod manual_list_copy;
mod try_except_in_loop;
mod unnecessary_list_cast;

View file

@ -19,6 +19,10 @@ use crate::registry::AsRule;
/// Removing the `list()` call will not change the behavior of the code, but
/// may improve performance.
///
/// Note that, as with all `perflint` rules, this is only intended as a
/// micro-optimization, and will have a negligible impact on performance in
/// most cases.
///
/// ## Example
/// ```python
/// items = (1, 2, 3)

View file

@ -0,0 +1,22 @@
---
source: crates/ruff/src/rules/perflint/mod.rs
---
PERF401.py:6:13: PERF401 Use a list comprehension to create a new filtered list
|
4 | for i in items:
5 | if i % 2:
6 | result.append(i) # PERF401
| ^^^^^^^^^^^^^^^^ PERF401
|
PERF401.py:14:13: PERF401 Use a list comprehension to create a new filtered list
|
12 | for i in items:
13 | if i % 2:
14 | result.append(i) # PERF401
| ^^^^^^^^^^^^^^^^ PERF401
15 | elif i % 2:
16 | result.append(i) # PERF401
|

View file

@ -0,0 +1,20 @@
---
source: crates/ruff/src/rules/perflint/mod.rs
---
PERF402.py:5:9: PERF402 Use `list` or `list.copy` to create a copy of a list
|
3 | result = []
4 | for i in items:
5 | result.append(i) # PERF402
| ^^^^^^^^^^^^^^^^ PERF402
|
PERF402.py:12:9: PERF402 Use `list` or `list.copy` to create a copy of a list
|
10 | result = []
11 | for i in items:
12 | result.insert(0, i) # PERF402
| ^^^^^^^^^^^^^^^^^^^ PERF402
|

4
ruff.schema.json generated
View file

@ -2093,6 +2093,10 @@
"PERF2",
"PERF20",
"PERF203",
"PERF4",
"PERF40",
"PERF401",
"PERF402",
"PGH",
"PGH0",
"PGH00",

View file

@ -186,10 +186,7 @@ def main(argv: Sequence[str] | None = None) -> int:
generate_docs()
# Get static docs
static_docs = []
for file in os.listdir("docs"):
if file.endswith(".md"):
static_docs.append(Path("docs") / file)
static_docs = [Path("docs") / f for f in os.listdir("docs") if f.endswith(".md")]
# Check rules generated
if not Path("docs/rules").exists():
@ -197,10 +194,9 @@ def main(argv: Sequence[str] | None = None) -> int:
return 1
# Get generated rules
generated_docs = []
for file in os.listdir("docs/rules"):
if file.endswith(".md"):
generated_docs.append(Path("docs/rules") / file)
generated_docs = [
Path("docs/rules") / f for f in os.listdir("docs/rules") if f.endswith(".md")
]
if len(generated_docs) == 0:
print("Please generate rules first.")

View file

@ -45,9 +45,7 @@ def format_confusables_rs(raw_data: dict[str, list[int]]) -> str:
for i in range(0, len(items), 2):
flattened_items.add((items[i], items[i + 1]))
tuples = []
for left, right in sorted(flattened_items):
tuples.append(f" {left}u32 => {right},\n")
tuples = [f" {left}u32 => {right},\n" for left, right in sorted(flattened_items)]
print(f"{len(tuples)} confusable tuples.")