From 54b542cb0ace083c2245cbcc5dbdd4d46a9e0c7a Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 20 Aug 2023 12:55:19 -0700 Subject: [PATCH 1/7] Add an autofix for PLW1510 --- .../rules/subprocess_run_without_check.rs | 20 +++++++++----- ...W1510_subprocess_run_without_check.py.snap | 26 +++++++++++++++++-- 2 files changed, 38 insertions(+), 8 deletions(-) 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 index f147e8446c270..15eab904269eb 100644 --- a/crates/ruff/src/rules/pylint/rules/subprocess_run_without_check.rs +++ b/crates/ruff/src/rules/pylint/rules/subprocess_run_without_check.rs @@ -1,4 +1,6 @@ -use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_text_size::TextSize; + +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_python_ast::Ranged; @@ -41,11 +43,15 @@ use crate::checkers::ast::Checker; #[violation] pub struct SubprocessRunWithoutCheck; -impl Violation for SubprocessRunWithoutCheck { +impl AlwaysAutofixableViolation for SubprocessRunWithoutCheck { #[derive_message_formats] fn message(&self) -> String { format!("`subprocess.run` without explicit `check` argument") } + + fn autofix_title(&self) -> String { + "Add an explicit check=False".to_string() + } } /// PLW1510 @@ -56,10 +62,12 @@ 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::automatic(Edit::insertion( + ", check=False".to_string(), + call.range().end() - TextSize::from(1), + ))); + checker.diagnostics.push(diagnostic); } } } 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 index 481f380608958..22a6b4132961d 100644 --- 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 @@ -1,15 +1,26 @@ --- source: crates/ruff/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) | + = help: Add an explicit check=False -subprocess_run_without_check.py:5:1: PLW1510 `subprocess.run` without explicit `check` argument +ℹ 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 | +7 7 | # Non-errors. + +subprocess_run_without_check.py:5:1: PLW1510 [*] `subprocess.run` without explicit `check` argument | 3 | # Errors. 4 | subprocess.run("ls") @@ -18,5 +29,16 @@ subprocess_run_without_check.py:5:1: PLW1510 `subprocess.run` without explicit ` 6 | 7 | # Non-errors. | + = help: Add an explicit check=False + +ℹ 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 | +7 7 | # Non-errors. +8 8 | subprocess.run("ls", check=True) From 51cca2730a8d2c2e82ad1ec6c318e29a4ce572e7 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 20 Aug 2023 13:01:15 -0700 Subject: [PATCH 2/7] fix comma handling --- .../test/fixtures/pylint/subprocess_run_without_check.py | 4 ++++ 1 file changed, 4 insertions(+) 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 index b329ba45109ab..d83917206e819 100644 --- a/crates/ruff/resources/test/fixtures/pylint/subprocess_run_without_check.py +++ b/crates/ruff/resources/test/fixtures/pylint/subprocess_run_without_check.py @@ -3,6 +3,10 @@ # Errors. subprocess.run("ls") subprocess.run("ls", shell=True) +subprocess.run( + ["ls"], + shell=False, +) # Non-errors. subprocess.run("ls", check=True) From b41dd36a5cf4447743489e2d86795a422582e9b3 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 20 Aug 2023 13:19:16 -0700 Subject: [PATCH 3/7] better comma handling --- .../rules/subprocess_run_without_check.rs | 11 +++++- ...W1510_subprocess_run_without_check.py.snap | 36 +++++++++++++++---- 2 files changed, 39 insertions(+), 8 deletions(-) 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 index 15eab904269eb..fe9f1a2941743 100644 --- a/crates/ruff/src/rules/pylint/rules/subprocess_run_without_check.rs +++ b/crates/ruff/src/rules/pylint/rules/subprocess_run_without_check.rs @@ -63,8 +63,17 @@ pub(crate) fn subprocess_run_without_check(checker: &mut Checker, call: &ast::Ex { if call.arguments.find_keyword("check").is_none() { let mut diagnostic = Diagnostic::new(SubprocessRunWithoutCheck, call.func.range()); + let text: &str = checker.locator().slice(call.range()); + let ends_with_comma = text[..text.len() - 1].trim_end().ends_with(','); diagnostic.set_fix(Fix::automatic(Edit::insertion( - ", check=False".to_string(), + { + if ends_with_comma { + "check=False" + } else { + ", check=False" + } + } + .to_string(), call.range().end() - TextSize::from(1), ))); checker.diagnostics.push(diagnostic); 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 index 22a6b4132961d..cf4129380c99e 100644 --- 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 @@ -7,6 +7,7 @@ subprocess_run_without_check.py:4:1: PLW1510 [*] `subprocess.run` without explic 4 | subprocess.run("ls") | ^^^^^^^^^^^^^^ PLW1510 5 | subprocess.run("ls", shell=True) +6 | subprocess.run( | = help: Add an explicit check=False @@ -17,8 +18,8 @@ subprocess_run_without_check.py:4:1: PLW1510 [*] `subprocess.run` without explic 4 |-subprocess.run("ls") 4 |+subprocess.run("ls", check=False) 5 5 | subprocess.run("ls", shell=True) -6 6 | -7 7 | # Non-errors. +6 6 | subprocess.run( +7 7 | ["ls"], subprocess_run_without_check.py:5:1: PLW1510 [*] `subprocess.run` without explicit `check` argument | @@ -26,8 +27,8 @@ subprocess_run_without_check.py:5:1: PLW1510 [*] `subprocess.run` without explic 4 | subprocess.run("ls") 5 | subprocess.run("ls", shell=True) | ^^^^^^^^^^^^^^ PLW1510 -6 | -7 | # Non-errors. +6 | subprocess.run( +7 | ["ls"], | = help: Add an explicit check=False @@ -37,8 +38,29 @@ subprocess_run_without_check.py:5:1: PLW1510 [*] `subprocess.run` without explic 4 4 | subprocess.run("ls") 5 |-subprocess.run("ls", shell=True) 5 |+subprocess.run("ls", shell=True, check=False) -6 6 | -7 7 | # Non-errors. -8 8 | subprocess.run("ls", check=True) +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 an explicit check=False + +ℹ Fix +6 6 | subprocess.run( +7 7 | ["ls"], +8 8 | shell=False, +9 |-) + 9 |+check=False) +10 10 | +11 11 | # Non-errors. +12 12 | subprocess.run("ls", check=True) From f34bb4cf4f7d526356c2e319dab27c49ce13e6a7 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 22 Nov 2023 01:15:49 +0000 Subject: [PATCH 4/7] Update references --- .../pylint/rules/subprocess_run_without_check.rs | 10 +++++----- ...sts__PLW1510_subprocess_run_without_check.py.snap | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) 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 cb0949eb49c27..49fa95fc3a2e2 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,6 +1,6 @@ use ruff_text_size::TextSize; -use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_text_size::Ranged; @@ -43,14 +43,14 @@ use crate::checkers::ast::Checker; #[violation] pub struct SubprocessRunWithoutCheck; -impl AlwaysAutofixableViolation for SubprocessRunWithoutCheck { +impl AlwaysFixableViolation for SubprocessRunWithoutCheck { #[derive_message_formats] fn message(&self) -> String { format!("`subprocess.run` without explicit `check` argument") } - fn autofix_title(&self) -> String { - "Add an explicit check=False".to_string() + fn fix_title(&self) -> String { + "Add explicit `check=False`".to_string() } } @@ -65,7 +65,7 @@ pub(crate) fn subprocess_run_without_check(checker: &mut Checker, call: &ast::Ex let mut diagnostic = Diagnostic::new(SubprocessRunWithoutCheck, call.func.range()); let text: &str = checker.locator().slice(call.range()); let ends_with_comma = text[..text.len() - 1].trim_end().ends_with(','); - diagnostic.set_fix(Fix::automatic(Edit::insertion( + diagnostic.set_fix(Fix::safe_edit(Edit::insertion( { if ends_with_comma { "check=False" 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 14edab0808554..d5ecd876cea1b 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 @@ -9,9 +9,9 @@ subprocess_run_without_check.py:4:1: PLW1510 [*] `subprocess.run` without explic 5 | subprocess.run("ls", shell=True) 6 | subprocess.run( | - = help: Add an explicit check=False + = help: Add explicit `check=False` -ℹ Fix +ℹ Safe fix 1 1 | import subprocess 2 2 | 3 3 | # Errors. @@ -30,9 +30,9 @@ subprocess_run_without_check.py:5:1: PLW1510 [*] `subprocess.run` without explic 6 | subprocess.run( 7 | ["ls"], | - = help: Add an explicit check=False + = help: Add explicit `check=False` -ℹ Fix +ℹ Safe fix 2 2 | 3 3 | # Errors. 4 4 | subprocess.run("ls") @@ -51,9 +51,9 @@ subprocess_run_without_check.py:6:1: PLW1510 [*] `subprocess.run` without explic 7 | ["ls"], 8 | shell=False, | - = help: Add an explicit check=False + = help: Add explicit `check=False` -ℹ Fix +ℹ Safe fix 6 6 | subprocess.run( 7 7 | ["ls"], 8 8 | shell=False, From 9688015c65dde44b574a17770b504323b1f975ca Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 3 Dec 2023 22:40:28 -0800 Subject: [PATCH 5/7] .fix --- .../rules/subprocess_run_without_check.rs | 26 +++++++------------ ...W1510_subprocess_run_without_check.py.snap | 12 ++++----- 2 files changed, 16 insertions(+), 22 deletions(-) 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 49fa95fc3a2e2..8429aad506022 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,8 @@ -use ruff_text_size::TextSize; - -use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_text_size::Ranged; +use crate::fix::edits::add_argument; use crate::checkers::ast::Checker; @@ -63,19 +62,14 @@ pub(crate) fn subprocess_run_without_check(checker: &mut Checker, call: &ast::Ex { if call.arguments.find_keyword("check").is_none() { let mut diagnostic = Diagnostic::new(SubprocessRunWithoutCheck, call.func.range()); - let text: &str = checker.locator().slice(call.range()); - let ends_with_comma = text[..text.len() - 1].trim_end().ends_with(','); - diagnostic.set_fix(Fix::safe_edit(Edit::insertion( - { - if ends_with_comma { - "check=False" - } else { - ", check=False" - } - } - .to_string(), - call.range().end() - TextSize::from(1), - ))); + diagnostic.set_fix(Fix::safe_edit( + add_argument( + "check=False", + &call.arguments, + checker.indexer().comment_ranges(), + checker.locator().contents(), + ) + )); 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 d5ecd876cea1b..aea6285829694 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 @@ -54,13 +54,13 @@ subprocess_run_without_check.py:6:1: PLW1510 [*] `subprocess.run` without explic = help: Add explicit `check=False` ℹ Safe fix -6 6 | subprocess.run( -7 7 | ["ls"], -8 8 | shell=False, -9 |-) - 9 |+check=False) +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 | 11 11 | # Non-errors. -12 12 | subprocess.run("ls", check=True) From 5db535499536c3848313b3a47b8fdf6621e762ce Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 3 Dec 2023 22:41:27 -0800 Subject: [PATCH 6/7] . --- .../rules/pylint/rules/subprocess_run_without_check.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) 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 8429aad506022..6d388edcfbbca 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,8 +1,8 @@ +use crate::fix::edits::add_argument; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_text_size::Ranged; -use crate::fix::edits::add_argument; use crate::checkers::ast::Checker; @@ -62,14 +62,12 @@ pub(crate) fn subprocess_run_without_check(checker: &mut Checker, call: &ast::Ex { if call.arguments.find_keyword("check").is_none() { let mut diagnostic = Diagnostic::new(SubprocessRunWithoutCheck, call.func.range()); - diagnostic.set_fix(Fix::safe_edit( - add_argument( + diagnostic.set_fix(Fix::safe_edit(add_argument( "check=False", &call.arguments, checker.indexer().comment_ranges(), checker.locator().contents(), - ) - )); + ))); checker.diagnostics.push(diagnostic); } } From c90d9b6d08dbcc77bf2a5b2c5ad20640a4c89e01 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 12 Dec 2023 00:02:28 -0500 Subject: [PATCH 7/7] Add unsafe for kwargs --- .../pylint/subprocess_run_without_check.py | 1 + .../rules/subprocess_run_without_check.rs | 34 ++++++++++++++----- ...W1510_subprocess_run_without_check.py.snap | 25 ++++++++++++-- 3 files changed, 50 insertions(+), 10 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 d83917206e819..af5dbbc4d0feb 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 @@ -7,6 +7,7 @@ ["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 6d388edcfbbca..67b494d6bb700 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,10 +1,10 @@ -use crate::fix::edits::add_argument; -use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; +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. @@ -37,6 +37,11 @@ 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] @@ -62,12 +67,25 @@ pub(crate) fn subprocess_run_without_check(checker: &mut Checker, call: &ast::Ex { if call.arguments.find_keyword("check").is_none() { let mut diagnostic = Diagnostic::new(SubprocessRunWithoutCheck, call.func.range()); - diagnostic.set_fix(Fix::safe_edit(add_argument( - "check=False", - &call.arguments, - checker.indexer().comment_ranges(), - checker.locator().contents(), - ))); + 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 aea6285829694..7419c16569584 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 @@ -60,7 +60,28 @@ subprocess_run_without_check.py:6:1: PLW1510 [*] `subprocess.run` without explic 8 |- shell=False, 8 |+ shell=False, check=False, 9 9 | ) -10 10 | -11 11 | # Non-errors. +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)