Skip to content

Commit

Permalink
[pylint] Implement subprocess-run-check (W1510) (#6487)
Browse files Browse the repository at this point in the history
## Summary

Implements [`subprocess-run-check`
(`W1510`)](https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/subprocess-run-check.html)
as `subprocess-run-without-check` (`PLW1510`). Includes documentation.

Related to #970.

## Test Plan

`cargo test`
  • Loading branch information
tjkuson authored Aug 11, 2023
1 parent 84ae00c commit 9ff80a8
Show file tree
Hide file tree
Showing 8 changed files with 112 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -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.
3 changes: 3 additions & 0 deletions crates/ruff/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
65 changes: 65 additions & 0 deletions crates/ruff/src/rules/pylint/rules/subprocess_run_without_check.rs
Original file line number Diff line number Diff line change
@@ -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(),
));
}
}
}
Original file line number Diff line number Diff line change
@@ -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.
|


2 changes: 2 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 9ff80a8

Please sign in to comment.