diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF019.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF019.py new file mode 100644 index 0000000000..ffcdb032e2 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF019.py @@ -0,0 +1,20 @@ +d = {} +# RUF019 +if "k" in d and d["k"]: + pass + +k = "k" +if k in d and d[k]: + pass + +if (k) in d and d[k]: + pass + +if k in d and d[(k)]: + pass + +# OK +v = "k" in d and d["k"] + +if f() in d and d[f()]: + pass diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 707d8e762b..041d566f2a 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1412,6 +1412,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::RepeatedEqualityComparison) { pylint::rules::repeated_equality_comparison(checker, bool_op); } + if checker.enabled(Rule::UnnecessaryKeyCheck) { + ruff::rules::unnecessary_key_check(checker, expr); + } } Expr::NamedExpr(..) => { if checker.enabled(Rule::AssignmentInAssert) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index f0b9a80804..dea29feeb0 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -866,6 +866,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { #[allow(deprecated)] (Ruff, "017") => (RuleGroup::Nursery, rules::ruff::rules::QuadraticListSummation), (Ruff, "018") => (RuleGroup::Preview, rules::ruff::rules::AssignmentInAssert), + (Ruff, "019") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryKeyCheck), (Ruff, "100") => (RuleGroup::Unspecified, rules::ruff::rules::UnusedNOQA), (Ruff, "200") => (RuleGroup::Unspecified, rules::ruff::rules::InvalidPyprojectToml), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 60ac081d0b..59cf6fa350 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -43,6 +43,7 @@ mod tests { #[test_case(Rule::QuadraticListSummation, Path::new("RUF017_1.py"))] #[test_case(Rule::QuadraticListSummation, Path::new("RUF017_0.py"))] #[test_case(Rule::AssignmentInAssert, Path::new("RUF018.py"))] + #[test_case(Rule::UnnecessaryKeyCheck, Path::new("RUF019.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 910c6e6379..eddad40cd5 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -12,6 +12,7 @@ pub(crate) use mutable_dataclass_default::*; pub(crate) use pairwise_over_zipped::*; pub(crate) use static_key_dict_comprehension::*; pub(crate) use unnecessary_iterable_allocation_for_first_element::*; +pub(crate) use unnecessary_key_check::*; #[cfg(feature = "unreachable-code")] pub(crate) use unreachable::*; pub(crate) use unused_noqa::*; @@ -32,6 +33,7 @@ mod mutable_dataclass_default; mod pairwise_over_zipped; mod static_key_dict_comprehension; mod unnecessary_iterable_allocation_for_first_element; +mod unnecessary_key_check; #[cfg(feature = "unreachable-code")] pub(crate) mod unreachable; mod unused_noqa; diff --git a/crates/ruff_linter/src/rules/ruff/rules/unnecessary_key_check.rs b/crates/ruff_linter/src/rules/ruff/rules/unnecessary_key_check.rs new file mode 100644 index 0000000000..8636619dac --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/unnecessary_key_check.rs @@ -0,0 +1,131 @@ +use ruff_python_ast::comparable::ComparableExpr; +use ruff_python_ast::{self as ast, BoolOp, CmpOp, Expr}; + +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::contains_effect; +use ruff_python_ast::parenthesize::parenthesized_range; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for unnecessary key checks prior to accessing a dictionary. +/// +/// ## Why is this bad? +/// When working with dictionaries, the `get` can be used to access a value +/// without having to check if the dictionary contains the relevant key, +/// returning `None` if the key is not present. +/// +/// ## Examples +/// ```python +/// if "key" in dct and dct["key"]: +/// ... +/// ``` +/// +/// Use instead: +/// ```python +/// if dct.get("key"): +/// ... +/// ``` +#[violation] +pub struct UnnecessaryKeyCheck; + +impl AlwaysFixableViolation for UnnecessaryKeyCheck { + #[derive_message_formats] + fn message(&self) -> String { + format!("Unnecessary key check before dictionary access") + } + + fn fix_title(&self) -> String { + format!("Replace with `dict.get`") + } +} + +/// RUF019 +pub(crate) fn unnecessary_key_check(checker: &mut Checker, expr: &Expr) { + if !checker.semantic().in_boolean_test() { + return; + } + + let Expr::BoolOp(ast::ExprBoolOp { + op: BoolOp::And, + values, + .. + }) = expr + else { + return; + }; + + let [left, right] = values.as_slice() else { + return; + }; + + // Left should be, e.g., `key in dct`. + let Expr::Compare(ast::ExprCompare { + left: key_left, + ops, + comparators, + .. + }) = left + else { + return; + }; + + if !matches!(ops.as_slice(), [CmpOp::In]) { + return; + } + + let [obj_left] = comparators.as_slice() else { + return; + }; + + // Right should be, e.g., `dct[key]`. + let Expr::Subscript(ast::ExprSubscript { + value: obj_right, + slice: key_right, + .. + }) = right + else { + return; + }; + + if ComparableExpr::from(obj_left) != ComparableExpr::from(obj_right) + || ComparableExpr::from(key_left) != ComparableExpr::from(key_right) + { + return; + } + + if contains_effect(obj_left, |id| checker.semantic().is_builtin(id)) + || contains_effect(key_left, |id| checker.semantic().is_builtin(id)) + { + return; + } + + let mut diagnostic = Diagnostic::new(UnnecessaryKeyCheck, expr.range()); + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + format!( + "{}.get({})", + checker.locator().slice( + parenthesized_range( + obj_right.into(), + right.into(), + checker.indexer().comment_ranges(), + checker.locator().contents(), + ) + .unwrap_or(obj_right.range()) + ), + checker.locator().slice( + parenthesized_range( + key_right.into(), + right.into(), + checker.indexer().comment_ranges(), + checker.locator().contents(), + ) + .unwrap_or(key_right.range()) + ), + ), + expr.range(), + ))); + checker.diagnostics.push(diagnostic); +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF019_RUF019.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF019_RUF019.py.snap new file mode 100644 index 0000000000..db7aa2a866 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF019_RUF019.py.snap @@ -0,0 +1,82 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF019.py:3:4: RUF019 [*] Unnecessary key check before dictionary access + | +1 | d = {} +2 | # RUF019 +3 | if "k" in d and d["k"]: + | ^^^^^^^^^^^^^^^^^^^ RUF019 +4 | pass + | + = help: Replace with `dict.get` + +ℹ Fix +1 1 | d = {} +2 2 | # RUF019 +3 |-if "k" in d and d["k"]: + 3 |+if d.get("k"): +4 4 | pass +5 5 | +6 6 | k = "k" + +RUF019.py:7:4: RUF019 [*] Unnecessary key check before dictionary access + | +6 | k = "k" +7 | if k in d and d[k]: + | ^^^^^^^^^^^^^^^ RUF019 +8 | pass + | + = help: Replace with `dict.get` + +ℹ Fix +4 4 | pass +5 5 | +6 6 | k = "k" +7 |-if k in d and d[k]: + 7 |+if d.get(k): +8 8 | pass +9 9 | +10 10 | if (k) in d and d[k]: + +RUF019.py:10:4: RUF019 [*] Unnecessary key check before dictionary access + | + 8 | pass + 9 | +10 | if (k) in d and d[k]: + | ^^^^^^^^^^^^^^^^^ RUF019 +11 | pass + | + = help: Replace with `dict.get` + +ℹ Fix +7 7 | if k in d and d[k]: +8 8 | pass +9 9 | +10 |-if (k) in d and d[k]: + 10 |+if d.get(k): +11 11 | pass +12 12 | +13 13 | if k in d and d[(k)]: + +RUF019.py:13:4: RUF019 [*] Unnecessary key check before dictionary access + | +11 | pass +12 | +13 | if k in d and d[(k)]: + | ^^^^^^^^^^^^^^^^^ RUF019 +14 | pass + | + = help: Replace with `dict.get` + +ℹ Fix +10 10 | if (k) in d and d[k]: +11 11 | pass +12 12 | +13 |-if k in d and d[(k)]: + 13 |+if d.get((k)): +14 14 | pass +15 15 | +16 16 | # OK + + diff --git a/ruff.schema.json b/ruff.schema.json index df646ec363..77b716a27d 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3185,6 +3185,7 @@ "RUF016", "RUF017", "RUF018", + "RUF019", "RUF1", "RUF10", "RUF100",