Skip to content

Commit

Permalink
[perflint] Add PERF403 (#6132)
Browse files Browse the repository at this point in the history
## Summary

Adds `PERF403` mirroring `W8403` from
https://github.com/tonybaloney/perflint

## Test Plan

Fixtures were added based on perflint tests

## Issue Links

Refers: #4789
  • Loading branch information
qdegraaf committed Sep 15, 2023
1 parent c88376f commit 067a4ac
Show file tree
Hide file tree
Showing 8 changed files with 322 additions and 1 deletion.
83 changes: 83 additions & 0 deletions crates/ruff/resources/test/fixtures/perflint/PERF403.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
def foo():
fruit = ["apple", "pear", "orange"]
result = {}
for idx, name in enumerate(fruit):
result[idx] = name # PERF403


def foo():
fruit = ["apple", "pear", "orange"]
result = {}
for idx, name in enumerate(fruit):
if idx % 2:
result[idx] = name # PERF403


def foo():
fruit = ["apple", "pear", "orange"]
result = {}
for idx, name in enumerate(fruit):
if idx % 2:
result[idx] = name # Ok (false negative: edge case where `else` is same as `if`)
else:
result[idx] = name


def foo():
result = {}
fruit = ["apple", "pear", "orange"]
for idx, name in enumerate(fruit):
if idx % 2:
result[idx] = name # PERF403


def foo():
fruit = ["apple", "pear", "orange"]
result = []
for idx, name in enumerate(fruit):
if idx % 2:
result[idx] = name # OK (result is not a dictionary)
else:
result[idx] = name


def foo():
fruit = ["apple", "pear", "orange"]
result = {}
for idx, name in enumerate(fruit):
if idx % 2:
result[idx] = name # OK (if/elif/else isn't replaceable)
elif idx % 3:
result[idx] = name
else:
result[idx] = name


def foo():
result = {1: "banana"}
fruit = ["apple", "pear", "orange"]
for idx, name in enumerate(fruit):
if idx % 2:
result[idx] = name # PERF403


def foo():
fruit = ["apple", "pear", "orange"]
result = {}
for idx, name in enumerate(fruit):
if idx in result:
result[idx] = name # PERF403


def foo():
fruit = ["apple", "pear", "orange"]
result = {}
for name in fruit:
result[name] = name # PERF403


def foo():
fruit = ["apple", "pear", "orange"]
result = {}
for idx, name in enumerate(fruit):
result[name] = idx # PERF403
5 changes: 4 additions & 1 deletion crates/ruff/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1184,7 +1184,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
iter,
orelse,
is_async,
..
range: _,
},
) => {
if checker.any_enabled(&[
Expand Down Expand Up @@ -1218,6 +1218,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::ManualListCopy) {
perflint::rules::manual_list_copy(checker, target, body);
}
if checker.enabled(Rule::ManualDictComprehension) {
perflint::rules::manual_dict_comprehension(checker, target, body);
}
if checker.enabled(Rule::UnnecessaryListCast) {
perflint::rules::unnecessary_list_cast(checker, iter);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -898,6 +898,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Perflint, "203") => (RuleGroup::Unspecified, rules::perflint::rules::TryExceptInLoop),
(Perflint, "401") => (RuleGroup::Unspecified, rules::perflint::rules::ManualListComprehension),
(Perflint, "402") => (RuleGroup::Unspecified, rules::perflint::rules::ManualListCopy),
(Perflint, "403") => (RuleGroup::Unspecified, rules::perflint::rules::ManualDictComprehension),

// flake8-fixme
(Flake8Fixme, "001") => (RuleGroup::Unspecified, rules::flake8_fixme::rules::LineContainsFixme),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/perflint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ mod tests {
#[test_case(Rule::TryExceptInLoop, Path::new("PERF203.py"))]
#[test_case(Rule::ManualListComprehension, Path::new("PERF401.py"))]
#[test_case(Rule::ManualListCopy, Path::new("PERF402.py"))]
#[test_case(Rule::ManualDictComprehension, Path::new("PERF403.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
178 changes: 178 additions & 0 deletions crates/ruff/src/rules/perflint/rules/manual_dict_comprehension.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::helpers::any_over_expr;
use ruff_python_ast::{self as ast, Expr, Stmt};
use ruff_python_semantic::analyze::typing::is_dict;
use ruff_python_semantic::Binding;

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for `for` loops that can be replaced by a dictionary comprehension.
///
/// ## Why is this bad?
/// When creating or extending a dictionary in a for-loop, prefer a dictionary
/// comprehension. Comprehensions are more readable and more performant.
///
/// For example, when comparing `{x: x for x in list(range(1000))}` to the `for`
/// loop version, the comprehension is ~10% 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
/// pairs = (("a", 1), ("b", 2))
/// result = {}
/// for x, y in pairs:
/// if y % 2:
/// result[x] = y
/// ```
///
/// Use instead:
/// ```python
/// pairs = (("a", 1), ("b", 2))
/// result = {x: y for x, y in pairs if y % 2}
/// ```
///
/// If you're appending to an existing dictionary, use the `update` method instead:
/// ```python
/// pairs = (("a", 1), ("b", 2))
/// result.update({x: y for x, y in pairs if y % 2})
/// ```
#[violation]
pub struct ManualDictComprehension;

impl Violation for ManualDictComprehension {
#[derive_message_formats]
fn message(&self) -> String {
format!("Use a dictionary comprehension instead of a for-loop")
}
}

/// PERF403
pub(crate) fn manual_dict_comprehension(checker: &mut Checker, target: &Expr, body: &[Stmt]) {
let (stmt, if_test) = match body {
// ```python
// for idx, name in enumerate(names):
// if idx % 2 == 0:
// result[name] = idx
// ```
[Stmt::If(ast::StmtIf {
body,
elif_else_clauses,
test,
..
})] => {
// TODO(charlie): If there's an `else` clause, verify that the `else` has the
// same structure.
if !elif_else_clauses.is_empty() {
return;
}
let [stmt] = body.as_slice() else {
return;
};
(stmt, Some(test))
}
// ```python
// for idx, name in enumerate(names):
// result[name] = idx
// ```
[stmt] => (stmt, None),
_ => return,
};

let Stmt::Assign(ast::StmtAssign {
targets,
value,
range,
}) = stmt
else {
return;
};

let [Expr::Subscript(ast::ExprSubscript {
value: subscript_value,
slice,
..
})] = targets.as_slice()
else {
return;
};

match target {
Expr::Tuple(ast::ExprTuple { elts, .. }) => {
if !elts
.iter()
.any(|elt| ComparableExpr::from(slice) == ComparableExpr::from(elt))
{
return;
}
if !elts
.iter()
.any(|elt| ComparableExpr::from(value) == ComparableExpr::from(elt))
{
return;
}
}
Expr::Name(_) => {
if ComparableExpr::from(slice) != ComparableExpr::from(target) {
return;
}
if ComparableExpr::from(value) != ComparableExpr::from(target) {
return;
}
}
_ => return,
}

// Exclude non-dictionary value.
let Expr::Name(ast::ExprName {
id: subscript_name, ..
}) = subscript_value.as_ref()
else {
return;
};
let scope = checker.semantic().current_scope();
let bindings: Vec<&Binding> = scope
.get_all(subscript_name)
.map(|binding_id| checker.semantic().binding(binding_id))
.collect();

let [binding] = bindings.as_slice() else {
return;
};

if !is_dict(binding, checker.semantic()) {
return;
}

// Avoid if the value is used in the conditional test, e.g.,
//
// ```python
// for x in y:
// if x in filtered:
// filtered[x] = y
// ```
//
// Converting this to a dictionary comprehension would raise a `NameError` as
// `filtered` is not defined yet:
//
// ```python
// filtered = {x: y for x in y if x in filtered}
// ```
if if_test.is_some_and(|test| {
any_over_expr(test, &|expr| {
expr.as_name_expr()
.is_some_and(|expr| expr.id == *subscript_name)
})
}) {
return;
}

checker
.diagnostics
.push(Diagnostic::new(ManualDictComprehension, *range));
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/perflint/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
pub(crate) use incorrect_dict_iterator::*;
pub(crate) use manual_dict_comprehension::*;
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_dict_comprehension;
mod manual_list_comprehension;
mod manual_list_copy;
mod try_except_in_loop;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
---
source: crates/ruff/src/rules/perflint/mod.rs
---
PERF403.py:5:9: PERF403 Use a dictionary comprehension instead of a for-loop
|
3 | result = {}
4 | for idx, name in enumerate(fruit):
5 | result[idx] = name # PERF403
| ^^^^^^^^^^^^^^^^^^ PERF403
|

PERF403.py:13:13: PERF403 Use a dictionary comprehension instead of a for-loop
|
11 | for idx, name in enumerate(fruit):
12 | if idx % 2:
13 | result[idx] = name # PERF403
| ^^^^^^^^^^^^^^^^^^ PERF403
|

PERF403.py:31:13: PERF403 Use a dictionary comprehension instead of a for-loop
|
29 | for idx, name in enumerate(fruit):
30 | if idx % 2:
31 | result[idx] = name # PERF403
| ^^^^^^^^^^^^^^^^^^ PERF403
|

PERF403.py:61:13: PERF403 Use a dictionary comprehension instead of a for-loop
|
59 | for idx, name in enumerate(fruit):
60 | if idx % 2:
61 | result[idx] = name # PERF403
| ^^^^^^^^^^^^^^^^^^ PERF403
|

PERF403.py:76:9: PERF403 Use a dictionary comprehension instead of a for-loop
|
74 | result = {}
75 | for name in fruit:
76 | result[name] = name # PERF403
| ^^^^^^^^^^^^^^^^^^^ PERF403
|

PERF403.py:83:9: PERF403 Use a dictionary comprehension instead of a for-loop
|
81 | result = {}
82 | for idx, name in enumerate(fruit):
83 | result[name] = idx # PERF403
| ^^^^^^^^^^^^^^^^^^ PERF403
|


1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 067a4ac

Please sign in to comment.