New warning: shallow copy of os.environ (#1733)

Shallow copy of os.environ doesn't work as people may expect. os.environ
is not a dict object but rather a proxy object, so any changes made
on the copy may have unexpected effects on os.environ

Instead of copy.copy(os.environ) method os.environ.copy() should be
used.

Message id is: `shallow-copy-environ`

See https://bugs.python.org/issue15373 for details.

Resolves: #1301

Author: MartinBasti

CONTRIBUTORS.txt

@@ -145,3 +145,6 @@ Order doesn't matter (not that much, at least ;)
 * Daniel Miller: contributor.
 
 * Bryce Guinta: contributor
+
+* Martin Bašti: contributor
+  Added new check for shallow copy of os.environ

ChangeLog

@@ -4,6 +4,19 @@ Pylint's ChangeLog
 What's New in Pylint 1.8?
 =========================
 
+    * New warning `shallow-copy-environ` added
+
+      Shallow copy of os.environ doesn't work as people may expect. os.environ
+      is not a dict object but rather a proxy object, so any changes made
+      on copy may have unexpected effects on os.environ
+
+      Instead of copy.copy(os.environ) method os.environ.copy() should be
+      used.
+
+      See https://bugs.python.org/issue15373 for details.
+
+      Close #1301
+
     * Do not display no-absolute-import warning multiple times per file.
 
     * `trailing-comma-tuple` refactor check now extends to assignment with

doc/whatsnew/1.8.rst

@@ -14,6 +14,29 @@ Summary -- Release highlights
 New checkers
 ============
 
+* A new check was added, ``shallow-copy-environ``.
+
+  This warning message is emitted when shallow copy of os.environ is created.
+  Shallow copy of os.environ doesn't work as people may expect. os.environ
+  is not a dict object but rather a proxy object, so any changes made
+  on copy may have unexpected effects on os.environ
+
+  Instead of copy.copy(os.environ) method os.environ.copy() should be used.
+
+  See https://bugs.python.org/issue15373 for details.
+
+  .. code-block:: python
+
+     import copy
+     import os
+     wrong_env_copy = copy.copy(os.environ)  # will emit pylint warning
+     wrong_env_copy['ENV_VAR'] = 'new_value'  # changes os.environ
+     assert os.environ['ENV_VAR'] == 'new_value'
+
+     good_env_copy = os.environ.copy()  # the right way
+     good_env_copy['ENV_VAR'] = 'different_value'  # doesn't change os.environ
+     assert os.environ['ENV_VAR'] == 'new_value'
+
 * A new check was added, ``keyword-arg-before-vararg``.
 
   This warning message is emitted when a function is defined with a keyword

pylint/checkers/stdlib.py

@@ -23,6 +23,8 @@
 OPEN_FILES = {'open', 'file'}
 UNITTEST_CASE = 'unittest.case'
 THREADING_THREAD = 'threading.Thread'
+COPY_COPY = 'copy.copy'
+OS_ENVIRON = 'os._Environ'
 if sys.version_info >= (3, 0):
     OPEN_MODULE = '_io'
 else:
@@ -105,6 +107,12 @@ class StdlibChecker(BaseChecker):
                   'The warning is emitted when a threading.Thread class '
                   'is instantiated without the target function being passed. '
                   'By default, the first parameter is the group param, not the target param. '),
+        'W1507': ('Using copy.copy(os.environ). Use os.environ.copy() '  # TODO: number
+                  'instead. ',
+                  'shallow-copy-environ',
+                  'os.environ is not a dict object but proxy object, so '
+                  'shallow copy has still effects on original object. '
+                  'See https://bugs.python.org/issue15373 for reference. '),
     }
 
     deprecated = {
@@ -186,9 +194,17 @@ def _check_bad_thread_instantiation(self, node):
         if not node.kwargs and node.args:
             self.add_message('bad-thread-instantiation', node=node)
 
+    def _check_shallow_copy_environ(self, node):
+        arg = utils.get_argument_from_call(node, position=0)
+        for inferred in arg.inferred():
+            if inferred.qname() == OS_ENVIRON:
+                self.add_message('shallow-copy-environ', node=node)
+                break
+
     @utils.check_messages('bad-open-mode', 'redundant-unittest-assert',
                           'deprecated-method',
-                          'bad-thread-instantiation')
+                          'bad-thread-instantiation',
+                          'shallow-copy-environ')
     def visit_call(self, node):
         """Visit a Call node."""
         try:
@@ -202,6 +218,8 @@ def visit_call(self, node):
                     self._check_redundant_assert(node, inferred)
                 if isinstance(inferred, astroid.ClassDef) and inferred.qname() == THREADING_THREAD:
                     self._check_bad_thread_instantiation(node)
+                if isinstance(inferred, astroid.FunctionDef) and inferred.qname() == COPY_COPY:
+                    self._check_shallow_copy_environ(node)
                 self._check_deprecated_method(node, inferred)
         except astroid.InferenceError:
             return

pylint/test/unittest_checker_stdlib.py

@@ -9,7 +9,8 @@
 import astroid
 
 from pylint.checkers import stdlib
-from pylint.testutils import CheckerTestCase
+from pylint.testutils import CheckerTestCase, Message
+from pylint.interfaces import UNDEFINED
 
 
 @contextlib.contextmanager
@@ -46,3 +47,59 @@ def infer_func(node, context=None):
             ''')
             with self.assertNoMessages():
                 self.checker.visit_call(node)
+
+    def test_copy_environ(self):
+        # shallow copy of os.environ should be reported
+        node = astroid.extract_node("""
+        import copy, os
+        copy.copy(os.environ)
+        """)
+        with self.assertAddsMessages(
+            Message(
+                msg_id='shallow-copy-environ', node=node, confidence=UNDEFINED)
+        ):
+            self.checker.visit_call(node)
+
+    def test_copy_environ_hidden(self):
+        # shallow copy of os.environ should be reported
+        # hide function names to be sure that checker is not just matching text
+        node = astroid.extract_node("""
+        from copy import copy as test_cp
+        import os as o
+        test_cp(o.environ)
+        """)
+        with self.assertAddsMessages(
+            Message(
+                msg_id='shallow-copy-environ', node=node, confidence=UNDEFINED)
+        ):
+            self.checker.visit_call(node)
+
+    def test_copy_dict(self):
+        # copy of dict is OK
+        node = astroid.extract_node("""
+        import copy
+        test_dict = {}
+        copy.copy(test_dict)
+        """)
+        with self.assertNoMessages():
+            self.checker.visit_call(node)
+
+    def test_copy_uninferable(self):
+        # copy of uninferable object should not raise exception, nor make
+        # the checker crash
+        node = astroid.extract_node("""
+        import copy
+        from missing_library import MissingObject
+        copy.copy(MissingObject)
+        """)
+        with self.assertNoMessages():
+            self.checker.visit_call(node)
+
+    def test_deepcopy_environ(self):
+        # deepcopy of os.environ is OK
+        node = astroid.extract_node("""
+        import copy, os
+        copy.deepcopy(os.environ)
+        """)
+        with self.assertNoMessages():
+            self.checker.visit_call(node)