diff --git a/crates/ruff_linter/src/rules/pylint/rules/subprocess_run_without_check.rs b/crates/ruff_linter/src/rules/pylint/rules/subprocess_run_without_check.rs index 21a4643f3908f..4fab3ff6a22d8 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/subprocess_run_without_check.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/subprocess_run_without_check.rs @@ -5,7 +5,7 @@ use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::fix::edits::add_argument; -use crate::{AlwaysFixableViolation, Applicability, Fix}; +use crate::{Fix, FixAvailability, Violation}; /// ## What it does /// Checks for uses of `subprocess.run` without an explicit `check` argument. @@ -39,9 +39,12 @@ use crate::{AlwaysFixableViolation, Applicability, Fix}; /// ``` /// /// ## Fix safety -/// This rule's fix is marked as unsafe for function calls that contain -/// `**kwargs`, as adding a `check` keyword argument to such a call may lead -/// to a duplicate keyword argument error. +/// +/// This rule's fix is marked as display-only because it's not clear whether the +/// potential exception was meant to be ignored by setting `check=False` or if +/// the author simply forgot to include `check=True`. The fix adds +/// `check=False`, making the existing behavior explicit but possibly masking +/// the original intention. /// /// ## References /// - [Python documentation: `subprocess.run`](https://docs.python.org/3/library/subprocess.html#subprocess.run) @@ -49,14 +52,18 @@ use crate::{AlwaysFixableViolation, Applicability, Fix}; #[violation_metadata(stable_since = "v0.0.285")] pub(crate) struct SubprocessRunWithoutCheck; -impl AlwaysFixableViolation for SubprocessRunWithoutCheck { +impl Violation for SubprocessRunWithoutCheck { + // The fix is always set on the diagnostic, but display-only fixes aren't + // considered "fixable" in the tests. + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { "`subprocess.run` without explicit `check` argument".to_string() } - fn fix_title(&self) -> String { - "Add explicit `check=False`".to_string() + fn fix_title(&self) -> Option { + Some("Add explicit `check=False`".to_string()) } } @@ -74,20 +81,11 @@ pub(crate) fn subprocess_run_without_check(checker: &Checker, call: &ast::ExprCa if call.arguments.find_keyword("check").is_none() { let mut diagnostic = checker.report_diagnostic(SubprocessRunWithoutCheck, call.func.range()); - diagnostic.set_fix(Fix::applicable_edit( - add_argument("check=False", &call.arguments, checker.tokens()), - // If the function call contains `**kwargs`, mark the fix as unsafe. - if call - .arguments - .keywords - .iter() - .any(|keyword| keyword.arg.is_none()) - { - Applicability::Unsafe - } else { - Applicability::Safe - }, - )); + diagnostic.set_fix(Fix::display_only_edit(add_argument( + "check=False", + &call.arguments, + checker.tokens(), + ))); } } } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1510_subprocess_run_without_check.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1510_subprocess_run_without_check.py.snap index 360048b2d6053..8028f82997dd6 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1510_subprocess_run_without_check.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1510_subprocess_run_without_check.py.snap @@ -19,6 +19,7 @@ help: Add explicit `check=False` 5 | subprocess.run("ls", shell=True) 6 | subprocess.run( 7 | ["ls"], +note: This is a display-only fix and is likely to be incorrect PLW1510 [*] `subprocess.run` without explicit `check` argument --> subprocess_run_without_check.py:5:1 @@ -39,6 +40,7 @@ help: Add explicit `check=False` 6 | subprocess.run( 7 | ["ls"], 8 | shell=False, +note: This is a display-only fix and is likely to be incorrect PLW1510 [*] `subprocess.run` without explicit `check` argument --> subprocess_run_without_check.py:6:1 @@ -59,6 +61,7 @@ help: Add explicit `check=False` 9 | ) 10 | subprocess.run(["ls"], **kwargs) 11 | +note: This is a display-only fix and is likely to be incorrect PLW1510 [*] `subprocess.run` without explicit `check` argument --> subprocess_run_without_check.py:10:1 @@ -79,4 +82,4 @@ help: Add explicit `check=False` 11 | 12 | # Non-errors. 13 | subprocess.run("ls", check=True) -note: This is an unsafe fix and may change runtime behavior +note: This is a display-only fix and is likely to be incorrect