Skip to content

Commit

Permalink
Implement new rule: Prefer augmented assignment (#8877)
Browse files Browse the repository at this point in the history
  • Loading branch information
lshi18 committed Feb 11, 2024
1 parent a50e278 commit 2241be2
Show file tree
Hide file tree
Showing 8 changed files with 753 additions and 0 deletions.
41 changes: 41 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
some_string = "some string" # RUF028
index, a_number, to_multiply, to_divide, to_cube, timeDiffSeconds, flags = 0, 1, 2, 3, 4, 5, 0x3 # RUF028
a_list = [1,2] # RUF028
some_set = {"elem"} # RUF028
mat1, mat2 = None, None # RUF028

some_string = (
some_string
+ "a very long end of string"
) # RUF028
index = index - 1 # RUF028
a_list = a_list + ["to concat"] # RUF028
some_set = some_set | {"to concat"} # RUF028
to_multiply = to_multiply * 5 # RUF028
to_divide = to_divide / 5 # RUF028
to_divide = to_divide // 5 # RUF028
to_cube = to_cube ** 3 # RUF028
timeDiffSeconds = timeDiffSeconds % 60 # RUF028
flags = flags & 0x1 # RUF028
flags = flags | 0x1 # RUF028
flags = flags ^ 0x1 # RUF028
flags = flags << 1 # RUF028
flags = flags >> 1 # RUF028
mat1 = mat1 @ mat2 # RUF028
a_list[1] = a_list[1] + 1 #RUF028

a_list[0:2] = a_list[0:2] * 3 # RUF028
a_list[:2] = a_list[:2] * 3 # RUF028
a_list[1:] = a_list[1:] * 3 # RUF028
a_list[:] = a_list[:] * 3 # RUF028

class T:
def t(self):
self.a = self.a + 1 # RUF028

obj = T()
obj.a = obj.a + 1 # RUF028

a_list[0] = a_list[:] * 3 # OK
index = a_number = a_number + 1 # OK
a_number = index = a_number + 1 # OK
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1476,6 +1476,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::UnsortedDunderSlots) {
ruff::rules::sort_dunder_slots_assign(checker, assign);
}
if checker.enabled(Rule::BinaryOpAndNormalAssignment) {
ruff::rules::binary_op_and_normal_assignment(checker, assign);
}
if checker.source_type.is_stub() {
if checker.any_enabled(&[
Rule::UnprefixedTypeParam,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "025") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryDictComprehensionForIterable),
(Ruff, "026") => (RuleGroup::Preview, rules::ruff::rules::DefaultFactoryKwarg),
(Ruff, "027") => (RuleGroup::Preview, rules::ruff::rules::MissingFStringSyntax),
(Ruff, "028") => (RuleGroup::Preview, rules::ruff::rules::BinaryOpAndNormalAssignment),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),
#[cfg(feature = "test-rules")]
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ mod tests {
#[test_case(Rule::DefaultFactoryKwarg, Path::new("RUF026.py"))]
#[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_0.py"))]
#[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_1.py"))]
#[test_case(Rule::BinaryOpAndNormalAssignment, Path::new("RUF028.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
Original file line number Diff line number Diff line change
@@ -0,0 +1,236 @@
use ast::{Expr, StmtAugAssign};
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_text_size::{Ranged, TextRange};

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

/// ## What it does?
/// Checks for normal assignment statements whose target is the same as the
/// left operand of a binary operator, in which cases, augmented assignment
/// could potentially be used instead.
///
/// ## Why is this bad?
/// Augmented assignment operators are more concise to perform a binary
/// operation and assign results back to one of the operands.
///
/// ## Example
/// ```python
///
/// a = 1
/// a = a + 1
/// ```
///
/// Use instead:
/// ```python
///
/// a = 1
/// a += 1
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as being unsafe, in that it could alter semantics
/// of the given Python code in some scenarios.
///
/// ## Example
/// The following code using mutable data types as the assignment target
/// ```python
///
/// a = [1]
/// b = a
/// a = a + [2]
/// assert (a, b) == ([1, 2], [1])
/// ```
///
/// is not the same as
/// ```python
///
/// a = [1]
/// b = a
/// a += [2]
/// assert (a, b) == ([1,2], [1,2])
/// ```
#[violation]
pub struct BinaryOpAndNormalAssignment;

impl Violation for BinaryOpAndNormalAssignment {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
format!(
"Normal assignment with left operand of binary operator being the same as the target."
)
}

fn fix_title(&self) -> Option<String> {
Some("Use augmented assignment instead.".to_string())
}
}

pub(crate) fn binary_op_and_normal_assignment(
checker: &mut Checker,
assign @ ast::StmtAssign { value, targets, .. }: &ast::StmtAssign,
) {
if targets.len() != 1 {
return;
}
let target = targets.first().unwrap();

let rhs_expr = value
.as_bin_op_expr()
.map(|e| (e.left.as_ref(), e.op, e.right.as_ref()));
if rhs_expr.is_none() {
return;
}
let (left_operand, operator, right_operand) = rhs_expr.unwrap();

if name_expr(target, left_operand)
|| object_attribute_expr(target, left_operand)
|| index_subscript_expr(target, left_operand)
|| slice_subscript_expr(target, left_operand)
{
let diagnostic = Diagnostic::new(BinaryOpAndNormalAssignment, assign.range()).with_fix(
generate_fix(checker, target, operator, right_operand, assign.range()),
);
checker.diagnostics.push(diagnostic);
}
}

fn name_expr(target: &Expr, left_operand: &Expr) -> bool {
matches!(
(
target.as_name_expr(),
left_operand.as_name_expr()
),
(
Some(ast::ExprName {
id: target_name_id, ..
}),
Some(ast::ExprName {
id: left_name_id, ..
}),
) if target_name_id == left_name_id
)
}

fn object_attribute_expr(target: &Expr, left_operand: &Expr) -> bool {
matches!((
target
.as_attribute_expr()
.and_then(|attr| attr.value.as_name_expr())
.map(|name| &name.id),
target
.as_attribute_expr()
.map(|attr| attr.attr.as_str()),
left_operand
.as_attribute_expr()
.and_then(|attr| attr.value.as_name_expr())
.map(|name| &name.id),
left_operand
.as_attribute_expr()
.map(|attr| attr.attr.as_str())
),
(
Some(target_name_id),
Some(target_attr_id),
Some(left_name_id),
Some(left_attr_id)
)
if target_name_id == left_name_id && target_attr_id == left_attr_id
)
}

fn index_subscript_expr(target: &Expr, left_operand: &Expr) -> bool {
matches!((
target
.as_subscript_expr()
.and_then(|subs| subs.value.as_name_expr())
.map(|name| &name.id),
target
.as_subscript_expr()
.and_then(|subs| subs.slice.as_number_literal_expr())
.map(|num| &num.value),
left_operand
.as_subscript_expr()
.and_then(|subs| subs.value.as_name_expr())
.map(|name| &name.id),
left_operand
.as_subscript_expr()
.and_then(|subs| subs.slice.as_number_literal_expr())
.map(|num| &num.value),
),
(
Some(target_name),
Some(target_subs),
Some(left_name),
Some(left_subs)
)
if target_name == left_name && target_subs == left_subs
)
}

fn slice_subscript_expr(target: &Expr, left_operand: &Expr) -> bool {
match (
target
.as_subscript_expr()
.and_then(|subs| subs.value.as_name_expr())
.map(|name| &name.id),
target
.as_subscript_expr()
.and_then(|subs| subs.slice.as_slice_expr()),
left_operand
.as_subscript_expr()
.and_then(|subs| subs.value.as_name_expr())
.map(|name| &name.id),
left_operand
.as_subscript_expr()
.and_then(|subs| subs.slice.as_slice_expr()),
) {
(Some(target_name), Some(target_slice), Some(left_name), Some(left_slice))
if target_name == left_name =>
{
let target_lower = target_slice
.lower
.as_ref()
.and_then(|lower| lower.as_number_literal_expr())
.map(|num| &num.value);
let target_upper = target_slice
.upper
.as_ref()
.and_then(|upper| upper.as_number_literal_expr())
.map(|num| &num.value);
let left_lower = left_slice
.lower
.as_ref()
.and_then(|lower| lower.as_number_literal_expr())
.map(|num| &num.value);
let left_upper = left_slice
.upper
.as_ref()
.and_then(|upper| upper.as_number_literal_expr())
.map(|num| &num.value);

target_lower == left_lower && target_upper == left_upper
}
_ => false,
}
}

fn generate_fix(
checker: &Checker,
target: &Expr,
operator: ast::Operator,
right_operand: &Expr,
text_range: TextRange,
) -> Fix {
let new_stmt = ast::Stmt::AugAssign(StmtAugAssign {
range: TextRange::default(),
target: Box::new(target.clone()),
op: operator,
value: Box::new(right_operand.clone()),
});
let content = checker.generator().stmt(&new_stmt);
Fix::unsafe_edit(Edit::range_replacement(content, text_range))
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
pub(crate) use ambiguous_unicode_character::*;
pub(crate) use assignment_in_assert::*;
pub(crate) use asyncio_dangling_task::*;
pub(crate) use binary_op_and_normal_assignment::*;
pub(crate) use collection_literal_concatenation::*;
pub(crate) use default_factory_kwarg::*;
pub(crate) use explicit_f_string_type_conversion::*;
Expand Down Expand Up @@ -29,6 +30,7 @@ pub(crate) use unused_noqa::*;
mod ambiguous_unicode_character;
mod assignment_in_assert;
mod asyncio_dangling_task;
mod binary_op_and_normal_assignment;
mod collection_literal_concatenation;
mod confusables;
mod default_factory_kwarg;
Expand Down
Loading

0 comments on commit 2241be2

Please sign in to comment.