Skip to content

Commit

Permalink
Implement shallow-copy-environ / W1507 (#14241)
Browse files Browse the repository at this point in the history
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

<!-- What's the purpose of the change? What does it do, and why? -->

Related to #970. Implement [`shallow-copy-environ /
W1507`](https://pylint.readthedocs.io/en/stable/user_guide/messages/warning/shallow-copy-environ.html).

## Test Plan

<!-- How was it tested? -->

Unit test

---------

Co-authored-by: Simon Brugman <[email protected]>
Co-authored-by: Charlie Marsh <[email protected]>
  • Loading branch information
3 people authored Nov 10, 2024
1 parent 5d91ba0 commit d4cf61d
Show file tree
Hide file tree
Showing 9 changed files with 122 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import copy
import os

copied_env = copy.copy(os.environ) # [shallow-copy-environ]
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::BadStrStripCall) {
pylint::rules::bad_str_strip_call(checker, func, args);
}
if checker.enabled(Rule::ShallowCopyEnviron) {
pylint::rules::shallow_copy_environ(checker, call);
}
if checker.enabled(Rule::InvalidEnvvarDefault) {
pylint::rules::invalid_envvar_default(checker, call);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "W0642") => (RuleGroup::Stable, rules::pylint::rules::SelfOrClsAssignment),
(Pylint, "W0711") => (RuleGroup::Stable, rules::pylint::rules::BinaryOpException),
(Pylint, "W1501") => (RuleGroup::Stable, rules::pylint::rules::BadOpenMode),
(Pylint, "W1507") => (RuleGroup::Preview, rules::pylint::rules::ShallowCopyEnviron),
(Pylint, "W1508") => (RuleGroup::Stable, rules::pylint::rules::InvalidEnvvarDefault),
(Pylint, "W1509") => (RuleGroup::Stable, rules::pylint::rules::SubprocessPopenPreexecFn),
(Pylint, "W1510") => (RuleGroup::Stable, rules::pylint::rules::SubprocessRunWithoutCheck),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ mod tests {
Rule::InvalidCharacterBackspace,
Path::new("invalid_characters_syntax_error.py")
)]
#[test_case(Rule::ShallowCopyEnviron, Path::new("shallow_copy_environ.py"))]
#[test_case(Rule::InvalidEnvvarDefault, Path::new("invalid_envvar_default.py"))]
#[test_case(Rule::InvalidEnvvarValue, Path::new("invalid_envvar_value.py"))]
#[test_case(Rule::IterationOverSet, Path::new("iteration_over_set.py"))]
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ pub(crate) use repeated_keyword_argument::*;
pub(crate) use return_in_init::*;
pub(crate) use self_assigning_variable::*;
pub(crate) use self_or_cls_assignment::*;
pub(crate) use shallow_copy_environ::*;
pub(crate) use single_string_slots::*;
pub(crate) use singledispatch_method::*;
pub(crate) use singledispatchmethod_function::*;
Expand Down Expand Up @@ -172,6 +173,7 @@ mod repeated_keyword_argument;
mod return_in_init;
mod self_assigning_variable;
mod self_or_cls_assignment;
mod shallow_copy_environ;
mod single_string_slots;
mod singledispatch_method;
mod singledispatchmethod_function;
Expand Down
90 changes: 90 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/shallow_copy_environ.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast};
use ruff_python_semantic::Modules;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;

/// ## What it does
/// Check for shallow `os.environ` copies.
///
/// ## Why is this bad?
/// `os.environ` is not a `dict` object, but rather, a proxy object. As such, mutating a shallow
/// copy of `os.environ` will also mutate the original object.
///
/// See: [#15373] for more information.
///
/// ## Example
/// ```python
/// import copy
/// import os
///
/// env = copy.copy(os.environ)
/// ```
///
/// Use instead:
/// ```python
/// import os
///
/// env = os.environ.copy()
/// ```
///
/// ## References
/// - [Python documentation: `copy` — Shallow and deep copy operations](https://docs.python.org/3/library/copy.html)
/// - [Python documentation: `os.environ`](https://docs.python.org/3/library/os.html#os.environ)
///
/// [#15373]: https://bugs.python.org/issue15373
#[violation]
pub struct ShallowCopyEnviron;

impl AlwaysFixableViolation for ShallowCopyEnviron {
#[derive_message_formats]
fn message(&self) -> String {
"Shallow copy of `os.environ` via `copy.copy(os.environ)`".to_string()
}

fn fix_title(&self) -> String {
"Replace with `os.environ.copy()`".to_string()
}
}

/// PLW1507
pub(crate) fn shallow_copy_environ(checker: &mut Checker, call: &ast::ExprCall) {
if !(checker.semantic().seen_module(Modules::OS)
&& checker.semantic().seen_module(Modules::COPY))
{
return;
}

if !checker
.semantic()
.resolve_qualified_name(&call.func)
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["copy", "copy"]))
{
return;
}

if !call.arguments.keywords.is_empty() {
return;
}

let [arg] = call.arguments.args.as_ref() else {
return;
};

if !checker
.semantic()
.resolve_qualified_name(arg)
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["os", "environ"]))
{
return;
}

let mut diagnostic = Diagnostic::new(ShallowCopyEnviron, call.range());
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
format!("{}.copy()", checker.locator().slice(arg)),
call.range(),
)));
checker.diagnostics.push(diagnostic);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
shallow_copy_environ.py:4:14: PLW1507 [*] Shallow copy of `os.environ` via `copy.copy(os.environ)`
|
2 | import os
3 |
4 | copied_env = copy.copy(os.environ) # [shallow-copy-environ]
| ^^^^^^^^^^^^^^^^^^^^^ PLW1507
|
= help: Replace with `os.environ.copy()`

Safe fix
1 1 | import copy
2 2 | import os
3 3 |
4 |-copied_env = copy.copy(os.environ) # [shallow-copy-environ]
4 |+copied_env = os.environ.copy() # [shallow-copy-environ]
2 changes: 2 additions & 0 deletions crates/ruff_python_semantic/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1266,6 +1266,7 @@ impl<'a> SemanticModel<'a> {
"anyio" => self.seen.insert(Modules::ANYIO),
"builtins" => self.seen.insert(Modules::BUILTINS),
"collections" => self.seen.insert(Modules::COLLECTIONS),
"copy" => self.seen.insert(Modules::COPY),
"contextvars" => self.seen.insert(Modules::CONTEXTVARS),
"dataclasses" => self.seen.insert(Modules::DATACLASSES),
"datetime" => self.seen.insert(Modules::DATETIME),
Expand Down Expand Up @@ -1856,6 +1857,7 @@ bitflags! {
const CONTEXTVARS = 1 << 19;
const ANYIO = 1 << 20;
const FASTAPI = 1 << 21;
const COPY = 1 << 22;
}
}

Expand Down
1 change: 1 addition & 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 d4cf61d

Please sign in to comment.