Do not error on dead, mandated statements at end of switch cases

pkg/analyzer/lib/src/generated/resolver.dart

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library analyzer.src.generated.resolver;
 
 import 'dart:collection';
 
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart';
@@ skipping 1895 matching lines @@
         for (Combinator combinator in node.combinators) {
           _checkCombinator(library, combinator);
         }
       }
     }
     return super.visitImportDirective(node);
   }
 
   @override
   Object visitSwitchCase(SwitchCase node) {
-    _checkForDeadStatementsInNodeList(node.statements);
+    _checkForDeadStatementsInNodeList(node.statements, allowMandated: true);
     return super.visitSwitchCase(node);
   }
 
   @override
   Object visitSwitchDefault(SwitchDefault node) {
-    _checkForDeadStatementsInNodeList(node.statements);
+    _checkForDeadStatementsInNodeList(node.statements, allowMandated: true);
     return super.visitSwitchDefault(node);
   }
 
   @override
   Object visitTryStatement(TryStatement node) {
     node.body?.accept(this);
     node.finallyBlock?.accept(this);
     NodeList<CatchClause> catchClauses = node.catchClauses;
     int numOfCatchClauses = catchClauses.length;
     List<DartType> visitedTypes = new List<DartType>();
@@ skipping 105 matching lines @@
             .reportErrorForNode(hintCode, name, [library.identifier, nameStr]);
       }
     }
   }
 
   /**
    * Given some [NodeList] of [Statement]s, from either a [Block] or
    * [SwitchMember], this loops through the list searching for dead statements.
    *
    * @param statements some ordered list of statements in a [Block] or [SwitchMember]
+   * @param allowMandated allow dead statements mandated by the language spec.
+   *            This allows for a final break, continue, return, or throw statement
+   *            at the end of a switch case, that are mandated by the language spec.
    */
-  void _checkForDeadStatementsInNodeList(NodeList<Statement> statements) {
+  void _checkForDeadStatementsInNodeList(
+      NodeList<Statement> statements, {bool allowMandated: false}) {
     bool statementExits(Statement statement) {
       if (statement is BreakStatement) {
         return statement.label == null;
       } else if (statement is ContinueStatement) {
         return statement.label == null;
       }
       return ExitDetector.exits(statement);
     }
 
     int size = statements.length;
     for (int i = 0; i < size; i++) {
       Statement currentStatement = statements[i];
       currentStatement?.accept(this);
       if (statementExits(currentStatement) && i != size - 1) {
         Statement nextStatement = statements[i + 1];
         Statement lastStatement = statements[size - 1];
+        // If mandated statements are allowed, and only the last statement is
+        // dead, maybe we should _not_ report an error.
+        if (allowMandated && i == size - 2) {
+          if (nextStatement is BreakStatement ||

[Paul Berry, 2016/06/14 18:39:27]

IMHO this is overly permissive.  The important case we want to allow is a
mandated "break".  If someone puts some other statement at the end of a switch
case (a return, continue, return, or throw), then I think we should go ahead and
report a hint, because in those other cases it's highly likely that the user has
made a mistake.

[Brian Wilkerson, 2016/06/14 18:59:41]

I could buy that; we're essentially leaving a single idiom for getting around a
warning in the spec.

How does this deal with something like

switch(intValue) {
  case 0:
    return false;
    break;
}

It would be nice if we could still report the break statement as being dead
code, but maybe that's too much for now.

[srawlins, 2016/06/14 21:09:55]

Yeah... I think at this point we just won't be smart enough to catch simple
baddies like this. But imho, this is very ok: the DEAD_CODE warnings are for
_non-obvious_ dead code paths, and our brains can be used for the obvious ones
:P.

It _will_ still catch:

return false;
int a = 1;
break;

[srawlins, 2016/06/14 21:09:55]

Ah, good point. I had not thought this through all the way.

+              nextStatement is ContinueStatement ||
+              nextStatement is ReturnStatement ||
+              nextStatement is ThrowExpression) {
+            return;
+          }
+        }
         int offset = nextStatement.offset;
         int length = lastStatement.end - offset;
         _errorReporter.reportErrorForOffset(HintCode.DEAD_CODE, offset, length);
         return;
       }
     }
   }
 
   /**
    * Given some [Expression], this method returns [ValidResult.RESULT_TRUE] if it is
@@ skipping 8866 matching lines @@
       return null;
     }
     if (identical(node.staticElement, variable)) {
       if (node.inSetterContext()) {
         result = true;
       }
     }
     return null;
   }
 }