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

authorTorsten Marek <tmarek@google.com>
changeset962fff72742b
branchdefault
phasepublic
hiddenno
parent revision#4c4a8edd9115 Lambdas can contain yields, do not warn about them.
child revision#9a48c3ce5e5e a few pylint fixes and copyright cleanups
files modified by this revision
ChangeLog
checkers/base.py
test/input/func_useless_else_on_loop.py
test/messages/func_useless_else_on_loop.txt
# HG changeset patch
# User Torsten Marek <tmarek@google.com>
# Date 1364578403 -3600
# Fri Mar 29 18:33:23 2013 +0100
# Node ID 962fff72742b39048be2ec50648cd432fd3fded7
# Parent 4c4a8edd911569ecc1a3d466cd5dc033fad150c0
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

diff --git a/ChangeLog b/ChangeLog
@@ -6,23 +6,26 @@
1        {l,r,}strip method contains duplicate characters (patch by Torsten Marek)
2 
3      * #123233: new E0108[duplicate-argument-name] message reporting duplicate
4        argument names
5 
6 +    * #81378: emit C0109[useless-else-on-loop] for loops without break and
7 +      return
8 +
9      * #124660: internal dependencies should not appear in external dependencies
10        report
11 
12      * #124662: fix name error causing crash when symbols are included in output
13        messages
14 
15 -    * #123285: apply pragmas for warnings attached to lines to
16 -      physical source code lines
17 +    * #123285: apply pragmas for warnings attached to lines to physical source
18 +      code lines
19 
20      * #123259: do not emit E0105 for yield expressions inside lambdas
21 
22 -    * Simplify checks for dangerous default values by unifying tests
23 -      for all different mutable compound literals.
24 +    * Simplify checks for dangerous default values by unifying tests for all
25 +      different mutable compound literals.
26 
27      * Improve the description for E1124[redundant-keyword-arg]
28 
29 
30  2013-02-26 -- 0.27.0
diff --git a/checkers/base.py b/checkers/base.py
@@ -64,10 +64,23 @@
31                  return True
32          elif elmt == obj:
33              return True
34      return False
35 
36 +def _loop_exits_early(loop):
37 +    """Returns true if a loop has a break or return statement in its body."""
38 +    loop_nodes = (astng.For, astng.While)
39 +    exit_stmts = (astng.Break, astng.Return)
40 +    # Loop over body explicitly to avoid matching break/return statements
41 +    # in orelse.
42 +    for child in loop.body:
43 +        if isinstance(child, loop_nodes):
44 +            continue
45 +        for _ in child.nodes_of_class(exit_stmts, skip_klass=loop_nodes):
46 +            return True
47 +
48 +
49  def report_by_type_stats(sect, stats, old_stats):
50      """make a report of
51 
52      * percentage of different types documented
53      * percentage of different types with a bad name
@@ -162,10 +175,15 @@
54                "pre-decrement operator -- and ++, which doesn't exist in Python."),
55      'E0108': ('Duplicate argument name %s in function definition',
56                'duplicate-argument-name',
57                'Duplicate argument names in function definitions are syntax'
58                ' errors.'),
59 +    'C0109': ('Else clause on loop without break or return statement',
60 +              'useless-else-on-loop',
61 +              'Loops should only have an else clause if they can exit early '
62 +              'with a break statement, otherwise the statements under else '
63 +              'should be on the same scope as the loop itself.'),
64      }
65 
66      def __init__(self, linter):
67          _BasicChecker.__init__(self, linter)
68 
@@ -220,18 +238,35 @@
69 
70      @check_messages('E0103')
71      def visit_break(self, node):
72          self._check_in_loop(node, 'break')
73 
74 +    @check_messages('C0109')
75 +    def visit_for(self, node):
76 +        self._check_else_on_loop(node)
77 +
78 +    @check_messages('C0109')
79 +    def visit_while(self, node):
80 +        self._check_else_on_loop(node)
81 +
82      @check_messages('E0107')
83      def visit_unaryop(self, node):
84          """check use of the non-existent ++ adn -- operator operator"""
85          if ((node.op in '+-') and
86              isinstance(node.operand, astng.UnaryOp) and
87              (node.operand.op == node.op)):
88              self.add_message('E0107', node=node, args=node.op*2)
89 
90 +    def _check_else_on_loop(self, node):
91 +        """Check that any loop with an else clause has a break statement."""
92 +        if node.orelse and not _loop_exits_early(node):
93 +            self.add_message('C0109', node=node,
94 +                             # This is not optimal, but the line previous
95 +                             # to the first statement in the else clause
96 +                             # will usually be the one that contains the else:.
97 +                             line=node.orelse[0].lineno - 1)
98 +
99      def _check_in_loop(self, node, node_name):
100          """check that a node is inside a for or while loop"""
101          _node = node.parent
102          while _node:
103              if isinstance(_node, (astng.For, astng.While)):
diff --git a/test/input/func_useless_else_on_loop.py b/test/input/func_useless_else_on_loop.py
@@ -0,0 +1,41 @@
104 +"""Check for else branches on loops with break an return only."""
105 +
106 +__revision__ = 0
107 +
108 +def test_return_for():
109 +    """else + return is accetable."""
110 +    for i in range(10):
111 +        if i % 2:
112 +            return i
113 +    else:
114 +        print 'math is broken'
115 +
116 +def test_return_while():
117 +    """else + return is accetable."""
118 +    while True:
119 +        return 1
120 +    else:
121 +        print 'math is broken'
122 +
123 +
124 +while True:
125 +    def short_fun():
126 +        """A function with a loop."""
127 +        for _ in range(10):
128 +            break
129 +else:
130 +    print 'or else!'
131 +
132 +
133 +while True:
134 +    while False:
135 +        break
136 +else:
137 +    print 'or else!'
138 +
139 +for j in range(10):
140 +    pass
141 +else:
142 +    print 'fat chance'
143 +    for j in range(10):
144 +        break
diff --git a/test/messages/func_useless_else_on_loop.txt b/test/messages/func_useless_else_on_loop.txt
@@ -0,0 +1,3 @@
145 +C: 26: Else clause on loop without break or return statement
146 +C: 33: Else clause on loop without break or return statement
147 +C: 38: Else clause on loop without break or return statement