mirror of https://github.com/astral-sh/ruff
[`pylint`] Allow fix with comments and document performance implications (`PLW3301`) (#18936)
Summary -- Closes #18849 by adding a `## Known issues` section describing the potential performance issues when fixing nested iterables. I also deleted the comment check since the fix is already unsafe and added a note to the `## Fix safety` docs. Test Plan -- Existing tests, updated to allow a fix when comments are present since the fix is already unsafe.
This commit is contained in:
parent
7783cea14f
commit
5aab49880a
|
|
@ -14,7 +14,7 @@ min(1, min(2, 3, key=test))
|
|||
# This will still trigger, to merge the calls without keyword args.
|
||||
min(1, min(2, 3, key=test), min(4, 5))
|
||||
|
||||
# Don't provide a fix if there are comments within the call.
|
||||
# The fix is already unsafe, so deleting comments is okay.
|
||||
min(
|
||||
1, # This is a comment.
|
||||
min(2, 3),
|
||||
|
|
|
|||
|
|
@ -36,6 +36,26 @@ pub(crate) enum MinMax {
|
|||
/// diff = maximum - minimum
|
||||
/// ```
|
||||
///
|
||||
/// ## Known issues
|
||||
///
|
||||
/// The resulting code may be slower and use more memory, especially for nested iterables. For
|
||||
/// example, this code:
|
||||
///
|
||||
/// ```python
|
||||
/// iterable = range(3)
|
||||
/// min(1, min(iterable))
|
||||
/// ```
|
||||
///
|
||||
/// will be fixed to:
|
||||
///
|
||||
/// ```python
|
||||
/// iterable = range(3)
|
||||
/// min(1, *iterable)
|
||||
/// ```
|
||||
///
|
||||
/// At least on current versions of CPython, this allocates a collection for the whole iterable
|
||||
/// before calling `min` and could cause performance regressions, at least for large iterables.
|
||||
///
|
||||
/// ## Fix safety
|
||||
///
|
||||
/// This fix is always unsafe and may change the program's behavior for types without full
|
||||
|
|
@ -49,6 +69,8 @@ pub(crate) enum MinMax {
|
|||
/// print(max(1.0, float("nan"), 2.0)) # after fix: 2.0
|
||||
/// ```
|
||||
///
|
||||
/// The fix will also remove any comments within the outer call.
|
||||
///
|
||||
/// ## References
|
||||
/// - [Python documentation: `min`](https://docs.python.org/3/library/functions.html#min)
|
||||
/// - [Python documentation: `max`](https://docs.python.org/3/library/functions.html#max)
|
||||
|
|
@ -176,25 +198,20 @@ pub(crate) fn nested_min_max(
|
|||
}) {
|
||||
let mut diagnostic =
|
||||
checker.report_diagnostic(NestedMinMax { func: min_max }, expr.range());
|
||||
if !checker
|
||||
.comment_ranges()
|
||||
.has_comments(expr, checker.source())
|
||||
{
|
||||
let flattened_expr = Expr::Call(ast::ExprCall {
|
||||
func: Box::new(func.clone()),
|
||||
arguments: Arguments {
|
||||
args: collect_nested_args(min_max, args, checker.semantic()).into_boxed_slice(),
|
||||
keywords: Box::from(keywords),
|
||||
range: TextRange::default(),
|
||||
node_index: ruff_python_ast::AtomicNodeIndex::dummy(),
|
||||
},
|
||||
let flattened_expr = Expr::Call(ast::ExprCall {
|
||||
func: Box::new(func.clone()),
|
||||
arguments: Arguments {
|
||||
args: collect_nested_args(min_max, args, checker.semantic()).into_boxed_slice(),
|
||||
keywords: Box::from(keywords),
|
||||
range: TextRange::default(),
|
||||
node_index: ruff_python_ast::AtomicNodeIndex::dummy(),
|
||||
});
|
||||
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
|
||||
checker.generator().expr(&flattened_expr),
|
||||
expr.range(),
|
||||
)));
|
||||
}
|
||||
},
|
||||
range: TextRange::default(),
|
||||
node_index: ruff_python_ast::AtomicNodeIndex::dummy(),
|
||||
});
|
||||
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
|
||||
checker.generator().expr(&flattened_expr),
|
||||
expr.range(),
|
||||
)));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -169,7 +169,7 @@ nested_min_max.py:15:1: PLW3301 [*] Nested `min` calls can be flattened
|
|||
15 | min(1, min(2, 3, key=test), min(4, 5))
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW3301
|
||||
16 |
|
||||
17 | # Don't provide a fix if there are comments within the call.
|
||||
17 | # The fix is already unsafe, so deleting comments is okay.
|
||||
|
|
||||
= help: Flatten nested `min` calls
|
||||
|
||||
|
|
@ -180,12 +180,12 @@ nested_min_max.py:15:1: PLW3301 [*] Nested `min` calls can be flattened
|
|||
15 |-min(1, min(2, 3, key=test), min(4, 5))
|
||||
15 |+min(1, min(2, 3, key=test), 4, 5)
|
||||
16 16 |
|
||||
17 17 | # Don't provide a fix if there are comments within the call.
|
||||
17 17 | # The fix is already unsafe, so deleting comments is okay.
|
||||
18 18 | min(
|
||||
|
||||
nested_min_max.py:18:1: PLW3301 Nested `min` calls can be flattened
|
||||
nested_min_max.py:18:1: PLW3301 [*] Nested `min` calls can be flattened
|
||||
|
|
||||
17 | # Don't provide a fix if there are comments within the call.
|
||||
17 | # The fix is already unsafe, so deleting comments is okay.
|
||||
18 | / min(
|
||||
19 | | 1, # This is a comment.
|
||||
20 | | min(2, 3),
|
||||
|
|
@ -196,6 +196,19 @@ nested_min_max.py:18:1: PLW3301 Nested `min` calls can be flattened
|
|||
|
|
||||
= help: Flatten nested `min` calls
|
||||
|
||||
ℹ Unsafe fix
|
||||
15 15 | min(1, min(2, 3, key=test), min(4, 5))
|
||||
16 16 |
|
||||
17 17 | # The fix is already unsafe, so deleting comments is okay.
|
||||
18 |-min(
|
||||
19 |- 1, # This is a comment.
|
||||
20 |- min(2, 3),
|
||||
21 |-)
|
||||
18 |+min(1, 2, 3)
|
||||
22 19 |
|
||||
23 20 | # Handle iterable expressions.
|
||||
24 21 | min(1, min(a))
|
||||
|
||||
nested_min_max.py:24:1: PLW3301 [*] Nested `min` calls can be flattened
|
||||
|
|
||||
23 | # Handle iterable expressions.
|
||||
|
|
|
|||
Loading…
Reference in New Issue