Emit a warning for loops that have an else clause but no break or return.

Technically, the else: is superfluous with the return statement, but after much internal
discussion, I've changed the patch to allow this. The main point is that the indented block
is useful as a visual marker to say that the loop may end early and end execution of the whole
block, or something else might happen afterwards. That being said, I wouldn't be terribly sad
if we took it out again.

Closes #81378

4 files changed, 86 insertions, 4 deletions

diff --git a/ChangeLog b/ChangeLog
index 70421b4..95a8bce 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -8,19 +8,22 @@ ChangeLog for PyLint
     * #123233: new E0108[duplicate-argument-name] message reporting duplicate
       argument names
 
+    * #81378: emit C0109[useless-else-on-loop] for loops without break and
+      return
+
     * #124660: internal dependencies should not appear in external dependencies
       report
 
     * #124662: fix name error causing crash when symbols are included in output
       messages
 
-    * #123285: apply pragmas for warnings attached to lines to
-      physical source code lines
+    * #123285: apply pragmas for warnings attached to lines to physical source
+      code lines
 
     * #123259: do not emit E0105 for yield expressions inside lambdas
 
-    * Simplify checks for dangerous default values by unifying tests
-      for all different mutable compound literals.
+    * Simplify checks for dangerous default values by unifying tests for all
+      different mutable compound literals.
 
     * Improve the description for E1124[redundant-keyword-arg]
 
diff --git a/checkers/base.py b/checkers/base.py
index 5cc2a30..3b687d6 100644
--- a/checkers/base.py
+++ b/checkers/base.py
@@ -66,6 +66,19 @@ def in_nested_list(nested_list, obj):
             return True
     return False
 
+def _loop_exits_early(loop):
+    """Returns true if a loop has a break or return statement in its body."""
+    loop_nodes = (astng.For, astng.While)
+    exit_stmts = (astng.Break, astng.Return)
+    # Loop over body explicitly to avoid matching break/return statements
+    # in orelse.
+    for child in loop.body:
+        if isinstance(child, loop_nodes):
+            continue
+        for _ in child.nodes_of_class(exit_stmts, skip_klass=loop_nodes):
+            return True
+
+
 def report_by_type_stats(sect, stats, old_stats):
     """make a report of
 
@@ -164,6 +177,11 @@ class BasicErrorChecker(_BasicChecker):
               'duplicate-argument-name',
               'Duplicate argument names in function definitions are syntax'
               ' errors.'),
+    'C0109': ('Else clause on loop without break or return statement',
+              'useless-else-on-loop',
+              'Loops should only have an else clause if they can exit early '
+              'with a break statement, otherwise the statements under else '
+              'should be on the same scope as the loop itself.'),
     }
 
     def __init__(self, linter):
@@ -222,6 +240,14 @@ class BasicErrorChecker(_BasicChecker):
     def visit_break(self, node):
         self._check_in_loop(node, 'break')
 
+    @check_messages('C0109')
+    def visit_for(self, node):
+        self._check_else_on_loop(node)
+
+    @check_messages('C0109')
+    def visit_while(self, node):
+        self._check_else_on_loop(node)
+
     @check_messages('E0107')
     def visit_unaryop(self, node):
         """check use of the non-existent ++ adn -- operator operator"""
@@ -230,6 +256,15 @@ class BasicErrorChecker(_BasicChecker):
             (node.operand.op == node.op)):
             self.add_message('E0107', node=node, args=node.op*2)
 
+    def _check_else_on_loop(self, node):
+        """Check that any loop with an else clause has a break statement."""
+        if node.orelse and not _loop_exits_early(node):
+            self.add_message('C0109', node=node,
+                             # This is not optimal, but the line previous
+                             # to the first statement in the else clause
+                             # will usually be the one that contains the else:.
+                             line=node.orelse[0].lineno - 1)
+
     def _check_in_loop(self, node, node_name):
         """check that a node is inside a for or while loop"""
         _node = node.parent
diff --git a/test/input/func_useless_else_on_loop.py b/test/input/func_useless_else_on_loop.py
new file mode 100644
index 0000000..79be8e6
--- /dev/null
+++ b/test/input/func_useless_else_on_loop.py
@@ -0,0 +1,41 @@
+"""Check for else branches on loops with break an return only."""
+
+__revision__ = 0
+
+def test_return_for():
+    """else + return is accetable."""
+    for i in range(10):
+        if i % 2:
+            return i
+    else:
+        print 'math is broken'
+
+def test_return_while():
+    """else + return is accetable."""
+    while True:
+        return 1
+    else:
+        print 'math is broken'
+
+
+while True:
+    def short_fun():
+        """A function with a loop."""
+        for _ in range(10):
+            break
+else:
+    print 'or else!'
+
+
+while True:
+    while False:
+        break
+else:
+    print 'or else!'
+
+for j in range(10):
+    pass
+else:
+    print 'fat chance'
+    for j in range(10):
+        break
diff --git a/test/messages/func_useless_else_on_loop.txt b/test/messages/func_useless_else_on_loop.txt
new file mode 100644
index 0000000..ceaf356
--- /dev/null
+++ b/test/messages/func_useless_else_on_loop.txt
@@ -0,0 +1,3 @@
+C: 26: Else clause on loop without break or return statement
+C: 33: Else clause on loop without break or return statement
+C: 38: Else clause on loop without break or return statement