From 0f78f27713ed7a636b817abbbc45b4c2ab3ae1f5 Mon Sep 17 00:00:00 2001 From: Jacob Latonis Date: Sat, 11 Mar 2023 15:44:42 -0600 Subject: [PATCH] pylint: W1508 invalid-envvar-default (#3449) --- .../fixtures/pylint/invalid_envvar_default.py | 8 ++ 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_default.rs | 90 +++++++++++++++++++ crates/ruff/src/rules/pylint/rules/mod.rs | 2 + ...ts__PLW1508_invalid_envvar_default.py.snap | 44 +++++++++ ruff.schema.json | 4 + 9 files changed, 154 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/pylint/invalid_envvar_default.py create mode 100644 crates/ruff/src/rules/pylint/rules/invalid_envvar_default.rs create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1508_invalid_envvar_default.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_default.py b/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_default.py new file mode 100644 index 0000000000..c557b76837 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/invalid_envvar_default.py @@ -0,0 +1,8 @@ +import os + +tempVar = os.getenv("TEST", 12) # [invalid-envvar-default] +goodVar = os.getenv("TESTING", None) +dictVarBad = os.getenv("AAA", {"a", 7}) # [invalid-envvar-default] +print(os.getenv("TEST", False)) # [invalid-envvar-default] +os.getenv("AA", "GOOD") +os.getenv("B", Z) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 68732332c5..47b8068dda 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2866,6 +2866,9 @@ where if self.settings.rules.enabled(&Rule::BadStrStripCall) { pylint::rules::bad_str_strip_call(self, func, args); } + if self.settings.rules.enabled(&Rule::InvalidEnvvarDefault) { + pylint::rules::invalid_envvar_default(self, func, args, keywords); + } // 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 2c91ccd32d..35c8a85878 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -170,6 +170,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Pylint, "E0118") => Rule::UsedPriorGlobalDeclaration, (Pylint, "E0604") => Rule::InvalidAllObject, (Pylint, "E0605") => Rule::InvalidAllFormat, + (Pylint, "W1508") => Rule::InvalidEnvvarDefault, (Pylint, "E1142") => Rule::AwaitOutsideAsync, (Pylint, "E1205") => Rule::LoggingTooManyArgs, (Pylint, "E1206") => Rule::LoggingTooFewArgs, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index d74c0d4c97..01063b6289 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -146,6 +146,7 @@ ruff_macros::register_rules!( rules::pylint::rules::YieldInInit, rules::pylint::rules::InvalidAllObject, rules::pylint::rules::InvalidAllFormat, + rules::pylint::rules::InvalidEnvvarDefault, 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 71a923cba4..5a2f0767c9 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -43,6 +43,7 @@ mod tests { #[test_case(Rule::GlobalStatement, Path::new("global_statement.py"); "PLW0603")] #[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::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_default.rs b/crates/ruff/src/rules/pylint/rules/invalid_envvar_default.rs new file mode 100644 index 0000000000..069a49ff24 --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/invalid_envvar_default.rs @@ -0,0 +1,90 @@ +use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword}; + +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 invalid default values. +/// +/// ## Why is this bad? +/// If an environment variable is set, `env.getenv` will return its value as +/// a string. If the environment variable is _not_ set, `env.getenv` will +/// return `None`, or the default value if one is provided. +/// +/// If the default value is not a string or `None`, then it will be +/// inconsistent with the return type of `env.getenv`, which can lead to +/// confusing behavior. +/// +/// ## Example +/// ```python +/// int(env.getenv("FOO", 1)) +/// ``` +/// +/// Use instead: +/// ```python +/// int(env.getenv("FOO", "1")) +/// ``` +#[violation] +pub struct InvalidEnvvarDefault; + +impl Violation for InvalidEnvvarDefault { + #[derive_message_formats] + fn message(&self) -> String { + format!("Invalid type for environment variable default; expected `str` or `None`") + } +} + +fn is_valid_default(expr: &Expr) -> bool { + // We can't infer the types of these defaults, so assume they're valid. + if matches!( + expr.node, + ExprKind::Name { .. } + | ExprKind::Attribute { .. } + | ExprKind::Subscript { .. } + | ExprKind::Call { .. } + ) { + return true; + } + + // Otherwise, the default must be a string or `None`. + matches!( + expr.node, + ExprKind::Constant { + value: Constant::Str { .. } | Constant::None { .. }, + .. + } + ) +} + +/// PLW1508 +pub fn invalid_envvar_default( + 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"]) + { + // Find the `default` argument, if it exists. + let Some(expr) = args.get(1).or_else(|| { + keywords + .iter() + .find(|keyword| keyword.node.arg.as_ref().map_or(false, |arg| arg == "default")) + .map(|keyword| &keyword.node.value) + }) else { + return; + }; + + if !is_valid_default(expr) { + checker + .diagnostics + .push(Diagnostic::new(InvalidEnvvarDefault, Range::from(expr))); + } + } +} diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index 7efae626c1..a94fb63e53 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -10,6 +10,7 @@ pub use global_statement::{global_statement, GlobalStatement}; 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 logging::{logging_call, LoggingTooFewArgs, LoggingTooManyArgs}; pub use magic_value_comparison::{magic_value_comparison, MagicValueComparison}; pub use merge_isinstance::{merge_isinstance, ConsiderMergingIsinstance}; @@ -44,6 +45,7 @@ mod global_statement; mod global_variable_not_assigned; mod invalid_all_format; mod invalid_all_object; +mod invalid_envvar_default; mod logging; mod magic_value_comparison; mod merge_isinstance; diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1508_invalid_envvar_default.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1508_invalid_envvar_default.py.snap new file mode 100644 index 0000000000..ea6f86774e --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1508_invalid_envvar_default.py.snap @@ -0,0 +1,44 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +expression: diagnostics +--- +- kind: + name: InvalidEnvvarDefault + body: "Invalid type for environment variable default; expected `str` or `None`" + suggestion: ~ + fixable: false + location: + row: 3 + column: 28 + end_location: + row: 3 + column: 30 + fix: ~ + parent: ~ +- kind: + name: InvalidEnvvarDefault + body: "Invalid type for environment variable default; expected `str` or `None`" + suggestion: ~ + fixable: false + location: + row: 5 + column: 30 + end_location: + row: 5 + column: 38 + fix: ~ + parent: ~ +- kind: + name: InvalidEnvvarDefault + body: "Invalid type for environment variable default; expected `str` or `None`" + suggestion: ~ + fixable: false + location: + row: 6 + column: 24 + end_location: + row: 6 + column: 29 + fix: ~ + parent: ~ + diff --git a/ruff.schema.json b/ruff.schema.json index 04400a4292..2f72a92d20 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1895,6 +1895,10 @@ "PLW060", "PLW0602", "PLW0603", + "PLW1", + "PLW15", + "PLW150", + "PLW1508", "PLW2", "PLW29", "PLW290",