Implement new rule: Prefer augmented assignment (astral-sh#8877)

crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py

@@ -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

crates/ruff_linter/src/checkers/ast/analyze/statement.rs

@@ -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,

crates/ruff_linter/src/codes.rs

@@ -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")]

crates/ruff_linter/src/rules/ruff/mod.rs

@@ -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(

crates/ruff_linter/src/rules/ruff/rules/binary_op_and_normal_assignment.rs

@@ -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))
+}

crates/ruff_linter/src/rules/ruff/rules/mod.rs

@@ -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::*;
@@ -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;