[ruff] if k in d: del d[k] (RUF051) (#14553)

## Summary

Resolves #7537.

## Test Plan

`cargo nextest run` and `cargo insta test`.

---------

Co-authored-by: Micha Reiser <[email protected]>

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

@@ -0,0 +1,130 @@
+d = {}
+l = []
+
+
+### Errors
+
+if k in d:                          # Bare name
+    del d[k]
+
+if '' in d:                         # String
+    del d[""]                       # Different quotes
+
+if b"" in d:                        # Bytes
+    del d[                          # Multiline slice
+        b''''''                     # Triple quotes
+    ]
+
+if 0 in d: del d[0]                 # Single-line statement
+
+if 3j in d:                         # Complex
+    del d[3j]
+
+if 0.1234 in d:                     # Float
+    del d[.1_2_3_4]                 # Number separators and shorthand syntax
+
+if True in d:                       # True
+    del d[True]
+
+if False in d:                      # False
+    del d[False]
+
+if None in d:                       # None
+    del d[
+        # Comment in the middle
+        None
+    ]
+
+if ... in d:                        # Ellipsis
+    del d[
+                                    # Comment in the middle, indented
+        ...]
+
+if "a" "bc" in d:                   # String concatenation
+    del d['abc']
+
+if r"\foo" in d:                    # Raw string
+    del d['\\foo']
+
+if b'yt' b'es' in d:                # Bytes concatenation
+    del d[rb"""ytes"""]             # Raw bytes
+
+if k in d:
+    # comment that gets dropped
+    del d[k]
+
+### Safely fixable
+
+if k in d:
+    del d[k]
+
+if '' in d:
+    del d[""]
+
+if b"" in d:
+    del d[
+        b''''''
+    ]
+
+if 0 in d: del d[0]
+
+if 3j in d:
+    del d[3j]
+
+if 0.1234 in d:
+    del d[.1_2_3_4]
+
+if True in d:
+    del d[True]
+
+if False in d:
+    del d[False]
+
+if None in d:
+    del d[
+        None
+    ]
+
+if ... in d:
+    del d[
+        ...]
+
+if "a" "bc" in d:
+    del d['abc']
+
+if r"\foo" in d:
+    del d['\\foo']
+
+if b'yt' b'es' in d:
+    del d[rb"""ytes"""]             # This should not make the fix unsafe
+
+
+
+### No errors
+
+if k in l:                          # Not a dict
+    del l[k]
+
+if d.__contains__(k):               # Explicit dunder call
+    del d[k]
+
+if a.k in d:                        # Attribute
+    del d[a.k]
+
+if (a, b) in d:                     # Tuple
+    del d[a, b]
+
+if 2 in d:                          # Different key value (int)
+    del d[3]
+
+if 2_4j in d:                       # Different key value (complex)
+    del d[3.6]                      # Different key value (float)
+
+if 0.1 + 0.2 in d:                  # Complex expression
+    del d[0.3]
+
+if f"0" in d:                       # f-string
+    del d[f"0"]
+
+if k in a.d:                        # Attribute dict
+    del a.d[k]

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

@@ -1235,6 +1235,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
                     }
                 }
             }
+            if checker.enabled(Rule::IfKeyInDictDel) {
+                ruff::rules::if_key_in_dict_del(checker, if_);
+            }
         }
         Stmt::Assert(
             assert_stmt @ ast::StmtAssert {

crates/ruff_linter/src/checkers/ast/mod.rs

@@ -222,7 +222,7 @@ pub(crate) struct Checker<'a> {
     analyze: deferred::Analyze,
     /// The cumulative set of diagnostics computed across all lint rules.
     pub(crate) diagnostics: Vec<Diagnostic>,
-    /// The list of names already seen by flake8-bugbear diagnostics, to avoid duplicate violations..
+    /// The list of names already seen by flake8-bugbear diagnostics, to avoid duplicate violations.
     pub(crate) flake8_bugbear_seen: Vec<TextRange>,
     /// The end offset of the last visited statement.
     last_stmt_end: TextSize,

crates/ruff_linter/src/codes.rs

@@ -987,6 +987,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Ruff, "041") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryNestedLiteral),
         (Ruff, "046") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryCastToInt),
         (Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::MapIntVersionParsing),
+        (Ruff, "051") => (RuleGroup::Preview, rules::ruff::rules::IfKeyInDictDel),
         (Ruff, "052") => (RuleGroup::Preview, rules::ruff::rules::UsedDummyVariable),
         (Ruff, "055") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryRegularExpression),
         (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),

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

@@ -71,6 +71,7 @@ mod tests {
     #[test_case(Rule::InvalidAssertMessageLiteralArgument, Path::new("RUF040.py"))]
     #[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.py"))]
     #[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.pyi"))]
+    #[test_case(Rule::IfKeyInDictDel, Path::new("RUF051.py"))]
     #[test_case(Rule::UsedDummyVariable, Path::new("RUF052.py"))]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());

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

@@ -0,0 +1,154 @@
+use crate::checkers::ast::Checker;
+use ruff_diagnostics::{AlwaysFixableViolation, Applicability, Diagnostic, Edit, Fix};
+use ruff_macros::{derive_message_formats, ViolationMetadata};
+use ruff_python_ast::{CmpOp, Expr, ExprName, ExprSubscript, Stmt, StmtIf};
+use ruff_python_semantic::analyze::typing;
+use ruff_python_semantic::SemanticModel;
+
+type Key = Expr;
+type Dict = ExprName;
+
+/// ## What it does
+/// Checks for `if key in dictionary: del dictionary[key]`.
+///
+/// ## Why is this bad?
+/// To remove a key-value pair from a dictionary, it's more concise to use `.pop(..., None)`.
+///
+/// ## Example
+///
+/// ```python
+/// if key in dictionary:
+///     del dictionary[key]
+/// ```
+///
+/// Use instead:
+///
+/// ```python
+/// dictionary.pop(key, None)
+/// ```
+///
+/// ## Fix safety
+/// This rule's fix is marked as safe, unless the if statement contains comments.
+#[derive(ViolationMetadata)]
+pub(crate) struct IfKeyInDictDel;
+
+impl AlwaysFixableViolation for IfKeyInDictDel {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        "Use `pop` instead of `key in dict` followed by `delete dict[key]`".to_string()
+    }
+
+    fn fix_title(&self) -> String {
+        "Replace `if` statement with `.pop(..., None)`".to_string()
+    }
+}
+
+/// RUF051
+pub(crate) fn if_key_in_dict_del(checker: &mut Checker, stmt: &StmtIf) {
+    let [Stmt::Delete(delete)] = &stmt.body[..] else {
+        return;
+    };
+
+    let Some((test_dict, test_key)) = extract_dict_and_key_from_test(&stmt.test) else {
+        return;
+    };
+    let Some((del_dict, del_key)) = extract_dict_and_key_from_del(&delete.targets) else {
+        return;
+    };
+
+    if !is_same_key(test_key, del_key) || !is_same_dict(test_dict, del_dict) {
+        return;
+    }
+
+    if !is_known_to_be_of_type_dict(checker.semantic(), test_dict) {
+        return;
+    }
+
+    let fix = replace_with_dict_pop_fix(checker, stmt, test_dict, test_key);
+
+    let diagnostic = Diagnostic::new(IfKeyInDictDel, delete.range);
+
+    checker.diagnostics.push(diagnostic.with_fix(fix));
+}
+
+fn extract_dict_and_key_from_test(test: &Expr) -> Option<(&Dict, &Key)> {
+    let Expr::Compare(comp) = test else {
+        return None;
+    };
+
+    let [Expr::Name(dict)] = comp.comparators.as_ref() else {
+        return None;
+    };
+
+    if !matches!(comp.ops.as_ref(), [CmpOp::In]) {
+        return None;
+    }
+
+    Some((dict, &comp.left))
+}
+
+fn extract_dict_and_key_from_del(targets: &[Expr]) -> Option<(&Dict, &Key)> {
+    let [Expr::Subscript(ExprSubscript { value, slice, .. })] = targets else {
+        return None;
+    };
+
+    let Expr::Name(dict) = value.as_ref() else {
+        return None;
+    };
+
+    Some((dict, slice))
+}
+
+fn is_same_key(test: &Key, del: &Key) -> bool {
+    match (test, del) {
+        (Expr::Name(ExprName { id: test, .. }), Expr::Name(ExprName { id: del, .. })) => {
+            test.as_str() == del.as_str()
+        }
+
+        (Expr::NoneLiteral(..), Expr::NoneLiteral(..)) => true,
+        (Expr::EllipsisLiteral(..), Expr::EllipsisLiteral(..)) => true,
+
+        (Expr::BooleanLiteral(test), Expr::BooleanLiteral(del)) => test.value == del.value,
+        (Expr::NumberLiteral(test), Expr::NumberLiteral(del)) => test.value == del.value,
+
+        (Expr::BytesLiteral(test), Expr::BytesLiteral(del)) => {
+            Iterator::eq(test.value.bytes(), del.value.bytes())
+        }
+
+        (Expr::StringLiteral(test), Expr::StringLiteral(del)) => {
+            Iterator::eq(test.value.chars(), del.value.chars())
+        }
+
+        _ => false,
+    }
+}
+
+fn is_same_dict(test: &Dict, del: &Dict) -> bool {
+    test.id.as_str() == del.id.as_str()
+}
+
+fn is_known_to_be_of_type_dict(semantic: &SemanticModel, dict: &Dict) -> bool {
+    let Some(binding) = semantic.only_binding(dict).map(|id| semantic.binding(id)) else {
+        return false;
+    };
+
+    typing::is_dict(binding, semantic)
+}
+
+fn replace_with_dict_pop_fix(checker: &Checker, stmt: &StmtIf, dict: &Dict, key: &Key) -> Fix {
+    let locator = checker.locator();
+    let dict_expr = locator.slice(dict);
+    let key_expr = locator.slice(key);
+
+    let replacement = format!("{dict_expr}.pop({key_expr}, None)");
+    let edit = Edit::range_replacement(replacement, stmt.range);
+
+    let comment_ranges = checker.comment_ranges();
+    let applicability = if comment_ranges.intersects(stmt.range) {
+        Applicability::Unsafe
+    } else {
+        Applicability::Safe
+    };
+
+    Fix::applicable_edit(edit, applicability)
+}

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

@@ -7,6 +7,7 @@ pub(crate) use decimal_from_float_literal::*;
 pub(crate) use default_factory_kwarg::*;
 pub(crate) use explicit_f_string_type_conversion::*;
 pub(crate) use function_call_in_dataclass_default::*;
+pub(crate) use if_key_in_dict_del::*;
 pub(crate) use implicit_optional::*;
 pub(crate) use incorrectly_parenthesized_tuple_in_subscript::*;
 pub(crate) use invalid_assert_message_literal_argument::*;
@@ -54,6 +55,7 @@ mod default_factory_kwarg;
 mod explicit_f_string_type_conversion;
 mod function_call_in_dataclass_default;
 mod helpers;
+mod if_key_in_dict_del;
 mod implicit_optional;
 mod incorrectly_parenthesized_tuple_in_subscript;
 mod invalid_assert_message_literal_argument;