Migrator: Add a progress bar for explainMigration, which can take a long time.

This shows progress for work which happens after the migration suggestions have
been generated. This work is generating instrumentation details for the front
end.

When migrating the analyzer package, the progress indicated by this third
progress bar takes approximately the same time as generating suggestions (on
the same order of magnitude).

Fixes https://github.com/dart-lang/sdk/issues/44074

Change-Id: I0b40dd5e587b718bd1ef56d64f03929924cc430a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/170563
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>

pkg/nnbd_migration/lib/migration_cli.dart

@@ -4,7 +4,6 @@
 
 import 'dart:async';
 import 'dart:convert' show jsonDecode;
-import 'dart:math';
 import 'dart:io' hide File;
 
 import 'package:analyzer/dart/analysis/analysis_context_collection.dart';
@@ -34,6 +33,7 @@ import 'package:nnbd_migration/src/front_end/migration_state.dart';
 import 'package:nnbd_migration/src/front_end/non_nullable_fix.dart';
 import 'package:nnbd_migration/src/messages.dart';
 import 'package:nnbd_migration/src/utilities/json.dart' as json;
+import 'package:nnbd_migration/src/utilities/progress_bar.dart';
 import 'package:nnbd_migration/src/utilities/source_edit_diff_formatter.dart';
 import 'package:path/path.dart' show Context;
 
@@ -695,7 +695,8 @@ class MigrationCliRunner {
       int preferredPort,
       String summaryPath,
       @required String sdkPath}) {
-    return NonNullableFix(listener, resourceProvider, getLineInfo, bindAddress,
+    return NonNullableFix(
+        listener, resourceProvider, getLineInfo, bindAddress, logger,
         included: included,
         preferredPort: preferredPort,
         summaryPath: summaryPath,
@@ -1064,7 +1065,7 @@ class _FixCodeProcessor extends Object {
 
   Set<String> pathsToProcess;
 
-  _ProgressBar _progressBar;
+  ProgressBar _progressBar;
 
   final MigrationCliRunner _migrationCli;
 
@@ -1131,7 +1132,7 @@ class _FixCodeProcessor extends Object {
     var analysisErrors = <AnalysisError>[];
 
     // All tasks should be registered; [numPhases] should be finalized.
-    _progressBar = _ProgressBar(_migrationCli.logger, pathsToProcess.length);
+    _progressBar = ProgressBar(_migrationCli.logger, pathsToProcess.length);
 
     // Process each source file.
     await processResources((ResolvedUnitResult result) async {
@@ -1153,7 +1154,7 @@ class _FixCodeProcessor extends Object {
   }
 
   Future<MigrationState> runLaterPhases() async {
-    _progressBar = _ProgressBar(
+    _progressBar = ProgressBar(
         _migrationCli.logger, pathsToProcess.length * (numPhases - 1));
 
     await processResources((ResolvedUnitResult result) async {
@@ -1166,11 +1167,13 @@ class _FixCodeProcessor extends Object {
         await _task.finalizeUnit(result);
       }
     });
+    _progressBar.complete();
+    _migrationCli.logger.stdout(_migrationCli.ansi
+        .emphasized('Compiling instrumentation information...'));
     var state = await _task.finish();
     if (_migrationCli.options.webPreview) {
       await _task.startPreviewServer(state, _migrationCli.applyHook);
     }
-    _progressBar.complete();
     state.previewUrls = _task.previewUrls;
 
     return state;
@@ -1204,83 +1207,6 @@ class _IssueRenderer {
   }
 }
 
-/// A facility for drawing a progress bar in the terminal.
-///
-/// The bar is instantiated with the total number of "ticks" to be completed,
-/// and progress is made by calling [tick]. The bar is drawn across one entire
-/// line, like so:
-///
-///     [----------                                                   ]
-///
-/// The hyphens represent completed progress, and the whitespace represents
-/// remaining progress.
-///
-/// If there is no terminal, the progress bar will not be drawn.
-class _ProgressBar {
-  /// Whether the progress bar should be drawn.
-  /*late*/ bool _shouldDrawProgress;
-
-  /// The width of the terminal, in terms of characters.
-  /*late*/
-  int _width;
-
-  final Logger _logger;
-
-  /// The inner width of the terminal, in terms of characters.
-  ///
-  /// This represents the number of characters available for drawing progress.
-  /*late*/
-  int _innerWidth;
-
-  final int _totalTickCount;
-
-  int _tickCount = 0;
-
-  _ProgressBar(this._logger, this._totalTickCount) {
-    if (!stdout.hasTerminal) {
-      _shouldDrawProgress = false;
-    } else {
-      _shouldDrawProgress = true;
-      _width = stdout.terminalColumns;
-      _innerWidth = stdout.terminalColumns - 2;
-      _logger.write('[' + ' ' * _innerWidth + ']');
-    }
-  }
-
-  /// Clear the progress bar from the terminal, allowing other logging to be
-  /// printed.
-  void clear() {
-    if (!_shouldDrawProgress) {
-      return;
-    }
-    _logger.write('\r' + ' ' * _width + '\r');
-  }
-
-  /// Draw the progress bar as complete, and print two newlines.
-  void complete() {
-    if (!_shouldDrawProgress) {
-      return;
-    }
-    _logger.write('\r[' + '-' * _innerWidth + ']\n\n');
-  }
-
-  /// Progress the bar by one tick.
-  void tick() {
-    if (!_shouldDrawProgress) {
-      return;
-    }
-    _tickCount++;
-    var fractionComplete =
-        max(0, _tickCount * _innerWidth ~/ _totalTickCount - 1);
-    var remaining = _innerWidth - fractionComplete - 1;
-    _logger.write('\r[' + // Bring cursor back to the start of the line.
-        '-' * fractionComplete + // Print complete work.
-        AnsiProgress.kAnimationItems[_tickCount % 4] + // Print spinner.
-        ' ' * remaining + // Print remaining work.
-        ']');
-  }
-}
-
 extension on Severity {
   /// Returns the simple name of the Severity, as a String.
   String get name {

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

@@ -12,6 +12,7 @@ import 'package:analyzer_plugin/protocol/protocol_common.dart'
 import 'package:analyzer_plugin/protocol/protocol_common.dart' as protocol;
 import 'package:analyzer_plugin/src/utilities/navigation/navigation.dart';
 import 'package:analyzer_plugin/utilities/navigation/navigation_dart.dart';
+import 'package:cli_util/cli_logging.dart';
 import 'package:meta/meta.dart';
 import 'package:nnbd_migration/fix_reason_target.dart';
 import 'package:nnbd_migration/instrumentation.dart';
@@ -22,12 +23,16 @@ import 'package:nnbd_migration/src/front_end/driver_provider_impl.dart';
 import 'package:nnbd_migration/src/front_end/instrumentation_information.dart';
 import 'package:nnbd_migration/src/front_end/migration_info.dart';
 import 'package:nnbd_migration/src/front_end/offset_mapper.dart';
+import 'package:nnbd_migration/src/utilities/progress_bar.dart';
 
 /// A builder used to build the migration information for a library.
 class InfoBuilder {
   /// The node mapper for the migration state.
   NodeMapper nodeMapper;
 
+  /// The logger to use for showing progress when explaining the migration.
+  final Logger _logger;
+
   /// The resource provider used to access the file system.
   ResourceProvider provider;
 
@@ -49,7 +54,7 @@ class InfoBuilder {
 
   /// Initialize a newly created builder.
   InfoBuilder(this.provider, this.includedPath, this.info, this.listener,
-      this.migration, this.nodeMapper);
+      this.migration, this.nodeMapper, this._logger);
 
   /// The provider used to get information about libraries.
   DriverProviderImpl get driverProvider => listener.server;
@@ -60,7 +65,10 @@ class InfoBuilder {
     var sourceInfoMap = info.sourceInformation;
     Set<UnitInfo> units =
         SplayTreeSet<UnitInfo>((u1, u2) => u1.path.compareTo(u2.path));
+    var progressBar = ProgressBar(_logger, sourceInfoMap.length);
+
     for (var source in sourceInfoMap.keys) {
+      progressBar.tick();
       var filePath = source.fullName;
       var session = driverProvider.getAnalysisSession(filePath);
       if (!session.getFile(filePath).isPart) {
@@ -86,6 +94,7 @@ class InfoBuilder {
         }
       }
     }
+    progressBar.complete();
     return units;
   }
 

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

@@ -2,6 +2,7 @@
 // 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.
 
+import 'package:cli_util/cli_logging.dart';
 import 'package:nnbd_migration/instrumentation.dart';
 import 'package:nnbd_migration/migration_cli.dart';
 import 'package:nnbd_migration/nnbd_migration.dart';
@@ -41,16 +42,18 @@ class MigrationState {
 
   final AnalysisResult analysisResult;
 
+  /*late*/ List<String> previewUrls;
+
   /// Initialize a newly created migration state with the given values.
   MigrationState(this.migration, this.includedRoot, this.listener,
       this.instrumentationListener,
       [this.analysisResult]);
 
-  bool get hasErrors => analysisResult?.hasErrors ?? false;
-
   /// If the migration has been applied to disk.
   bool get hasBeenApplied => _hasBeenApplied;
 
+  bool get hasErrors => analysisResult?.hasErrors ?? false;
+
   /// Mark that the migration has been applied to disk.
   void markApplied() {
     assert(!hasBeenApplied);
@@ -58,17 +61,15 @@ class MigrationState {
   }
 
   /// Refresh the state of the migration after the migration has been updated.
-  Future<void> refresh() async {
+  Future<void> refresh(Logger logger) async {
     assert(!hasBeenApplied);
     var provider = listener.server.resourceProvider;
     var infoBuilder = InfoBuilder(provider, includedRoot,
-        instrumentationListener.data, listener, migration, nodeMapper);
+        instrumentationListener.data, listener, migration, nodeMapper, logger);
     var unitInfos = await infoBuilder.explainMigration();
     var pathContext = provider.pathContext;
     migrationInfo = MigrationInfo(
         unitInfos, infoBuilder.unitMap, pathContext, includedRoot);
     pathMapper = PathMapper(provider);
   }
-
-  /*late*/ List<String> previewUrls;
 }

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

@@ -10,6 +10,7 @@ import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/file_system/file_system.dart';
 import 'package:analyzer/src/generated/source.dart';
 import 'package:analyzer_plugin/protocol/protocol_common.dart';
+import 'package:cli_util/cli_logging.dart';
 import 'package:meta/meta.dart';
 import 'package:nnbd_migration/nnbd_migration.dart';
 import 'package:nnbd_migration/src/front_end/charcodes.dart';
@@ -36,6 +37,8 @@ class NonNullableFix {
   /// [InternetAddress].
   final Object bindAddress;
 
+  final Logger _logger;
+
   final int preferredPort;
 
   final DartFixListener listener;
@@ -76,8 +79,8 @@ class NonNullableFix {
   /// A list of the URLs corresponding to the included roots.
   List<String> previewUrls;
 
-  NonNullableFix(
-      this.listener, this.resourceProvider, this._getLineInfo, this.bindAddress,
+  NonNullableFix(this.listener, this.resourceProvider, this._getLineInfo,
+      this.bindAddress, this._logger,
       {List<String> included = const [],
       this.preferredPort,
       this.summaryPath,
@@ -113,7 +116,7 @@ class NonNullableFix {
     migration.finish();
     final state = MigrationState(
         migration, includedRoot, listener, instrumentationListener);
-    await state.refresh();
+    await state.refresh(_logger);
     return state;
   }
 
@@ -167,7 +170,7 @@ class NonNullableFix {
   Future<MigrationState> rerun() async {
     reset();
     var state = await rerunFunction();
-    await state.refresh();
+    await state.refresh(_logger);
     return state;
   }
 

pkg/nnbd_migration/lib/src/utilities/progress_bar.dart

@@ -0,0 +1,83 @@
+// Copyright (c) 2020, 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.
+
+import 'dart:io' hide File;
+import 'dart:math';
+
+import 'package:cli_util/cli_logging.dart';
+
+/// A facility for drawing a progress bar in the terminal.
+///
+/// The bar is instantiated with the total number of "ticks" to be completed,
+/// and progress is made by calling [tick]. The bar is drawn across one entire
+/// line, like so:
+///
+///     [----------                                                   ]
+///
+/// The hyphens represent completed progress, and the whitespace represents
+/// remaining progress.
+///
+/// If there is no terminal, the progress bar will not be drawn.
+class ProgressBar {
+  /// Whether the progress bar should be drawn.
+  /*late*/ bool _shouldDrawProgress;
+
+  /// The width of the terminal, in terms of characters.
+  /*late*/ int _width;
+
+  final Logger _logger;
+
+  /// The inner width of the terminal, in terms of characters.
+  ///
+  /// This represents the number of characters available for drawing progress.
+  /*late*/ int _innerWidth;
+
+  final int _totalTickCount;
+
+  int _tickCount = 0;
+
+  ProgressBar(this._logger, this._totalTickCount) {
+    if (!stdout.hasTerminal) {
+      _shouldDrawProgress = false;
+    } else {
+      _shouldDrawProgress = true;
+      _width = stdout.terminalColumns;
+      _innerWidth = stdout.terminalColumns - 2;
+      _logger.write('[' + ' ' * _innerWidth + ']');
+    }
+  }
+
+  /// Clear the progress bar from the terminal, allowing other logging to be
+  /// printed.
+  void clear() {
+    if (!_shouldDrawProgress) {
+      return;
+    }
+    _logger.write('\r' + ' ' * _width + '\r');
+  }
+
+  /// Draw the progress bar as complete, and print two newlines.
+  void complete() {
+    if (!_shouldDrawProgress) {
+      return;
+    }
+    _logger.write('\r[' + '-' * _innerWidth + ']\n\n');
+  }
+
+  /// Progress the bar by one tick.
+  void tick() {
+    if (!_shouldDrawProgress) {
+      return;
+    }
+    _tickCount++;
+    var fractionComplete =
+        max(0, _tickCount * _innerWidth ~/ _totalTickCount - 1);
+    var remaining = _innerWidth - fractionComplete - 1;
+    _logger.write('\r[' + // Bring cursor back to the start of the line.
+        '-' * fractionComplete + // Print complete work.
+        AnsiProgress.kAnimationItems[_tickCount % 4] + // Print spinner.
+        ' ' * remaining + // Print remaining work.
+        ']');
+  }
+}

pkg/nnbd_migration/test/front_end/nnbd_migration_test_base.dart

@@ -18,6 +18,7 @@ import 'package:test/test.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
 import 'analysis_abstract.dart';
+import '../utilities/test_logger.dart';
 
 @reflectiveTest
 class NnbdMigrationTestBase extends AbstractAnalysisTest {
@@ -223,8 +224,9 @@ class NnbdMigrationTestBase extends AbstractAnalysisTest {
     migration.finish();
     // Build the migration info.
     var info = instrumentationListener.data;
-    var builder = InfoBuilder(
-        resourceProvider, includedRoot, info, listener, migration, nodeMapper);
+    var logger = TestLogger(false);
+    var builder = InfoBuilder(resourceProvider, includedRoot, info, listener,
+        migration, nodeMapper, logger);
     infos = await builder.explainMigration();
   }
 

pkg/nnbd_migration/test/migration_cli_test.dart

@@ -33,6 +33,8 @@ import 'package:path/path.dart' as path;
 import 'package:test/test.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
+import 'utilities/test_logger.dart';
+
 main() {
   defineReflectiveSuite(() {
     defineReflectiveTests(_MigrationCliTestPosix);
@@ -65,11 +67,12 @@ class _ExceptionGeneratingNonNullableFix extends NonNullableFix {
       ResourceProvider resourceProvider,
       LineInfo Function(String) getLineInfo,
       Object bindAddress,
+      Logger logger,
       {List<String> included = const <String>[],
       int preferredPort,
       String summaryPath,
       @required String sdkPath})
-      : super(listener, resourceProvider, getLineInfo, bindAddress,
+      : super(listener, resourceProvider, getLineInfo, bindAddress, logger,
             included: included,
             preferredPort: preferredPort,
             summaryPath: summaryPath,
@@ -92,7 +95,7 @@ class _MigrationCli extends MigrationCli {
   _MigrationCli(this._test)
       : super(
             binaryName: 'nnbd_migration',
-            loggerFactory: (isVerbose) => _test.logger = _TestLogger(isVerbose),
+            loggerFactory: (isVerbose) => _test.logger = TestLogger(isVerbose),
             defaultSdkPathOverride:
                 _test.resourceProvider.convertPath(mock_sdk.sdkRoot),
             resourceProvider: _test.resourceProvider,
@@ -156,7 +159,7 @@ class _MigrationCliRunner extends MigrationCliRunner {
       @required String sdkPath}) {
     if (cli._test.injectArtificialException) {
       return _ExceptionGeneratingNonNullableFix(
-          listener, resourceProvider, getLineInfo, bindAddress,
+          listener, resourceProvider, getLineInfo, bindAddress, logger,
           included: included,
           preferredPort: preferredPort,
           summaryPath: summaryPath,
@@ -198,7 +201,7 @@ abstract class _MigrationCliTestBase {
 
   bool Function(String) overrideShouldBeMigrated;
 
-  set logger(_TestLogger logger);
+  set logger(TestLogger logger);
 
   _MockProcessManager get processManager;
 
@@ -207,7 +210,7 @@ abstract class _MigrationCliTestBase {
 
 mixin _MigrationCliTestMethods on _MigrationCliTestBase {
   @override
-  /*late*/ _TestLogger logger;
+  /*late*/ TestLogger logger;
 
   final hasVerboseHelpMessage = contains('for verbose help output');
 
@@ -308,7 +311,7 @@ mixin _MigrationCliTestMethods on _MigrationCliTestBase {
       String pubOutdatedStderr = ''}) {
     processManager._mockResult = ProcessResult(123 /* pid */,
         pubOutdatedExitCode, pubOutdatedStdout, pubOutdatedStderr);
-    logger = _TestLogger(true);
+    logger = TestLogger(true);
     var projectContents = simpleProject(sourceText: 'int x;');
     var projectDir = createProjectDir(projectContents);
     var success = DependencyChecker(
@@ -323,7 +326,7 @@ mixin _MigrationCliTestMethods on _MigrationCliTestBase {
       String pubOutdatedStderr = ''}) {
     processManager._mockResult = ProcessResult(123 /* pid */,
         pubOutdatedExitCode, pubOutdatedStdout, pubOutdatedStderr);
-    logger = _TestLogger(true);
+    logger = TestLogger(true);
     var projectContents = simpleProject(sourceText: 'int x;');
     var projectDir = createProjectDir(projectContents);
     var success = DependencyChecker(
@@ -2049,52 +2052,3 @@ class _MockProcessManager implements ProcessManager {
         '' /* stderr */,
       );
 }
-
-/// TODO(paulberry): move into cli_util
-class _TestLogger implements Logger {
-  final stderrBuffer = StringBuffer();
-
-  final stdoutBuffer = StringBuffer();
-
-  final bool isVerbose;
-
-  _TestLogger(this.isVerbose);
-
-  @override
-  Ansi get ansi => Ansi(false);
-
-  @override
-  void flush() {
-    throw UnimplementedError('TODO(paulberry)');
-  }
-
-  @override
-  Progress progress(String message) {
-    return SimpleProgress(this, message);
-  }
-
-  @override
-  void stderr(String message) {
-    stderrBuffer.writeln(message);
-  }
-
-  @override
-  void stdout(String message) {
-    stdoutBuffer.writeln(message);
-  }
-
-  @override
-  void trace(String message) {
-    throw UnimplementedError('TODO(paulberry)');
-  }
-
-  @override
-  void write(String message) {
-    stdoutBuffer.write(message);
-  }
-
-  @override
-  void writeCharCode(int charCode) {
-    stdoutBuffer.writeCharCode(charCode);
-  }
-}

pkg/nnbd_migration/test/utilities/test_logger.dart

@@ -0,0 +1,54 @@
+// Copyright (c) 2020, 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.
+
+import 'package:cli_util/cli_logging.dart';
+
+/// TODO(paulberry): move into cli_util
+class TestLogger implements Logger {
+  final stderrBuffer = StringBuffer();
+
+  final stdoutBuffer = StringBuffer();
+
+  final bool isVerbose;
+
+  TestLogger(this.isVerbose);
+
+  @override
+  Ansi get ansi => Ansi(false);
+
+  @override
+  void flush() {
+    throw UnimplementedError('TODO(paulberry)');
+  }
+
+  @override
+  Progress progress(String message) {
+    return SimpleProgress(this, message);
+  }
+
+  @override
+  void stderr(String message) {
+    stderrBuffer.writeln(message);
+  }
+
+  @override
+  void stdout(String message) {
+    stdoutBuffer.writeln(message);
+  }
+
+  @override
+  void trace(String message) {
+    throw UnimplementedError('TODO(paulberry)');
+  }
+
+  @override
+  void write(String message) {
+    stdoutBuffer.write(message);
+  }
+
+  @override
+  void writeCharCode(int charCode) {
+    stdoutBuffer.writeCharCode(charCode);
+  }
+}