Skip to content

Commit

Permalink
Add dict inference, add test case, do not flag if/elif/else stmts
Browse files Browse the repository at this point in the history
  • Loading branch information
qdegraaf committed Aug 29, 2023
1 parent dc80e9f commit 66c6153
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 13 deletions.
13 changes: 12 additions & 1 deletion crates/ruff/resources/test/fixtures/perflint/PERF403.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def foo():
fruit = ["apple", "pear", "orange"]
for idx, name in enumerate(fruit):
if idx % 2:
result[idx] = name # PERF403
result[idx] = name # Ok because if/elif/else not replaceable by dict comprehension
elif idx % 3:
result[idx] = name
else:
Expand All @@ -33,3 +33,14 @@ def foo():
result[idx] = name # PERF403
else:
result[idx] = name


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

30 changes: 28 additions & 2 deletions crates/ruff/src/rules/perflint/rules/manual_dict_comprehension.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ use ruff_python_ast::{self as ast, Expr, Stmt};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_semantic::analyze::typing::is_dict;
use ruff_python_semantic::Binding;

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

Expand Down Expand Up @@ -55,6 +57,7 @@ pub(crate) fn manual_dict_comprehension(checker: &mut Checker, target: &Expr, bo
return;
};


let names = elts
.iter()
.filter_map(|elt| {
Expand All @@ -70,8 +73,12 @@ pub(crate) fn manual_dict_comprehension(checker: &mut Checker, target: &Expr, bo
return;
}

if let Stmt::If(ast::StmtIf { body: if_body, .. }) = stmt {
if let Stmt::If(ast::StmtIf { body: if_body, elif_else_clauses, .. }) = stmt {
// Key-value assignment within an if-statement (e.g., `if condition: result[key] = value`).
// Exclude if/elif/else statements as they cannot easily be replaced by a dict comprehension
if elif_else_clauses.len() > 1 {
return;
}
for stmt in if_body {
check_for_slow_dict_creation(checker, &names, stmt);
}
Expand All @@ -92,9 +99,28 @@ fn check_for_slow_dict_creation(checker: &mut Checker, names: &[&str], stmt: &St
return;
};

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

// Exclude non dictionary values from the check
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;
};

// It should only apply to variables that are known to be lists or dicts.
if !is_dict(binding, checker.semantic()) {
return
}

let Expr::Name(ast::ExprName { id: key, .. }) = slice.as_ref() else {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,6 @@ PERF403.py:13:13: PERF403 Use a dictionary comprehension instead of a for-loop
| ^^^^^^^^^^^^^^^^^^ PERF403
|

PERF403.py:21:13: PERF403 Use a dictionary comprehension instead of a for-loop
|
19 | for idx, name in enumerate(fruit):
20 | if idx % 2:
21 | result[idx] = name # PERF403
| ^^^^^^^^^^^^^^^^^^ PERF403
22 | elif idx % 3:
23 | result[idx] = name
|

PERF403.py:33:13: PERF403 Use a dictionary comprehension instead of a for-loop
|
31 | for idx, name in enumerate(fruit):
Expand Down

0 comments on commit 66c6153

Please sign in to comment.