pylint: W1508 invalid-envvar-default (#3449)

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)

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) {

crates/ruff/src/codes.rs

@@ -170,6 +170,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
         (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,

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,

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")]

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)));
+        }
+    }
+}

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;

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: ~
+