[refurb] Parenthesize lambda and ternary expressions in iter (FURB122, FURB142)#18592
[refurb] Parenthesize lambda and ternary expressions in iter (FURB122, FURB142)#18592
refurb] Parenthesize lambda and ternary expressions in iter (FURB122, FURB142)#18592Conversation
Summary -- Fixes #18590 by adding parentheses around lambdas and if expressions in `for` loop iterators for FURB122 and FURB142. I also updated the docs on the helper function to reflect the part actually being parenthesized and the new checks. The `lambda` case actually causes a `TypeError` at runtime, but I think it's still worth handling to avoid causing a syntax error. ```pycon >>> s = set() ... for x in (1,) if True else (2,): ... s.add(-x) ... for x in lambda: 0: ... s.discard(-x) ... Traceback (most recent call last): File "<python-input-0>", line 4, in <module> for x in lambda: 0: ^^^^^^^^^ TypeError: 'function' object is not iterable ``` Test Plan -- New test cases based on the bug report
|
|
I think we only want to parenthesize if the fix would write out a comprehension and if it's not already parenthesized, right? So maybe we need to do something like this: diff --git c/crates/ruff_linter/src/rules/refurb/rules/for_loop_set_mutations.rs w/crates/ruff_linter/src/rules/refurb/rules/for_loop_set_mutations.rs
index 08ccf08d7..84dd5e9a7 100644
--- c/crates/ruff_linter/src/rules/refurb/rules/for_loop_set_mutations.rs
+++ w/crates/ruff_linter/src/rules/refurb/rules/for_loop_set_mutations.rs
@@ -5,7 +5,7 @@ use ruff_python_semantic::analyze::typing;
use crate::checkers::ast::Checker;
use crate::{AlwaysFixableViolation, Applicability, Edit, Fix};
-use super::helpers::parenthesize_loop_iter_if_necessary;
+use super::helpers::{IterLocation, parenthesize_loop_iter_if_necessary};
/// ## What it does
/// Checks for code that updates a set with the contents of an iterable by
@@ -106,7 +106,7 @@ pub(crate) fn for_loop_set_mutations(checker: &Checker, for_stmt: &StmtFor) {
format!(
"{}.{batch_method_name}({})",
set.id,
- parenthesize_loop_iter_if_necessary(for_stmt, checker),
+ parenthesize_loop_iter_if_necessary(for_stmt, checker, IterLocation::CallArgument),
)
}
(for_target, arg) => format!(
@@ -114,7 +114,7 @@ pub(crate) fn for_loop_set_mutations(checker: &Checker, for_stmt: &StmtFor) {
set.id,
locator.slice(arg),
locator.slice(for_target),
- parenthesize_loop_iter_if_necessary(for_stmt, checker),
+ parenthesize_loop_iter_if_necessary(for_stmt, checker, IterLocation::InComprehension),
),
};
diff --git c/crates/ruff_linter/src/rules/refurb/rules/for_loop_writes.rs w/crates/ruff_linter/src/rules/refurb/rules/for_loop_writes.rs
index 11ec83337..85180bd33 100644
--- c/crates/ruff_linter/src/rules/refurb/rules/for_loop_writes.rs
+++ w/crates/ruff_linter/src/rules/refurb/rules/for_loop_writes.rs
@@ -5,6 +5,7 @@ use ruff_python_semantic::{Binding, ScopeId, SemanticModel, TypingOnlyBindingsSt
use ruff_text_size::{Ranged, TextRange, TextSize};
use crate::checkers::ast::Checker;
+use crate::rules::refurb::rules::helpers::IterLocation;
use crate::{AlwaysFixableViolation, Applicability, Edit, Fix};
use super::helpers::parenthesize_loop_iter_if_necessary;
@@ -182,7 +183,7 @@ fn for_loop_writes(
format!(
"{}.writelines({})",
locator.slice(io_object_name),
- parenthesize_loop_iter_if_necessary(for_stmt, checker),
+ parenthesize_loop_iter_if_necessary(for_stmt, checker, IterLocation::CallArgument),
)
}
(for_target, write_arg) => {
@@ -191,7 +192,11 @@ fn for_loop_writes(
locator.slice(io_object_name),
locator.slice(write_arg),
locator.slice(for_target),
- parenthesize_loop_iter_if_necessary(for_stmt, checker),
+ parenthesize_loop_iter_if_necessary(
+ for_stmt,
+ checker,
+ IterLocation::InComprehension
+ ),
)
}
};
diff --git c/crates/ruff_linter/src/rules/refurb/rules/helpers.rs w/crates/ruff_linter/src/rules/refurb/rules/helpers.rs
index 8f431ffcc..fa6b4a4d2 100644
--- c/crates/ruff_linter/src/rules/refurb/rules/helpers.rs
+++ w/crates/ruff_linter/src/rules/refurb/rules/helpers.rs
@@ -21,6 +21,7 @@ use crate::checkers::ast::Checker;
pub(super) fn parenthesize_loop_iter_if_necessary<'a>(
for_stmt: &'a ast::StmtFor,
checker: &'a Checker,
+ iter_location: IterLocation,
) -> Cow<'a, str> {
let locator = checker.locator();
let iter = for_stmt.iter.as_ref();
@@ -42,7 +43,25 @@ pub(super) fn parenthesize_loop_iter_if_necessary<'a>(
ast::Expr::Tuple(tuple) if !tuple.parenthesized => {
Cow::Owned(format!("({iter_in_source})"))
}
- ast::Expr::Lambda(_) | ast::Expr::If(_) => Cow::Owned(format!("({iter_in_source})")),
+ ast::Expr::If(_) | ast::Expr::Lambda(_)
+ if matches!(iter_location, IterLocation::InComprehension) =>
+ {
+ if let Some(_) = parenthesized_range(
+ iter.into(),
+ for_stmt.into(),
+ checker.comment_ranges(),
+ checker.source(),
+ ) {
+ Cow::Borrowed(iter_in_source)
+ } else {
+ Cow::Owned(format!("({iter_in_source})"))
+ }
+ }
_ => Cow::Borrowed(iter_in_source),
}
}
+
+pub(crate) enum IterLocation {
+ CallArgument,
+ InComprehension,
+} |
|
Incidentally, and unrelated to your PR - in addition to the doc-comment being a little off, we also have two separate helpers modules for refurb rules:
maybe they should be merged? |
|
I think we will have already bailed out if the range is already parenthesized: ruff/crates/ruff_linter/src/rules/refurb/rules/helpers.rs Lines 28 to 37 in 9315772 but you're probably right about the callable part. I'll work on a test case for that. I did also notice the two > fd helpers.rs
crates/ruff_linter/src/cst/helpers.rs
crates/ruff_linter/src/rules/airflow/helpers.rs
crates/ruff_linter/src/rules/flake8_2020/helpers.rs
crates/ruff_linter/src/rules/flake8_annotations/helpers.rs
crates/ruff_linter/src/rules/flake8_async/helpers.rs
crates/ruff_linter/src/rules/flake8_bandit/helpers.rs
crates/ruff_linter/src/rules/flake8_boolean_trap/helpers.rs
crates/ruff_linter/src/rules/flake8_bugbear/helpers.rs
crates/ruff_linter/src/rules/flake8_builtins/helpers.rs
crates/ruff_linter/src/rules/flake8_comprehensions/rules/helpers.rs
crates/ruff_linter/src/rules/flake8_datetimez/rules/helpers.rs
crates/ruff_linter/src/rules/flake8_django/rules/helpers.rs
crates/ruff_linter/src/rules/flake8_executable/helpers.rs
crates/ruff_linter/src/rules/flake8_logging/rules/helpers.rs
crates/ruff_linter/src/rules/flake8_pytest_style/rules/helpers.rs
crates/ruff_linter/src/rules/flake8_quotes/helpers.rs
crates/ruff_linter/src/rules/flake8_return/helpers.rs
crates/ruff_linter/src/rules/flake8_slots/rules/helpers.rs
crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs
crates/ruff_linter/src/rules/flynt/helpers.rs
crates/ruff_linter/src/rules/isort/helpers.rs
crates/ruff_linter/src/rules/numpy/helpers.rs
crates/ruff_linter/src/rules/pandas_vet/helpers.rs
crates/ruff_linter/src/rules/pep8_naming/helpers.rs
crates/ruff_linter/src/rules/perflint/helpers.rs
crates/ruff_linter/src/rules/pycodestyle/helpers.rs
crates/ruff_linter/src/rules/pydocstyle/helpers.rs
crates/ruff_linter/src/rules/pylint/helpers.rs
crates/ruff_linter/src/rules/pyupgrade/helpers.rs
crates/ruff_linter/src/rules/refurb/helpers.rs
crates/ruff_linter/src/rules/refurb/rules/helpers.rs
crates/ruff_linter/src/rules/ruff/rules/helpers.rs
crates/ruff_linter/src/rules/tryceratops/helpers.rs
crates/ruff_linter/src/text_helpers.rs
crates/ruff_python_ast/src/helpers.rs
crates/ruff_python_parser/src/parser/helpers.rs |
yikes! yeah I agree - but maybe should be a separate PR |
…122`, `FURB142`) (astral-sh#18592) Summary -- Fixes astral-sh#18590 by adding parentheses around lambdas and if expressions in `for` loop iterators for FURB122 and FURB142. I also updated the docs on the helper function to reflect the part actually being parenthesized and the new checks. The `lambda` case actually causes a `TypeError` at runtime, but I think it's still worth handling to avoid causing a syntax error. ```pycon >>> s = set() ... for x in (1,) if True else (2,): ... s.add(-x) ... for x in lambda: 0: ... s.discard(-x) ... Traceback (most recent call last): File "<python-input-0>", line 4, in <module> for x in lambda: 0: ^^^^^^^^^ TypeError: 'function' object is not iterable ``` Test Plan -- New test cases based on the bug report --------- Co-authored-by: Dylan <[email protected]>
* main: [`pylint`] De-emphasize `__hash__ = Parent.__hash__` (`PLW1641`) (#18613) [`flake8-pyi`] Avoid syntax error in the case of starred and keyword arguments (`PYI059`) (#18611) [ty] Add support for global __debug__ constant (#18540) [`ruff`] Preserve parentheses around `deque` in fix for `unnecessary-empty-iterable-within-deque-call` (`RUF037`) (#18598) [`refurb`] Parenthesize lambda and ternary expressions in iter (`FURB122`, `FURB142`) (#18592)
Summary
Fixes #18590 by adding parentheses around lambdas and if expressions in
forloop iterators for FURB122 and FURB142. I also updated the docs on the helper function to reflect the part actually being parenthesized and the new checks.The
lambdacase actually causes aTypeErrorat runtime, but I think it's still worth handling to avoid causing a syntax error.Test Plan
New test cases based on the bug report