Merge heads.

5 files changed, 168 insertions, 46 deletions

diff --git a/ChangeLog b/ChangeLog
index 5e21556..7f44f14 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -365,6 +365,10 @@ ChangeLog for Pylint
       encountered in languages with variabile assignments in conditional
       statements.
 
+    * Add a new convention message, 'consider-using-enumerate', which is
+      emitted when code that uses `range` and `len` for iterating is encountered.
+      Closes issue #684.
+
 
 
 2015-03-14 -- 1.4.3
diff --git a/pylint/checkers/base.py b/pylint/checkers/base.py
index c9e8b75..c6f6ac5 100644
--- a/pylint/checkers/base.py
+++ b/pylint/checkers/base.py
@@ -62,6 +62,14 @@ REVERSED_PROTOCOL_METHOD = '__reversed__'
 SEQUENCE_PROTOCOL_METHODS = ('__getitem__', '__len__')
 REVERSED_METHODS = (SEQUENCE_PROTOCOL_METHODS,
                     (REVERSED_PROTOCOL_METHOD, ))
+TYPECHECK_COMPARISON_OPERATORS = frozenset(('is', 'is not', '==',
+                                            '!=', 'in', 'not in'))
+LITERAL_NODE_TYPES = (astroid.Const, astroid.Dict, astroid.List, astroid.Set)
+UNITTEST_CASE = 'unittest.case'
+if sys.version_info >= (3, 0):
+    TYPE_QNAME = 'builtins.type'
+else:
+    TYPE_QNAME = '__builtin__.type'
 
 PY33 = sys.version_info >= (3, 3)
 PY3K = sys.version_info >= (3, 0)
@@ -1455,6 +1463,82 @@ class LambdaForComprehensionChecker(_BasicChecker):
             self.add_message('deprecated-lambda', node=node)
 
 
+class RecommandationChecker(_BasicChecker):
+    msgs = {'C0200': ('Consider using enumerate instead of iterating with range and len',
+                      'consider-using-enumerate',
+                      'Emitted when code that iterates with range and len is '
+                      'encountered. Such code can be simplified by using the '
+                      'enumerate builtin.')
+           }
+
+    @staticmethod
+    def _is_builtin(node, function):
+        inferred = helpers.safe_infer(node)
+        if not inferred:
+             return False
+        return is_builtin_object(inferred) and inferred.name == function
+
+    @check_messages('consider-using-enumerate')
+    def visit_for(self, node):
+        """Emit a convention whenever range and len are used for indexing."""
+        # Verify that we have a `range(len(...))` call and that the object
+        # which is iterated is used as a subscript in the body of the for.
+
+        # Is it a proper range call?
+        if not isinstance(node.iter, astroid.Call):
+            return
+        if not self._is_builtin(node.iter.func, 'range'):
+            return
+        if len(node.iter.args) != 1:
+            return
+
+        # Is it a proper len call?
+        if not isinstance(node.iter.args[0], astroid.Call):
+            return
+        second_func = node.iter.args[0].func
+        if not self._is_builtin(second_func, 'len'):
+            return
+        len_args = node.iter.args[0].args
+        if not len_args or len(len_args) != 1:
+            return
+        iterating_object = len_args[0]
+        if not isinstance(iterating_object, astroid.Name):
+            return
+
+        # Verify that the body of the for loop uses a subscript
+        # with the object that was iterated. This uses some heuristics
+        # in order to make sure that the same object is used in the
+        # for body.
+        for child in node.body:
+            for subscript in child.nodes_of_class(astroid.Subscript):
+                if not isinstance(subscript.value, astroid.Name):
+                    continue
+                if not isinstance(subscript.slice, astroid.Index):
+                    continue
+                if not isinstance(subscript.slice.value, astroid.Name):
+                    continue
+                if subscript.slice.value.name != node.target.name:
+                    continue
+                if iterating_object.name != subscript.value.name:
+                    continue                
+                if subscript.value.scope() != node.scope():
+                    # Ignore this subscript if it's not in the same
+                    # scope. This means that in the body of the for
+                    # loop, another scope was created, where the same
+                    # name for the iterating object was used.
+                    continue    
+                self.add_message('consider-using-enumerate', node=node)
+                return            
+
+
+def _is_one_arg_pos_call(call):
+    """Is this a call with exactly 1 argument,
+    where that argument is positional?
+    """
+    return (isinstance(call, astroid.Call)
+            and len(call.args) == 1 and not call.keywords)
+
+
 class ComparisonChecker(_BasicChecker):
     """Checks for comparisons
 
@@ -1472,6 +1556,13 @@ class ComparisonChecker(_BasicChecker):
                       'Used when the constant is placed on the left side'
                       'of a comparison. It is usually clearer in intent to '
                       'place it in the right hand side of the comparison.'),
+            'C0123': ('Using type() instead of isinstance() for a typecheck.',
+                      'unidiomatic-typecheck',
+                      'The idiomatic way to perform an explicit typecheck in '
+                      'Python is to use isinstance(x, Y) rather than '
+                      'type(x) == Y, type(x) is Y. Though there are unusual '
+                      'situations where these give different results.',
+                      {'old_names': [('W0154', 'unidiomatic-typecheck')]}),
            }
 
     def _check_singleton_comparison(self, singleton, root_node):
@@ -1498,8 +1589,10 @@ class ComparisonChecker(_BasicChecker):
         self.add_message('misplaced-comparison-constant', node=node,
                          args=(suggestion,))
 
-    @check_messages('singleton-comparison', 'misplaced-comparison-constant')
+    @check_messages('singleton-comparison', 'misplaced-comparison-constant',
+                    'unidiomatic-typecheck')
     def visit_compare(self, node):
+        self._check_unidiomatic_typecheck(node)
         # NOTE: this checker only works with binary comparisons like 'x == 42'
         # but not 'x == y == 42'
         if len(node.ops) != 1:
@@ -1516,6 +1609,31 @@ class ComparisonChecker(_BasicChecker):
             elif isinstance(right, astroid.Const):
                 self._check_singleton_comparison(right, node)
 
+    def _check_unidiomatic_typecheck(self, node):
+        operator, right = node.ops[0]
+        if operator in TYPECHECK_COMPARISON_OPERATORS:
+            left = node.left
+            if _is_one_arg_pos_call(left):
+                self._check_type_x_is_y(node, left, operator, right)
+
+    def _check_type_x_is_y(self, node, left, operator, right):
+        """Check for expressions like type(x) == Y."""
+        left_func = helpers.safe_infer(left.func)
+        if not (isinstance(left_func, astroid.ClassDef)
+                and left_func.qname() == TYPE_QNAME):
+            return
+
+        if operator in ('is', 'is not') and _is_one_arg_pos_call(right):
+            right_func = helpers.safe_infer(right.func)
+            if (isinstance(right_func, astroid.ClassDef)
+                    and right_func.qname() == TYPE_QNAME):
+                # type(x) == type(a)
+                right_arg = helpers.safe_infer(right.args[0])
+                if not isinstance(right_arg, LITERAL_NODE_TYPES):
+                    # not e.g. type(x) == type([])
+                    return
+        self.add_message('unidiomatic-typecheck', node=node)
+
 
 def register(linter):
     """required method to auto register this checker"""
@@ -1526,3 +1644,4 @@ def register(linter):
     linter.register_checker(PassChecker(linter))
     linter.register_checker(LambdaForComprehensionChecker(linter))
     linter.register_checker(ComparisonChecker(linter))
+    linter.register_checker(RecommandationChecker(linter))
diff --git a/pylint/checkers/stdlib.py b/pylint/checkers/stdlib.py
index 7df71b2..f583215 100644
--- a/pylint/checkers/stdlib.py
+++ b/pylint/checkers/stdlib.py
@@ -27,17 +27,12 @@ from pylint.checkers import BaseChecker
 from pylint.checkers import utils
 
 
-TYPECHECK_COMPARISON_OPERATORS = frozenset(('is', 'is not', '==',
-                                            '!=', 'in', 'not in'))
-LITERAL_NODE_TYPES = (astroid.Const, astroid.Dict, astroid.List, astroid.Set)
 OPEN_FILES = {'open', 'file'}
 UNITTEST_CASE = 'unittest.case'
 if sys.version_info >= (3, 0):
     OPEN_MODULE = '_io'
-    TYPE_QNAME = 'builtins.type'
 else:
     OPEN_MODULE = '__builtin__'
-    TYPE_QNAME = '__builtin__.type'
 
 
 def _check_mode_str(mode):
@@ -82,14 +77,6 @@ def _check_mode_str(mode):
     return True
 
 
-def _is_one_arg_pos_call(call):
-    """Is this a call with exactly 1 argument,
-    where that argument is positional?
-    """
-    return (isinstance(call, astroid.Call)
-            and len(call.args) == 1 and not call.keywords)
-
-
 class StdlibChecker(BaseChecker):
     __implements__ = (IAstroidChecker,)
     name = 'stdlib'
@@ -114,12 +101,6 @@ class StdlibChecker(BaseChecker):
                   'a condition. If a constant is passed as parameter, that '
                   'condition will be always true. In this case a warning '
                   'should be emitted.'),
-        'W1504': ('Using type() instead of isinstance() for a typecheck.',
-                  'unidiomatic-typecheck',
-                  'The idiomatic way to perform an explicit typecheck in '
-                  'Python is to use isinstance(x, Y) rather than '
-                  'type(x) == Y, type(x) is Y. Though there are unusual '
-                  'situations where these give different results.'),
         'W1505': ('Using deprecated method %s()',
                   'deprecated-method',
                   'The method is marked as deprecated and will be removed in '
@@ -231,14 +212,6 @@ class StdlibChecker(BaseChecker):
         for value in node.values:
             self._check_datetime(value)
 
-    @utils.check_messages('unidiomatic-typecheck')
-    def visit_compare(self, node):
-        operator, right = node.ops[0]
-        if operator in TYPECHECK_COMPARISON_OPERATORS:
-            left = node.left
-            if _is_one_arg_pos_call(left):
-                self._check_type_x_is_y(node, left, operator, right)
-
     def _check_deprecated_method(self, node, infer):
         py_vers = sys.version_info[0]
 
@@ -296,24 +269,6 @@ class StdlibChecker(BaseChecker):
                 self.add_message('bad-open-mode', node=node,
                                  args=mode_arg.value)
 
-    def _check_type_x_is_y(self, node, left, operator, right):
-        """Check for expressions like type(x) == Y."""
-        left_func = helpers.safe_infer(left.func)
-        if not (isinstance(left_func, astroid.ClassDef)
-                and left_func.qname() == TYPE_QNAME):
-            return
-
-        if operator in ('is', 'is not') and _is_one_arg_pos_call(right):
-            right_func = helpers.safe_infer(right.func)
-            if (isinstance(right_func, astroid.ClassDef)
-                    and right_func.qname() == TYPE_QNAME):
-                # type(x) == type(a)
-                right_arg = helpers.safe_infer(right.args[0])
-                if not isinstance(right_arg, LITERAL_NODE_TYPES):
-                    # not e.g. type(x) == type([])
-                    return
-        self.add_message('unidiomatic-typecheck', node=node)
-
 
 def register(linter):
     """required method to auto register this checker """
diff --git a/pylint/test/functional/consider_using_enumerate.py b/pylint/test/functional/consider_using_enumerate.py
new file mode 100644
index 0000000..f906da0
--- /dev/null
+++ b/pylint/test/functional/consider_using_enumerate.py
@@ -0,0 +1,43 @@
+"""Emit a message for iteration through range and len is encountered."""

+

+# pylint: disable=missing-docstring, import-error

+

+def bad():

+    iterable = [1, 2, 3]

+    for obj in range(len(iterable)): # [consider-using-enumerate]

+        yield iterable[obj]

+

+

+def good():

+    iterable = other_obj = [1, 2, 3]

+    total = 0

+    for obj in range(len(iterable)):

+        total += obj

+        yield total

+        yield iterable[obj + 1: 2]

+        yield iterable[len(obj)]

+    for obj in iterable:

+        yield iterable[obj - 1]

+

+    for index, obj in enumerate(iterable):

+        yield iterable[index]

+    for index in range(0, 10):

+        yield iterable[index + 1]

+    for index in range(10):

+        yield iterable[index]

+    for index in range(len([1, 2, 3, 4])):

+        yield index

+    for index in range(len(iterable)):

+        yield [1, 2, 3][index]

+        yield len([1, 2, 3])

+    for index in range(len(iterable)):

+        yield other_obj[index]

+

+    from unknown import unknown

+    for index in range(unknown(iterable)):

+        yield iterable[index]

+

+    for index in range(len(iterable)):

+        def test(iterable):

+            return iterable[index]

+        yield test([1, 2, 3])

diff --git a/pylint/test/functional/consider_using_enumerate.txt b/pylint/test/functional/consider_using_enumerate.txt
new file mode 100644
index 0000000..36cd55d
--- /dev/null
+++ b/pylint/test/functional/consider_using_enumerate.txt
@@ -0,0 +1 @@
+consider-using-enumerate:7:bad:Consider using enumerate instead of iterating with range and len