Handle more corner cases in iterable/mapping checker

5 files changed, 109 insertions, 33 deletions

diff --git a/pylint/checkers/typecheck.py b/pylint/checkers/typecheck.py
index d54c5d3..83d9742 100644
--- a/pylint/checkers/typecheck.py
+++ b/pylint/checkers/typecheck.py
@@ -90,7 +90,13 @@ def _is_comprehension(node):
     return isinstance(node, comprehensions)
 
 
+def _is_class_def(node):
+    return isinstance(node, astroid.ClassDef)
+
+
 def _is_iterable(value):
+    if _is_class_def(value):
+        return False
     try:
         value.getattr('__iter__')
         return True
@@ -106,6 +112,8 @@ def _is_iterable(value):
 
 
 def _is_iterator(value):
+    if _is_class_def(value):
+        return False
     try:
         value.getattr('__iter__')
         if six.PY2:
@@ -118,6 +126,8 @@ def _is_iterator(value):
 
 
 def _is_old_style_iterator(value):
+    if _is_class_def(value):
+        return False
     try:
         value.getattr('__getitem__')
         value.getattr('__len__')
@@ -127,10 +137,12 @@ def _is_old_style_iterator(value):
 
 
 def _is_mapping(value):
+    if _is_class_def(value):
+        return False
     try:
         value.getattr('__getitem__')
         value.getattr('__iter__')
-        value.getattr('__len__')
+        value.getattr('keys')
         return True
     except astroid.NotFoundError:
         return False
@@ -873,7 +885,7 @@ class IterableChecker(BaseChecker):
 
     def _check_iterable(self, node, root_node):
         # for/set/dict-comprehensions can't be infered with astroid,
-        # so we check for them before the inference
+        # so we check for them before checking infered value
         if _is_comprehension(node):
             return
         infered = helpers.safe_infer(node)
diff --git a/pylint/test/functional/iterable_context.py b/pylint/test/functional/iterable_context.py
index 0571583..47400f9 100644
--- a/pylint/test/functional/iterable_context.py
+++ b/pylint/test/functional/iterable_context.py
@@ -2,32 +2,28 @@
 Checks that primitive values are not used in an
 iterating/mapping context.
 """
-# pylint: disable=missing-docstring,invalid-name,too-few-public-methods,no-init
+# pylint: disable=missing-docstring,invalid-name,too-few-public-methods,no-init,no-self-use
 from __future__ import print_function
 
-# for-statement
-for i in 42:  # [not-an-iterable]
-    pass
+# primitives
+numbers = [1, 2, 3]
 
-for i in True:  # [not-an-iterable]
+for i in numbers:
     pass
 
-for i in range(10):
+for i in iter(numbers):
     pass
 
-for i in ''.join(x ** 2 for x in range(10)):
+for i in "123":
     pass
 
-numbers = [1, 2, 3]
-inumbers = iter(numbers)
-
-for i in inumbers:
+for i in u"123":
     pass
 
-for i in "123":
+for i in b"123":
     pass
 
-for i in u"123":
+for i in bytearray(b"123"):
     pass
 
 for i in set(numbers):
@@ -39,6 +35,7 @@ for i in frozenset(numbers):
 for i in dict(a=1, b=2):
     pass
 
+# comprehensions
 for i in [x for x in range(10)]:
     pass
 
@@ -48,6 +45,7 @@ for i in {x for x in range(1, 100, 2)}:
 for i in {x: 10 - x for x in range(10)}:
     pass
 
+# generators
 def powers_of_two():
     k = 0
     while k < 10:
@@ -57,9 +55,25 @@ def powers_of_two():
 for i in powers_of_two():
     pass
 
+for i in powers_of_two:  # [not-an-iterable]
+    pass
+
+# check for custom iterators
+class A(object):
+    pass
+
+class B(object):
+    def __iter__(self):
+        return self
+
+    def __next__(self):
+        return 1
+
+    def next(self):
+        return 1
 
-# check for old-style iterator
 class C(object):
+    "old-style iterator"
     def __getitem__(self, k):
         if k > 10:
             raise IndexError
@@ -72,27 +86,32 @@ for i in C():
     print(i)
 
 
-# check for custom iterators
-class A:
-    pass
-
+def test(*args):
+    print(args)
 
-class B:
-    def __iter__(self):
-        return self
 
-    def __next__(self):
-        return 1
+test(*A())  # [not-an-iterable]
+test(*B())
+test(*B)  # [not-an-iterable]
+for i in A():  # [not-an-iterable]
+    pass
+for i in B():
+    pass
+for i in B:  # [not-an-iterable]
+    pass
 
-    def next(self):
-        return 1
+for i in range:  # [not-an-iterable]
+    pass
 
+# check that primitive non-iterable types are catched
+for i in True:  # [not-an-iterable]
+    pass
 
-def test(*args):
+for i in None:  # [not-an-iterable]
     pass
 
+for i in 8.5:  # [not-an-iterable]
+    pass
 
-test(*A())  # [not-an-iterable]
-test(*B())
-for i in A():  # [not-an-iterable]
+for i in 10:  # [not-an-iterable]
     pass
diff --git a/pylint/test/functional/iterable_context.txt b/pylint/test/functional/iterable_context.txt
index b618ffb..fbe1433 100644
--- a/pylint/test/functional/iterable_context.txt
+++ b/pylint/test/functional/iterable_context.txt
@@ -1,2 +1,10 @@
-not-an-iterable:9::Non-iterable value 42 is used in an iterating context
-not-an-iterable:12::Non-iterable value True is used in an iterating context
+not-an-iterable:58::Non-iterable value powers_of_two is used in an iterating context
+not-an-iterable:93::Non-iterable value A() is used in an iterating context
+not-an-iterable:95::Non-iterable value B is used in an iterating context
+not-an-iterable:96::Non-iterable value A() is used in an iterating context
+not-an-iterable:100::Non-iterable value B is used in an iterating context
+not-an-iterable:103::Non-iterable value range is used in an iterating context
+not-an-iterable:107::Non-iterable value True is used in an iterating context
+not-an-iterable:110::Non-iterable value None is used in an iterating context
+not-an-iterable:113::Non-iterable value 8.5 is used in an iterating context
+not-an-iterable:116::Non-iterable value 10 is used in an iterating context
diff --git a/pylint/test/functional/mapping_context.py b/pylint/test/functional/mapping_context.py
new file mode 100644
index 0000000..d9e0d2c
--- /dev/null
+++ b/pylint/test/functional/mapping_context.py
@@ -0,0 +1,36 @@
+"""
+Checks that only valid values are used in a mapping context.
+"""
+# pylint: disable=missing-docstring,invalid-name,too-few-public-methods
+from __future__ import print_function
+
+
+def test(**kwargs):
+    print(kwargs)
+
+
+# dictionary value/comprehension
+dict_value = dict(a=1, b=2, c=3)
+dict_comp = {chr(x): x for x in range(256)}
+test(**dict_value)
+test(**dict_comp)
+
+
+# in order to be used in kwargs custom mapping class should define
+# __iter__(), __getitem__(key) and keys().
+class CustomMapping(object):
+    def __init__(self):
+        self.data = dict(a=1, b=2, c=3, d=4, e=5)
+
+    def __iter__(self):
+        return iter(self.data)
+
+    def __getitem__(self, key):
+        return self.data[key]
+
+    def keys(self):
+        return self.data.keys()
+
+test(**CustomMapping())
+
+test(**CustomMapping)  # [not-a-mapping]
diff --git a/pylint/test/functional/mapping_context.txt b/pylint/test/functional/mapping_context.txt
new file mode 100644
index 0000000..f27ca97
--- /dev/null
+++ b/pylint/test/functional/mapping_context.txt
@@ -0,0 +1 @@
+not-a-mapping:36::Non-mapping value CustomMapping is used in a mapping context