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>
changeseteb226fe8265e
branchdefault
phasedraft
hiddenyes
parent revision#4c4a8edd9115 Lambdas can contain yields, do not warn about them.
child revision<not specified>
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 1364577604 -3600
# Fri Mar 29 18:20:04 2013 +0100
# Node ID eb226fe8265e068c58b7b674aae6045963db1f1a
# 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,22 @@
31                  return True
32          elif elmt == obj:
33              return True
34      return False
35 
36 +def _loop_exits_early(loop):
37 +  """Returns true iff 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  def report_by_type_stats(sect, stats, old_stats):
49      """make a report of
50 
51      * percentage of different types documented
52      * percentage of different types with a bad name
@@ -162,10 +174,15 @@
53                "pre-decrement operator -- and ++, which doesn't exist in Python."),
54      'E0108': ('Duplicate argument name %s in function definition',
55                'duplicate-argument-name',
56                'Duplicate argument names in function definitions are syntax'
57                ' errors.'),
58 +    'C0109': ('Else clause on loop without break or return statement',
59 +              'useless-else-on-loop',
60 +              'Loops should only have an else clause if they can exit early '
61 +              'with a break statement, otherwise the statements under else '
62 +              'should be on the same scope as the loop itself.'),
63      }
64 
65      def __init__(self, linter):
66          _BasicChecker.__init__(self, linter)
67 
@@ -220,18 +237,35 @@
68 
69      @check_messages('E0103')
70      def visit_break(self, node):
71          self._check_in_loop(node, 'break')
72 
73 +    @check_messages('C0109')
74 +    def visit_for(self, node):
75 +        self._check_else_on_loop(node)
76 +
77 +    @check_messages('C0109')
78 +    def visit_while(self, node):
79 +        self._check_else_on_loop(node)
80 +
81      @check_messages('E0107')
82      def visit_unaryop(self, node):
83          """check use of the non-existent ++ adn -- operator operator"""
84          if ((node.op in '+-') and
85              isinstance(node.operand, astng.UnaryOp) and
86              (node.operand.op == node.op)):
87              self.add_message('E0107', node=node, args=node.op*2)
88 
89 +    def _check_else_on_loop(self, node):
90 +        """Check that any loop with an else clause has a break statement."""
91 +        if node.orelse and not _loop_exits_early(node):
92 +            self.add_message('C0109', node=node,
93 +                             # This is not optimal, but the line previous
94 +                             # to the first statement in the else clause
95 +                             # will usually be the one that contains the else:.
96 +                             line=node.orelse[0].lineno - 1)
97 +
98      def _check_in_loop(self, node, node_name):
99          """check that a node is inside a for or while loop"""
100          _node = node.parent
101          while _node:
102              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 @@
103 +"""Check for else branches on loops with break an return only."""
104 +
105 +__revision__ = 0
106 +
107 +def test_return_for():
108 +    """else + return is accetable."""
109 +    for i in range(10):
110 +        if i % 2:
111 +            return i
112 +    else:
113 +        print 'math is broken'
114 +
115 +def test_return_while():
116 +    """else + return is accetable."""
117 +    while True:
118 +        return 1
119 +    else:
120 +        print 'math is broken'
121 +    
122 +
123 +while True:
124 +    def short_fun():
125 +        """A function with a loop."""
126 +        for _ in range(10):
127 +            break
128 +else:
129 +    print 'or else!'
130 +
131 +
132 +while True:
133 +    while False:
134 +        break
135 +else:
136 +    print 'or else!'
137 +
138 +for j in range(10):
139 +    pass
140 +else:
141 +    print 'fat chance'
142 +    for j in range(10):
143 +        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 @@
144 +C: 26: Else clause on loop without break or return statement
145 +C: 33: Else clause on loop without break or return statement
146 +C: 38: Else clause on loop without break or return statement