From ffb09c84f298b739bb70ca06792246e11788f096 Mon Sep 17 00:00:00 2001 From: Victor Hugo Gomes Date: Fri, 20 Jun 2025 14:25:36 -0300 Subject: [PATCH] [`flake8-simplify`] Fix false negatives for shadowed bindings (`SIM910`, `SIM911`) (#18794) ## Summary I also noticed that the tests for SIM911 were note being run, so I fixed that. Fixes #18777 ## Test Plan Add regression test --- .../test/fixtures/flake8_simplify/SIM910.py | 6 ++ .../test/fixtures/flake8_simplify/SIM911.py | 6 ++ .../src/rules/flake8_simplify/mod.rs | 1 + .../rules/flake8_simplify/rules/ast_expr.rs | 2 +- .../rules/zip_dict_keys_and_values.rs | 2 +- ...ke8_simplify__tests__SIM910_SIM910.py.snap | 16 ++++ ...ke8_simplify__tests__SIM911_SIM911.py.snap | 77 +++++++++++++++++++ 7 files changed, 108 insertions(+), 2 deletions(-) create mode 100644 crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM911_SIM911.py.snap 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 31c4af016f..550aa2c22c 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM910.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM910.py @@ -49,3 +49,9 @@ def foo(some_other: object): # OK def foo(some_other): a = some_other.get('a', None) + + +# https://github.com/astral-sh/ruff/issues/18777 +def foo(): + dict = {"Tom": 23, "Maria": 23, "Dog": 11} + age = dict.get("Cat", None) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM911.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM911.py index 6a96e6c860..87ba968868 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM911.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM911.py @@ -23,3 +23,9 @@ for k, v in zip(d2.keys(), d2.values()): # SIM911 items = zip(x.keys(), x.values()) # OK items.bar = zip(x.keys(), x.values()) # OK + +# https://github.com/astral-sh/ruff/issues/18777 +def foo(): + dict = {} + for country, stars in zip(dict.keys(), dict.values()): + ... diff --git a/crates/ruff_linter/src/rules/flake8_simplify/mod.rs b/crates/ruff_linter/src/rules/flake8_simplify/mod.rs index 6a9d8c43f9..1fd42b3450 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/mod.rs @@ -48,6 +48,7 @@ mod tests { #[test_case(Rule::IfElseBlockInsteadOfDictGet, Path::new("SIM401.py"))] #[test_case(Rule::SplitStaticString, Path::new("SIM905.py"))] #[test_case(Rule::DictGetWithNoneDefault, Path::new("SIM910.py"))] + #[test_case(Rule::ZipDictKeysAndValues, Path::new("SIM911.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/flake8_simplify/rules/ast_expr.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_expr.rs index d32f2cb1b0..afc2e88b04 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 @@ -280,7 +280,7 @@ pub(crate) fn dict_get_with_none_default(checker: &Checker, expr: &Expr) { Expr::Name(name) => { let Some(binding) = checker .semantic() - .only_binding(name) + .resolve_name(name) .map(|id| checker.semantic().binding(id)) else { return; diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/zip_dict_keys_and_values.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/zip_dict_keys_and_values.rs index 60384439bd..1eff193e3c 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/zip_dict_keys_and_values.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/zip_dict_keys_and_values.rs @@ -92,7 +92,7 @@ pub(crate) fn zip_dict_keys_and_values(checker: &Checker, expr: &ast::ExprCall) let Some(binding) = checker .semantic() - .only_binding(var1) + .resolve_name(var1) .map(|id| checker.semantic().binding(id)) else { return; 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 262e131eec..8bbbdc5fd5 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 @@ -160,3 +160,19 @@ SIM910.py:43:9: SIM910 [*] Use `some_dict.get('a')` instead of `some_dict.get('a 44 44 | 45 45 | # OK 46 46 | def foo(some_other: object): + +SIM910.py:57:11: SIM910 [*] Use `dict.get("Cat")` instead of `dict.get("Cat", None)` + | +55 | def foo(): +56 | dict = {"Tom": 23, "Maria": 23, "Dog": 11} +57 | age = dict.get("Cat", None) + | ^^^^^^^^^^^^^^^^^^^^^ SIM910 + | + = help: Replace `dict.get("Cat", None)` with `dict.get("Cat")` + +ℹ Safe fix +54 54 | # https://github.com/astral-sh/ruff/issues/18777 +55 55 | def foo(): +56 56 | dict = {"Tom": 23, "Maria": 23, "Dog": 11} +57 |- age = dict.get("Cat", None) + 57 |+ age = dict.get("Cat") diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM911_SIM911.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM911_SIM911.py.snap new file mode 100644 index 0000000000..dd92e6fa83 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM911_SIM911.py.snap @@ -0,0 +1,77 @@ +--- +source: crates/ruff_linter/src/rules/flake8_simplify/mod.rs +--- +SIM911.py:2:17: SIM911 [*] Use `d.items()` instead of `zip(d.keys(), d.values())` + | +1 | def foo(d: dict[str, str]) -> None: +2 | for k, v in zip(d.keys(), d.values()): # SIM911 + | ^^^^^^^^^^^^^^^^^^^^^^^^^ SIM911 +3 | ... + | + = help: Replace `zip(d.keys(), d.values())` with `d.items()` + +ℹ Safe fix +1 1 | def foo(d: dict[str, str]) -> None: +2 |- for k, v in zip(d.keys(), d.values()): # SIM911 + 2 |+ for k, v in d.items(): # SIM911 +3 3 | ... +4 4 | +5 5 | for k, v in zip(d.keys(), d.values(), strict=True): # SIM911 + +SIM911.py:5:17: SIM911 [*] Use `d.items()` instead of `zip(d.keys(), d.values(), strict=True)` + | +3 | ... +4 | +5 | for k, v in zip(d.keys(), d.values(), strict=True): # SIM911 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SIM911 +6 | ... + | + = help: Replace `zip(d.keys(), d.values(), strict=True)` with `d.items()` + +ℹ Safe fix +2 2 | for k, v in zip(d.keys(), d.values()): # SIM911 +3 3 | ... +4 4 | +5 |- for k, v in zip(d.keys(), d.values(), strict=True): # SIM911 + 5 |+ for k, v in d.items(): # SIM911 +6 6 | ... +7 7 | +8 8 | for k, v in zip(d.keys(), d.values(), struct=True): # OK + +SIM911.py:20:13: SIM911 [*] Use `d2.items()` instead of `zip(d2.keys(), d2.values())` + | +18 | ... +19 | +20 | for k, v in zip(d2.keys(), d2.values()): # SIM911 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ SIM911 +21 | ... + | + = help: Replace `zip(d2.keys(), d2.values())` with `d2.items()` + +ℹ Safe fix +17 17 | for k, v in zip(d1.items(), d2.values()): # OK +18 18 | ... +19 19 | +20 |-for k, v in zip(d2.keys(), d2.values()): # SIM911 + 20 |+for k, v in d2.items(): # SIM911 +21 21 | ... +22 22 | +23 23 | items = zip(x.keys(), x.values()) # OK + +SIM911.py:30:27: SIM911 [*] Use `dict.items()` instead of `zip(dict.keys(), dict.values())` + | +28 | def foo(): +29 | dict = {} +30 | for country, stars in zip(dict.keys(), dict.values()): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SIM911 +31 | ... + | + = help: Replace `zip(dict.keys(), dict.values())` with `dict.items()` + +ℹ Safe fix +27 27 | # https://github.com/astral-sh/ruff/issues/18777 +28 28 | def foo(): +29 29 | dict = {} +30 |- for country, stars in zip(dict.keys(), dict.values()): + 30 |+ for country, stars in dict.items(): +31 31 | ...