Skip to content

Commit

Permalink
Add Option<i64> instead of Result. Change successive pair definition
Browse files Browse the repository at this point in the history
  • Loading branch information
evanrittenhouse committed Mar 16, 2023
1 parent 237c636 commit 63001bc
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 46 deletions.
14 changes: 8 additions & 6 deletions crates/ruff/resources/test/fixtures/ruff/RUF007.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@
otherInput = [2, 3, 4]

# Don't trigger
zip(input, otherInput)
list(zip(input, otherInput))
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

# Error - prefer pairwise here
zip(input, input[1:])
list(zip(input, input[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:]))

# Don't want the error triggered here since it's not successive - pairwise() is not a valid substitute!
zip(input, input[2:])
67 changes: 37 additions & 30 deletions crates/ruff/src/rules/ruff/rules/pairwise_over_zipped.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
use std::num::ParseIntError;

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::source_code::Stylist;
use rustpython_parser::ast::{Expr, ExprKind};
use rustpython_parser::ast::{Constant, Expr, ExprKind, Unaryop};

use crate::checkers::ast::Checker;
use crate::settings::types::PythonVersion;
Expand All @@ -23,16 +21,12 @@ impl Violation for PairwiseOverZipped {
#[derive(Debug)]
struct SliceInfo {
arg_name: String,
slice_start: Result<i64, ParseIntError>,
slice_end: Result<i64, ParseIntError>,
slice_start: Option<i64>,
slice_end: Option<i64>,
}

impl SliceInfo {
pub fn new(
arg_name: String,
slice_start: Result<i64, ParseIntError>,
slice_end: Result<i64, ParseIntError>,
) -> Self {
pub fn new(arg_name: String, slice_start: Option<i64>, slice_end: Option<i64>) -> Self {
Self {
arg_name,
slice_start,
Expand All @@ -51,42 +45,56 @@ fn get_slice_info(expr: &Expr, stylist: &Stylist) -> Option<SliceInfo> {
return None;
};

let mut lower_bound = Ok(0i64);
let mut upper_bound = Ok(0i64);
let mut lower_bound = None;
let mut upper_bound = None;
if let ExprKind::Slice { lower, upper, .. } = &slice.node {
if lower.is_some() {
if let ExprKind::Constant {
value: lower_value, ..
} = &lower.as_ref().unwrap().node
{
lower_bound = unparse_constant(lower_value, stylist).parse::<i64>();
}
lower_bound = get_bound(&lower.as_ref().unwrap().node, stylist);
}

if upper.is_some() {
if let ExprKind::Constant {
value: upper_value, ..
} = &upper.as_ref().unwrap().node
{
upper_bound = unparse_constant(upper_value, stylist).parse::<i64>();
}
upper_bound = get_bound(&upper.as_ref().unwrap().node, stylist);
}
};

Some(SliceInfo::new(arg_id.to_string(), lower_bound, upper_bound))
}

fn get_bound(expr: &ExprKind, stylist: &Stylist) -> Option<i64> {
fn get_constant_value(constant: &Constant, stylist: &Stylist) -> Option<i64> {
unparse_constant(constant, stylist).parse::<i64>().ok()
}

let mut bound = None;
match expr {
ExprKind::Constant { value, .. } => bound = get_constant_value(&value, stylist),
ExprKind::UnaryOp {
op: Unaryop::USub | Unaryop::Invert,
operand,
} => match &operand.node {
ExprKind::Constant { value, .. } => {
bound = get_constant_value(&value, stylist).map(|v| v * -1)
}
_ => (),
},
_ => (),
}

bound
}

pub fn pairwise_over_zipped(checker: &mut Checker, func: &Expr, args: &[Expr]) {
if let ExprKind::Name { id, .. } = &func.node {
if checker.settings.target_version >= PythonVersion::Py310
&& args.len() > 1
&& id == "zip"
&& checker.ctx.is_builtin(id)
{
// First arg can be a Name or a Subscript
// First arg can be a only be a Name or a Subscript. If it's a Name, we want the "slice" to
// default to 0
let first_arg_info_opt = match &args[0].node {
ExprKind::Name { id: arg_id, .. } => {
Some(SliceInfo::new(arg_id.to_string(), Ok(0i64), Ok(0i64)))
Some(SliceInfo::new(arg_id.to_string(), Some(0i64), None))
}
ExprKind::Subscript { .. } => get_slice_info(&args[0], checker.stylist),
_ => None,
Expand All @@ -102,10 +110,9 @@ pub fn pairwise_over_zipped(checker: &mut Checker, func: &Expr, args: &[Expr]) {

let Some(second_arg_info) = get_slice_info(&args[1], checker.stylist) else { return; };

// unwrap_or forces no diagnostic if there was a parsing error
let args_are_successive = (first_arg_info.slice_start.unwrap_or(1) == 0
|| first_arg_info.slice_end.unwrap_or(1) == -1)
&& second_arg_info.slice_start.unwrap_or(0) == 1;
let args_are_successive = second_arg_info.slice_start.unwrap_or(0)
- first_arg_info.slice_start.unwrap_or(0)
== 1;

if first_arg_info.arg_name == second_arg_info.arg_name && args_are_successive {
checker
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ expression: diagnostics
suggestion: ~
fixable: false
location:
row: 9
row: 12
column: 0
end_location:
row: 9
row: 12
column: 3
fix: ~
parent: ~
Expand All @@ -21,11 +21,11 @@ expression: diagnostics
suggestion: ~
fixable: false
location:
row: 10
column: 5
row: 13
column: 0
end_location:
row: 10
column: 8
row: 13
column: 3
fix: ~
parent: ~
- kind:
Expand All @@ -34,10 +34,10 @@ expression: diagnostics
suggestion: ~
fixable: false
location:
row: 11
row: 14
column: 0
end_location:
row: 11
row: 14
column: 3
fix: ~
parent: ~
Expand All @@ -47,10 +47,36 @@ expression: diagnostics
suggestion: ~
fixable: false
location:
row: 12
row: 15
column: 0
end_location:
row: 15
column: 3
fix: ~
parent: ~
- kind:
name: PairwiseOverZipped
body: "Prefer `itertools.pairwise()` to `zip()` if trying to fetch an iterable's successive overlapping pairs."
suggestion: ~
fixable: false
location:
row: 16
column: 5
end_location:
row: 12
row: 16
column: 8
fix: ~
parent: ~
- kind:
name: PairwiseOverZipped
body: "Prefer `itertools.pairwise()` to `zip()` if trying to fetch an iterable's successive overlapping pairs."
suggestion: ~
fixable: false
location:
row: 17
column: 5
end_location:
row: 17
column: 8
fix: ~
parent: ~
Expand Down

0 comments on commit 63001bc

Please sign in to comment.