From 64f7c11b1bbeb8378091c825708a8802c40038f0 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 18 Nov 2023 18:50:43 -0500 Subject: [PATCH] Extend dict-get-with-none-default (SIM910) to non-literals --- .../test/fixtures/flake8_simplify/SIM910.py | 8 ++++ .../rules/flake8_simplify/rules/ast_expr.rs | 39 +++++++++++++++---- ...ke8_simplify__tests__SIM910_SIM910.py.snap | 26 +++++++++++++ 3 files changed, 66 insertions(+), 7 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM910.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM910.py index 83b103d7605c53..16b2f8ebef1e9a 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM910.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM910.py @@ -25,3 +25,11 @@ # SIM910 ({}).get(key, None) + +# SIM910 +ages = {"Tom": 23, "Maria": 23, "Dog": 11} +age = ages.get("Cat", None) + +# OK +ages = ["Tom", "Maria", "Dog"] +age = ages.get("Cat", None) diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_expr.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_expr.rs index d9f66572153bf3..4a3b5a7d524abe 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_expr.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_expr.rs @@ -4,6 +4,7 @@ use ruff_text_size::Ranged; use crate::fix::snippet::SourceCodeSnippet; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_semantic::analyze::typing::is_dict; use crate::checkers::ast::Checker; @@ -62,26 +63,32 @@ impl Violation for UncapitalizedEnvironmentVariables { } /// ## What it does -/// Check for `dict.get()` calls that pass `None` as the default value. +/// Checks for `dict.get()` calls that pass `None` as the default value. /// /// ## Why is this bad? /// `None` is the default value for `dict.get()`, so it is redundant to pass it /// explicitly. /// +/// In [preview], this rule applies to variables that are inferred to be +/// dictionaries; in stable, it's limited to dictionary literals (e.g., +/// `{"foo": 1}.get("foo", None)`). +/// /// ## Example /// ```python /// ages = {"Tom": 23, "Maria": 23, "Dog": 11} -/// age = ages.get("Cat", None) # None +/// age = ages.get("Cat", None) /// ``` /// /// Use instead: /// ```python /// ages = {"Tom": 23, "Maria": 23, "Dog": 11} -/// age = ages.get("Cat") # None +/// age = ages.get("Cat") /// ``` /// /// ## References /// - [Python documentation: `dict.get`](https://docs.python.org/3/library/stdtypes.html#dict.get) +/// +/// [preview]: https://docs.astral.sh/ruff/preview/ #[violation] pub struct DictGetWithNoneDefault { expected: SourceCodeSnippet, @@ -244,9 +251,6 @@ pub(crate) fn dict_get_with_none_default(checker: &mut Checker, expr: &Expr) { let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else { return; }; - if !value.is_dict_expr() { - return; - } if attr != "get" { return; } @@ -263,6 +267,28 @@ pub(crate) fn dict_get_with_none_default(checker: &mut Checker, expr: &Expr) { return; } + // Check if the value is a dictionary. + match value.as_ref() { + Expr::Dict(_) | Expr::DictComp(_) => {} + Expr::Name(name) => { + if checker.settings.preview.is_disabled() { + return; + } + + let Some(binding) = checker + .semantic() + .only_binding(name) + .map(|id| checker.semantic().binding(id)) + else { + return; + }; + if !is_dict(binding, checker.semantic()) { + return; + } + } + _ => return, + } + let expected = format!( "{}({})", checker.locator().slice(func.as_ref()), @@ -277,7 +303,6 @@ pub(crate) fn dict_get_with_none_default(checker: &mut Checker, expr: &Expr) { }, expr.range(), ); - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( expected, expr.range(), diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM910_SIM910.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM910_SIM910.py.snap index 8f4d9263f22a7e..a7951aa5e56ce8 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM910_SIM910.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM910_SIM910.py.snap @@ -83,6 +83,8 @@ SIM910.py:27:1: SIM910 [*] Use `({}).get(key)` instead of `({}).get(key, None)` 26 | # SIM910 27 | ({}).get(key, None) | ^^^^^^^^^^^^^^^^^^^ SIM910 +28 | +29 | # SIM910 | = help: Replace `({}).get(key, None)` with `({}).get(key)` @@ -92,5 +94,29 @@ SIM910.py:27:1: SIM910 [*] Use `({}).get(key)` instead of `({}).get(key, None)` 26 26 | # SIM910 27 |-({}).get(key, None) 27 |+({}).get(key) +28 28 | +29 29 | # SIM910 +30 30 | ages = {"Tom": 23, "Maria": 23, "Dog": 11} + +SIM910.py:31:7: SIM910 [*] Use `ages.get("Cat")` instead of `ages.get("Cat", None)` + | +29 | # SIM910 +30 | ages = {"Tom": 23, "Maria": 23, "Dog": 11} +31 | age = ages.get("Cat", None) + | ^^^^^^^^^^^^^^^^^^^^^ SIM910 +32 | +33 | # OK + | + = help: Replace `ages.get("Cat", None)` with `ages.get("Cat")` + +ℹ Safe fix +28 28 | +29 29 | # SIM910 +30 30 | ages = {"Tom": 23, "Maria": 23, "Dog": 11} +31 |-age = ages.get("Cat", None) + 31 |+age = ages.get("Cat") +32 32 | +33 33 | # OK +34 34 | ages = ["Tom", "Maria", "Dog"]