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 963a6f4e4a3..cb06d569d3d 100644 --- a/pkg/analysis_server/lib/src/services/refactoring/extract_method.dart +++ b/pkg/analysis_server/lib/src/services/refactoring/extract_method.dart @@ -25,6 +25,7 @@ import 'package:analyzer/src/generated/element.dart'; import 'package:analyzer/src/generated/java_core.dart'; import 'package:analyzer/src/generated/scanner.dart'; import 'package:analyzer/src/generated/source.dart'; +import 'package:analyzer/src/generated/resolver.dart' show ExitDetector; const String _TOKEN_SEPARATOR = '\uFFFF'; @@ -57,6 +58,10 @@ Map _inverseMap(Map map) { */ class ExtractMethodRefactoringImpl extends RefactoringImpl implements ExtractMethodRefactoring { + static const ERROR_EXITS = + 'Selected statements contain a return statement, but not all possible ' + 'execuion flows exit. Semantics may not be preserved.'; + final SearchEngine searchEngine; final CompilationUnit unit; final int selectionOffset; @@ -572,7 +577,14 @@ 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); + if (hasReturn && !ExitDetector.exits(_selectionStatements.last)) { + result.addError(ERROR_EXITS); + } + } + // maybe ends with "return" statement if (_selectionStatements != null) { _ReturnTypeComputer returnTypeComputer = new _ReturnTypeComputer(); _selectionStatements.forEach((statement) { @@ -580,7 +592,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) { @@ -698,6 +710,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 +886,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 { final ExtractMethodRefactoringImpl ref; final _SourcePattern selectionPattern; @@ -972,7 +1007,6 @@ class _InitializeOccurrencesVisitor extends GeneralizingAstVisitor { } } - class _InitializeParametersVisitor extends GeneralizingAstVisitor { final ExtractMethodRefactoringImpl ref; final List assignedUsedVariables; @@ -1031,6 +1065,7 @@ class _InitializeParametersVisitor extends GeneralizingAstVisitor { } } + class _IsUsedAfterSelectionVisitor extends GeneralizingAstVisitor { final ExtractMethodRefactoringImpl ref; final VariableElement element; diff --git a/pkg/analysis_server/test/services/refactoring/extract_method_test.dart b/pkg/analysis_server/test/services/refactoring/extract_method_test.dart index 98de0cab65e..95c05a5da76 100644 --- a/pkg/analysis_server/test/services/refactoring/extract_method_test.dart +++ b/pkg/analysis_server/test/services/refactoring/extract_method_test.dart @@ -424,6 +424,21 @@ main() { "Extend selection to a valid range."); } + test_bad_statements_exit_notAllExecutionFlows() { + indexTestUnit(''' +main(int p) { +// start + if (p == 0) { + return; + } +// end + print(p); +} +'''); + _createRefactoringForStartEndComments(); + return _assertConditionsError(ExtractMethodRefactoringImpl.ERROR_EXITS); + } + test_bad_statements_return_andAssignsVariable() { indexTestUnit(''' main() { @@ -2080,6 +2095,21 @@ void res() { '''); } + test_statements_exit_throws() async { + indexTestUnit(''' +main(int p) { +// start + if (p == 0) { + return; + } + throw 'boo!'; +// end +} +'''); + _createRefactoringForStartEndComments(); + await assertRefactoringConditionsOK(); + } + test_statements_inSwitchMember() { indexTestUnit(''' class A { @@ -2292,8 +2322,10 @@ int res() { main(bool b) { // start if (b) { + print(true); return []; } else { + print(false); return []; } // end @@ -2310,8 +2342,10 @@ main(bool b) { List res(bool b) { if (b) { + print(true); return []; } else { + print(false); return []; } } diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart index 9f3ab6db8bc..5dcbcfb806b 100644 --- a/pkg/analyzer/lib/src/generated/resolver.dart +++ b/pkg/analyzer/lib/src/generated/resolver.dart @@ -4331,6 +4331,13 @@ class ExitDetector extends GeneralizingAstVisitor { } return false; } + + /** + * Return `true` if the given [node] exits. + */ + static bool exits(AstNode node) { + return new ExitDetector()._nodeExits(node); + } } /**