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

pkg/analysis_server/lib/src/services/refactoring/extract_method.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 services.src.refactoring.extract_method;
 
 import 'dart:async';
 
 import 'package:analysis_server/src/protocol_server.dart' hide Element;
 import 'package:analysis_server/src/services/correction/name_suggestion.dart';
@@ skipping 554 matching lines @@
     _parameters.clear();
     _parametersMap.clear();
     _parameterReferencesMap.clear();
     RefactoringStatus result = new RefactoringStatus();
     List<VariableElement> assignedUsedVariables = [];
     unit.accept(new _InitializeParametersVisitor(this, assignedUsedVariables));
     // single expression
     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) {
         statement.accept(returnTypeComputer);
       });
       _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) {
         result.addFatalError(
             'Ambiguous return value: Selected block contains assignment(s) to '
                 'local variables and return statement.');
         return result;
       }
       // prepare to return an assigned variable
       VariableElement returnVariable = assignedUsedVariables[0];
@@ skipping 90 matching lines @@
   void _prepareOffsetsLengths() {
     offsets.clear();
     lengths.clear();
     for (_Occurrence occurrence in _occurrences) {
       offsets.add(occurrence.range.offset);
       lengths.add(occurrence.range.length);
     }
   }
 
   /**
+   * 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) {
     var visitor = new _HasMethodInvocationVisitor();
     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;
+  }
 }
 
 
 /**
  * [SelectionAnalyzer] for [ExtractMethodRefactoringImpl].
  */
 class _ExtractMethodAnalyzer extends StatementAnalyzer {
   _ExtractMethodAnalyzer(CompilationUnit unit, SourceRange selection)
       : super(unit, selection);
 
@@ skipping 147 matching lines @@
 class _HasMethodInvocationVisitor extends RecursiveAstVisitor {
   bool result = false;
 
   @override
   visitMethodInvocation(MethodInvocation node) {
     result = true;
   }
 }
 
 
+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;
   final Map<String, String> patternToSelectionName;
 
   bool forceStatic = false;
 
   _InitializeOccurrencesVisitor(this.ref, this.selectionPattern,
       this.patternToSelectionName);
 
@@ skipping 87 matching lines @@
       // next statement
       if (found) {
         beginStatementIndex += selectionCount;
       } else {
         beginStatementIndex++;
       }
     }
   }
 }
 
-
 class _InitializeParametersVisitor extends GeneralizingAstVisitor<Object> {
   final ExtractMethodRefactoringImpl ref;
   final List<VariableElement> assignedUsedVariables;
 
   _InitializeParametersVisitor(this.ref, this.assignedUsedVariables);
 
   @override
   Object visitSimpleIdentifier(SimpleIdentifier node) {
     SourceRange nodeRange = rangeNode(node);
     if (ref.selectionRange.covers(nodeRange)) {
@@ skipping 38 matching lines @@
       }
       // remember declaration names
       if (node.inDeclarationContext()) {
         ref._usedNames.add(node.name);
       }
     }
     return null;
   }
 }
 
+
 class _IsUsedAfterSelectionVisitor extends GeneralizingAstVisitor {
   final ExtractMethodRefactoringImpl ref;
   final VariableElement element;
   bool result = false;
 
   _IsUsedAfterSelectionVisitor(this.ref, this.element);
 
   @override
   visitSimpleIdentifier(SimpleIdentifier node) {
     VariableElement nodeElement = getLocalVariableElement(node);
@@ skipping 103 matching lines @@
       return false;
     }
     for (int i = 0; i < parameterTypes.length; i++) {
       if (other.parameterTypes[i] != parameterTypes[i]) {
         return false;
       }
     }
     return true;
   }
 }