Migration: rework how MigrationCli.run() reports an exit code.

Previously, MigrationCli.run() would always return, but it would set
MigrationCli.exitCode to indicate whether an error occurred.  This
made it necessary to thread a lot of tricky logic around to ensure
that we would exit quickly if an error occurred.  This CL switches to
using an exception, so we get the same effect automatically.

Change-Id: I2b885092dec551700900b118ec3d9acd2f3649d7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/150760
Reviewed-by: Mike Fairhurst <[email protected]>

Author: stereotype441

pkg/nnbd_migration/bin/migrate.dart

@@ -11,10 +11,13 @@ main(List<String> args) async {
   var cli = MigrationCli(binaryName: 'nnbd_migration');
   ArgResults argResults;
   try {
-    argResults = MigrationCli.createParser().parse(args);
-  } on FormatException catch (e) {
-    cli.handleArgParsingException(e);
+    try {
+      argResults = MigrationCli.createParser().parse(args);
+    } on FormatException catch (e) {
+      cli.handleArgParsingException(e);
+    }
+    await cli.run(argResults);
+  } on MigrationExit catch (migrationExit) {
+    exitCode = migrationExit.exitCode;
   }
-  if (cli.exitCode == null) await cli.run(argResults);
-  exitCode = cli.exitCode;
 }

pkg/nnbd_migration/lib/migration_cli.dart

@@ -195,8 +195,12 @@ class MigrateCommand extends Command<dynamic> {
   @override
   FutureOr<int> run() async {
     var cli = MigrationCli(binaryName: 'dart $name');
-    await cli.run(argResults, isVerbose: verbose);
-    return cli.exitCode;
+    try {
+      await cli.run(argResults, isVerbose: verbose);
+    } on MigrationExit catch (migrationExit) {
+      return migrationExit.exitCode;
+    }
+    return 0;
   }
 }
 
@@ -230,10 +234,6 @@ class MigrationCli {
   @visibleForTesting
   /*late*/ CommandLineOptions options;
 
-  /// The exit code that should be used when the process terminates, or `null`
-  /// if there is still more work to do.
-  int exitCode;
-
   final Map<String, List<AnalysisError>> fileErrors = {};
 
   final Map<String, LineInfo> lineInfo = {};
@@ -327,7 +327,6 @@ class MigrationCli {
       isVerbose ??= argResults[CommandLineOptions.verboseFlag] as bool;
       if (argResults[CommandLineOptions.helpFlag] as bool) {
         _showUsage(isVerbose);
-        exitCode = 0;
         return;
       }
       var rest = argResults.rest;
@@ -398,18 +397,16 @@ class MigrationCli {
     }
     logger.stderr(message);
     _showUsage(false);
-    exitCode = 1;
-    return;
+    throw MigrationExit(1);
   }
 
   /// Runs the full migration process.
-  void run(ArgResults argResults, {bool isVerbose}) async {
+  Future<void> run(ArgResults argResults, {bool isVerbose}) async {
     decodeCommandLineArgs(argResults, isVerbose: isVerbose);
-    if (exitCode != null) return;
+    if (options == null) return;
     if (!options.skipPubOutdated) {
       _checkDependencies();
     }
-    if (exitCode != null) return;
 
     logger.stdout('Migrating ${options.directory}');
     logger.stdout('');
@@ -447,16 +444,13 @@ class MigrationCli {
       if (sdkPathVar != null) {
         logger.stdout('$sdkPathEnvironmentVariableSet: $sdkPathVar');
       }
-      exitCode = 1;
+      throw MigrationExit(1);
     }
-    if (exitCode != null) return;
 
     logger.stdout('');
     logger.stdout(ansi.emphasized('Generating migration suggestions...'));
     previewUrls = await _fixCodeProcessor.runLaterPhases(resetProgress: true);
 
-    if (exitCode != null) return;
-
     if (options.applyChanges) {
       logger.stdout(ansi.emphasized('Applying changes:'));
 
@@ -470,7 +464,6 @@ class MigrationCli {
       // Note: do not open the web preview if apply-changes is specified, as we
       // currently cannot tell the web preview to disable the "apply migration"
       // button.
-      exitCode = 0;
       return;
     }
 
@@ -504,7 +497,6 @@ Use this interactive web view to review, improve, or apply the results.
       logger.stdout('To apply these changes, re-run the tool with '
           '--${CommandLineOptions.applyChangesFlag}.');
     }
-    exitCode = 0;
   }
 
   /// Perform the indicated source edits to the given source, returning the
@@ -547,7 +539,7 @@ Use this interactive web view to review, improve, or apply the results.
             options.directory, pathContext, logger, processManager)
         .check();
     if (!successful) {
-      exitCode = 1;
+      throw MigrationExit(1);
     }
   }
 
@@ -586,8 +578,7 @@ Use this interactive web view to review, improve, or apply the results.
             'Please fix the analysis issues (or, force generation of migration '
             'suggestions by re-running with '
             '--${CommandLineOptions.ignoreErrorsFlag}).');
-        exitCode = 1;
-        return;
+        throw MigrationExit(1);
       }
     }
   }
@@ -655,7 +646,6 @@ migration anyway due to the use of --${CommandLineOptions.ignoreExceptionsFlag}.
 To see exception details, re-run without --${CommandLineOptions.ignoreExceptionsFlag}.
 ''');
     } else {
-      exitCode = 1;
       if (_hasAnalysisErrors) {
         logger.stderr('''
 Aborting migration due to an exception.  This may be due to a bug in
@@ -678,6 +668,7 @@ To attempt to perform migration anyway, you may re-run with
 Exception details:
 ''');
       logger.stderr(detail);
+      throw MigrationExit(1);
     }
   }
 
@@ -794,6 +785,14 @@ Exception details:
   }
 }
 
+/// Exception thrown by [MigrationCli] if the client should exit.
+class MigrationExit {
+  /// The exit code that the client should set.
+  final int exitCode;
+
+  MigrationExit(this.exitCode);
+}
+
 /// An abstraction over the static methods on [Process].
 ///
 /// Used in tests to run mock processes.
@@ -871,7 +870,6 @@ class _FixCodeProcessor extends Object {
               if (pathsToProcess.contains(unit.path) &&
                   !pathsProcessed.contains(unit.path)) {
                 await process(unit);
-                if (_migrationCli.exitCode != null) return;
                 pathsProcessed.add(unit.path);
               }
             }
@@ -888,7 +886,6 @@ class _FixCodeProcessor extends Object {
         continue;
       }
       await process(result);
-      if (_migrationCli.exitCode != null) return;
     }
   }
 
@@ -934,7 +931,7 @@ class _FixCodeProcessor extends Object {
       });
     }
     var state = await _task.finish();
-    if (_migrationCli.exitCode == null && _migrationCli.options.webPreview) {
+    if (_migrationCli.options.webPreview) {
       await _task.startPreviewServer(state);
     }
     _progressBar.complete();

pkg/nnbd_migration/test/migration_cli_test.dart

@@ -152,16 +152,25 @@ mixin _MigrationCliTestMethods on _MigrationCliTestBase {
 
   Future<String> assertDecodeArgsFailure(List<String> args) async {
     var cli = _createCli();
-    await cli.run(MigrationCli.createParser().parse(args));
-    var stderrText = assertErrorExit(cli);
+    var stderrText = await assertErrorExit(
+        cli, () => cli.run(MigrationCli.createParser().parse(args)),
+        withUsage: true);
     expect(stderrText, isNot(contains('Exception')));
     return stderrText;
   }
 
-  String assertErrorExit(MigrationCli cli, {bool withUsage = true}) {
+  Future<String> assertErrorExit(
+      MigrationCli cli, Future<void> Function() callback,
+      {@required bool withUsage, dynamic expectedExitCode = anything}) async {
+    try {
+      await callback();
+      fail('Migration succeeded; expected it to abort with an error');
+    } on MigrationExit catch (migrationExit) {
+      expect(migrationExit.exitCode, isNotNull);
+      expect(migrationExit.exitCode, isNot(0));
+      expect(migrationExit.exitCode, expectedExitCode);
+    }
     expect(cli.isPreviewServerRunning, isFalse);
-    expect(cli.exitCode, isNotNull);
-    expect(cli.exitCode, isNot(0));
     var stderrText = logger.stderrBuffer.toString();
     expect(stderrText, withUsage ? hasUsageText : isNot(hasUsageText));
     expect(stderrText,
@@ -186,7 +195,6 @@ mixin _MigrationCliTestMethods on _MigrationCliTestBase {
 
   void assertNormalExit(MigrationCli cli) {
     expect(cli.isPreviewServerRunning, isFalse);
-    expect(cli.exitCode, 0);
   }
 
   Future<String> assertParseArgsFailure(List<String> args) async {
@@ -202,8 +210,9 @@ mixin _MigrationCliTestMethods on _MigrationCliTestBase {
   CommandLineOptions assertParseArgsSuccess(List<String> args) {
     var cli = _createCli();
     cli.decodeCommandLineArgs(MigrationCli.createParser().parse(args));
-    expect(cli.exitCode, isNull);
+    assertNormalExit(cli);
     var options = cli.options;
+    expect(options, isNotNull);
     return options;
   }
 
@@ -253,6 +262,16 @@ mixin _MigrationCliTestMethods on _MigrationCliTestBase {
     expect(success, isTrue);
   }
 
+  Future<String> assertRunFailure(List<String> args,
+      {MigrationCli cli,
+      bool withUsage = false,
+      dynamic expectedExitCode = anything}) async {
+    cli ??= _createCli();
+    return await assertErrorExit(
+        cli, () => cli.run(MigrationCli.createParser().parse(args)),
+        withUsage: withUsage, expectedExitCode: expectedExitCode);
+  }
+
   String createProjectDir(Map<String, String> contents,
       {String posixPath = '/test_project'}) {
     for (var entry in contents.entries) {
@@ -346,15 +365,14 @@ int${migrated ? '?' : ''} f() => null;
     expect(newCoreLibText, isNot(oldCoreLibText));
     coreLib.writeAsStringSync(newCoreLibText);
     var projectDir = await createProjectDir(simpleProject());
-    await cli.run(MigrationCli.createParser().parse([projectDir]));
-    assertErrorExit(cli, withUsage: false);
+    await assertRunFailure([projectDir], cli: cli);
     var output = logger.stdoutBuffer.toString();
     expect(output, contains(messages.sdkNnbdOff));
   }
 
   test_detect_old_sdk_environment_variable() async {
     environmentVariables['SDK_PATH'] = '/fake-old-sdk-path';
-    var cli = _createCli();
+    var cli = _createCli(); // Creates the mock SDK as a side effect
     // Alter the mock SDK, changing the signature of Object.operator== to match
     // the signature that was present prior to NNBD.  (This is what the
     // migration tool uses to detect an old SDK).
@@ -367,8 +385,7 @@ int${migrated ? '?' : ''} f() => null;
     expect(newCoreLibText, isNot(oldCoreLibText));
     coreLib.writeAsStringSync(newCoreLibText);
     var projectDir = await createProjectDir(simpleProject());
-    await cli.run(MigrationCli.createParser().parse([projectDir]));
-    assertErrorExit(cli, withUsage: false);
+    await assertRunFailure([projectDir], cli: cli);
     var output = logger.stdoutBuffer.toString();
     expect(output, contains(messages.sdkNnbdOff));
     expect(output, contains(messages.sdkPathEnvironmentVariableSet));
@@ -522,8 +539,7 @@ linter:
     var projectContents = simpleProject(sourceText: 'main() { print(0); }');
     var projectDir = await createProjectDir(projectContents);
     var cli = _createCli(injectArtificialException: true);
-    await cli.run(_parseArgs([projectDir]));
-    assertErrorExit(cli, withUsage: false);
+    await assertRunFailure([projectDir], cli: cli);
     var errorOutput = logger.stderrBuffer.toString();
     expect(errorOutput, contains('Artificial exception triggered'));
     expect(
@@ -556,8 +572,7 @@ linter:
         simpleProject(sourceText: 'main() { print(0); print(1); }');
     var projectDir = await createProjectDir(projectContents);
     var cli = _createCli(injectArtificialException: true);
-    await cli.run(_parseArgs([projectDir]));
-    assertErrorExit(cli, withUsage: false);
+    await assertRunFailure([projectDir], cli: cli);
     var errorOutput = logger.stderrBuffer.toString();
     expect(
         'Artificial exception triggered'.allMatches(errorOutput), hasLength(1));
@@ -571,8 +586,7 @@ linter:
         simpleProject(sourceText: 'main() { print(0); unresolved; }');
     var projectDir = await createProjectDir(projectContents);
     var cli = _createCli(injectArtificialException: true);
-    await cli.run(_parseArgs(['--ignore-errors', projectDir]));
-    assertErrorExit(cli, withUsage: false);
+    await assertRunFailure(['--ignore-errors', projectDir], cli: cli);
     var errorOutput = logger.stderrBuffer.toString();
     expect(errorOutput, contains('Artificial exception triggered'));
     expect(errorOutput, contains('try to fix errors in the source code'));
@@ -584,9 +598,7 @@ linter:
 int f() => null
 ''');
     var projectDir = await createProjectDir(projectContents);
-    var cli = _createCli();
-    await cli.run(_parseArgs([projectDir]));
-    assertErrorExit(cli, withUsage: false);
+    await assertRunFailure([projectDir]);
     var output = logger.stdoutBuffer.toString();
     expect(output, contains('1 analysis issue found'));
     var sep = resourceProvider.pathContext.separator;
@@ -1047,11 +1059,8 @@ int f() => null;
 }
 ''' /* stdout */,
         '' /* stderr */);
-    var cli = _createCli();
-    await cli.run(_parseArgs([projectDir]));
-    var output = assertErrorExit(cli, withUsage: false);
+    var output = await assertRunFailure([projectDir], expectedExitCode: 1);
     expect(output, contains('Warning: dependencies are outdated.'));
-    expect(cli.exitCode, 1);
   }
 
   test_lifecycle_skip_pub_outdated_enable() async {
@@ -1146,9 +1155,7 @@ import 'package:does_not/exist.dart';
 int f() => null;
 ''');
     var projectDir = await createProjectDir(projectContents);
-    var cli = _createCli();
-    await cli.run(_parseArgs([projectDir]));
-    assertErrorExit(cli, withUsage: false);
+    await assertRunFailure([projectDir]);
     var output = logger.stdoutBuffer.toString();
     expect(output, contains('1 analysis issue found'));
     expect(output, contains('uri_does_not_exist'));
@@ -1199,9 +1206,7 @@ int f() => null;
   }
 
   test_migrate_path_two() async {
-    var cli = _createCli();
-    await cli.run(_parseArgs(['foo', 'bar']));
-    var stderrText = assertErrorExit(cli);
+    var stderrText = await assertRunFailure(['foo', 'bar'], withUsage: true);
     expect(stderrText, contains('No more than one path may be specified'));
   }
 
@@ -1581,7 +1586,7 @@ name: test
     var cli = _createCli();
     await cli.run(_parseArgs(
         ['--${CommandLineOptions.helpFlag}', if (verbose) '--verbose']));
-    expect(cli.exitCode, 0);
+    assertNormalExit(cli);
     var helpText = logger.stderrBuffer.toString();
     return helpText;
   }