[flake8-comprehensions] Detect overshadowed list/set/dict, ig…

…nore variadics and named expressions (`C417`) (#15955) ## Summary Part of #15809 and #15876. This change brings several bugfixes: * The nested `map()` call in `list(map(lambda x: x, []))` where `list` is overshadowed is now correctly reported. * The call will no longer reported if: * Any arguments given to `map()` are variadic. * Any of the iterables contain a named expression. ## Test Plan `cargo nextest run` and `cargo insta test`. --------- Co-authored-by: Micha Reiser <[email protected]>
astral-sh · Feb 7, 2025 · 7db5a92 · 7db5a92
1 parent 349f933
commit 7db5a92
Show file tree

Hide file tree

Showing 6 changed files with 155 additions and 126 deletions.
diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C417_1.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C417_1.py
@@ -0,0 +1,28 @@
+##### https://github.com/astral-sh/ruff/issues/15809
+
+### Errors
+
+def overshadowed_list():
+    list = ...
+    list(map(lambda x: x, []))
+
+
+### No errors
+
+dict(map(lambda k: (k,), a))
+dict(map(lambda k: (k, v, 0), a))
+dict(map(lambda k: [k], a))
+dict(map(lambda k: [k, v, 0], a))
+dict(map(lambda k: {k, v}, a))
+dict(map(lambda k: {k: 0, v: 1}, a))
+
+a = [(1, 2), (3, 4)]
+map(lambda x: [*x, 10], *a)
+map(lambda x: [*x, 10], *a, *b)
+map(lambda x: [*x, 10], a, *b)
+
+
+map(lambda x: x + 10, (a := []))
+list(map(lambda x: x + 10, (a := [])))
+set(map(lambda x: x + 10, (a := [])))
+dict(map(lambda x: (x, 10), (a := [])))
diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs
@@ -842,13 +842,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
                 flake8_comprehensions::rules::unnecessary_subscript_reversal(checker, call);
             }
             if checker.enabled(Rule::UnnecessaryMap) {
-                flake8_comprehensions::rules::unnecessary_map(
-                    checker,
-                    expr,
-                    checker.semantic.current_expression_parent(),
-                    func,
-                    args,
-                );
+                flake8_comprehensions::rules::unnecessary_map(checker, call);
             }
             if checker.enabled(Rule::UnnecessaryComprehensionInCall) {
                 flake8_comprehensions::rules::unnecessary_comprehension_in_call(

diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs
@@ -11,7 +11,7 @@ use libcst_native::{
 };
 
 use ruff_diagnostics::{Edit, Fix};
-use ruff_python_ast::{self as ast, Expr};
+use ruff_python_ast::{self as ast, Expr, ExprCall};
 use ruff_python_codegen::Stylist;
 use ruff_python_semantic::SemanticModel;
 use ruff_text_size::{Ranged, TextRange};
@@ -649,13 +649,13 @@ pub(crate) fn fix_unnecessary_comprehension(
 
 /// (C417) Convert `map(lambda x: x * 2, bar)` to `(x * 2 for x in bar)`.
 pub(crate) fn fix_unnecessary_map(
-    expr: &Expr,
+    call_ast_node: &ExprCall,
     parent: Option<&Expr>,
     object_type: ObjectType,
     locator: &Locator,
     stylist: &Stylist,
 ) -> Result<Edit> {
-    let module_text = locator.slice(expr);
+    let module_text = locator.slice(call_ast_node);
     let mut tree = match_expression(module_text)?;
     let call = match_call_mut(&mut tree)?;
 
@@ -800,7 +800,7 @@ pub(crate) fn fix_unnecessary_map(
         }
     }
 
-    Ok(Edit::range_replacement(content, expr.range()))
+    Ok(Edit::range_replacement(content, call_ast_node.range()))
 }
 
 /// (C419) Convert `[i for i in a]` into `i for i in a`

diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs
@@ -36,6 +36,7 @@ mod tests {
     #[test_case(Rule::UnnecessaryLiteralWithinListCall, Path::new("C410.py"))]
     #[test_case(Rule::UnnecessaryLiteralWithinTupleCall, Path::new("C409.py"))]
     #[test_case(Rule::UnnecessaryMap, Path::new("C417.py"))]
+    #[test_case(Rule::UnnecessaryMap, Path::new("C417_1.py"))]
     #[test_case(Rule::UnnecessarySubscriptReversal, Path::new("C415.py"))]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());

diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_map.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_map.rs
@@ -4,17 +4,14 @@ use ruff_diagnostics::{Diagnostic, Fix};
 use ruff_diagnostics::{FixAvailability, Violation};
 use ruff_macros::{derive_message_formats, ViolationMetadata};
 use ruff_python_ast::helpers::any_over_expr;
-use ruff_python_ast::visitor;
 use ruff_python_ast::visitor::Visitor;
-use ruff_python_ast::{self as ast, Arguments, Expr, ExprContext, Parameters, Stmt};
-use ruff_text_size::Ranged;
+use ruff_python_ast::{self as ast, Expr, ExprContext, Parameters, Stmt};
+use ruff_python_ast::{visitor, ExprLambda};
+use ruff_python_semantic::SemanticModel;
 
 use crate::checkers::ast::Checker;
-
 use crate::rules::flake8_comprehensions::fixes;
 
-use super::helpers;
-
 /// ## What it does
 /// Checks for unnecessary `map()` calls with lambda functions.
 ///
@@ -67,140 +64,84 @@ impl Violation for UnnecessaryMap {
 }
 
 /// C417
-pub(crate) fn unnecessary_map(
-    checker: &mut Checker,
-    expr: &Expr,
-    parent: Option<&Expr>,
-    func: &Expr,
-    args: &[Expr],
-) {
-    let Some(builtin_name) = checker.semantic().resolve_builtin_symbol(func) else {
+pub(crate) fn unnecessary_map(checker: &mut Checker, call: &ast::ExprCall) {
+    let semantic = checker.semantic();
+    let (func, arguments) = (&call.func, &call.arguments);
+
+    if !arguments.keywords.is_empty() {
         return;
-    };
+    }
 
-    let object_type = match builtin_name {
-        "map" => ObjectType::Generator,
-        "list" => ObjectType::List,
-        "set" => ObjectType::Set,
-        "dict" => ObjectType::Dict,
-        _ => return,
+    let Some(object_type) = ObjectType::from(func, semantic) else {
+        return;
     };
 
-    match object_type {
-        ObjectType::Generator => {
-            // Exclude the parent if already matched by other arms.
-            if parent
-                .and_then(Expr::as_call_expr)
-                .and_then(|call| call.func.as_name_expr())
-                .is_some_and(|name| matches!(name.id.as_str(), "list" | "set" | "dict"))
-            {
-                return;
-            }
-
-            // Only flag, e.g., `map(lambda x: x + 1, iterable)`.
-            let [Expr::Lambda(ast::ExprLambda {
-                parameters, body, ..
-            }), iterable] = args
-            else {
-                return;
-            };
+    let parent = semantic.current_expression_parent();
 
-            // For example, (x+1 for x in (c:=a)) is invalid syntax
-            // so we can't suggest it.
-            if any_over_expr(iterable, &|expr| expr.is_named_expr()) {
-                return;
-            }
-
-            if !lambda_has_expected_arity(parameters.as_deref(), body) {
-                return;
-            }
-        }
-        ObjectType::List | ObjectType::Set => {
-            // Only flag, e.g., `list(map(lambda x: x + 1, iterable))`.
-            let [Expr::Call(ast::ExprCall {
-                func,
-                arguments: Arguments { args, keywords, .. },
-                ..
-            })] = args
-            else {
-                return;
+    let (lambda, iterables) = match object_type {
+        ObjectType::Generator => {
+            let parent_call_func = match parent {
+                Some(Expr::Call(call)) => Some(&call.func),
+                _ => None,
             };
 
-            if args.len() != 2 {
-                return;
-            }
-
-            if !keywords.is_empty() {
+            // Exclude the parent if already matched by other arms.
+            if parent_call_func.is_some_and(|func| is_list_set_or_dict(func, semantic)) {
                 return;
             }
 
-            let Some(argument) = helpers::first_argument_with_matching_function("map", func, args)
-            else {
+            let Some(result) = map_lambda_and_iterables(call, semantic) else {
                 return;
             };
 
-            let Expr::Lambda(ast::ExprLambda {
-                parameters, body, ..
-            }) = argument
-            else {
-                return;
-            };
-
-            if !lambda_has_expected_arity(parameters.as_deref(), body) {
-                return;
-            }
+            result
         }
-        ObjectType::Dict => {
-            // Only flag, e.g., `dict(map(lambda v: (v, v ** 2), values))`.
-            let [Expr::Call(ast::ExprCall {
-                func,
-                arguments: Arguments { args, keywords, .. },
-                ..
-            })] = args
-            else {
-                return;
-            };
 
-            if args.len() != 2 {
-                return;
-            }
-
-            if !keywords.is_empty() {
-                return;
-            }
-
-            let Some(argument) = helpers::first_argument_with_matching_function("map", func, args)
-            else {
+        ObjectType::List | ObjectType::Set | ObjectType::Dict => {
+            let [Expr::Call(inner_call)] = arguments.args.as_ref() else {
                 return;
             };
 
-            let Expr::Lambda(ast::ExprLambda {
-                parameters, body, ..
-            }) = argument
-            else {
+            let Some((lambda, iterables)) = map_lambda_and_iterables(inner_call, semantic) else {
                 return;
             };
 
-            let (Expr::Tuple(ast::ExprTuple { elts, .. }) | Expr::List(ast::ExprList { elts, .. })) =
-                body.as_ref()
-            else {
-                return;
-            };
+            if object_type == ObjectType::Dict {
+                let (Expr::Tuple(ast::ExprTuple { elts, .. })
+                | Expr::List(ast::ExprList { elts, .. })) = &*lambda.body
+                else {
+                    return;
+                };
 
-            if elts.len() != 2 {
-                return;
+                if elts.len() != 2 {
+                    return;
+                }
             }
 
-            if !lambda_has_expected_arity(parameters.as_deref(), body) {
-                return;
-            }
+            (lambda, iterables)
         }
     };
 
-    let mut diagnostic = Diagnostic::new(UnnecessaryMap { object_type }, expr.range());
+    for iterable in iterables {
+        // For example, (x+1 for x in (c:=a)) is invalid syntax
+        // so we can't suggest it.
+        if any_over_expr(iterable, &|expr| expr.is_named_expr()) {
+            return;
+        }
+
+        if iterable.is_starred_expr() {
+            return;
+        }
+    }
+
+    if !lambda_has_expected_arity(lambda) {
+        return;
+    }
+
+    let mut diagnostic = Diagnostic::new(UnnecessaryMap { object_type }, call.range);
     diagnostic.try_set_fix(|| {
         fixes::fix_unnecessary_map(
-            expr,
+            call,
             parent,
             object_type,
             checker.locator(),
@@ -211,17 +152,49 @@ pub(crate) fn unnecessary_map(
     checker.diagnostics.push(diagnostic);
 }
 
+fn is_list_set_or_dict(func: &Expr, semantic: &SemanticModel) -> bool {
+    semantic
+        .resolve_qualified_name(func)
+        .is_some_and(|qualified_name| {
+            matches!(
+                qualified_name.segments(),
+                ["" | "builtins", "list" | "set" | "dict"]
+            )
+        })
+}
+
+fn map_lambda_and_iterables<'a>(
+    call: &'a ast::ExprCall,
+    semantic: &'a SemanticModel,
+) -> Option<(&'a ExprLambda, &'a [Expr])> {
+    if !semantic.match_builtin_expr(&call.func, "map") {
+        return None;
+    }
+
+    let arguments = &call.arguments;
+
+    if !arguments.keywords.is_empty() {
+        return None;
+    }
+
+    let Some((Expr::Lambda(lambda), iterables)) = arguments.args.split_first() else {
+        return None;
+    };
+
+    Some((lambda, iterables))
+}
+
 /// A lambda as the first argument to `map()` has the "expected" arity when:
 ///
 /// * It has exactly one parameter
 /// * That parameter is not variadic
 /// * That parameter does not have a default value
-fn lambda_has_expected_arity(parameters: Option<&Parameters>, body: &Expr) -> bool {
-    let Some(parameters) = parameters else {
+fn lambda_has_expected_arity(lambda: &ExprLambda) -> bool {
+    let Some(parameters) = lambda.parameters.as_deref() else {
         return false;
     };
 
-    let [parameter] = &parameters.args[..] else {
+    let [parameter] = &*parameters.args else {
         return false;
     };
 
@@ -233,7 +206,7 @@ fn lambda_has_expected_arity(parameters: Option<&Parameters>, body: &Expr) -> bo
         return false;
     }
 
-    if late_binding(parameters, body) {
+    if late_binding(parameters, &lambda.body) {
         return false;
     }
 
@@ -248,6 +221,18 @@ pub(crate) enum ObjectType {
     Dict,
 }
 
+impl ObjectType {
+    fn from(func: &Expr, semantic: &SemanticModel) -> Option<Self> {
+        match semantic.resolve_builtin_symbol(func) {
+            Some("map") => Some(Self::Generator),
+            Some("list") => Some(Self::List),
+            Some("set") => Some(Self::Set),
+            Some("dict") => Some(Self::Dict),
+            _ => None,
+        }
+    }
+}
+
 impl fmt::Display for ObjectType {
     fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
         match self {

diff --git a/...ehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C417_C417_1.py.snap b/...ehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C417_C417_1.py.snap
@@ -0,0 +1,21 @@
+---
+source: crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs
+---
+C417_1.py:7:10: C417 [*] Unnecessary `map()` usage (rewrite using a generator expression)
+  |
+5 | def overshadowed_list():
+6 |     list = ...
+7 |     list(map(lambda x: x, []))
+  |          ^^^^^^^^^^^^^^^^^^^^ C417
+  |
+  = help: Replace `map()` with a generator expression
+
+ℹ Unsafe fix
+4 4 | 
+5 5 | def overshadowed_list():
+6 6 |     list = ...
+7   |-    list(map(lambda x: x, []))
+  7 |+    list((x for x in []))
+8 8 | 
+9 9 | 
+10 10 | ### No errors