Skip to content

Commit

Permalink
Prefer itertools.pairwise() over zip() for successive pairs (`RUF…
Browse files Browse the repository at this point in the history
…007`) (#3501)
  • Loading branch information
evanrittenhouse committed Mar 17, 2023
1 parent 373a77e commit 33d2457
Show file tree
Hide file tree
Showing 9 changed files with 273 additions and 5 deletions.
19 changes: 19 additions & 0 deletions crates/ruff/resources/test/fixtures/ruff/RUF007.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
input = [1, 2, 3]
otherInput = [2, 3, 4]

# OK
zip(input, otherInput) # different inputs
zip(input, otherInput[1:]) # different inputs
zip(input, input[2:]) # not successive
zip(input[:-1], input[2:]) # not successive
list(zip(input, otherInput)) # nested call
zip(input, input[1::2]) # not successive

# Errors
zip(input, input[1:])
zip(input, input[1::1])
zip(input[:-1], input[1:])
zip(input[1:], input[2:])
zip(input[1:-1], input[2:])
list(zip(input, input[1:]))
list(zip(input[:-1], input[1:]))
6 changes: 6 additions & 0 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2866,6 +2866,12 @@ where
flake8_pytest_style::rules::fail_call(self, func, args, keywords);
}

if self.settings.rules.enabled(Rule::PairwiseOverZipped) {
if self.settings.target_version >= PythonVersion::Py310 {
ruff::rules::pairwise_over_zipped(self, func, args);
}
}

// flake8-simplify
if self
.settings
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 @@ -661,6 +661,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Ruff, "003") => Rule::AmbiguousUnicodeCharacterComment,
(Ruff, "005") => Rule::UnpackInsteadOfConcatenatingToCollectionLiteral,
(Ruff, "006") => Rule::AsyncioDanglingTask,
(Ruff, "007") => Rule::PairwiseOverZipped,
(Ruff, "100") => Rule::UnusedNOQA,

// flake8-django
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 @@ -602,6 +602,7 @@ ruff_macros::register_rules!(
rules::ruff::rules::UnpackInsteadOfConcatenatingToCollectionLiteral,
rules::ruff::rules::AsyncioDanglingTask,
rules::ruff::rules::UnusedNOQA,
rules::ruff::rules::PairwiseOverZipped,
// flake8-django
rules::flake8_django::rules::NullableModelStringField,
rules::flake8_django::rules::LocalsInRenderFunction,
Expand Down
10 changes: 10 additions & 0 deletions crates/ruff/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,4 +142,14 @@ mod tests {
assert_yaml_snapshot!(diagnostics);
Ok(())
}

#[test]
fn ruff_pairwise_over_zipped() -> Result<()> {
let diagnostics = test_path(
Path::new("ruff/RUF007.py"),
&settings::Settings::for_rules(vec![Rule::PairwiseOverZipped]),
)?;
assert_yaml_snapshot!(diagnostics);
Ok(())
}
}
12 changes: 7 additions & 5 deletions crates/ruff/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
mod ambiguous_unicode_character;
mod asyncio_dangling_task;
mod pairwise_over_zipped;
mod unpack_instead_of_concatenating_to_collection_literal;
mod unused_noqa;

pub use ambiguous_unicode_character::{
ambiguous_unicode_character, AmbiguousUnicodeCharacterComment,
AmbiguousUnicodeCharacterDocstring, AmbiguousUnicodeCharacterString,
};
pub use asyncio_dangling_task::{asyncio_dangling_task, AsyncioDanglingTask};
pub use pairwise_over_zipped::{pairwise_over_zipped, PairwiseOverZipped};
pub use unpack_instead_of_concatenating_to_collection_literal::{
unpack_instead_of_concatenating_to_collection_literal,
UnpackInsteadOfConcatenatingToCollectionLiteral,
};
pub use unused_noqa::{UnusedCodes, UnusedNOQA};

mod ambiguous_unicode_character;
mod asyncio_dangling_task;
mod unpack_instead_of_concatenating_to_collection_literal;
mod unused_noqa;

#[derive(Clone, Copy)]
pub enum Context {
String,
Expand Down
132 changes: 132 additions & 0 deletions crates/ruff/src/rules/ruff/rules/pairwise_over_zipped.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
use num_traits::ToPrimitive;
use rustpython_parser::ast::{Constant, Expr, ExprKind, Unaryop};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};

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

#[violation]
pub struct PairwiseOverZipped;

impl Violation for PairwiseOverZipped {
#[derive_message_formats]
fn message(&self) -> String {
format!("Prefer `itertools.pairwise()` over `zip()` when iterating over successive pairs")
}
}

#[derive(Debug)]
struct SliceInfo {
arg_name: String,
slice_start: Option<i64>,
}

impl SliceInfo {
pub fn new(arg_name: String, slice_start: Option<i64>) -> Self {
Self {
arg_name,
slice_start,
}
}
}

/// Return the argument name, lower bound, and upper bound for an expression, if it's a slice.
fn match_slice_info(expr: &Expr) -> Option<SliceInfo> {
let ExprKind::Subscript { value, slice, .. } = &expr.node else {
return None;
};

let ExprKind::Name { id: arg_id, .. } = &value.node else {
return None;
};

let ExprKind::Slice { lower, step, .. } = &slice.node else {
return None;
};

// Avoid false positives for slices with a step.
if let Some(step) = step {
if let Some(step) = to_bound(step) {
if step != 1 {
return None;
}
} else {
return None;
}
}

Some(SliceInfo::new(
arg_id.to_string(),
lower.as_ref().and_then(|expr| to_bound(expr)),
))
}

fn to_bound(expr: &Expr) -> Option<i64> {
match &expr.node {
ExprKind::Constant {
value: Constant::Int(value),
..
} => value.to_i64(),
ExprKind::UnaryOp {
op: Unaryop::USub | Unaryop::Invert,
operand,
} => {
if let ExprKind::Constant {
value: Constant::Int(value),
..
} = &operand.node
{
value.to_i64().map(|v| -v)
} else {
None
}
}
_ => None,
}
}

/// RUF007
pub fn pairwise_over_zipped(checker: &mut Checker, func: &Expr, args: &[Expr]) {
let ExprKind::Name { id, .. } = &func.node else {
return;
};

if !(args.len() > 1 && id == "zip" && checker.ctx.is_builtin(id)) {
return;
};

// Allow the first argument to be a `Name` or `Subscript`.
let Some(first_arg_info) = ({
if let ExprKind::Name { id, .. } = &args[0].node {
Some(SliceInfo::new(id.to_string(), None))
} else {
match_slice_info(&args[0])
}
}) else {
return;
};

// Require second argument to be a `Subscript`.
let ExprKind::Subscript { .. } = &args[1].node else {
return;
};
let Some(second_arg_info) = match_slice_info(&args[1]) else {
return;
};

// Verify that the arguments match the same name.
if first_arg_info.arg_name != second_arg_info.arg_name {
return;
}

// Verify that the arguments are successive.
if second_arg_info.slice_start.unwrap_or(0) - first_arg_info.slice_start.unwrap_or(0) != 1 {
return;
}

checker
.diagnostics
.push(Diagnostic::new(PairwiseOverZipped, Range::from(func)));
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
---
source: crates/ruff/src/rules/ruff/mod.rs
expression: diagnostics
---
- kind:
name: PairwiseOverZipped
body: "Prefer `itertools.pairwise()` over `zip()` when iterating over successive pairs"
suggestion: ~
fixable: false
location:
row: 13
column: 0
end_location:
row: 13
column: 3
fix: ~
parent: ~
- kind:
name: PairwiseOverZipped
body: "Prefer `itertools.pairwise()` over `zip()` when iterating over successive pairs"
suggestion: ~
fixable: false
location:
row: 14
column: 0
end_location:
row: 14
column: 3
fix: ~
parent: ~
- kind:
name: PairwiseOverZipped
body: "Prefer `itertools.pairwise()` over `zip()` when iterating over successive pairs"
suggestion: ~
fixable: false
location:
row: 15
column: 0
end_location:
row: 15
column: 3
fix: ~
parent: ~
- kind:
name: PairwiseOverZipped
body: "Prefer `itertools.pairwise()` over `zip()` when iterating over successive pairs"
suggestion: ~
fixable: false
location:
row: 16
column: 0
end_location:
row: 16
column: 3
fix: ~
parent: ~
- kind:
name: PairwiseOverZipped
body: "Prefer `itertools.pairwise()` over `zip()` when iterating over successive pairs"
suggestion: ~
fixable: false
location:
row: 17
column: 0
end_location:
row: 17
column: 3
fix: ~
parent: ~
- kind:
name: PairwiseOverZipped
body: "Prefer `itertools.pairwise()` over `zip()` when iterating over successive pairs"
suggestion: ~
fixable: false
location:
row: 18
column: 5
end_location:
row: 18
column: 8
fix: ~
parent: ~
- kind:
name: PairwiseOverZipped
body: "Prefer `itertools.pairwise()` over `zip()` when iterating over successive pairs"
suggestion: ~
fixable: false
location:
row: 19
column: 5
end_location:
row: 19
column: 8
fix: ~
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.

0 comments on commit 33d2457

Please sign in to comment.