From cb8eea64a89940fb9b7dc3620c1346d1e4da805b Mon Sep 17 00:00:00 2001 From: Shantanu <12621235+hauntsaninja@users.noreply.github.com> Date: Mon, 11 Dec 2023 21:08:17 -0800 Subject: [PATCH] [`pylint`] Add fix for `subprocess-run-without-check` (`PLW1510`) (#6708) --- .../pylint/subprocess_run_without_check.py | 5 ++ .../rules/subprocess_run_without_check.rs | 37 ++++++++-- ...W1510_subprocess_run_without_check.py.snap | 73 ++++++++++++++++++- 3 files changed, 106 insertions(+), 9 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/subprocess_run_without_check.py b/crates/ruff_linter/resources/test/fixtures/pylint/subprocess_run_without_check.py index b329ba4510..af5dbbc4d0 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/subprocess_run_without_check.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/subprocess_run_without_check.py @@ -3,6 +3,11 @@ import subprocess # Errors. subprocess.run("ls") subprocess.run("ls", shell=True) +subprocess.run( + ["ls"], + shell=False, +) +subprocess.run(["ls"], **kwargs) # Non-errors. subprocess.run("ls", check=True) 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 4758de8d01..67b494d6bb 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 @@ -1,9 +1,10 @@ -use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_diagnostics::{AlwaysFixableViolation, Applicability, Diagnostic, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; +use crate::fix::edits::add_argument; /// ## What it does /// Checks for uses of `subprocess.run` without an explicit `check` argument. @@ -36,16 +37,25 @@ use crate::checkers::ast::Checker; /// subprocess.run(["ls", "nonexistent"], check=False) # Explicitly no check. /// ``` /// +/// ## 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. +/// /// ## References /// - [Python documentation: `subprocess.run`](https://docs.python.org/3/library/subprocess.html#subprocess.run) #[violation] pub struct SubprocessRunWithoutCheck; -impl Violation for SubprocessRunWithoutCheck { +impl AlwaysFixableViolation for SubprocessRunWithoutCheck { #[derive_message_formats] fn message(&self) -> String { format!("`subprocess.run` without explicit `check` argument") } + + fn fix_title(&self) -> String { + "Add explicit `check=False`".to_string() + } } /// PLW1510 @@ -56,10 +66,27 @@ pub(crate) fn subprocess_run_without_check(checker: &mut Checker, call: &ast::Ex .is_some_and(|call_path| matches!(call_path.as_slice(), ["subprocess", "run"])) { if call.arguments.find_keyword("check").is_none() { - checker.diagnostics.push(Diagnostic::new( - SubprocessRunWithoutCheck, - call.func.range(), + let mut diagnostic = Diagnostic::new(SubprocessRunWithoutCheck, call.func.range()); + diagnostic.set_fix(Fix::applicable_edit( + add_argument( + "check=False", + &call.arguments, + checker.indexer().comment_ranges(), + checker.locator().contents(), + ), + // 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 + }, )); + checker.diagnostics.push(diagnostic); } } } 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 b0306f17a2..7419c16569 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 @@ -1,22 +1,87 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -subprocess_run_without_check.py:4:1: PLW1510 `subprocess.run` without explicit `check` argument +subprocess_run_without_check.py:4:1: PLW1510 [*] `subprocess.run` without explicit `check` argument | 3 | # Errors. 4 | subprocess.run("ls") | ^^^^^^^^^^^^^^ PLW1510 5 | subprocess.run("ls", shell=True) +6 | subprocess.run( | + = help: Add explicit `check=False` -subprocess_run_without_check.py:5:1: PLW1510 `subprocess.run` without explicit `check` argument +ℹ Safe fix +1 1 | import subprocess +2 2 | +3 3 | # Errors. +4 |-subprocess.run("ls") + 4 |+subprocess.run("ls", check=False) +5 5 | subprocess.run("ls", shell=True) +6 6 | subprocess.run( +7 7 | ["ls"], + +subprocess_run_without_check.py:5:1: PLW1510 [*] `subprocess.run` without explicit `check` argument | 3 | # Errors. 4 | subprocess.run("ls") 5 | subprocess.run("ls", shell=True) | ^^^^^^^^^^^^^^ PLW1510 -6 | -7 | # Non-errors. +6 | subprocess.run( +7 | ["ls"], | + = help: Add explicit `check=False` + +ℹ Safe fix +2 2 | +3 3 | # Errors. +4 4 | subprocess.run("ls") +5 |-subprocess.run("ls", shell=True) + 5 |+subprocess.run("ls", shell=True, check=False) +6 6 | subprocess.run( +7 7 | ["ls"], +8 8 | shell=False, + +subprocess_run_without_check.py:6:1: PLW1510 [*] `subprocess.run` without explicit `check` argument + | +4 | subprocess.run("ls") +5 | subprocess.run("ls", shell=True) +6 | subprocess.run( + | ^^^^^^^^^^^^^^ PLW1510 +7 | ["ls"], +8 | shell=False, + | + = help: Add explicit `check=False` + +ℹ Safe fix +5 5 | subprocess.run("ls", shell=True) +6 6 | subprocess.run( +7 7 | ["ls"], +8 |- shell=False, + 8 |+ shell=False, check=False, +9 9 | ) +10 10 | subprocess.run(["ls"], **kwargs) +11 11 | + +subprocess_run_without_check.py:10:1: PLW1510 [*] `subprocess.run` without explicit `check` argument + | + 8 | shell=False, + 9 | ) +10 | subprocess.run(["ls"], **kwargs) + | ^^^^^^^^^^^^^^ PLW1510 +11 | +12 | # Non-errors. + | + = help: Add explicit `check=False` + +ℹ Unsafe fix +7 7 | ["ls"], +8 8 | shell=False, +9 9 | ) +10 |-subprocess.run(["ls"], **kwargs) + 10 |+subprocess.run(["ls"], **kwargs, check=False) +11 11 | +12 12 | # Non-errors. +13 13 | subprocess.run("ls", check=True)