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

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

@@ -0,0 +1,126 @@
+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
+
+
+### 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

@@ -984,6 +984,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Ruff, "040") => (RuleGroup::Preview, rules::ruff::rules::InvalidAssertMessageLiteralArgument),
         (Ruff, "041") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryNestedLiteral),
         (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

@@ -413,6 +413,7 @@ mod tests {
     #[test_case(Rule::UnrawRePattern, Path::new("RUF039_concat.py"))]
     #[test_case(Rule::UnnecessaryRegularExpression, Path::new("RUF055_0.py"))]
     #[test_case(Rule::UnnecessaryRegularExpression, Path::new("RUF055_1.py"))]
+    #[test_case(Rule::IfKeyInDictDel, Path::new("RUF051.py"))]
     fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!(
             "preview__{}_{}",

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

@@ -0,0 +1,182 @@
+use ruff_diagnostics::{AlwaysFixableViolation, Applicability, Diagnostic, Edit, Fix};
+use ruff_macros::{derive_message_formats, ViolationMetadata};
+use ruff_python_ast::comparable::ComparableExpr;
+use ruff_python_ast::{
+    CmpOp, Expr, ExprCompare, ExprName, ExprSubscript, Stmt, StmtDelete, StmtIf,
+};
+use ruff_python_semantic::analyze::typing;
+use ruff_python_semantic::SemanticModel;
+use ruff_text_size::{Ranged, TextRange};
+
+use crate::checkers::ast::Checker;
+
+// Real type: Expr::Name;
+type Key = Expr;
+type Dict = Expr;
+
+/// ## What it does
+/// Checks for `if key in dictionary: del dictionary[key]`.
+///
+/// ## Why is this bad?
+/// When removing a key from a dictionary, it is unnecessary to check for its existence.
+/// `.pop(..., None)` is simpler and has the same semantic.
+///
+/// ## Example
+///
+/// ```python
+/// if key in dictionary:
+///     del dictionary[key]
+/// ```
+///
+/// Use instead:
+///
+/// ```python
+/// dictionary.pop(key, None)
+/// ```
+#[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 with `.pop(..., None)`".to_string()
+    }
+}
+
+/// RUF051
+pub(crate) fn if_key_in_dict_del(checker: &mut Checker, stmt: &StmtIf) {
+    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(&stmt.body) 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 diagnostic = Diagnostic::new(IfKeyInDictDel, stmt.range);
+    let Some(fix) = replace_with_dict_pop_fix(checker, stmt, test_dict, test_key) else {
+        // This is only reached when the `if` body has no statement,
+        // which is impossible as we have already checked for this above.
+        return;
+    };
+
+    checker.diagnostics.push(diagnostic.with_fix(fix));
+}
+
+fn extract_dict_and_key_from_test(test: &Expr) -> Option<(&Dict, &Key)> {
+    let Expr::Compare(ExprCompare {
+        left,
+        ops,
+        comparators,
+        ..
+    }) = test
+    else {
+        return None;
+    };
+
+    if !matches!(ops.as_ref(), [CmpOp::In]) {
+        return None;
+    }
+
+    let [right] = comparators.as_ref() else {
+        return None;
+    };
+
+    dict_and_key_verified(right, left.as_ref())
+}
+
+fn extract_dict_and_key_from_del(body: &[Stmt]) -> Option<(&Dict, &Key)> {
+    let [Stmt::Delete(StmtDelete { targets, .. })] = body else {
+        return None;
+    };
+    let [Expr::Subscript(ExprSubscript { value, slice, .. })] = &targets[..] else {
+        return None;
+    };
+
+    dict_and_key_verified(value.as_ref(), slice.as_ref())
+}
+
+fn dict_and_key_verified<'d, 'k>(dict: &'d Dict, key: &'k Key) -> Option<(&'d Dict, &'k Key)> {
+    if !key.is_name_expr() && !key.is_literal_expr() {
+        return None;
+    }
+
+    if !dict.is_name_expr() {
+        return None;
+    }
+
+    Some((dict, key))
+}
+
+fn is_same_key(test: &Expr, del: &Expr) -> bool {
+    match (test, del) {
+        (Expr::Name(..), Expr::Name(..))
+        | (Expr::NoneLiteral(..), Expr::NoneLiteral(..))
+        | (Expr::EllipsisLiteral(..), Expr::EllipsisLiteral(..))
+        | (Expr::BooleanLiteral(..), Expr::BooleanLiteral(..))
+        | (Expr::NumberLiteral(..), Expr::NumberLiteral(..))
+        | (Expr::BytesLiteral(..), Expr::BytesLiteral(..))
+        | (Expr::StringLiteral(..), Expr::StringLiteral(..)) => {
+            ComparableExpr::from(test) == ComparableExpr::from(del)
+        }
+
+        _ => false,
+    }
+}
+
+fn is_same_dict(test: &Expr, del: &Expr) -> bool {
+    match (test, del) {
+        (Expr::Name(ExprName { id: test, .. }), Expr::Name(ExprName { id: del, .. })) => {
+            test.as_str() == del.as_str()
+        }
+
+        _ => false,
+    }
+}
+
+fn is_known_to_be_of_type_dict(semantic: &SemanticModel, dict: &Expr) -> bool {
+    dict.as_name_expr().is_some_and(|name| {
+        let Some(binding) = semantic.only_binding(name).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,
+) -> Option<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 test_expr = &stmt.test;
+    let del_stmt = stmt.body.first()?;
+    let test_to_del = TextRange::new(test_expr.end(), del_stmt.start());
+
+    let comment_ranges = checker.comment_ranges();
+    let applicability = if comment_ranges.has_comments(&test_to_del, checker.source()) {
+        Applicability::Unsafe
+    } else {
+        Applicability::Safe
+    };
+
+    Some(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::*;
@@ -53,6 +54,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;