From 56d4c95e1c466954dd9bf44e8ae56d5d9502ad7b Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Thu, 10 Aug 2023 22:35:23 +0100 Subject: [PATCH] Add rule --- .../pylint/subprocess_run_without_check.py | 13 ++++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/pylint/mod.rs | 4 ++ crates/ruff/src/rules/pylint/rules/mod.rs | 2 + .../rules/subprocess_run_without_check.rs | 65 +++++++++++++++++++ ...W1510_subprocess_run_without_check.py.snap | 22 +++++++ ruff.schema.json | 2 + 8 files changed, 112 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/pylint/subprocess_run_without_check.py create mode 100644 crates/ruff/src/rules/pylint/rules/subprocess_run_without_check.rs create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1510_subprocess_run_without_check.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/subprocess_run_without_check.py b/crates/ruff/resources/test/fixtures/pylint/subprocess_run_without_check.py new file mode 100644 index 00000000000000..b329ba45109ab2 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/subprocess_run_without_check.py @@ -0,0 +1,13 @@ +import subprocess + +# Errors. +subprocess.run("ls") +subprocess.run("ls", shell=True) + +# Non-errors. +subprocess.run("ls", check=True) +subprocess.run("ls", check=False) +subprocess.run("ls", shell=True, check=True) +subprocess.run("ls", shell=True, check=False) +foo.run("ls") # Not a subprocess.run call. +subprocess.bar("ls") # Not a subprocess.run call. diff --git a/crates/ruff/src/checkers/ast/analyze/expression.rs b/crates/ruff/src/checkers/ast/analyze/expression.rs index 8589509796804b..d452a718a3aa92 100644 --- a/crates/ruff/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff/src/checkers/ast/analyze/expression.rs @@ -763,6 +763,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::SubprocessPopenPreexecFn) { pylint::rules::subprocess_popen_preexec_fn(checker, call); } + if checker.enabled(Rule::SubprocessRunWithoutCheck) { + pylint::rules::subprocess_run_without_check(checker, call); + } if checker.any_enabled(&[ Rule::PytestRaisesWithoutException, Rule::PytestRaisesTooBroad, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 5e3cfe478a7f5a..f7e3499fc8c8f6 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -226,6 +226,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "W0711") => (RuleGroup::Unspecified, rules::pylint::rules::BinaryOpException), (Pylint, "W1508") => (RuleGroup::Unspecified, rules::pylint::rules::InvalidEnvvarDefault), (Pylint, "W1509") => (RuleGroup::Unspecified, rules::pylint::rules::SubprocessPopenPreexecFn), + (Pylint, "W1510") => (RuleGroup::Unspecified, rules::pylint::rules::SubprocessRunWithoutCheck), (Pylint, "W1641") => (RuleGroup::Nursery, rules::pylint::rules::EqWithoutHash), (Pylint, "W2901") => (RuleGroup::Unspecified, rules::pylint::rules::RedefinedLoopName), (Pylint, "W3301") => (RuleGroup::Unspecified, rules::pylint::rules::NestedMinMax), diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index fbbb859126a3d5..d1dbf8990147f4 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -126,6 +126,10 @@ mod tests { Rule::SubprocessPopenPreexecFn, Path::new("subprocess_popen_preexec_fn.py") )] + #[test_case( + Rule::SubprocessRunWithoutCheck, + Path::new("subprocess_run_without_check.py") + )] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index bfd44d030e610d..c84dd03b3b86f9 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -37,6 +37,7 @@ pub(crate) use return_in_init::*; pub(crate) use self_assigning_variable::*; pub(crate) use single_string_slots::*; pub(crate) use subprocess_popen_preexec_fn::*; +pub(crate) use subprocess_run_without_check::*; pub(crate) use sys_exit_alias::*; pub(crate) use too_many_arguments::*; pub(crate) use too_many_branches::*; @@ -92,6 +93,7 @@ mod return_in_init; mod self_assigning_variable; mod single_string_slots; mod subprocess_popen_preexec_fn; +mod subprocess_run_without_check; mod sys_exit_alias; mod too_many_arguments; mod too_many_branches; diff --git a/crates/ruff/src/rules/pylint/rules/subprocess_run_without_check.rs b/crates/ruff/src/rules/pylint/rules/subprocess_run_without_check.rs new file mode 100644 index 00000000000000..f147e8446c2701 --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/subprocess_run_without_check.rs @@ -0,0 +1,65 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast as ast; +use ruff_python_ast::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for uses of `subprocess.run` without an explicit `check` argument. +/// +/// ## Why is this bad? +/// By default, `subprocess.run` does not check the return code of the process +/// it runs. This can lead to silent failures. +/// +/// Instead, consider using `check=True` to raise an exception if the process +/// fails, or set `check=False` explicitly to mark the behavior as intentional. +/// +/// ## Example +/// ```python +/// import subprocess +/// +/// subprocess.run(["ls", "nonexistent"]) # No exception raised. +/// ``` +/// +/// Use instead: +/// ```python +/// import subprocess +/// +/// subprocess.run(["ls", "nonexistent"], check=True) # Raises exception. +/// ``` +/// +/// Or: +/// ```python +/// import subprocess +/// +/// subprocess.run(["ls", "nonexistent"], check=False) # Explicitly no check. +/// ``` +/// +/// ## References +/// - [Python documentation: `subprocess.run`](https://docs.python.org/3/library/subprocess.html#subprocess.run) +#[violation] +pub struct SubprocessRunWithoutCheck; + +impl Violation for SubprocessRunWithoutCheck { + #[derive_message_formats] + fn message(&self) -> String { + format!("`subprocess.run` without explicit `check` argument") + } +} + +/// PLW1510 +pub(crate) fn subprocess_run_without_check(checker: &mut Checker, call: &ast::ExprCall) { + if checker + .semantic() + .resolve_call_path(&call.func) + .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(), + )); + } + } +} diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1510_subprocess_run_without_check.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1510_subprocess_run_without_check.py.snap new file mode 100644 index 00000000000000..481f3806089581 --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1510_subprocess_run_without_check.py.snap @@ -0,0 +1,22 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +--- +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) + | + +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. + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 98c3518fe940ed..ac9a08c5592747 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2283,6 +2283,8 @@ "PLW150", "PLW1508", "PLW1509", + "PLW151", + "PLW1510", "PLW1641", "PLW2", "PLW29",