From da9c6e5309a51bc0d34d8123597de5df427e02b5 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 18 Dec 2024 15:26:33 -0600 Subject: [PATCH 1/5] update fixture --- .../resources/test/fixtures/perflint/PERF401.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index ad92de80ad094..24ff17e6c7e41 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -251,3 +251,12 @@ def f(): for i in values: result.append(i + 1) # Ok del i + +# The fix here must parenthesize the walrus operator +# https://github.com/astral-sh/ruff/issues/15047 +def f(): + items = [] + + for i in range(5): + if j := i: + items.append(j) From 2acc0ef8dc7e571c8b5f2e5fd37db0a966027344 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 18 Dec 2024 15:28:09 -0600 Subject: [PATCH 2/5] remind users to use parens in documentation --- .../src/rules/perflint/rules/manual_list_comprehension.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index e7f57944fa7c9..8e58159bc4f01 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -46,6 +46,11 @@ use ruff_text_size::{Ranged, TextRange}; /// original = list(range(10000)) /// filtered.extend(x for x in original if x % 2) /// ``` +/// +/// Take care that if the original for-loop uses an assignment expression +/// as a conditional, such as `if match:=re.match("\d+")`, then +/// the corresponding comprehension must wrap the assignment +/// expression in parentheses to avoid a syntax error. #[derive(ViolationMetadata)] pub(crate) struct ManualListComprehension { is_async: bool, From 827864ffdee8411907cbccc5aea3f3abb2af6df7 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 18 Dec 2024 15:28:26 -0600 Subject: [PATCH 3/5] parenthesize assignment exprs in autofix --- .../perflint/rules/manual_list_comprehension.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index 8e58159bc4f01..bc726e5aef946 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -352,7 +352,21 @@ fn convert_to_list_extend( let semantic = checker.semantic(); let locator = checker.locator(); let if_str = match if_test { - Some(test) => format!(" if {}", locator.slice(test.range())), + Some(test) => { + // If the test is an assignment expression, + // we must parenthesize it when it appears + // inside the comprehension to avoid a syntax error. + // + // Notice that we do not need `any_over_expr` here, + // since if the assignment expression appears + // internally (e.g. as an operand in a boolean + // operation) then it will already be parenthesized. + if test.is_named_expr() { + format!(" if ({})", locator.slice(test.range())) + } else { + format!(" if {}", locator.slice(test.range())) + } + } None => String::new(), }; From f3d596409dac5b6f3ab40b4e90fa67a570d697ca Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 18 Dec 2024 15:28:38 -0600 Subject: [PATCH 4/5] update snapshots --- ...__perflint__tests__PERF401_PERF401.py.snap | 9 +++++++++ ...t__tests__preview__PERF401_PERF401.py.snap | 20 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap index 2257cbe8b714a..8a660dd70938f 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap @@ -202,3 +202,12 @@ PERF401.py:245:13: PERF401 Use `list.extend` to create a transformed list 246 | result = [] | = help: Replace for loop with list.extend + +PERF401.py:262:13: PERF401 Use a list comprehension to create a transformed list + | +260 | for i in range(5): +261 | if j := i: +262 | items.append(j) + | ^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap index 83b2cf55f0468..dda14552ee4a4 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap @@ -486,3 +486,23 @@ PERF401.py:245:13: PERF401 [*] Use `list.extend` to create a transformed list 246 245 | result = [] 247 246 | 248 247 | def f(): + +PERF401.py:262:13: PERF401 [*] Use a list comprehension to create a transformed list + | +260 | for i in range(5): +261 | if j := i: +262 | items.append(j) + | ^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +255 255 | # The fix here must parenthesize the walrus operator +256 256 | # https://github.com/astral-sh/ruff/issues/15047 +257 257 | def f(): +258 |- items = [] +259 258 | +260 |- for i in range(5): +261 |- if j := i: +262 |- items.append(j) + 259 |+ items = [j for i in range(5) if (j := i)] From 29a63c2061eb22173ca07e80da9829c4b167517f Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 18 Dec 2024 15:31:29 -0600 Subject: [PATCH 5/5] fix example in doc-comment --- .../src/rules/perflint/rules/manual_list_comprehension.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index bc726e5aef946..e38e8efae64ae 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -48,7 +48,7 @@ use ruff_text_size::{Ranged, TextRange}; /// ``` /// /// Take care that if the original for-loop uses an assignment expression -/// as a conditional, such as `if match:=re.match("\d+")`, then +/// as a conditional, such as `if match:=re.match("\d+","123")`, then /// the corresponding comprehension must wrap the assignment /// expression in parentheses to avoid a syntax error. #[derive(ViolationMetadata)]