Merged in dmand/pylint/fix-685 (pull request #294)

Make iterable checker skip more abstract classes

7 files changed, 220 insertions, 107 deletions

diff --git a/pylint/checkers/classes.py b/pylint/checkers/classes.py
index 86ccb19..c65e10c 100644
--- a/pylint/checkers/classes.py
+++ b/pylint/checkers/classes.py
@@ -34,7 +34,7 @@ from pylint.checkers.utils import (
     overrides_a_method, check_messages, is_attr_private,
     is_attr_protected, node_frame_class, is_builtin_object,
     decorated_with_property, unimplemented_abstract_methods,
-    decorated_with)
+    decorated_with, class_is_abstract)
 from pylint.utils import deprecated_option, get_global_option
 import six
 
@@ -102,15 +102,6 @@ def _called_in_methods(func, klass, methods):
                     return True
     return False
 
-def class_is_abstract(node):
-    """return true if the given class node should be considered as an abstract
-    class
-    """
-    for method in node.methods():
-        if method.parent.frame() is node:
-            if method.is_abstract(pass_is_abstract=False):
-                return True
-    return False
 
 def _is_attribute_property(name, klass):
     """ Check if the given attribute *name* is a property
diff --git a/pylint/checkers/typecheck.py b/pylint/checkers/typecheck.py
index e9fddbd..7baaaf6 100644
--- a/pylint/checkers/typecheck.py
+++ b/pylint/checkers/typecheck.py
@@ -35,7 +35,7 @@ from pylint.interfaces import IAstroidChecker, INFERENCE, INFERENCE_FAILURE
 from pylint.checkers import BaseChecker
 from pylint.checkers.utils import (
     is_super, check_messages, decorated_with_property,
-    decorated_with, node_ignores_exception)
+    decorated_with, node_ignores_exception, class_is_abstract)
 from pylint import utils
 
 
@@ -99,36 +99,91 @@ def _hasattr(value, attr):
 def _is_comprehension(node):
     comprehensions = (astroid.ListComp,
                       astroid.SetComp,
-                      astroid.DictComp)
+                      astroid.DictComp,
+                      astroid.GeneratorExp)
     return isinstance(node, comprehensions)
 
-
-def _is_iterable(value):
-    # '__iter__' is for standard iterables
-    # '__getitem__' is for strings and other old-style iterables
-    return _hasattr(value, ITER_METHOD) or _hasattr(value, GETITEM_METHOD)
-
-
-def _is_iterator(value):
-    return _hasattr(value, NEXT_METHOD) and _hasattr(value, ITER_METHOD)
-
-
-def _is_mapping(value):
+def _supports_mapping_protocol(value):
     return _hasattr(value, GETITEM_METHOD) and _hasattr(value, KEYS_METHOD)
 
-def _supports_membership_test(value):
+def _supports_membership_test_protocol(value):
     return _hasattr(value, CONTAINS_METHOD)
 
+def _supports_iteration_protocol(value):
+    return _hasattr(value, ITER_METHOD) or _hasattr(value, GETITEM_METHOD)
+
+def _is_abstract_class_name(name):
+    lname = name.lower()
+    is_mixin = lname.endswith('mixin')
+    is_abstract = lname.startswith('abstract')
+    is_base = lname.startswith('base') or lname.endswith('base')
+    return is_mixin or is_abstract or is_base
 
-def _is_inside_mixin_declaration(node):
+def _is_inside_abstract_class(node):
     while node is not None:
         if isinstance(node, astroid.ClassDef):
+            if class_is_abstract(node):
+                return True
             name = getattr(node, 'name', None)
-            if name is not None and name.lower().endswith("mixin"):
+            if name is not None and _is_abstract_class_name(name):
                 return True
         node = node.parent
     return False
 
+def _should_skip_iterable_check(node):
+    if _is_inside_abstract_class(node):
+        return True
+    infered = helpers.safe_infer(node)
+    if infered is None or infered is astroid.YES:
+        return True
+    if isinstance(infered, (astroid.ClassDef, astroid.Instance)):
+        if not helpers.has_known_bases(infered):
+            return True
+    return False
+
+def _is_iterable(node):
+    # for/set/dict-comprehensions can't be infered with astroid
+    # so we have to check for them explicitly
+    if _is_comprehension(node):
+        return True
+    infered = helpers.safe_infer(node)
+    if isinstance(infered, astroid.ClassDef):
+        # classobj can only be iterable if it has an iterable metaclass
+        meta = infered.metaclass()
+        if meta is not None:
+            if _supports_iteration_protocol(meta):
+                return True
+    if isinstance(infered, astroid.Instance):
+        if _supports_iteration_protocol(infered):
+            return True
+    return False
+
+def _is_mapping(node):
+    if isinstance(node, astroid.DictComp):
+        return True
+    infered = helpers.safe_infer(node)
+    if isinstance(infered, astroid.ClassDef):
+        # classobj can only be iterable if it has an iterable metaclass
+        meta = infered.metaclass()
+        if meta is not None:
+            if _supports_mapping_protocol(meta):
+                return True
+    if isinstance(infered, astroid.Instance):
+        if _supports_mapping_protocol(infered):
+            return True
+    return False
+
+def _supports_membership_test(node):
+    infered = helpers.safe_infer(node)
+    if isinstance(infered, astroid.ClassDef):
+        meta = infered.metaclass()
+        if meta is not None and _supports_membership_test_protocol(meta):
+            return True
+    if isinstance(infered, astroid.Instance):
+        if _supports_membership_test_protocol(infered):
+            return True
+    return _is_iterable(node)
+
 
 MSGS = {
     'E1101': ('%s %r has no %r member',
@@ -841,33 +896,13 @@ accessed. Python regular expressions are accepted.'}
                              args=str(error), node=node)
 
     def _check_membership_test(self, node):
-        # instance supports membership test in either of those cases:
-        # 1. instance defines __contains__ method
-        # 2. instance is iterable (defines __iter__ or __getitem__)
-        if _is_comprehension(node) or _is_inside_mixin_declaration(node):
+        # value supports membership test in either of those cases:
+        # 1. value defines __contains__ method
+        # 2. value is iterable (defines __iter__ or __getitem__)
+        if _should_skip_iterable_check(node):
             return
-
-        infered = helpers.safe_infer(node)
-        if infered is None or infered is astroid.YES:
+        if _supports_membership_test(node):
             return
-
-        # classes can be iterables/containers too
-        if isinstance(infered, astroid.ClassDef):
-            if not helpers.has_known_bases(infered):
-                return
-            meta = infered.metaclass()
-            if meta is not None:
-                if _supports_membership_test(meta):
-                    return
-                if _is_iterable(meta):
-                    return
-
-        if isinstance(infered, astroid.Instance):
-            if not helpers.has_known_bases(infered):
-                return
-            if _supports_membership_test(infered) or _is_iterable(infered):
-                return
-
         self.add_message('unsupported-membership-test',
                          args=node.as_string(),
                          node=node)
@@ -907,57 +942,19 @@ class IterableChecker(BaseChecker):
            }
 
     def _check_iterable(self, node, root_node):
-        # for/set/dict-comprehensions can't be infered with astroid
-        # so we have to check for them explicitly
-        if _is_comprehension(node) or _is_inside_mixin_declaration(node):
+        if _should_skip_iterable_check(node):
             return
-
-        infered = helpers.safe_infer(node)
-        if infered is None or infered is astroid.YES:
+        if _is_iterable(node):
             return
-
-        if isinstance(infered, astroid.ClassDef):
-            if not helpers.has_known_bases(infered):
-                return
-            # classobj can only be iterable if it has an iterable metaclass
-            meta = infered.metaclass()
-            if meta is not None:
-                if _is_iterable(meta):
-                    return
-                if _is_iterator(meta):
-                    return
-
-        if isinstance(infered, astroid.Instance):
-            if not helpers.has_known_bases(infered):
-                return
-            if _is_iterable(infered) or _is_iterator(infered):
-                return
-
         self.add_message('not-an-iterable',
                          args=node.as_string(),
                          node=root_node)
 
     def _check_mapping(self, node, root_node):
-        if isinstance(node, astroid.DictComp) or _is_inside_mixin_declaration(node):
+        if _should_skip_iterable_check(node):
             return
-
-        infered = helpers.safe_infer(node)
-        if infered is None or infered is astroid.YES:
+        if _is_mapping(node):
             return
-
-        if isinstance(infered, astroid.ClassDef):
-            if not helpers.has_known_bases(infered):
-                return
-            meta = infered.metaclass()
-            if meta is not None and _is_mapping(meta):
-                return
-
-        if isinstance(infered, astroid.Instance):
-            if not helpers.has_known_bases(infered):
-                return
-            if _is_mapping(infered):
-                return
-
         self.add_message('not-a-mapping',
                          args=node.as_string(),
                          node=root_node)
diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py
index 443e32d..8873d0e 100644
--- a/pylint/checkers/utils.py
+++ b/pylint/checkers/utils.py
@@ -573,6 +573,17 @@ def node_ignores_exception(node, exception):
     return False
 
 
+def class_is_abstract(node):
+    """return true if the given class node should be considered as an abstract
+    class
+    """
+    for method in node.methods():
+        if method.parent.frame() is node:
+            if method.is_abstract(pass_is_abstract=False):
+                return True
+    return False
+
+
 # TODO(cpopa): deprecate these or leave them as aliases?
 safe_infer = astroid.helpers.safe_infer
 has_known_bases = astroid.helpers.has_known_bases
diff --git a/pylint/test/functional/iterable_context.py b/pylint/test/functional/iterable_context.py
index 8dfcbbe..fa4e617 100644
--- a/pylint/test/functional/iterable_context.py
+++ b/pylint/test/functional/iterable_context.py
@@ -127,7 +127,7 @@ m = MyClass()
 for i in m:
     print(i)
 
-# skip checks if statement is inside mixin class
+# skip checks if statement is inside mixin/base/abstract class
 class ManagedAccessViewMixin(object):
     access_requirements = None
 
@@ -137,5 +137,43 @@ class ManagedAccessViewMixin(object):
     def dispatch(self, *_args, **_kwargs):
         klasses = self.get_access_requirements()
 
+        # no error should be emitted here
         for requirement in klasses:
             print(requirement)
+
+class BaseType(object):
+    valid_values = None
+
+    def validate(self, value):
+        if self.valid_values is None:
+            return True
+        else:
+            # error should not be emitted here
+            for v in self.valid_values:
+                if value == v:
+                    return True
+            return False
+
+class AbstractUrlMarkManager(object):
+    def __init__(self):
+        self._lineparser = None
+        self._init_lineparser()
+        # error should not be emitted here
+        for line in self._lineparser:
+            print(line)
+
+    def _init_lineparser(self):
+        raise NotImplementedError
+
+# class is not named as abstract
+# but still is deduceably abstract
+class UrlMarkManager(object):
+    def __init__(self):
+        self._lineparser = None
+        self._init_lineparser()
+        # error should not be emitted here
+        for line in self._lineparser:
+            print(line)
+
+    def _init_lineparser(self):
+        raise NotImplementedError
diff --git a/pylint/test/functional/mapping_context.py b/pylint/test/functional/mapping_context.py
index cfab8dc..72457b8 100644
--- a/pylint/test/functional/mapping_context.py
+++ b/pylint/test/functional/mapping_context.py
@@ -36,7 +36,7 @@ class NotMapping(object):
 
 test(**NotMapping())  # [not-a-mapping]
 
-# skip checks if statement is inside mixin class
+# skip checks if statement is inside mixin/base/abstract class
 class SomeMixin(object):
     kwargs = None
 
@@ -50,6 +50,44 @@ class SomeMixin(object):
         kws = self.get_kwargs()
         self.run(**kws)
 
+class AbstractThing(object):
+    kwargs = None
+
+    def get_kwargs(self):
+        return self.kwargs
+
+    def run(self, **kwargs):
+        print(kwargs)
+
+    def dispatch(self):
+        kws = self.get_kwargs()
+        self.run(**kws)
+
+class BaseThing(object):
+    kwargs = None
+
+    def get_kwargs(self):
+        return self.kwargs
+
+    def run(self, **kwargs):
+        print(kwargs)
+
+    def dispatch(self):
+        kws = self.get_kwargs()
+        self.run(**kws)
+
+# abstract class
+class Thing(object):
+    def get_kwargs(self):
+        raise NotImplementedError
+
+    def run(self, **kwargs):
+        print(kwargs)
+
+    def dispatch(self):
+        kwargs = self.get_kwargs()
+        self.run(**kwargs)
+
 # skip uninferable instances
 from some_missing_module import Mapping
 
diff --git a/pylint/test/functional/membership_protocol.py b/pylint/test/functional/membership_protocol.py
index 7b3a46f..0fd4886 100644
--- a/pylint/test/functional/membership_protocol.py
+++ b/pylint/test/functional/membership_protocol.py
@@ -5,9 +5,9 @@
 1 in {'a': 1, 'b': 2}
 1 in {1, 2, 3}
 1 in (1, 2, 3)
-1 in "123"
-1 in u"123"
-1 in bytearray(b"123")
+'1' in "123"
+'1' in u"123"
+'1' in bytearray(b"123")
 1 in frozenset([1, 2, 3])
 
 # comprehensions
@@ -59,7 +59,7 @@ class MaybeIterable(ImportedClass):
 
 10 in MaybeIterable()
 
-# do not emit warning inside mixins
+# do not emit warning inside mixins/abstract/base classes
 class UsefulMixin(object):
     stuff = None
 
@@ -71,6 +71,44 @@ class UsefulMixin(object):
         if thing in stuff:
             pass
 
+class BaseThing(object):
+    valid_values = None
+
+    def validate(self, value):
+        if self.valid_values is None:
+            return True
+        else:
+            # error should not be emitted here
+            return value in self.valid_values
+
+class AbstractThing(object):
+    valid_values = None
+
+    def validate(self, value):
+        if self.valid_values is None:
+            return True
+        else:
+            # error should not be emitted here
+            return value in self.valid_values
+
+# class is not named as abstract
+# but still is deduceably abstract
+class Thing(object):
+    valid_values = None
+
+    def __init__(self):
+        self._init_values()
+
+    def validate(self, value):
+        if self.valid_values is None:
+            return True
+        else:
+            # error should not be emitted here
+            return value in self.valid_values
+
+    def _init_values(self):
+        raise NotImplementedError
+
 # error cases
 42 in 42  # [unsupported-membership-test]
 42 not in None  # [unsupported-membership-test]
diff --git a/pylint/test/functional/membership_protocol.txt b/pylint/test/functional/membership_protocol.txt
index 6e9bd8e..edb2227 100644
--- a/pylint/test/functional/membership_protocol.txt
+++ b/pylint/test/functional/membership_protocol.txt
@@ -1,7 +1,7 @@
-unsupported-membership-test:75::Value '42' doesn't support membership test
-unsupported-membership-test:76::Value 'None' doesn't support membership test
-unsupported-membership-test:77::Value '8.5' doesn't support membership test
-unsupported-membership-test:82::Value 'EmptyClass()' doesn't support membership test
-unsupported-membership-test:83::Value 'EmptyClass' doesn't support membership test
-unsupported-membership-test:84::Value 'count' doesn't support membership test
-unsupported-membership-test:85::Value 'range' doesn't support membership test
+unsupported-membership-test:113::Value '42' doesn't support membership test
+unsupported-membership-test:114::Value 'None' doesn't support membership test
+unsupported-membership-test:115::Value '8.5' doesn't support membership test
+unsupported-membership-test:120::Value 'EmptyClass()' doesn't support membership test
+unsupported-membership-test:121::Value 'EmptyClass' doesn't support membership test
+unsupported-membership-test:122::Value 'count' doesn't support membership test
+unsupported-membership-test:123::Value 'range' doesn't support membership test