From d0c592567205fe77e372e12c10352a9c55b924be Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 16 Jul 2024 10:49:26 -0400 Subject: [PATCH] Consider expression before statement when determining binding kind (#12346) ## Summary I believe these should always bind more tightly -- e.g., in: ```python for _ in bar(baz for foo in [1]): pass ``` The inner `baz` and `foo` should be considered comprehension variables, not for loop bindings. We need to revisit this more holistically. In some of these cases, `BindingKind` should probably be a flag, not an enum, since the values aren't mutually exclusive. Separately, we should probably be more precise in how we set it (e.g., by passing down from the parent rather than sniffing in `handle_node_store`). Closes https://github.com/astral-sh/ruff/issues/12339 --- .../pylint/redefined_argument_from_local.py | 10 ++ .../checkers/ast/analyze/deferred_scopes.rs | 4 + crates/ruff_linter/src/checkers/ast/mod.rs | 64 ++++----- ...1704_redefined_argument_from_local.py.snap | 130 +++++++++--------- 4 files changed, 111 insertions(+), 97 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/redefined_argument_from_local.py b/crates/ruff_linter/resources/test/fixtures/pylint/redefined_argument_from_local.py index d25413738f..4977629378 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/redefined_argument_from_local.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/redefined_argument_from_local.py @@ -26,6 +26,16 @@ def func(_): ... +def func(foo): + for _ in bar(foo for foo in [1]): + pass + + +def func(foo): + for _ in bar((foo := 1) for foo in [1]): + pass + + # Errors def func(a): for a in range(1): diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index 1a2c1c18ff..003db1d741 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -139,6 +139,10 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { if checker.settings.dummy_variable_rgx.is_match(name) { continue; } + let scope = &checker.semantic.scopes[binding.scope]; + if scope.kind.is_generator() { + continue; + } checker.diagnostics.push(Diagnostic::new( pylint::rules::RedefinedArgumentFromLocal { name: name.to_string(), diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index cb1580b5f7..5b01b0ed14 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -1950,38 +1950,6 @@ impl<'a> Checker<'a> { flags.insert(BindingFlags::UNPACKED_ASSIGNMENT); } - // Match the left-hand side of an annotated assignment without a value, - // like `x` in `x: int`. N.B. In stub files, these should be viewed - // as assignments on par with statements such as `x: int = 5`. - if matches!( - parent, - Stmt::AnnAssign(ast::StmtAnnAssign { value: None, .. }) - ) && !self.semantic.in_annotation() - { - self.add_binding(id, expr.range(), BindingKind::Annotation, flags); - return; - } - - // A binding within a `for` must be a loop variable, as in: - // ```python - // for x in range(10): - // ... - // ``` - if parent.is_for_stmt() { - self.add_binding(id, expr.range(), BindingKind::LoopVar, flags); - return; - } - - // A binding within a `with` must be an item, as in: - // ```python - // with open("file.txt") as fp: - // ... - // ``` - if parent.is_with_stmt() { - self.add_binding(id, expr.range(), BindingKind::WithItemVar, flags); - return; - } - let scope = self.semantic.current_scope(); if scope.kind.is_module() @@ -2051,6 +2019,38 @@ impl<'a> Checker<'a> { return; } + // Match the left-hand side of an annotated assignment without a value, + // like `x` in `x: int`. N.B. In stub files, these should be viewed + // as assignments on par with statements such as `x: int = 5`. + if matches!( + parent, + Stmt::AnnAssign(ast::StmtAnnAssign { value: None, .. }) + ) && !self.semantic.in_annotation() + { + self.add_binding(id, expr.range(), BindingKind::Annotation, flags); + return; + } + + // A binding within a `for` must be a loop variable, as in: + // ```python + // for x in range(10): + // ... + // ``` + if parent.is_for_stmt() { + self.add_binding(id, expr.range(), BindingKind::LoopVar, flags); + return; + } + + // A binding within a `with` must be an item, as in: + // ```python + // with open("file.txt") as fp: + // ... + // ``` + if parent.is_with_stmt() { + self.add_binding(id, expr.range(), BindingKind::WithItemVar, flags); + return; + } + self.add_binding(id, expr.range(), BindingKind::Assignment, flags); } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1704_redefined_argument_from_local.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1704_redefined_argument_from_local.py.snap index ce4a2efaa6..246e255ce8 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1704_redefined_argument_from_local.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1704_redefined_argument_from_local.py.snap @@ -1,113 +1,113 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -redefined_argument_from_local.py:31:9: PLR1704 Redefining argument with the local name `a` +redefined_argument_from_local.py:41:9: PLR1704 Redefining argument with the local name `a` | -29 | # Errors -30 | def func(a): -31 | for a in range(1): +39 | # Errors +40 | def func(a): +41 | for a in range(1): | ^ PLR1704 -32 | ... - | - -redefined_argument_from_local.py:36:9: PLR1704 Redefining argument with the local name `i` - | -35 | def func(i): -36 | for i in range(10): - | ^ PLR1704 -37 | print(i) - | - -redefined_argument_from_local.py:43:25: PLR1704 Redefining argument with the local name `e` - | -41 | try: 42 | ... -43 | except Exception as e: + | + +redefined_argument_from_local.py:46:9: PLR1704 Redefining argument with the local name `i` + | +45 | def func(i): +46 | for i in range(10): + | ^ PLR1704 +47 | print(i) + | + +redefined_argument_from_local.py:53:25: PLR1704 Redefining argument with the local name `e` + | +51 | try: +52 | ... +53 | except Exception as e: | ^ PLR1704 -44 | print(e) +54 | print(e) | -redefined_argument_from_local.py:48:24: PLR1704 Redefining argument with the local name `f` +redefined_argument_from_local.py:58:24: PLR1704 Redefining argument with the local name `f` | -47 | def func(f): -48 | with open('', ) as f: +57 | def func(f): +58 | with open('', ) as f: | ^ PLR1704 -49 | print(f) +59 | print(f) | -redefined_argument_from_local.py:53:24: PLR1704 Redefining argument with the local name `a` +redefined_argument_from_local.py:63:24: PLR1704 Redefining argument with the local name `a` | -52 | def func(a, b): -53 | with context() as (a, b, c): +62 | def func(a, b): +63 | with context() as (a, b, c): | ^ PLR1704 -54 | print(a, b, c) +64 | print(a, b, c) | -redefined_argument_from_local.py:53:27: PLR1704 Redefining argument with the local name `b` +redefined_argument_from_local.py:63:27: PLR1704 Redefining argument with the local name `b` | -52 | def func(a, b): -53 | with context() as (a, b, c): +62 | def func(a, b): +63 | with context() as (a, b, c): | ^ PLR1704 -54 | print(a, b, c) +64 | print(a, b, c) | -redefined_argument_from_local.py:58:24: PLR1704 Redefining argument with the local name `a` +redefined_argument_from_local.py:68:24: PLR1704 Redefining argument with the local name `a` | -57 | def func(a, b): -58 | with context() as [a, b, c]: +67 | def func(a, b): +68 | with context() as [a, b, c]: | ^ PLR1704 -59 | print(a, b, c) +69 | print(a, b, c) | -redefined_argument_from_local.py:58:27: PLR1704 Redefining argument with the local name `b` +redefined_argument_from_local.py:68:27: PLR1704 Redefining argument with the local name `b` | -57 | def func(a, b): -58 | with context() as [a, b, c]: +67 | def func(a, b): +68 | with context() as [a, b, c]: | ^ PLR1704 -59 | print(a, b, c) +69 | print(a, b, c) | -redefined_argument_from_local.py:63:51: PLR1704 Redefining argument with the local name `a` +redefined_argument_from_local.py:73:51: PLR1704 Redefining argument with the local name `a` | -62 | def func(a): -63 | with open('foo.py', ) as f, open('bar.py') as a: +72 | def func(a): +73 | with open('foo.py', ) as f, open('bar.py') as a: | ^ PLR1704 -64 | ... +74 | ... | -redefined_argument_from_local.py:69:13: PLR1704 Redefining argument with the local name `a` +redefined_argument_from_local.py:79:13: PLR1704 Redefining argument with the local name `a` | -67 | def func(a): -68 | def bar(b): -69 | for a in range(1): +77 | def func(a): +78 | def bar(b): +79 | for a in range(1): | ^ PLR1704 -70 | print(a) +80 | print(a) | -redefined_argument_from_local.py:75:13: PLR1704 Redefining argument with the local name `b` +redefined_argument_from_local.py:85:13: PLR1704 Redefining argument with the local name `b` | -73 | def func(a): -74 | def bar(b): -75 | for b in range(1): +83 | def func(a): +84 | def bar(b): +85 | for b in range(1): | ^ PLR1704 -76 | print(b) +86 | print(b) | -redefined_argument_from_local.py:81:13: PLR1704 Redefining argument with the local name `a` +redefined_argument_from_local.py:91:13: PLR1704 Redefining argument with the local name `a` | -79 | def func(a=1): -80 | def bar(b=2): -81 | for a in range(1): +89 | def func(a=1): +90 | def bar(b=2): +91 | for a in range(1): | ^ PLR1704 -82 | print(a) -83 | for b in range(1): +92 | print(a) +93 | for b in range(1): | -redefined_argument_from_local.py:83:13: PLR1704 Redefining argument with the local name `b` +redefined_argument_from_local.py:93:13: PLR1704 Redefining argument with the local name `b` | -81 | for a in range(1): -82 | print(a) -83 | for b in range(1): +91 | for a in range(1): +92 | print(a) +93 | for b in range(1): | ^ PLR1704 -84 | print(b) +94 | print(b) |