Migration: Add a flag to allow the user to ignore exceptions.

This should allow users to work around bugs while waiting for fixes
from the migration team.

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

Author: stereotype441

pkg/nnbd_migration/lib/migration_cli.dart

@@ -47,6 +47,7 @@ class CommandLineOptions {
   static const applyChangesFlag = 'apply-changes';
   static const helpFlag = 'help';
   static const ignoreErrorsFlag = 'ignore-errors';
+  static const ignoreExceptionsFlag = 'ignore-exceptions';
   static const previewPortOption = 'preview-port';
   static const sdkPathOption = 'sdk-path';
   static const skipPubOutdatedFlag = 'skip-pub-outdated';
@@ -60,6 +61,8 @@ class CommandLineOptions {
 
   final bool ignoreErrors;
 
+  final bool ignoreExceptions;
+
   final int previewPort;
 
   final String sdkPath;
@@ -74,6 +77,7 @@ class CommandLineOptions {
       {@required this.applyChanges,
       @required this.directory,
       @required this.ignoreErrors,
+      @required this.ignoreExceptions,
       @required this.previewPort,
       @required this.sdkPath,
       @required this.skipPubOutdated,
@@ -238,6 +242,10 @@ class MigrationCli {
 
   AnalysisContextCollection _contextCollection;
 
+  bool _hasExceptions = false;
+
+  bool _hasAnalysisErrors = false;
+
   MigrationCli(
       {@required this.binaryName,
       @visibleForTesting this.loggerFactory = _defaultLoggerFactory,
@@ -275,6 +283,10 @@ class MigrationCli {
     return contextCollection.contexts.length > 1;
   }
 
+  @visibleForTesting
+  bool get isPreviewServerRunning =>
+      _fixCodeProcessor?.isPreviewServerRunnning ?? false;
+
   Context get pathContext => resourceProvider.pathContext;
 
   /// Blocks until an interrupt signal (control-C) is received.  Tests may
@@ -289,12 +301,10 @@ class MigrationCli {
       ResourceProvider resourceProvider, LineInfo getLineInfo(String path),
       {List<String> included = const <String>[],
       int preferredPort,
-      bool enablePreview = true,
       String summaryPath}) {
     return NonNullableFix(listener, resourceProvider, getLineInfo,
         included: included,
         preferredPort: preferredPort,
-        enablePreview: enablePreview,
         summaryPath: summaryPath);
   }
 
@@ -349,6 +359,8 @@ class MigrationCli {
           applyChanges: applyChanges,
           directory: migratePath,
           ignoreErrors: argResults[CommandLineOptions.ignoreErrorsFlag] as bool,
+          ignoreExceptions:
+              argResults[CommandLineOptions.ignoreExceptionsFlag] as bool,
           previewPort: previewPort,
           sdkPath: argResults[CommandLineOptions.sdkPathOption] as String ??
               defaultSdkPathOverride ??
@@ -408,13 +420,12 @@ class MigrationCli {
     await _withProgress(
         '${ansi.emphasized('Generating migration suggestions')}', () async {
       _fixCodeProcessor = _FixCodeProcessor(context, this);
-      _dartFixListener =
-          DartFixListener(DriverProviderImpl(resourceProvider, context));
+      _dartFixListener = DartFixListener(
+          DriverProviderImpl(resourceProvider, context), _exceptionReported);
       nonNullableFix = createNonNullableFix(
           _dartFixListener, resourceProvider, _fixCodeProcessor.getLineInfo,
           included: [options.directory],
           preferredPort: options.previewPort,
-          enablePreview: options.webPreview,
           summaryPath: options.summary);
       nonNullableFix.rerunFunction = _rerunFunction;
       _fixCodeProcessor.registerCodeTask(nonNullableFix);
@@ -547,6 +558,7 @@ Use this interactive web view to review, improve, or apply the results.
       logger.stdout(
           'Note: analysis errors will result in erroneous migration suggestions.');
 
+      _hasAnalysisErrors = true;
       if (options.ignoreErrors) {
         logger.stdout('Continuing with migration suggestions due to the use of '
             '--${CommandLineOptions.ignoreErrorsFlag}.');
@@ -620,6 +632,43 @@ Use this interactive web view to review, improve, or apply the results.
     }
   }
 
+  void _exceptionReported(String detail) {
+    if (_hasExceptions) return;
+    _hasExceptions = true;
+    if (options.ignoreExceptions) {
+      logger.stdout('''
+Exception(s) occurred during migration.  Attempting to perform
+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
+the migration tool, or it may be due to errors in the source code
+being migrated.  If possible, try to fix errors in the source code and
+re-try migrating.  If that doesn't work, consider filing a bug report
+at:
+''');
+      } else {
+        logger.stderr('''
+Aborting migration due to an exception.  This most likely is due to a
+bug in the migration tool.  Please consider filing a bug report at:
+''');
+      }
+      logger.stderr('https://github.com/dart-lang/sdk/issues/new');
+      logger.stderr('''
+To attempt to perform migration anyway, you may re-run with
+--${CommandLineOptions.ignoreExceptionsFlag}.
+
+Exception details:
+''');
+      logger.stderr(detail);
+    }
+  }
+
   bool _isUriError(AnalysisError error) =>
       error.errorCode == CompileTimeErrorCode.URI_DOES_NOT_EXIST;
 
@@ -697,6 +746,12 @@ Use this interactive web view to review, improve, or apply the results.
       help: 'Attempt to perform null safety analysis even if there are '
           'analysis errors in the project.',
     );
+    parser.addFlag(CommandLineOptions.ignoreExceptionsFlag,
+        defaultsTo: false,
+        negatable: false,
+        help:
+            'Attempt to perform null safety analysis even if exceptions occur.',
+        hide: hide);
     parser.addOption(CommandLineOptions.previewPortOption,
         help:
             'Run the preview server on the specified port.  If not specified, '
@@ -780,6 +835,9 @@ class _FixCodeProcessor extends Object {
   _FixCodeProcessor(this.context, this._migrationCli)
       : pathsToProcess = _computePathsToProcess(context);
 
+  bool get isPreviewServerRunnning =>
+      nonNullableFixTask?.isPreviewServerRunning ?? false;
+
   LineInfo getLineInfo(String path) =>
       context.currentSession.getFile(path).lineInfo;
 
@@ -863,7 +921,10 @@ class _FixCodeProcessor extends Object {
         await _task.processUnit(phase, result);
       });
     }
-    await _task.finish();
+    var state = await _task.finish();
+    if (_migrationCli.exitCode == null && _migrationCli.options.webPreview) {
+      await _task.startPreviewServer(state);
+    }
     _progressBar.complete();
 
     return nonNullableFixTask.previewUrls;

pkg/nnbd_migration/lib/src/front_end/dartfix_listener.dart

@@ -15,13 +15,11 @@ class DartFixListener {
 
   final List<DartFixSuggestion> suggestions = [];
 
-  DartFixListener(this.server);
-
   /// Add the given [detail] to the list of details to be returned to the
   /// client.
-  void addDetail(String detail) {
-    throw UnimplementedError('TODO(paulberry)');
-  }
+  final void Function(String detail) reportException;
+
+  DartFixListener(this.server, this.reportException);
 
   /// Record an edit to be sent to the client.
   ///

pkg/nnbd_migration/lib/src/front_end/non_nullable_fix.dart

@@ -26,9 +26,6 @@ import 'package:yaml/yaml.dart';
 /// and determines whether the associated variable or parameter can be null
 /// then adds or removes a '?' trailing the named type as appropriate.
 class NonNullableFix {
-  /// TODO(paulberry): allow this to be controlled by a command-line parameter.
-  static const bool _usePermissiveMode = false;
-
   // TODO(srawlins): Refactor to use
   //  `Feature.non_nullable.firstSupportedVersion` when this becomes non-null.
   static const String _intendedMinimumSdkVersion = '2.9.0';
@@ -52,9 +49,6 @@ class NonNullableFix {
   /// which all included paths share.
   final String includedRoot;
 
-  /// Indicates whether the web preview of migration results should be launched.
-  final bool enablePreview;
-
   /// If non-null, the path to which a machine-readable summary of migration
   /// results should be written.
   final String summaryPath;
@@ -89,44 +83,26 @@ class NonNullableFix {
   List<String> previewUrls;
 
   NonNullableFix(this.listener, this.resourceProvider, this._getLineInfo,
-      {List<String> included = const [],
-      this.preferredPort,
-      this.enablePreview = true,
-      this.summaryPath})
+      {List<String> included = const [], this.preferredPort, this.summaryPath})
       : includedRoot =
             _getIncludedRoot(included, listener.server.resourceProvider) {
     reset();
   }
 
+  bool get isPreviewServerRunning => _server != null;
+
   int get numPhases => 3;
 
   InstrumentationListener createInstrumentationListener(
           {MigrationSummary migrationSummary}) =>
       InstrumentationListener(migrationSummary: migrationSummary);
 
-  Future<void> finish() async {
+  Future<MigrationState> finish() async {
     migration.finish();
     final state = MigrationState(
         migration, includedRoot, listener, instrumentationListener);
     await state.refresh();
-
-    if (enablePreview && _server == null) {
-      _server = HttpPreviewServer(state, rerun, preferredPort);
-      _server.serveHttp();
-      _allServers.add(_server);
-      port = await _server.boundPort;
-      authToken = await _server.authToken;
-
-      previewUrls = [
-        // TODO(jcollins-g): Change protocol to only return a single string.
-        Uri(
-            scheme: 'http',
-            host: 'localhost',
-            port: port,
-            path: state.pathMapper.map(includedRoot),
-            queryParameters: {'authToken': authToken}).toString()
-      ];
-    }
+    return state;
   }
 
   /// Processes the non-source files of the package rooted at [pkgFolder].
@@ -197,12 +173,32 @@ class NonNullableFix {
             : MigrationSummary(summaryPath, resourceProvider, includedRoot));
     adapter = NullabilityMigrationAdapter(listener);
     migration = NullabilityMigration(adapter, _getLineInfo,
-        permissive: _usePermissiveMode,
-        instrumentation: instrumentationListener);
+        permissive: true, instrumentation: instrumentationListener);
   }
 
   void shutdownServer() {
     _server?.close();
+    _server = null;
+  }
+
+  Future<void> startPreviewServer(MigrationState state) async {
+    if (_server == null) {
+      _server = HttpPreviewServer(state, rerun, preferredPort);
+      _server.serveHttp();
+      _allServers.add(_server);
+      port = await _server.boundPort;
+      authToken = await _server.authToken;
+
+      previewUrls = [
+        // TODO(jcollins-g): Change protocol to only return a single string.
+        Uri(
+            scheme: 'http',
+            host: 'localhost',
+            port: port,
+            path: state.pathMapper.map(includedRoot),
+            queryParameters: {'authToken': authToken}).toString()
+      ];
+    }
   }
 
   /// Updates the Package Config file to specify a minimum Dart SDK version
@@ -447,7 +443,7 @@ class NullabilityMigrationAdapter implements NullabilityMigrationListener {
   @override
   void reportException(
       Source source, AstNode node, Object exception, StackTrace stackTrace) {
-    listener.addDetail('''
+    listener.reportException('''
 $exception
 
 $stackTrace''');

pkg/nnbd_migration/test/front_end/nnbd_migration_test_base.dart

@@ -201,7 +201,7 @@ class NnbdMigrationTestBase extends AbstractAnalysisTest {
     // Compute the analysis results.
     var server = DriverProviderImpl(resourceProvider, driver.analysisContext);
     // Run the migration engine.
-    var listener = DartFixListener(server);
+    var listener = DartFixListener(server, _exceptionReported);
     var instrumentationListener = InstrumentationListener();
     var adapter = NullabilityMigrationAdapter(listener);
     var migration = NullabilityMigration(adapter, getLineInfo,
@@ -227,4 +227,8 @@ class NnbdMigrationTestBase extends AbstractAnalysisTest {
         resourceProvider, includedRoot, info, listener, migration, nodeMapper);
     infos = await builder.explainMigration();
   }
+
+  void _exceptionReported(String detail) {
+    fail('Unexpected error during migration: $detail');
+  }
 }