From 6e1e8ea32d978c94e16de6348bae660b00cd7441 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 22 Aug 2023 09:50:00 +0530 Subject: [PATCH 1/5] Avoid `C417` autofix for `lambda` with default parameters --- .../rules/unnecessary_map.rs | 60 +++++++++++++------ 1 file changed, 42 insertions(+), 18 deletions(-) diff --git a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs index 8675682925fb7..f58b388648e72 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs +++ b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs @@ -84,7 +84,7 @@ pub(crate) fn unnecessary_map( return; } - match object_type { + let has_default_parameters = match object_type { ObjectType::Generator => { // Exclude the parent if already matched by other arms. if parent @@ -103,11 +103,19 @@ pub(crate) fn unnecessary_map( return; }; - if parameters - .as_ref() - .is_some_and(|parameters| late_binding(parameters, body)) - { - return; + if let Some(parameters) = parameters.as_ref() { + if late_binding(parameters, body) { + return; + } + + parameters + .posonlyargs + .iter() + .chain(¶meters.args) + .chain(¶meters.kwonlyargs) + .any(|param| param.default.is_some()) + } else { + false } } ObjectType::List | ObjectType::Set => { @@ -137,11 +145,19 @@ pub(crate) fn unnecessary_map( return; }; - if parameters - .as_ref() - .is_some_and(|parameters| late_binding(parameters, body)) - { - return; + if let Some(parameters) = parameters.as_ref() { + if late_binding(parameters, body) { + return; + } + + parameters + .posonlyargs + .iter() + .chain(¶meters.args) + .chain(¶meters.kwonlyargs) + .any(|param| param.default.is_some()) + } else { + false } } ObjectType::Dict => { @@ -177,17 +193,25 @@ pub(crate) fn unnecessary_map( return; } - if parameters - .as_ref() - .is_some_and(|parameters| late_binding(parameters, body)) - { - return; + if let Some(parameters) = parameters.as_ref() { + if late_binding(parameters, body) { + return; + } + + parameters + .posonlyargs + .iter() + .chain(¶meters.args) + .chain(¶meters.kwonlyargs) + .any(|param| param.default.is_some()) + } else { + false } } - } + }; let mut diagnostic = Diagnostic::new(UnnecessaryMap { object_type }, expr.range()); - if checker.patch(diagnostic.kind.rule()) { + if checker.patch(diagnostic.kind.rule()) && !has_default_parameters { diagnostic.try_set_fix(|| { fixes::fix_unnecessary_map( checker.locator(), From 88408955b818138a4242104376274c2d36443a3d Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 22 Aug 2023 22:34:00 +0530 Subject: [PATCH 2/5] Avoid autofix if `*args`/`**kwargs` is present --- .../fixtures/flake8_comprehensions/C417.py | 4 ++++ .../rules/unnecessary_map.rs | 6 +++++ ...8_comprehensions__tests__C417_C417.py.snap | 23 +++++++++++++++++++ 3 files changed, 33 insertions(+) diff --git a/crates/ruff/resources/test/fixtures/flake8_comprehensions/C417.py b/crates/ruff/resources/test/fixtures/flake8_comprehensions/C417.py index 7077acfad6bc5..829ccf6906ab0 100644 --- a/crates/ruff/resources/test/fixtures/flake8_comprehensions/C417.py +++ b/crates/ruff/resources/test/fixtures/flake8_comprehensions/C417.py @@ -37,3 +37,7 @@ def func(arg1: int, arg2: int = 4): # Error: the `x` is overridden by the inner lambda. map(lambda x: lambda x: x, range(4)) + +# Error, but unfixable +map(lambda *args: len(args), range(4)) +map(lambda **kwargs: len(kwargs), range(4)) diff --git a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs index f58b388648e72..7f572978b91ae 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs +++ b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs @@ -114,6 +114,8 @@ pub(crate) fn unnecessary_map( .chain(¶meters.args) .chain(¶meters.kwonlyargs) .any(|param| param.default.is_some()) + || parameters.vararg.is_some() + || parameters.kwarg.is_some() } else { false } @@ -156,6 +158,8 @@ pub(crate) fn unnecessary_map( .chain(¶meters.args) .chain(¶meters.kwonlyargs) .any(|param| param.default.is_some()) + || parameters.vararg.is_some() + || parameters.kwarg.is_some() } else { false } @@ -204,6 +208,8 @@ pub(crate) fn unnecessary_map( .chain(¶meters.args) .chain(¶meters.kwonlyargs) .any(|param| param.default.is_some()) + || parameters.vararg.is_some() + || parameters.kwarg.is_some() } else { false } diff --git a/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap b/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap index acb6700d3fbed..eaa07ae25754b 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap +++ b/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap @@ -265,6 +265,8 @@ C417.py:39:1: C417 [*] Unnecessary `map` usage (rewrite using a generator expres 38 | # Error: the `x` is overridden by the inner lambda. 39 | map(lambda x: lambda x: x, range(4)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C417 +40 | +41 | # Error, but unfixable | = help: Replace `map` with a generator expression @@ -274,5 +276,26 @@ C417.py:39:1: C417 [*] Unnecessary `map` usage (rewrite using a generator expres 38 38 | # Error: the `x` is overridden by the inner lambda. 39 |-map(lambda x: lambda x: x, range(4)) 39 |+(lambda x: x for x in range(4)) +40 40 | +41 41 | # Error, but unfixable +42 42 | map(lambda *args: len(args), range(4)) + +C417.py:42:1: C417 Unnecessary `map` usage (rewrite using a generator expression) + | +41 | # Error, but unfixable +42 | map(lambda *args: len(args), range(4)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C417 +43 | map(lambda **kwargs: len(kwargs), range(4)) + | + = help: Replace `map` with a generator expression + +C417.py:43:1: C417 Unnecessary `map` usage (rewrite using a generator expression) + | +41 | # Error, but unfixable +42 | map(lambda *args: len(args), range(4)) +43 | map(lambda **kwargs: len(kwargs), range(4)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C417 + | + = help: Replace `map` with a generator expression From 45f538cfad8ef9911f185192a5e9ee13bee010fb Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 22 Aug 2023 23:12:21 +0530 Subject: [PATCH 3/5] Avoid flagging instead --- .../fixtures/flake8_comprehensions/C417.py | 8 ++--- .../rules/unnecessary_map.rs | 25 +++++++++------- ...8_comprehensions__tests__C417_C417.py.snap | 29 ------------------- 3 files changed, 16 insertions(+), 46 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_comprehensions/C417.py b/crates/ruff/resources/test/fixtures/flake8_comprehensions/C417.py index 829ccf6906ab0..a19c606a58986 100644 --- a/crates/ruff/resources/test/fixtures/flake8_comprehensions/C417.py +++ b/crates/ruff/resources/test/fixtures/flake8_comprehensions/C417.py @@ -15,11 +15,6 @@ _ = f"{set(map(lambda x: x % 2 == 0, nums))}" _ = f"{dict(map(lambda v: (v, v**2), nums))}" -# Error, but unfixable. -# For simple expressions, this could be: `(x if x else 1 for x in nums)`. -# For more complex expressions, this would differ: `(x + 2 if x else 3 for x in nums)`. -map(lambda x=1: x, nums) - # False negatives. map(lambda x=2, y=1: x + y, nums, nums) set(map(lambda x, y: x, nums, nums)) @@ -38,6 +33,7 @@ def func(arg1: int, arg2: int = 4): # Error: the `x` is overridden by the inner lambda. map(lambda x: lambda x: x, range(4)) -# Error, but unfixable +# Ok because of the default parameters, and variadic arguments. +map(lambda x=1: x, nums) map(lambda *args: len(args), range(4)) map(lambda **kwargs: len(kwargs), range(4)) diff --git a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs index 7f572978b91ae..cb02a80bd93d1 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs +++ b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs @@ -84,7 +84,7 @@ pub(crate) fn unnecessary_map( return; } - let has_default_parameters = match object_type { + match object_type { ObjectType::Generator => { // Exclude the parent if already matched by other arms. if parent @@ -108,7 +108,7 @@ pub(crate) fn unnecessary_map( return; } - parameters + if parameters .posonlyargs .iter() .chain(¶meters.args) @@ -116,8 +116,9 @@ pub(crate) fn unnecessary_map( .any(|param| param.default.is_some()) || parameters.vararg.is_some() || parameters.kwarg.is_some() - } else { - false + { + return; + } } } ObjectType::List | ObjectType::Set => { @@ -152,7 +153,7 @@ pub(crate) fn unnecessary_map( return; } - parameters + if parameters .posonlyargs .iter() .chain(¶meters.args) @@ -160,8 +161,9 @@ pub(crate) fn unnecessary_map( .any(|param| param.default.is_some()) || parameters.vararg.is_some() || parameters.kwarg.is_some() - } else { - false + { + return; + } } } ObjectType::Dict => { @@ -202,7 +204,7 @@ pub(crate) fn unnecessary_map( return; } - parameters + if parameters .posonlyargs .iter() .chain(¶meters.args) @@ -210,14 +212,15 @@ pub(crate) fn unnecessary_map( .any(|param| param.default.is_some()) || parameters.vararg.is_some() || parameters.kwarg.is_some() - } else { - false + { + return; + } } } }; let mut diagnostic = Diagnostic::new(UnnecessaryMap { object_type }, expr.range()); - if checker.patch(diagnostic.kind.rule()) && !has_default_parameters { + if checker.patch(diagnostic.kind.rule()) { diagnostic.try_set_fix(|| { fixes::fix_unnecessary_map( checker.locator(), diff --git a/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap b/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap index eaa07ae25754b..0efb23ad6c4e1 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap +++ b/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap @@ -249,17 +249,6 @@ C417.py:16:8: C417 [*] Unnecessary `map` usage (rewrite using a `dict` comprehen 18 18 | # Error, but unfixable. 19 19 | # For simple expressions, this could be: `(x if x else 1 for x in nums)`. -C417.py:21:1: C417 Unnecessary `map` usage (rewrite using a generator expression) - | -19 | # For simple expressions, this could be: `(x if x else 1 for x in nums)`. -20 | # For more complex expressions, this would differ: `(x + 2 if x else 3 for x in nums)`. -21 | map(lambda x=1: x, nums) - | ^^^^^^^^^^^^^^^^^^^^^^^^ C417 -22 | -23 | # False negatives. - | - = help: Replace `map` with a generator expression - C417.py:39:1: C417 [*] Unnecessary `map` usage (rewrite using a generator expression) | 38 | # Error: the `x` is overridden by the inner lambda. @@ -280,22 +269,4 @@ C417.py:39:1: C417 [*] Unnecessary `map` usage (rewrite using a generator expres 41 41 | # Error, but unfixable 42 42 | map(lambda *args: len(args), range(4)) -C417.py:42:1: C417 Unnecessary `map` usage (rewrite using a generator expression) - | -41 | # Error, but unfixable -42 | map(lambda *args: len(args), range(4)) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C417 -43 | map(lambda **kwargs: len(kwargs), range(4)) - | - = help: Replace `map` with a generator expression - -C417.py:43:1: C417 Unnecessary `map` usage (rewrite using a generator expression) - | -41 | # Error, but unfixable -42 | map(lambda *args: len(args), range(4)) -43 | map(lambda **kwargs: len(kwargs), range(4)) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C417 - | - = help: Replace `map` with a generator expression - From ee94fb8b2340392021bdfa9cf5b662767bc69bd8 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 22 Aug 2023 23:29:27 +0530 Subject: [PATCH 4/5] Update snapshots --- ...8_comprehensions__tests__C417_C417.py.snap | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap b/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap index 0efb23ad6c4e1..c04bcb03b9fd8 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap +++ b/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap @@ -226,7 +226,7 @@ C417.py:15:8: C417 [*] Unnecessary `map` usage (rewrite using a `set` comprehens 15 |+_ = f"{ {x % 2 == 0 for x in nums} }" 16 16 | _ = f"{dict(map(lambda v: (v, v**2), nums))}" 17 17 | -18 18 | # Error, but unfixable. +18 18 | # False negatives. C417.py:16:8: C417 [*] Unnecessary `map` usage (rewrite using a `dict` comprehension) | @@ -235,7 +235,7 @@ C417.py:16:8: C417 [*] Unnecessary `map` usage (rewrite using a `dict` comprehen 16 | _ = f"{dict(map(lambda v: (v, v**2), nums))}" | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C417 17 | -18 | # Error, but unfixable. +18 | # False negatives. | = help: Replace `map` with a `dict` comprehension @@ -246,27 +246,27 @@ C417.py:16:8: C417 [*] Unnecessary `map` usage (rewrite using a `dict` comprehen 16 |-_ = f"{dict(map(lambda v: (v, v**2), nums))}" 16 |+_ = f"{ {v: v**2 for v in nums} }" 17 17 | -18 18 | # Error, but unfixable. -19 19 | # For simple expressions, this could be: `(x if x else 1 for x in nums)`. +18 18 | # False negatives. +19 19 | map(lambda x=2, y=1: x + y, nums, nums) -C417.py:39:1: C417 [*] Unnecessary `map` usage (rewrite using a generator expression) +C417.py:34:1: C417 [*] Unnecessary `map` usage (rewrite using a generator expression) | -38 | # Error: the `x` is overridden by the inner lambda. -39 | map(lambda x: lambda x: x, range(4)) +33 | # Error: the `x` is overridden by the inner lambda. +34 | map(lambda x: lambda x: x, range(4)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C417 -40 | -41 | # Error, but unfixable +35 | +36 | # Ok because of the default parameters, and variadic arguments. | = help: Replace `map` with a generator expression ℹ Suggested fix -36 36 | map(lambda x: lambda: x, range(4)) -37 37 | -38 38 | # Error: the `x` is overridden by the inner lambda. -39 |-map(lambda x: lambda x: x, range(4)) - 39 |+(lambda x: x for x in range(4)) -40 40 | -41 41 | # Error, but unfixable -42 42 | map(lambda *args: len(args), range(4)) +31 31 | map(lambda x: lambda: x, range(4)) +32 32 | +33 33 | # Error: the `x` is overridden by the inner lambda. +34 |-map(lambda x: lambda x: x, range(4)) + 34 |+(lambda x: x for x in range(4)) +35 35 | +36 36 | # Ok because of the default parameters, and variadic arguments. +37 37 | map(lambda x=1: x, nums) From 2045f7505d054be9c83c0b517bc1be7ab5b39bb2 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 22 Aug 2023 23:59:42 +0530 Subject: [PATCH 5/5] Move back to `is_some_and` pattern --- .../rules/unnecessary_map.rs | 72 ++++++++----------- 1 file changed, 30 insertions(+), 42 deletions(-) diff --git a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs index cb02a80bd93d1..11c0d1a6094b7 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs +++ b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs @@ -103,22 +103,18 @@ pub(crate) fn unnecessary_map( return; }; - if let Some(parameters) = parameters.as_ref() { - if late_binding(parameters, body) { - return; - } - - if parameters - .posonlyargs - .iter() - .chain(¶meters.args) - .chain(¶meters.kwonlyargs) - .any(|param| param.default.is_some()) + if parameters.as_ref().is_some_and(|parameters| { + late_binding(parameters, body) + || parameters + .posonlyargs + .iter() + .chain(¶meters.args) + .chain(¶meters.kwonlyargs) + .any(|param| param.default.is_some()) || parameters.vararg.is_some() || parameters.kwarg.is_some() - { - return; - } + }) { + return; } } ObjectType::List | ObjectType::Set => { @@ -148,22 +144,18 @@ pub(crate) fn unnecessary_map( return; }; - if let Some(parameters) = parameters.as_ref() { - if late_binding(parameters, body) { - return; - } - - if parameters - .posonlyargs - .iter() - .chain(¶meters.args) - .chain(¶meters.kwonlyargs) - .any(|param| param.default.is_some()) + if parameters.as_ref().is_some_and(|parameters| { + late_binding(parameters, body) + || parameters + .posonlyargs + .iter() + .chain(¶meters.args) + .chain(¶meters.kwonlyargs) + .any(|param| param.default.is_some()) || parameters.vararg.is_some() || parameters.kwarg.is_some() - { - return; - } + }) { + return; } } ObjectType::Dict => { @@ -199,22 +191,18 @@ pub(crate) fn unnecessary_map( return; } - if let Some(parameters) = parameters.as_ref() { - if late_binding(parameters, body) { - return; - } - - if parameters - .posonlyargs - .iter() - .chain(¶meters.args) - .chain(¶meters.kwonlyargs) - .any(|param| param.default.is_some()) + if parameters.as_ref().is_some_and(|parameters| { + late_binding(parameters, body) + || parameters + .posonlyargs + .iter() + .chain(¶meters.args) + .chain(¶meters.kwonlyargs) + .any(|param| param.default.is_some()) || parameters.vararg.is_some() || parameters.kwarg.is_some() - { - return; - } + }) { + return; } } };