Issue 22188. Verify that none or all execution flows return.

pkg/analysis_server/lib/src/services/refactoring/extract_method.dart

Index: pkg/analysis_server/lib/src/services/refactoring/extract_method.dart
diff --git a/pkg/analysis_server/lib/src/services/refactoring/extract_method.dart b/pkg/analysis_server/lib/src/services/refactoring/extract_method.dart
index 963a6f4e4a391b8fa349d235fa9b5d67fdee3a92..131be27d5b299c9716212c61748cbacac222a4fe 100644
--- a/pkg/analysis_server/lib/src/services/refactoring/extract_method.dart
+++ b/pkg/analysis_server/lib/src/services/refactoring/extract_method.dart
@@ -572,7 +572,18 @@ class ExtractMethodRefactoringImpl extends RefactoringImpl implements
     if (_selectionExpression != null) {
       _returnType = _selectionExpression.bestType;
     }
-    // may be ends with "return" statement
+    // verify that none or all execution flows end with a "return"
+    if (_selectionStatements != null) {
+      bool hasReturn = _selectionStatements.any(_mayEndWithReturnStatement);
+      bool endsReturn = _endsWithReturnStatement(_selectionStatements.last);
+      if (hasReturn && !endsReturn) {
+        result.addError(
+            "Selected statements contain a return statement, but "
+                "not all possible execuion flows end in a return. "
+                "Semantics may not be preserved.");
+      }
+    }
+    // maybe ends with "return" statement
     if (_selectionStatements != null) {
       _ReturnTypeComputer returnTypeComputer = new _ReturnTypeComputer();
       _selectionStatements.forEach((statement) {
@@ -580,7 +591,7 @@ class ExtractMethodRefactoringImpl extends RefactoringImpl implements
       });
       _returnType = returnTypeComputer.returnType;
     }
-    // may be single variable to return
+    // maybe single variable to return
     if (assignedUsedVariables.length == 1) {
       // we cannot both return variable and have explicit return statement
       if (_returnType != null) {
@@ -691,6 +702,29 @@ class ExtractMethodRefactoringImpl extends RefactoringImpl implements
   }
 
   /**
+   * Returns `true` if the given [statement] is a [ReturnStatement] or
+   * unconditionally ends with one.
+   */
+  static bool _endsWithReturnStatement(Statement statement) {

[Brian Wilkerson, 2015/02/10 18:29:03]

Should this be using ExitDetector?

[scheglov, 2015/02/10 19:13:42]

Great idea.
Thank you!

+    if (statement is ReturnStatement) {
+      return true;
+    }
+    if (statement is Block) {
+      List<Statement> statements = statement.statements;
+      return statements.isNotEmpty && _endsWithReturnStatement(statements[0]);

[Brian Wilkerson, 2015/02/10 18:29:03]

Shouldn't this be "statements.last"?

[scheglov, 2015/02/10 19:13:42]

Acknowledged.

+    }
+    if (statement is IfStatement) {
+      Statement thenStatement = statement.thenStatement;
+      Statement elseStatement = statement.elseStatement;
+      return thenStatement != null &&
+          elseStatement != null &&
+          _endsWithReturnStatement(thenStatement) &&
+          _endsWithReturnStatement(elseStatement);
+    }
+    return false;
+  }
+
+  /**
    * Checks if [node] has a [MethodInvocation].
    */
   static bool _hasMethodInvocation(AstNode node) {
@@ -698,6 +732,15 @@ class ExtractMethodRefactoringImpl extends RefactoringImpl implements
     node.accept(visitor);
     return visitor.result;
   }
+
+  /**
+   * Returns `true` if the given [statement] may end with a [ReturnStatement].
+   */
+  static bool _mayEndWithReturnStatement(Statement statement) {
+    _HasReturnStatementVisitor visitor = new _HasReturnStatementVisitor();
+    statement.accept(visitor);
+    return visitor.hasReturn;
+  }
 }
 
 
@@ -865,6 +908,20 @@ class _HasMethodInvocationVisitor extends RecursiveAstVisitor {
 }
 
 
+class _HasReturnStatementVisitor extends RecursiveAstVisitor {
+  bool hasReturn = false;
+
+  @override
+  visitBlockFunctionBody(BlockFunctionBody node) {
+  }
+
+  @override
+  visitReturnStatement(ReturnStatement node) {
+    hasReturn = true;
+  }
+}
+
+
 class _InitializeOccurrencesVisitor extends GeneralizingAstVisitor<Object> {
   final ExtractMethodRefactoringImpl ref;
   final _SourcePattern selectionPattern;
@@ -972,7 +1029,6 @@ class _InitializeOccurrencesVisitor extends GeneralizingAstVisitor<Object> {
   }
 }
 
-
 class _InitializeParametersVisitor extends GeneralizingAstVisitor<Object> {
   final ExtractMethodRefactoringImpl ref;
   final List<VariableElement> assignedUsedVariables;
@@ -1031,6 +1087,7 @@ class _InitializeParametersVisitor extends GeneralizingAstVisitor<Object> {
   }
 }
 
+
 class _IsUsedAfterSelectionVisitor extends GeneralizingAstVisitor {
   final ExtractMethodRefactoringImpl ref;
   final VariableElement element;