From 9970d76a0a27a171bdf4e0d504c709119b38f119 Mon Sep 17 00:00:00 2001
From: Radu Ciorba <radu@devrandom.ro>
Date: Sun, 22 Mar 2015 00:32:42 +0200
Subject: [PATCH] add warning for confusing usage of with statement

Emitted when an ambiguous looking with statement is used.
For example `with open() as first, second` which looks like a
tuple assignment but is actually 2 context managers.

--HG--
branch : confusing-with-statement
---
 ChangeLog                                     |  5 +++
 pylint/checkers/base.py                       | 37 +++++++++++++++++++
 .../functional/confusing_with_statement.py    | 27 ++++++++++++++
 .../functional/confusing_with_statement.txt   |  1 +
 4 files changed, 70 insertions(+)
 create mode 100644 pylint/test/functional/confusing_with_statement.py
 create mode 100644 pylint/test/functional/confusing_with_statement.txt

diff --git a/ChangeLog b/ChangeLog
index cfc015d2d..e40d52122 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -2,6 +2,11 @@ ChangeLog for Pylint
 --------------------
 
 --
+    * Add a new warning, 'confusing-with-statement', emitted by the
+      base checker, when an ambiguous looking with statement is used.
+      For example `with open() as first, second` which looks like a
+      tuple assignment but is actually 2 context managers.
+
     * Add a new warning, 'duplicate-except', emitted when there is an
       exception handler which handles an exception type that was handled
       before. Closes issue #485.
diff --git a/pylint/checkers/base.py b/pylint/checkers/base.py
index 6ce88251a..7659fa485 100644
--- a/pylint/checkers/base.py
+++ b/pylint/checkers/base.py
@@ -508,6 +508,13 @@ functions, methods
                   'A call of assert on a tuple will always evaluate to true if '
                   'the tuple is not empty, and will always evaluate to false if '
                   'it is.'),
+        'W0124': ('Following "as" with another context manager looks like a tuple.',
+                  'confusing-with-statement',
+                  'Emitted when a `with` statement component returns multiple values '
+                  'and uses name binding with `as` only for a part of those values, '
+                  'as in with ctx() as a, b. This can be misleading, since it\'s not '
+                  'clear if the context manager returns a tuple or if the node without '
+                  'a name binding is another context manager.'),
         'C0121': ('Missing required attribute "%s"', # W0103
                   'missing-module-attribute',
                   'Used when an attribute required for modules is missing.'),
@@ -872,6 +879,36 @@ functions, methods
                 # everything else is not a proper sequence for reversed()
                 self.add_message('bad-reversed-sequence', node=node)
 
+    @check_messages('confusing-with-statement')
+    def visit_with(self, node):
+        if not PY3K:
+            # in Python 2 a "with" statement with multiple managers coresponds
+            # to multiple nested AST "With" nodes
+            pairs = []
+            parent_node = node.parent
+            if isinstance(parent_node, astroid.With):
+                # we only care about the direct parent, since this method
+                # gets called for each with node anyway
+                pairs.extend(parent_node.items)
+            pairs.extend(node.items)
+        else:
+            # in PY3K a "with" statement with multiple managers coresponds
+            # to one AST "With" node with multiple items
+            pairs = node.items
+        prev_pair = None
+        for pair in pairs:
+            if prev_pair is not None:
+                if (isinstance(prev_pair[1], astroid.AssName) and
+                        (pair[1] is None and not isinstance(pair[0], astroid.CallFunc))):
+                    # don't emit a message if the second is a function call
+                    # there's no way that can be mistaken for a name assignment
+                    if PY3K or node.lineno == node.parent.lineno:
+                        # if the line number doesn't match
+                        # we assume it's a nested "with"
+                        self.add_message('confusing-with-statement', node=node)
+            prev_pair = pair
+
+
 _NAME_TYPES = {
     'module': (MOD_NAME_RGX, 'module'),
     'const': (CONST_NAME_RGX, 'constant'),
diff --git a/pylint/test/functional/confusing_with_statement.py b/pylint/test/functional/confusing_with_statement.py
new file mode 100644
index 000000000..1444a9795
--- /dev/null
+++ b/pylint/test/functional/confusing_with_statement.py
@@ -0,0 +1,27 @@
+# pylint: disable=undefined-variable,not-context-manager,missing-docstring
+
+# both context managers are named
+with one as first, two as second:
+    pass
+
+# first matched as tuple; this is ok
+with one as (first, second), third:
+    pass
+
+# the first context manager doesn't have as name
+with one, two as second:
+    pass
+
+# the second is a function call; this is ok
+with one as first, two():
+    pass
+
+# nested with statements; make sure no message is emited
+# this could be a false positive on Py2
+with one as first:
+    with two:
+        pass
+
+# ambiguous looking with statement
+with one as first, two:  # [confusing-with-statement]
+    pass
diff --git a/pylint/test/functional/confusing_with_statement.txt b/pylint/test/functional/confusing_with_statement.txt
new file mode 100644
index 000000000..750ad714f
--- /dev/null
+++ b/pylint/test/functional/confusing_with_statement.txt
@@ -0,0 +1 @@
+confusing-with-statement:26::Following "as" with another context manager looks like a tuple.