Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-pie] Unnecessary list comprehension, with autofix (PIE802) #3149

Merged
merged 4 commits into from
Feb 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pie/PIE802.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# no error
all((x.id for x in bar))
all(x.id for x in bar)
all(x.id for x in bar)
any(x.id for x in bar)
any({x.id for x in bar})

# PIE 802
any([x.id for x in bar])
all([x.id for x in bar])
7 changes: 7 additions & 0 deletions crates/ruff/src/checkers/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2491,6 +2491,13 @@ where
if self.settings.rules.enabled(&Rule::UnnecessaryDictKwargs) {
flake8_pie::rules::no_unnecessary_dict_kwargs(self, expr, keywords);
}
if self
.settings
.rules
.enabled(&Rule::UnnecessaryComprehensionAnyAll)
{
flake8_pie::rules::unnecessary_comprehension_any_all(self, expr, func, args);
}

// flake8-bandit
if self.settings.rules.enabled(&Rule::ExecBuiltin) {
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 @@ -516,6 +516,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Flake8Pie, "794") => Rule::DupeClassFieldDefinitions,
(Flake8Pie, "796") => Rule::PreferUniqueEnums,
(Flake8Pie, "800") => Rule::UnnecessarySpread,
(Flake8Pie, "802") => Rule::UnnecessaryComprehensionAnyAll,
(Flake8Pie, "804") => Rule::UnnecessaryDictKwargs,
(Flake8Pie, "807") => Rule::PreferListBuiltin,
(Flake8Pie, "810") => Rule::SingleStartsEndsWith,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,7 @@ ruff_macros::register_rules!(
rules::flake8_pie::rules::UnnecessaryDictKwargs,
rules::flake8_pie::rules::PreferListBuiltin,
rules::flake8_pie::rules::SingleStartsEndsWith,
rules::flake8_pie::rules::UnnecessaryComprehensionAnyAll,
// flake8-commas
rules::flake8_commas::rules::TrailingCommaMissing,
rules::flake8_commas::rules::TrailingCommaOnBareTupleProhibited,
Expand Down
45 changes: 45 additions & 0 deletions crates/ruff/src/rules/flake8_pie/fixes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
use anyhow::{bail, Result};
use libcst_native::{Codegen, CodegenState, Expression, GeneratorExp};

use crate::ast::types::Range;
use crate::cst::matchers::{match_expr, match_module};
use crate::fix::Fix;
use crate::source_code::{Locator, Stylist};

/// (PIE802) Convert `[i for i in a]` into `i for i in a`
pub fn fix_unnecessary_comprehension_any_all(
locator: &Locator,
stylist: &Stylist,
expr: &rustpython_parser::ast::Expr,
) -> Result<Fix> {
// Expr(ListComp) -> Expr(GeneratorExp)
let module_text = locator.slice(&Range::from_located(expr));
let mut tree = match_module(module_text)?;
let mut body = match_expr(&mut tree)?;

let Expression::ListComp(list_comp) = &body.value else {
bail!(
"Expected Expression::ListComp"
);
};

body.value = Expression::GeneratorExp(Box::new(GeneratorExp {
elt: list_comp.elt.clone(),
for_in: list_comp.for_in.clone(),
lpar: list_comp.lpar.clone(),
rpar: list_comp.rpar.clone(),
}));

let mut state = CodegenState {
default_newline: stylist.line_ending(),
default_indent: stylist.indentation(),
..CodegenState::default()
};
tree.codegen(&mut state);

Ok(Fix::replacement(
state.to_string(),
expr.location,
expr.end_location.unwrap(),
))
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/flake8_pie/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
//! Rules from [flake8-pie](https://pypi.org/project/flake8-pie/).
mod fixes;
pub(crate) mod rules;

#[cfg(test)]
Expand All @@ -20,6 +21,7 @@ mod tests {
#[test_case(Rule::UnnecessarySpread, Path::new("PIE800.py"); "PIE800")]
#[test_case(Rule::PreferListBuiltin, Path::new("PIE807.py"); "PIE807")]
#[test_case(Rule::PreferUniqueEnums, Path::new("PIE796.py"); "PIE796")]
#[test_case(Rule::UnnecessaryComprehensionAnyAll, Path::new("PIE802.py"); "PIE802")]
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
79 changes: 79 additions & 0 deletions crates/ruff/src/rules/flake8_pie/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ use crate::message::Location;
use crate::registry::Diagnostic;
use crate::violation::{AlwaysAutofixableViolation, Violation};

use super::fixes;

define_violation!(
pub struct UnnecessaryPass;
);
Expand Down Expand Up @@ -58,6 +60,50 @@ impl Violation for PreferUniqueEnums {
}
}

define_violation!(
/// ## What it does
/// Checks for unnecessary list comprehensions passed to `any` and `all`.
///
/// ## Why is this bad?
/// `any` and `all` take any iterators, including generators. Converting a generator to a list
/// by way of a list comprehension is unnecessary and reduces performance due to the
/// overhead of creating the list.
///
/// For example, compare the performance of `all` with a list comprehension against that
/// of a generator (~40x faster here):
///
/// ```python
/// In [1]: %timeit all([i for i in range(1000)])
/// 8.14 µs ± 25.4 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
///
/// In [2]: %timeit all(i for i in range(1000))
/// 212 ns ± 0.892 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)
/// ```
///
/// ## Examples
/// ```python
/// any([x.id for x in bar])
/// all([x.id for x in bar])
/// ```
///
/// Use instead:
/// ```python
/// any(x.id for x in bar)
/// all(x.id for x in bar)
/// ```
pub struct UnnecessaryComprehensionAnyAll;
);
impl AlwaysAutofixableViolation for UnnecessaryComprehensionAnyAll {
#[derive_message_formats]
fn message(&self) -> String {
format!("Unnecessary list comprehension.")
}

fn autofix_title(&self) -> String {
"Remove unnecessary list comprehension".to_string()
}
}

define_violation!(
pub struct UnnecessarySpread;
);
Expand Down Expand Up @@ -282,6 +328,39 @@ pub fn no_unnecessary_spread(checker: &mut Checker, keys: &[Option<Expr>], value
}
}

/// PIE802
pub fn unnecessary_comprehension_any_all(
checker: &mut Checker,
expr: &Expr,
func: &Expr,
args: &[Expr],
) {
if let ExprKind::Name { id, .. } = &func.node {
if (id == "all" || id == "any") && args.len() == 1 {
if !checker.is_builtin(id) {
return;
}
if let ExprKind::ListComp { .. } = args[0].node {
let mut diagnostic =
Diagnostic::new(UnnecessaryComprehensionAnyAll, Range::from_located(expr));
if checker.patch(diagnostic.kind.rule()) {
match fixes::fix_unnecessary_comprehension_any_all(
checker.locator,
checker.stylist,
&args[0],
) {
Ok(fix) => {
diagnostic.amend(fix);
}
Err(e) => error!("Failed to generate fix: {e}"),
}
}
checker.diagnostics.push(diagnostic);
}
}
}
}

/// Return `true` if a key is a valid keyword argument name.
fn is_valid_kwarg_name(key: &Expr) -> bool {
if let ExprKind::Constant {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
---
source: crates/ruff/src/rules/flake8_pie/mod.rs
expression: diagnostics
---
- kind:
UnnecessaryComprehensionAnyAll: ~
location:
row: 9
column: 0
end_location:
row: 9
column: 24
fix:
content: x.id for x in bar
location:
row: 9
column: 4
end_location:
row: 9
column: 23
parent: ~
- kind:
UnnecessaryComprehensionAnyAll: ~
location:
row: 10
column: 0
end_location:
row: 10
column: 24
fix:
content: x.id for x in bar
location:
row: 10
column: 4
end_location:
row: 10
column: 23
parent: ~

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.