From a18f0cf787f3a955dfd65e56d20ef0e8c250fcfd Mon Sep 17 00:00:00 2001 From: vasco Date: Fri, 18 Apr 2025 14:48:47 +0200 Subject: [PATCH 1/3] make fix unsafe if delete comments --- .../test/fixtures/pylint/if_stmt_min_max.py | 12 ++++ .../src/rules/pylint/rules/if_stmt_min_max.rs | 29 ++++++++-- ...nt__tests__PLR1730_if_stmt_min_max.py.snap | 55 +++++++++++++++++-- 3 files changed, 87 insertions(+), 9 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/if_stmt_min_max.py b/crates/ruff_linter/resources/test/fixtures/pylint/if_stmt_min_max.py index e316c3383ca9f1..d5fa9d34c3e9f0 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/if_stmt_min_max.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/if_stmt_min_max.py @@ -237,3 +237,15 @@ def foo(self, value) -> None: # case 8: counter["a"] = max(counter["b"], counter["a"]) if counter["a"] > counter["b"]: counter["b"] = counter["a"] + +# https://github.com/astral-sh/ruff/issues/17311 + +# fix marked unsafe as delete comments +a, b = [], [] +if a >= b: + # very important comment + a = b + +# fix marked safe as preserve comments +if a >= b: + a = b # very important comment diff --git a/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs b/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs index 4e64179d1e373c..0ccc404f119bab 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs @@ -1,4 +1,4 @@ -use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::parenthesize::parenthesized_range; @@ -29,6 +29,19 @@ use crate::fix::snippet::SourceCodeSnippet; /// highest_score = max(highest_score, score) /// ``` /// +/// ## Fix safety +/// This fix is marked unsafe if it would delete any comments within the replacement range. +/// +/// An example to illustrate where comments are preserved and where they are not: +/// +/// ```pyi +/// a, b = 0, 10 +/// +/// if a >= b: # deleted comment +/// # deleted comment +/// a = b # preserved comment +/// ``` +/// /// ## References /// - [Python documentation: `max`](https://docs.python.org/3/library/functions.html#max) /// - [Python documentation: `min`](https://docs.python.org/3/library/functions.html#min) @@ -169,11 +182,17 @@ pub(crate) fn if_stmt_min_max(checker: &Checker, stmt_if: &ast::StmtIf) { stmt_if.range(), ); + let applicability = if checker.comment_ranges().intersects(stmt_if.range()) { + Applicability::Unsafe + } else { + Applicability::Safe + }; + if checker.semantic().has_builtin_binding(min_max.as_str()) { - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( - replacement, - stmt_if.range(), - ))); + diagnostic.set_fix(Fix::applicable_edit( + Edit::range_replacement(replacement, stmt_if.range()), + applicability, + )); } checker.report_diagnostic(diagnostic); diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_if_stmt_min_max.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_if_stmt_min_max.py.snap index 94b7385d0a67e4..89aec8775ee6c0 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_if_stmt_min_max.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_if_stmt_min_max.py.snap @@ -359,7 +359,7 @@ if_stmt_min_max.py:119:1: PLR1730 [*] Replace `if` statement with `A2 = max(A2, | = help: Replace with `A2 = max(A2, A1)` -ℹ Safe fix +ℹ Unsafe fix 116 116 | A1 = AA(0) 117 117 | A2 = AA(3) 118 118 | @@ -382,7 +382,7 @@ if_stmt_min_max.py:122:1: PLR1730 [*] Replace `if` statement with `A2 = max(A1, | = help: Replace with `A2 = max(A1, A2)` -ℹ Safe fix +ℹ Unsafe fix 119 119 | if A2 < A1: # [max-instead-of-if] 120 120 | A2 = A1 121 121 | @@ -405,7 +405,7 @@ if_stmt_min_max.py:125:1: PLR1730 [*] Replace `if` statement with `A2 = min(A2, | = help: Replace with `A2 = min(A2, A1)` -ℹ Safe fix +ℹ Unsafe fix 122 122 | if A2 <= A1: # [max-instead-of-if] 123 123 | A2 = A1 124 124 | @@ -428,7 +428,7 @@ if_stmt_min_max.py:128:1: PLR1730 [*] Replace `if` statement with `A2 = min(A1, | = help: Replace with `A2 = min(A1, A2)` -ℹ Safe fix +ℹ Unsafe fix 125 125 | if A2 > A1: # [min-instead-of-if] 126 126 | A2 = A1 127 127 | @@ -721,6 +721,8 @@ if_stmt_min_max.py:238:1: PLR1730 [*] Replace `if` statement with `counter["b"] 238 | / if counter["a"] > counter["b"]: 239 | | counter["b"] = counter["a"] | |_______________________________^ PLR1730 +240 | +241 | # https://github.com/astral-sh/ruff/issues/17311 | = help: Replace with `counter["b"] = max(counter["b"], counter["a"])` @@ -731,3 +733,48 @@ if_stmt_min_max.py:238:1: PLR1730 [*] Replace `if` statement with `counter["b"] 238 |-if counter["a"] > counter["b"]: 239 |- counter["b"] = counter["a"] 238 |+counter["b"] = max(counter["b"], counter["a"]) +240 239 | +241 240 | # https://github.com/astral-sh/ruff/issues/17311 +242 241 | + +if_stmt_min_max.py:245:1: PLR1730 [*] Replace `if` statement with `a = min(b, a)` + | +243 | # fix marked unsafe as delete comments +244 | a, b = [], [] +245 | / if a >= b: +246 | | # very important comment +247 | | a = b + | |_________^ PLR1730 +248 | +249 | # fix marked safe as preserve comments + | + = help: Replace with `a = min(b, a)` + +ℹ Unsafe fix +242 242 | +243 243 | # fix marked unsafe as delete comments +244 244 | a, b = [], [] +245 |-if a >= b: +246 |- # very important comment +247 |- a = b + 245 |+a = min(b, a) +248 246 | +249 247 | # fix marked safe as preserve comments +250 248 | if a >= b: + +if_stmt_min_max.py:250:1: PLR1730 [*] Replace `if` statement with `a = min(b, a)` + | +249 | # fix marked safe as preserve comments +250 | / if a >= b: +251 | | a = b # very important comment + | |_________^ PLR1730 + | + = help: Replace with `a = min(b, a)` + +ℹ Safe fix +247 247 | a = b +248 248 | +249 249 | # fix marked safe as preserve comments +250 |-if a >= b: +251 |- a = b # very important comment + 250 |+a = min(b, a) # very important comment From 334ab3c1e9e6ee355a1aabc172baca1effa654cb Mon Sep 17 00:00:00 2001 From: Vasco Schiavo <115561717+VascoSch92@users.noreply.github.com> Date: Fri, 18 Apr 2025 17:25:24 +0200 Subject: [PATCH 2/3] suggestion Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com> --- crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs b/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs index 0ccc404f119bab..f6aa966cd37ac6 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs @@ -34,7 +34,7 @@ use crate::fix::snippet::SourceCodeSnippet; /// /// An example to illustrate where comments are preserved and where they are not: /// -/// ```pyi +/// ```py /// a, b = 0, 10 /// /// if a >= b: # deleted comment From 527b9a6ea4b466bda4dd84d12c9f1b87425b28ea Mon Sep 17 00:00:00 2001 From: vasco Date: Fri, 18 Apr 2025 17:29:28 +0200 Subject: [PATCH 3/3] refactor into a variable stmt_if.range() --- crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs b/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs index f6aa966cd37ac6..4e3e923b62bccf 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs @@ -182,7 +182,8 @@ pub(crate) fn if_stmt_min_max(checker: &Checker, stmt_if: &ast::StmtIf) { stmt_if.range(), ); - let applicability = if checker.comment_ranges().intersects(stmt_if.range()) { + let range_replacement = stmt_if.range(); + let applicability = if checker.comment_ranges().intersects(range_replacement) { Applicability::Unsafe } else { Applicability::Safe @@ -190,7 +191,7 @@ pub(crate) fn if_stmt_min_max(checker: &Checker, stmt_if: &ast::StmtIf) { if checker.semantic().has_builtin_binding(min_max.as_str()) { diagnostic.set_fix(Fix::applicable_edit( - Edit::range_replacement(replacement, stmt_if.range()), + Edit::range_replacement(replacement, range_replacement), applicability, )); }