Add check for swapping variables with tuples(#1922) (#1929)

CONTRIBUTORS.txt

@@ -160,3 +160,4 @@ Order doesn't matter (not that much, at least ;)
 
 * Tobias Hernstig: contributor
 
+* Konstantin Manna: contributor

ChangeLog

@@ -4,6 +4,10 @@ Pylint's ChangeLog
 What's New in Pylint 2.0?
 =========================
 
+    * Add a check `consider-swap-variables` for swapping variables with tuple unpacking
+
+      Close #1922
+
     * Don't crash on invalid strings when checking for `logging-format-interpolation`
 
       Close #1944
@@ -141,7 +145,7 @@ Release date: 2017-12-15
 
       Close #1713
 
-    * Fixing u'' string in superfluous-parens message 
+    * Fixing u'' string in superfluous-parens message
 
       Close #1420
 
@@ -163,7 +167,7 @@ Release date: 2017-12-15
       This may lead to args list getting modified if keyword argument's value
       is not provided in the function call assuming it will take default value
       provided in the definition.
-	
+
     * The `invalid-name` check contains the name of the template that caused the failure
 
       Close #1176
@@ -215,7 +219,7 @@ Release date: 2017-12-15
     * Added a new key-value pair in json output. The key is ``message-id``
       and the value is the message id.
       Close #1512
-		
+
     * Added a new Python 3.0 check for raising a StopIteration inside a generator.
       The check about raising a StopIteration inside a generator is also valid if the exception
       raised inherit from StopIteration.
@@ -284,7 +288,7 @@ Release date: 2017-12-15
       Close #1681
 
     * Fix ``line-too-long`` message deactivated by wrong disable directive.
-      The directive ``disable=fixme`` doesn't deactivate anymore the emission 
+      The directive ``disable=fixme`` doesn't deactivate anymore the emission
       of ``line-too-long`` message for long commented lines.
       Close #1741
 
@@ -295,7 +299,7 @@ Release date: 2017-12-15
     * Fix the wrong scope of the ``disable=`` directive after a commented line.
       For example when a ``disable=line-too-long`` directive is at the end of
       a long commented line, it no longer disables the emission of ``line-too-long``
-      message for lines that follow. 
+      message for lines that follow.
       Close #1742
 
 What's New in Pylint 1.7.1?

doc/whatsnew/2.0.rst

@@ -20,6 +20,26 @@ New checkers
   deactivated by id instead of symbol.
   The use of symbol is more explicit and easier to remind.
 
+* A new check was added, ``consider-swap-variables``.
+
+  This refactoring message is emitted when using a temporary variable in order
+  to swap the values of two variables instead of the shorter, more idiomatic
+  approach with tuple-unpacking.
+
+  Instead of a temporary variable, the one-line syntax with commas should be used.
+
+  See http://docs.python-guide.org/en/latest/writing/style/ or
+  http://python.net/~goodger/projects/pycon/2007/idiomatic/handout.html#swap-values
+  for details.
+
+  .. code-block:: python
+
+     temp = a  # the wrong way
+     a = b
+     b = temp
+
+     a, b = b, a  # the right way
+
 Other Changes
 =============
 
@@ -29,15 +49,15 @@ Other Changes
 * Fix a false positive ``inconsistent-return-statements`` message when
   `while` loop are used.
 
-* Fix emission of false positive ``no-member`` message for class with 
+* Fix emission of false positive ``no-member`` message for class with
   "private" attributes whose name is mangled.
 
 * Fix ``unused-argument`` false positives with overshadowed variable in dictionary comprehension.
 
 * Fixing false positive ``inconsistent-return-statements`` when
   never returning functions are used (i.e such as sys.exit).
 
-* Fix false positive ``inconsistent-return-statements`` message when a 
+* Fix false positive ``inconsistent-return-statements`` message when a
   function is defined under an if statement.
 
 * Fix false positive ``inconsistent-return-statements`` message by

pylint/checkers/refactoring.py

@@ -130,6 +130,12 @@ class RefactoringChecker(checkers.BaseTokenChecker):
                   'at the end of function or method definition. This statement can safely be '
                   'removed because Python will implicitly return None'
                  ),
+        'R1712': ('Consider using tuple unpacking for swapping variables',
+                  'consider-swap-variables',
+                  'You do not have to use a temporary variable in order to '
+                  'swap variables. Using "tuple unpacking" to directly swap '
+                  'variables makes the intention more clear.'
+                 ),
     }
     options = (('max-nested-blocks',
                 {'default': 5, 'type': 'int', 'metavar': '<int>',
@@ -157,6 +163,7 @@ def _init(self):
         self._nested_blocks = []
         self._elifs = []
         self._nested_blocks_msg = None
+        self._reported_swap_nodes = set()
 
     def open(self):
         # do this in open since config not fully initialized in __init__
@@ -470,8 +477,35 @@ def visit_boolop(self, node):
                              node=node,
                              args=(duplicated_name, ', '.join(names)))
 
-    @utils.check_messages('simplify-boolean-expression', 'consider-using-ternary')
+    @staticmethod
+    def _is_simple_assignment(node):
+        return (isinstance(node, astroid.Assign)
+                and len(node.targets) == 1
+                and isinstance(node.targets[0], astroid.node_classes.AssignName)
+                and isinstance(node.value, astroid.node_classes.Name))
+
+    def _check_swap_variables(self, node):
+        if not node.next_sibling() or not node.next_sibling().next_sibling():
+            return
+        assignments = [
+            node, node.next_sibling(), node.next_sibling().next_sibling()
+        ]
+        if not all(self._is_simple_assignment(node) for node in assignments):
+            return
+        if any(node in self._reported_swap_nodes for node in assignments):
+            return
+        left = [node.targets[0].name for node in assignments]
+        right = [node.value.name for node in assignments]
+        if left[0] == right[-1] and left[1:] == right[:-1]:
+            self._reported_swap_nodes.update(assignments)
+            message = 'consider-swap-variables'
+            self.add_message(message, node=node)
+
+    @utils.check_messages('simplify-boolean-expression',
+                          'consider-using-ternary',
+                          'consider-swap-variables')
     def visit_assign(self, node):
+        self._check_swap_variables(node)
         if self._is_and_or_ternary(node.value):
             cond, truth_value, false_value = self._and_or_ternary_arguments(node.value)
         elif self._is_seq_based_ternary(node.value):

pylint/test/functional/consider_swap_variables.py

@@ -0,0 +1,25 @@
+# pylint: disable=missing-docstring,invalid-name,using-constant-test
+
+a, b, c, d = 'a b c d'.split()
+
+temp = a # [consider-swap-variables]
+a = b
+b = temp
+
+temp = a  # only simple swaps are reported
+a = b
+if True:
+    b = a
+
+temp = a  # this is no swap
+a = b
+b = a
+
+temp = a, b  # complex swaps are ignored
+a, b = c, d
+c, d = temp
+
+temp = a # [consider-swap-variables]
+a = b  # longer swap circles are only reported once
+b = temp
+temp = a

pylint/test/functional/consider_swap_variables.txt

@@ -0,0 +1,2 @@
+consider-swap-variables:5::Consider using tuple unpacking for swapping variables
+consider-swap-variables:22::Consider using tuple unpacking for swapping variables