From 7315336640c7f4c416420abe18744aa7f2d3f880 Mon Sep 17 00:00:00 2001 From: Jacob Latonis Date: Sun, 12 Mar 2023 12:41:20 -0500 Subject: [PATCH 1/9] pylint: E1507 invalid-envvar-value --- .../fixtures/pylint/invalid_envvar_value.py | 10 +++ crates/ruff/src/checkers/ast/mod.rs | 3 + crates/ruff/src/codes.rs | 1 + crates/ruff/src/registry.rs | 1 + crates/ruff/src/rules/pylint/mod.rs | 1 + .../pylint/rules/invalid_envvar_value.rs | 64 +++++++++++++++++++ crates/ruff/src/rules/pylint/rules/mod.rs | 2 + 7 files changed, 82 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/pylint/invalid_envvar_value.py create mode 100644 crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs diff --git a/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_value.py b/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_value.py new file mode 100644 index 0000000000000..139d684e90c72 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_value.py @@ -0,0 +1,10 @@ +import os + +os.getenv(1) # [invalid-envvar-value] +os.getenv("a") +os.getenv('test') + +os.getenv(["hello"]) # [invalid-envvar-value] + +AA = "aa" +os.getenv(AA) \ No newline at end of file diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 47b8068dda0e8..3148062235178 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2869,6 +2869,9 @@ where if self.settings.rules.enabled(&Rule::InvalidEnvvarDefault) { pylint::rules::invalid_envvar_default(self, func, args, keywords); } + if self.settings.rules.enabled(&Rule::InvalidEnvvarValue) { + pylint::rules::invalid_envvar_value(self, func, args); + } // flake8-pytest-style if self.settings.rules.enabled(&Rule::PatchWithLambda) { diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 35c8a85878eb0..48c45a7544433 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -176,6 +176,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Pylint, "E1206") => Rule::LoggingTooFewArgs, (Pylint, "E1307") => Rule::BadStringFormatType, (Pylint, "E1310") => Rule::BadStrStripCall, + (Pylint, "E1507") => Rule::InvalidEnvvarValue, (Pylint, "E2502") => Rule::BidirectionalUnicode, (Pylint, "R0133") => Rule::ComparisonOfConstant, (Pylint, "R0206") => Rule::PropertyWithParameters, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 01063b62893d7..426481818e1ae 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -147,6 +147,7 @@ ruff_macros::register_rules!( rules::pylint::rules::InvalidAllObject, rules::pylint::rules::InvalidAllFormat, rules::pylint::rules::InvalidEnvvarDefault, + rules::pylint::rules::InvalidEnvvarValue, rules::pylint::rules::BadStringFormatType, rules::pylint::rules::BidirectionalUnicode, rules::pylint::rules::BadStrStripCall, diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index 5a2f0767c96ee..052f3db1886bf 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -44,6 +44,7 @@ mod tests { #[test_case(Rule::InvalidAllFormat, Path::new("invalid_all_format.py"); "PLE0605")] #[test_case(Rule::InvalidAllObject, Path::new("invalid_all_object.py"); "PLE0604")] #[test_case(Rule::InvalidEnvvarDefault, Path::new("invalid_envvar_default.py"); "PLW1508")] + #[test_case(Rule::InvalidEnvvarValue, Path::new("invalid_envvar_value.py"); "PLE1507")] #[test_case(Rule::TooManyReturnStatements, Path::new("too_many_return_statements.py"); "PLR0911")] #[test_case(Rule::TooManyArguments, Path::new("too_many_arguments.py"); "PLR0913")] #[test_case(Rule::TooManyBranches, Path::new("too_many_branches.py"); "PLR0912")] diff --git a/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs new file mode 100644 index 0000000000000..0ecfc59953562 --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs @@ -0,0 +1,64 @@ +use rustpython_parser::ast::{Constant, Expr, ExprKind}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::types::Range; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for `env.getenv` calls with an invalid first argument +/// +/// ## Why is this bad? +/// `os.getenv` only suppors string type arguments. +/// +/// If the provided argument is not a string, `os.getenv` will not function properly. +/// +/// ## Example +/// ```python +/// os.getenv(1) +/// ``` +/// +/// Use instead: +/// ```python +/// os.getenv("1") +/// ``` +#[violation] +pub struct InvalidEnvvarValue; + +impl Violation for InvalidEnvvarValue { + #[derive_message_formats] + fn message(&self) -> String { + format!("Invalid type for initial `os.getenv` argument; expected `str`") + } +} + +/// PLE1507 +pub fn invalid_envvar_value(checker: &mut Checker, func: &Expr, args: &[Expr]) { + if checker + .ctx + .resolve_call_path(func) + .map_or(false, |call_path| call_path.as_slice() == ["os", "getenv"]) + { + // Get the first argument for `getenv` + if let Some(expr) = args.get(0) { + // Ignoring types that are inferred, only do direct constants + if !matches!( + expr.node, + ExprKind::Constant { + value: Constant::Str { .. }, + .. + } | ExprKind::Name { .. } + | ExprKind::Attribute { .. } + | ExprKind::Subscript { .. } + | ExprKind::Call { .. } + ) { + checker + .diagnostics + .push(Diagnostic::new(InvalidEnvvarValue, Range::from(expr))); + } + } else { + return; + } + } +} diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index a94fb63e53231..bd942aa4a7109 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -11,6 +11,7 @@ pub use global_variable_not_assigned::GlobalVariableNotAssigned; pub use invalid_all_format::{invalid_all_format, InvalidAllFormat}; pub use invalid_all_object::{invalid_all_object, InvalidAllObject}; pub use invalid_envvar_default::{invalid_envvar_default, InvalidEnvvarDefault}; +pub use invalid_envvar_value::{invalid_envvar_value, InvalidEnvvarValue}; pub use logging::{logging_call, LoggingTooFewArgs, LoggingTooManyArgs}; pub use magic_value_comparison::{magic_value_comparison, MagicValueComparison}; pub use merge_isinstance::{merge_isinstance, ConsiderMergingIsinstance}; @@ -46,6 +47,7 @@ mod global_variable_not_assigned; mod invalid_all_format; mod invalid_all_object; mod invalid_envvar_default; +mod invalid_envvar_value; mod logging; mod magic_value_comparison; mod merge_isinstance; From f3a2aa579fe7050f8aea70cf74c6430b4811cee7 Mon Sep 17 00:00:00 2001 From: Jacob Latonis Date: Sun, 12 Mar 2023 12:43:42 -0500 Subject: [PATCH 2/9] linting --- crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs index 0ecfc59953562..b07c48a744f53 100644 --- a/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs +++ b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs @@ -57,8 +57,6 @@ pub fn invalid_envvar_value(checker: &mut Checker, func: &Expr, args: &[Expr]) { .diagnostics .push(Diagnostic::new(InvalidEnvvarValue, Range::from(expr))); } - } else { - return; } } } From f61b2c65c44ad1d3686f4341ef4741fb68035c73 Mon Sep 17 00:00:00 2001 From: Jacob Latonis Date: Sun, 12 Mar 2023 12:45:48 -0500 Subject: [PATCH 3/9] test snapshot --- ...ests__PLE1507_invalid_envvar_value.py.snap | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1507_invalid_envvar_value.py.snap diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1507_invalid_envvar_value.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1507_invalid_envvar_value.py.snap new file mode 100644 index 0000000000000..7cc1bb5702d2b --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1507_invalid_envvar_value.py.snap @@ -0,0 +1,31 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +expression: diagnostics +--- +- kind: + name: InvalidEnvvarValue + body: "Invalid type for initial `os.getenv` argument; expected `str`" + suggestion: ~ + fixable: false + location: + row: 3 + column: 10 + end_location: + row: 3 + column: 11 + fix: ~ + parent: ~ +- kind: + name: InvalidEnvvarValue + body: "Invalid type for initial `os.getenv` argument; expected `str`" + suggestion: ~ + fixable: false + location: + row: 7 + column: 10 + end_location: + row: 7 + column: 19 + fix: ~ + parent: ~ + From 94d3d124b980e12ff4e4eaa21e520c9639efdb83 Mon Sep 17 00:00:00 2001 From: Jacob Latonis Date: Sun, 12 Mar 2023 12:47:43 -0500 Subject: [PATCH 4/9] generated code --- ruff.schema.json | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ruff.schema.json b/ruff.schema.json index 2f72a92d206c1..df2bece8a9d13 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1851,6 +1851,9 @@ "PLE1307", "PLE131", "PLE1310", + "PLE15", + "PLE150", + "PLE1507", "PLE2", "PLE25", "PLE250", From 1d1982a21cf566ca37a8fbd5176d1f18a5a1bafb Mon Sep 17 00:00:00 2001 From: Jacob Latonis Date: Sun, 12 Mar 2023 12:53:44 -0500 Subject: [PATCH 5/9] env to os in description --- crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs index b07c48a744f53..a9dc98a804ec0 100644 --- a/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs +++ b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs @@ -7,7 +7,7 @@ use ruff_python_ast::types::Range; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for `env.getenv` calls with an invalid first argument +/// Checks for `os.getenv` calls with an invalid first argument /// /// ## Why is this bad? /// `os.getenv` only suppors string type arguments. From 982c877e6924c9a428c1ccb03a5566b39ce9f46f Mon Sep 17 00:00:00 2001 From: Jacob Latonis Date: Sun, 12 Mar 2023 12:57:35 -0500 Subject: [PATCH 6/9] another description fix --- crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs index a9dc98a804ec0..1628d4dda0e0a 100644 --- a/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs +++ b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs @@ -10,7 +10,7 @@ use crate::checkers::ast::Checker; /// Checks for `os.getenv` calls with an invalid first argument /// /// ## Why is this bad? -/// `os.getenv` only suppors string type arguments. +/// `os.getenv` only supports string type arguments. /// /// If the provided argument is not a string, `os.getenv` will not function properly. /// From 0bb040d286a7e02df915e61242988b1265eb348e Mon Sep 17 00:00:00 2001 From: Jacob Latonis Date: Sun, 12 Mar 2023 13:51:32 -0500 Subject: [PATCH 7/9] implemented keyword check and formatted test file --- .../test/fixtures/pylint/invalid_envvar_value.py | 11 ++++++----- crates/ruff/src/checkers/ast/mod.rs | 2 +- .../rules/pylint/rules/invalid_envvar_value.rs | 16 +++++++++++++--- ...__tests__PLE1507_invalid_envvar_value.py.snap | 15 ++++++++++++++- 4 files changed, 34 insertions(+), 10 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_value.py b/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_value.py index 139d684e90c72..effe27ebfaab3 100644 --- a/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_value.py +++ b/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_value.py @@ -1,10 +1,11 @@ import os -os.getenv(1) # [invalid-envvar-value] +os.getenv(1) # [invalid-envvar-value] os.getenv("a") -os.getenv('test') - -os.getenv(["hello"]) # [invalid-envvar-value] +os.getenv("test") +os.getenv(key="testingAgain") +os.getenv(key=11) # [invalid-envvar-value] +os.getenv(["hello"]) # [invalid-envvar-value] AA = "aa" -os.getenv(AA) \ No newline at end of file +os.getenv(AA) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 3148062235178..8cc0d8b04705b 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2870,7 +2870,7 @@ where pylint::rules::invalid_envvar_default(self, func, args, keywords); } if self.settings.rules.enabled(&Rule::InvalidEnvvarValue) { - pylint::rules::invalid_envvar_value(self, func, args); + pylint::rules::invalid_envvar_value(self, func, args, keywords); } // flake8-pytest-style diff --git a/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs index 1628d4dda0e0a..1d0561ce5c195 100644 --- a/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs +++ b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs @@ -1,4 +1,4 @@ -use rustpython_parser::ast::{Constant, Expr, ExprKind}; +use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; @@ -34,14 +34,24 @@ impl Violation for InvalidEnvvarValue { } /// PLE1507 -pub fn invalid_envvar_value(checker: &mut Checker, func: &Expr, args: &[Expr]) { +pub fn invalid_envvar_value( + checker: &mut Checker, + func: &Expr, + args: &[Expr], + keywords: &[Keyword], +) { if checker .ctx .resolve_call_path(func) .map_or(false, |call_path| call_path.as_slice() == ["os", "getenv"]) { // Get the first argument for `getenv` - if let Some(expr) = args.get(0) { + if let Some(expr) = args.get(0).or_else(|| { + keywords + .iter() + .find(|keyword| keyword.node.arg.as_ref().map_or(false, |arg| arg == "key")) + .map(|keyword| &keyword.node.value) + }) { // Ignoring types that are inferred, only do direct constants if !matches!( expr.node, diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1507_invalid_envvar_value.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1507_invalid_envvar_value.py.snap index 7cc1bb5702d2b..1e3ad3ece73eb 100644 --- a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1507_invalid_envvar_value.py.snap +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1507_invalid_envvar_value.py.snap @@ -22,9 +22,22 @@ expression: diagnostics fixable: false location: row: 7 - column: 10 + column: 14 end_location: row: 7 + column: 16 + fix: ~ + parent: ~ +- kind: + name: InvalidEnvvarValue + body: "Invalid type for initial `os.getenv` argument; expected `str`" + suggestion: ~ + fixable: false + location: + row: 8 + column: 10 + end_location: + row: 8 column: 19 fix: ~ parent: ~ From ebfdc59ddf621abdaf205134028a859cee0bf978 Mon Sep 17 00:00:00 2001 From: Jacob Latonis Date: Sun, 12 Mar 2023 13:53:21 -0500 Subject: [PATCH 8/9] one more test --- .../ruff/resources/test/fixtures/pylint/invalid_envvar_value.py | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_value.py b/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_value.py index effe27ebfaab3..ac5208513072a 100644 --- a/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_value.py +++ b/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_value.py @@ -6,6 +6,7 @@ os.getenv(key="testingAgain") os.getenv(key=11) # [invalid-envvar-value] os.getenv(["hello"]) # [invalid-envvar-value] +os.getenv(key="foo", default="bar") AA = "aa" os.getenv(AA) From 25e369c0f76cee80080bf14c5e4a1e1b720ab22b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 12 Mar 2023 17:37:50 -0400 Subject: [PATCH 9/9] Tweak docs --- crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs index 1d0561ce5c195..562ffa198733e 100644 --- a/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs +++ b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs @@ -7,12 +7,13 @@ use ruff_python_ast::types::Range; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for `os.getenv` calls with an invalid first argument +/// Checks for `os.getenv` calls with an invalid `key` argument. /// /// ## Why is this bad? -/// `os.getenv` only supports string type arguments. +/// `os.getenv` only supports strings as the first argument (`key`). /// -/// If the provided argument is not a string, `os.getenv` will not function properly. +/// If the provided argument is not a string, `os.getenv` will throw a +/// `TypeError` at runtime. /// /// ## Example /// ```python