Merge pull request #2146 from RasmusWL/python-improve-iter-returns-non-iterator

Python: improve py/iter-returns-non-iterator

Author: taus-semmle

change-notes/1.23/analysis-python.md

@@ -19,4 +19,4 @@
 | **Query**                  | **Expected impact**    | **Change** |
 |----------------------------|------------------------|------------|
 | Unreachable code | Fewer false positives | Analysis now accounts for uses of `contextlib.suppress` to suppress exceptions. |
-
+| `__iter__` method returns a non-iterator | Better alert message | Alert now highlights which class is expected to be an iterator. |

python/ql/src/Functions/IterReturnsNonIterator.py

@@ -1,18 +1,33 @@
 class MyRange(object):
     def __init__(self, low, high):
-        self.current = low
+        self.low = low
         self.high = high
 
     def __iter__(self):
-        return self
+        return MyRangeIterator(self.low, self.high)
 
-#Fixed version
-class MyRange(object):
-    def __init__(self, low, high):
+    def skip(self, to_skip):
+        return MyRangeIterator(self.low, self.high, to_skip)
+
+class MyRangeIterator(object):
+    def __init__(self, low, high, skip=None):
         self.current = low
         self.high = high
+        self.skip = skip
 
-    def __iter__(self):
-        while self.current < self.high:
-            yield self.current
-            self.current += 1
+    def __next__(self):
+        if self.current >= self.high:
+            raise StopIteration
+        to_return = self.current
+        self.current += 1
+        if self.skip and to_return in self.skip:
+            return self.__next__()
+        return to_return
+
+    # Problem is fixed by uncommenting these lines
+    # def __iter__(self):
+    #     return self
+
+my_range = MyRange(0,10)
+x = sum(my_range) # x = 45
+y = sum(my_range.skip({6,9})) # TypeError: 'MyRangeIterator' object is not iterable

python/ql/src/Functions/IterReturnsNonIterator.qhelp

@@ -3,35 +3,33 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>The <code>__iter__</code> method of a class should return an iterator.
+<p>The <code>__iter__</code> method of a class should always return an iterator.</p>
 
-Iteration in Python relies on this behavior and attempting to iterate over an
-instance of a class with an incorrect <code>__iter__</code> method will raise a TypeError.
-</p>
+<p>Iterators must implement both <code>__next__</code> and <code>__iter__</code> for Python 3, or both <code>next</code> and <code>__iter__</code> for Python 2. The <code>__iter__</code> method of the iterator must return the iterator object itself.</p>
 
+<p>Iteration in Python relies on this behavior and attempting to iterate over an instance of a class with an incorrect <code>__iter__</code> method can raise a <code>TypeError</code>.
+</p>
 
 </overview>
 <recommendation>
-<p>Make the <code>__iter__</code> return a new iterator, either as an instance of
-a separate class or as a generator.</p>
-
+<p>Make sure the value returned by <code>__iter__</code> implements the full iterator protocol.</p>
 </recommendation>
 <example>
-<p>In this example the <code>MyRange</code> class's <code>__iter__</code> method does not
-return  an iterator. This will cause the program to fail when anyone attempts
-to use the iterator in a <code>for</code> loop or <code>in</code> statement.
-</p>
+<p>In this example, we have implemented our own version of <code>range</code>, extending the  normal functionality with the ability to skip some elements by using the <code>skip</code> method. However, the iterator <code>MyRangeIterator</code> does not fully implement the iterator protocol (namely it is missing <code>__iter__</code>).</p>
 
-<p>The fixed version implements the <code>__iter__</code> method as a generator function.</p>
+<p>Iterating over the elements in the range seems to work on the surface, for example the code <code>x = sum(my_range)</code> gives the expected result. However, if we run <code>sum(iter(my_range))</code> we get a <code>TypeError: 'MyRangeIterator' object is not iterable</code>.</p>
+
+<p>If we try to skip some elements using our custom method, for example <code>y = sum(my_range.skip({6,9}))</code>, this also raises a <code>TypeError</code>.</p>
+
+<p>The fix is to implement the <code>__iter__</code> method in <code>MyRangeIterator</code>.</p>
 
 <sample src="IterReturnsNonIterator.py" />
 
 </example>
 <references>
 
-  <li>Python Language Reference: <a href="http://docs.python.org/2.7/reference/datamodel.html#object.__iter__">object.__iter__</a>.</li>
-  <li>Python Standard Library: <a href="http://docs.python.org/2/library/stdtypes.html#typeiter">Iterator Types</a>.</li>
-
+  <li>Python Language Reference: <a href="https://docs.python.org/3/reference/datamodel.html#object.__iter__">object.__iter__</a>.</li>
+  <li>Python Standard Library: <a href="https://docs.python.org/3/library/stdtypes.html#typeiter">Iterator Types</a>.</li>
 
 </references>
 </qhelp>

python/ql/src/Functions/IterReturnsNonIterator.ql

@@ -12,21 +12,20 @@
 
 import python
 
-FunctionObject iter_method(ClassObject t) {
-		result = t.lookupAttribute("__iter__")
-}
-
-cached ClassObject return_type(FunctionObject f) {
+ClassObject return_type(FunctionObject f) {
     exists(ControlFlowNode n, Return ret |
-        ret.getScope() = f.getFunction() and ret.getValue() = n.getNode() and
+        ret.getScope() = f.getFunction() and
+        ret.getValue() = n.getNode() and
         n.refersTo(_, result, _)
     )
 }
 
-from ClassObject t, FunctionObject iter
-where exists(ClassObject ret_t | iter = iter_method(t) and
-          ret_t = return_type(iter) and
-          not ret_t.isIterator()
-      )
-
-select iter, "The '__iter__' method of iterable class $@ does not return an iterator.", t, t.getName()
+from ClassObject iterable, FunctionObject iter, ClassObject iterator
+where
+    iter = iterable.lookupAttribute("__iter__") and
+    iterator = return_type(iter) and
+    not iterator.isIterator()
+select iterator,
+    "Class " + iterator.getName() +
+        " is returned as an iterator (by $@) but does not fully implement the iterator interface.",
+    iter, iter.getName()

python/ql/test/query-tests/Functions/general/IncorrectRaiseInSpecialMethod.expected

@@ -1,2 +1,2 @@
-| protocols.py:66:5:66:33 | Function __getitem__ | Function always raises $@; raise LookupError instead | file://:Compiled Code:0:0:0:0 | builtin-class ZeroDivisionError | builtin-class ZeroDivisionError |
-| protocols.py:69:5:69:26 | Function __getattr__ | Function always raises $@; raise AttributeError instead | file://:Compiled Code:0:0:0:0 | builtin-class ZeroDivisionError | builtin-class ZeroDivisionError |
+| protocols.py:98:5:98:33 | Function __getitem__ | Function always raises $@; raise LookupError instead | file://:Compiled Code:0:0:0:0 | builtin-class ZeroDivisionError | builtin-class ZeroDivisionError |
+| protocols.py:101:5:101:26 | Function __getattr__ | Function always raises $@; raise AttributeError instead | file://:Compiled Code:0:0:0:0 | builtin-class ZeroDivisionError | builtin-class ZeroDivisionError |

python/ql/test/query-tests/Functions/general/IterReturnsNonIterator.expected

@@ -1 +1,4 @@
-| protocols.py:16:5:16:23 | Function __iter__ | The '__iter__' method of iterable class $@ does not return an iterator. | protocols.py:14:1:14:16 | class X | X |
+| file://:Compiled Code:0:0:0:0 | builtin-class object | Class object is returned as an iterator (by $@) but does not fully implement the iterator interface. | protocols.py:16:5:16:23 | Function __iter__ | __iter__ |
+| protocols.py:20:1:20:26 | class IteratorMissingNext | Class IteratorMissingNext is returned as an iterator (by $@) but does not fully implement the iterator interface. | protocols.py:22:5:22:23 | Function __iter__ | __iter__ |
+| protocols.py:20:1:20:26 | class IteratorMissingNext | Class IteratorMissingNext is returned as an iterator (by $@) but does not fully implement the iterator interface. | protocols.py:27:5:27:23 | Function __iter__ | __iter__ |
+| protocols.py:30:1:30:26 | class IteratorMissingIter | Class IteratorMissingIter is returned as an iterator (by $@) but does not fully implement the iterator interface. | protocols.py:40:5:40:23 | Function __iter__ | __iter__ |

python/ql/test/query-tests/Functions/general/IterReturnsNonSelf.expected

@@ -1 +1 @@
-| protocols.py:22:1:22:29 | class AlmostIterator | Class AlmostIterator is an iterator but its $@ method does not return 'self'. | protocols.py:30:5:30:23 | Function __iter__ | __iter__ |
+| protocols.py:54:1:54:29 | class AlmostIterator | Class AlmostIterator is an iterator but its $@ method does not return 'self'. | protocols.py:62:5:62:23 | Function __iter__ | __iter__ |

python/ql/test/query-tests/Functions/general/OverlyComplexDelMethod.expected

@@ -1 +1 @@
-| protocols.py:42:5:42:22 | Function __del__ | Overly complex '__del__' method. |
+| protocols.py:74:5:74:22 | Function __del__ | Overly complex '__del__' method. |

python/ql/test/query-tests/Functions/general/SignatureSpecialMethods.expected

@@ -5,5 +5,5 @@
 | om_test.py:71:5:71:19 | Function __repr__ | Too few parameters for special method __repr__, which has no parameters, but should have 1, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
 | om_test.py:74:5:74:46 | Function __add__ | 1 default values(s) will never be used for special method __add__, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
 | om_test.py:97:15:97:34 | Function lambda | Too few parameters for special method __sub__, which has 1 parameter, but should have 2, in class $@. | om_test.py:95:1:95:28 | class NotOKSpecials | NotOKSpecials |
-| protocols.py:72:1:72:12 | Function f | Too few parameters for special method __add__, which has 1 parameter, but should have 2, in class $@. | protocols.py:75:1:75:29 | class MissingMethods | MissingMethods |
-| protocols.py:72:1:72:12 | Function f | Too few parameters for special method __set__, which has 1 parameter, but should have 3, in class $@. | protocols.py:75:1:75:29 | class MissingMethods | MissingMethods |
+| protocols.py:104:1:104:12 | Function f | Too few parameters for special method __add__, which has 1 parameter, but should have 2, in class $@. | protocols.py:107:1:107:29 | class MissingMethods | MissingMethods |
+| protocols.py:104:1:104:12 | Function f | Too few parameters for special method __set__, which has 1 parameter, but should have 3, in class $@. | protocols.py:107:1:107:29 | class MissingMethods | MissingMethods |

python/ql/test/query-tests/Functions/general/protocols.py

@@ -17,6 +17,38 @@ def __iter__(self):
         return object()
 
 
+class IteratorMissingNext:
+
+    def __iter__(self):
+        return self
+
+class IterableMissingNext:
+
+    def __iter__(self):
+        return IteratorMissingNext()
+
+class IteratorMissingIter:
+
+    def next(self):
+        pass
+
+    def __next__(self):
+        pass
+
+class IterableMissingIter:
+
+    def __iter__(self):
+        return IteratorMissingIter()
+
+class IterableWithGenerator:
+    # returning a generator from __iter__ in an iterable is ok
+
+    def __iter__(self):
+        i = 0
+        while True:
+            yield i
+            i += 1
+
 #Iterator not returning self
 
 class AlmostIterator(object):
@@ -57,34 +89,30 @@ def close(self):
 
     def __del__(self):
         self.close()
-        
+
 class IncorrectSpecialMethods(object):
-    
+
     def __add__(self, other):
         raise NotImplementedError()
-    
+
     def __getitem__(self, index):
         raise ZeroDivisionError()
-        
+
     def __getattr__(self):
         raise ZeroDivisionError()
-    
+
 def f(self):
     pass
-    
+
 class MissingMethods(object):
-    
+
     __repr__ = f # This should be OK
     __add__ = f # But not this
     __set__ = f # or this
-    
+
 #OK Special method
 class OK(object):
-    
+
     def __call__(self):
         yield 0
         raise StopIteration
-
-        
-        
-