Improve handling of disable-dartdev-analytics

This is second try of https://dart-review.googlesource.com/c/sdk/+/171284
that was reverted to to faulty logic in main_options.

Some other refactorings are piggy-backed along.

TestProject.runSync no longer takes a 'command' argument. It was anyway
often not an argument.

Also stop the messy handling of pub arguments. It is no longer needed.

BUG: #44135
Change-Id: I49abf5810d9ea262409ba9d93f0471037cb8a753
TEST=The VM change is tested via all the pkg/dartdev/test/command/* tests that invoke dart with the --no-analytics flag.
TEST=Furthermore manual test that the --no-analytics flag is passed to dartdev.
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/174261
Reviewed-by: Jonas Jensen <[email protected]>
Commit-Queue: Sigurd Meldgaard <[email protected]>

Author: sigurdm

pkg/dartdev/lib/dartdev.dart

@@ -32,129 +32,34 @@ import 'src/vm_interop_handler.dart';
 /// analytics logic, it has been moved here.
 Future<void> runDartdev(List<String> args, SendPort port) async {
   VmInteropHandler.initialize(port);
-
-  int result;
-
-  // The exit code for the dartdev process; null indicates that it has not been
-  // set yet. The value is set in the catch and finally blocks below.
-  int exitCode;
-
-  // Any caught non-UsageExceptions when running the sub command.
-  Object exception;
-  StackTrace stackTrace;
-
-  // The Analytics instance used to report information back to Google Analytics;
-  // see lib/src/analytics.dart.
-  final analytics = createAnalyticsInstance(
-    args.contains('--disable-dartdev-analytics'),
-  );
-
-  // If we have not printed the analyticsNoticeOnFirstRunMessage to stdout,
-  // the user is on a terminal, and the machine is not a bot, then print the
-  // disclosure and set analytics.disclosureShownOnTerminal to true.
-  if (analytics is DartdevAnalytics &&
-      !analytics.disclosureShownOnTerminal &&
-      io.stdout.hasTerminal &&
-      !isBot()) {
-    print(analyticsNoticeOnFirstRunMessage);
-    analytics.disclosureShownOnTerminal = true;
+  if (args.contains('run')) {
+    // These flags have a format that can't be handled by package:args, so while
+    // they are valid flags we'll assume the VM has verified them by this point.
+    args = args
+        .where(
+          (element) => !(element.contains('--observe') ||
+              element.contains('--enable-vm-service')),
+        )
+        .toList();
   }
 
-  // When `--disable-analytics` or `--enable-analytics` are called we perform
-  // the respective intention and print any notices to standard out and exit.
-  if (args.contains('--disable-analytics')) {
-    // This block also potentially catches the case of (disableAnalytics &&
-    // enableAnalytics), in which we favor the disabling of analytics.
-    analytics.enabled = false;
-
-    // Alert the user that analytics has been disabled.
-    print(analyticsDisabledNoticeMessage);
-    VmInteropHandler.exit(0);
-    return;
-  } else if (args.contains('--enable-analytics')) {
-    analytics.enabled = true;
-
-    // Alert the user again that anonymous data will be collected.
-    print(analyticsNoticeOnFirstRunMessage);
-    VmInteropHandler.exit(0);
-    return;
-  }
+  // Finally, call the runner to execute the command; see DartdevRunner.
 
+  final runner = DartdevRunner(args);
+  var exitCode = 1;
   try {
-    final runner = DartdevRunner(args, analytics);
-
-    // Run can't be called with the '--disable-dartdev-analytics' flag; remove
-    // it if it is contained in args.
-    if (args.contains('--disable-dartdev-analytics')) {
-      args = List.from(args)..remove('--disable-dartdev-analytics');
-    }
-
-    if (args.contains('run')) {
-      // These flags have a format that can't be handled by package:args, so while
-      // they are valid flags we'll assume the VM has verified them by this point.
-      args = args
-          .where(
-            (element) => !(element.contains('--observe') ||
-                element.contains('--enable-vm-service')),
-          )
-          .toList();
-    }
-
-    // If ... help pub ... is in the args list, remove 'help', and add '--help'
-    // to the end of the list. This will make it possible to use the help
-    // command to access subcommands of pub such as `dart help pub publish`; see
-    // https://github.com/dart-lang/sdk/issues/42965.
-    if (PubUtils.shouldModifyArgs(args, runner.commands.keys.toList())) {
-      args = PubUtils.modifyArgs(args);
-    }
-
-    // Finally, call the runner to execute the command; see DartdevRunner.
-    result = await runner.run(args);
-  } catch (e, st) {
-    if (e is UsageException) {
-      io.stderr.writeln('$e');
-      exitCode = 64;
-    } else {
-      // Set the exception and stack trace only for non-UsageException cases:
-      exception = e;
-      stackTrace = st;
-      io.stderr.writeln('$e');
-      io.stderr.writeln('$st');
-      exitCode = 1;
-    }
+    exitCode = await runner.run(args);
+  } on UsageException catch (e) {
+    // TODO(sigurdm): It is unclear when a UsageException gets to here, and
+    // when it is in DartdevRunner.runCommand.
+    io.stderr.writeln('$e');
+    exitCode = 64;
   } finally {
-    // Set the exitCode, if it wasn't set in the catch block above.
-    exitCode ??= result ?? 0;
-
-    // Send analytics before exiting
-    if (analytics.enabled) {
-      // And now send the exceptions and events to Google Analytics:
-      if (exception != null) {
-        unawaited(
-          analytics.sendException(
-              '${exception.runtimeType}\n${sanitizeStacktrace(stackTrace)}',
-              fatal: true),
-        );
-      }
-
-      await analytics.waitForLastPing(
-          timeout: const Duration(milliseconds: 200));
-    }
-
-    // Set the enabled flag in the analytics object to true. Note: this will not
-    // enable the analytics unless the disclosure was shown (terminal detected),
-    // and the machine is not detected to be a bot.
-    if (analytics.firstRun) {
-      analytics.enabled = true;
-    }
-    analytics.close();
     VmInteropHandler.exit(exitCode);
   }
 }
 
 class DartdevRunner extends CommandRunner<int> {
-  final Analytics analytics;
-
   @override
   final ArgParser argParser = ArgParser(
     usageLineLength: dartdevUsageLineLength,
@@ -164,8 +69,7 @@ class DartdevRunner extends CommandRunner<int> {
   static const String dartdevDescription =
       'A command-line utility for Dart development';
 
-  DartdevRunner(List<String> args, this.analytics)
-      : super('dart', '$dartdevDescription.') {
+  DartdevRunner(List<String> args) : super('dart', '$dartdevDescription.') {
     final bool verbose = args.contains('-v') || args.contains('--verbose');
 
     argParser.addFlag('verbose',
@@ -180,12 +84,9 @@ class DartdevRunner extends CommandRunner<int> {
     argParser.addFlag('diagnostics',
         negatable: false, help: 'Show tool diagnostic output.', hide: !verbose);
 
-    // A hidden flag to disable analytics on this run, this constructor can be
-    // called with this flag, but should be removed before run() is called as
-    // the flag has not been added to all sub-commands.
     argParser.addFlag(
-      'disable-dartdev-analytics',
-      negatable: false,
+      'analytics',
+      negatable: true,
       help: 'Disable anonymous analytics for this `dart *` run',
       hide: true,
     );
@@ -212,7 +113,38 @@ class DartdevRunner extends CommandRunner<int> {
   @override
   Future<int> runCommand(ArgResults topLevelResults) async {
     final stopwatch = Stopwatch()..start();
-    assert(!topLevelResults.arguments.contains('--disable-dartdev-analytics'));
+    // The Analytics instance used to report information back to Google Analytics;
+    // see lib/src/analytics.dart.
+    final analytics = createAnalyticsInstance(!topLevelResults['analytics']);
+
+    // If we have not printed the analyticsNoticeOnFirstRunMessage to stdout,
+    // the user is on a terminal, and the machine is not a bot, then print the
+    // disclosure and set analytics.disclosureShownOnTerminal to true.
+    if (analytics is DartdevAnalytics &&
+        !analytics.disclosureShownOnTerminal &&
+        io.stdout.hasTerminal &&
+        !isBot()) {
+      print(analyticsNoticeOnFirstRunMessage);
+      analytics.disclosureShownOnTerminal = true;
+    }
+
+    // When `--disable-analytics` or `--enable-analytics` are called we perform
+    // the respective intention and print any notices to standard out and exit.
+    if (topLevelResults['disable-analytics']) {
+      // This block also potentially catches the case of (disableAnalytics &&
+      // enableAnalytics), in which we favor the disabling of analytics.
+      analytics.enabled = false;
+
+      // Alert the user that analytics has been disabled.
+      print(analyticsDisabledNoticeMessage);
+      return 0;
+    } else if (topLevelResults['enable-analytics']) {
+      analytics.enabled = true;
+
+      // Alert the user again that anonymous data will be collected.
+      print(analyticsNoticeOnFirstRunMessage);
+      return 0;
+    }
 
     if (topLevelResults.command == null &&
         topLevelResults.arguments.isNotEmpty) {
@@ -222,14 +154,12 @@ class DartdevRunner extends CommandRunner<int> {
         io.stderr.writeln(
             "Error when reading '$firstArg': No such file or directory.");
         // This is the exit code used by the frontend.
-        VmInteropHandler.exit(254);
+        return 254;
       }
     }
 
-    isDiagnostics = topLevelResults['diagnostics'];
-
     final Ansi ansi = Ansi(Ansi.terminalSupportsAnsi);
-    log = isDiagnostics
+    log = topLevelResults['diagnostics']
         ? Logger.verbose(ansi: ansi)
         : Logger.standard(ansi: ansi);
 
@@ -247,8 +177,15 @@ class DartdevRunner extends CommandRunner<int> {
       analytics.sendScreenView(path),
     );
 
+    // The exit code for the dartdev process; null indicates that it has not been
+    // set yet. The value is set in the catch and finally blocks below.
+    int exitCode;
+
+    // Any caught non-UsageExceptions when running the sub command.
+    Object exception;
+    StackTrace stackTrace;
     try {
-      final exitCode = await super.runCommand(topLevelResults);
+      exitCode = await super.runCommand(topLevelResults);
 
       if (path != null && analytics.enabled) {
         // Send the event to analytics
@@ -268,8 +205,16 @@ class DartdevRunner extends CommandRunner<int> {
           ),
         );
       }
-
-      return exitCode;
+    } on UsageException catch (e) {
+      io.stderr.writeln('$e');
+      exitCode = 64;
+    } catch (e, st) {
+      // Set the exception and stack trace only for non-UsageException cases:
+      exception = e;
+      stackTrace = st;
+      io.stderr.writeln('$e');
+      io.stderr.writeln('$st');
+      exitCode = 1;
     } finally {
       stopwatch.stop();
       if (analytics.enabled) {
@@ -281,6 +226,32 @@ class DartdevRunner extends CommandRunner<int> {
           ),
         );
       }
+      // Set the exitCode, if it wasn't set in the catch block above.
+      exitCode ??= 0;
+
+      // Send analytics before exiting
+      if (analytics.enabled) {
+        // And now send the exceptions and events to Google Analytics:
+        if (exception != null) {
+          unawaited(
+            analytics.sendException(
+                '${exception.runtimeType}\n${sanitizeStacktrace(stackTrace)}',
+                fatal: true),
+          );
+        }
+
+        await analytics.waitForLastPing(
+            timeout: const Duration(milliseconds: 200));
+      }
+
+      // Set the enabled flag in the analytics object to true. Note: this will not
+      // enable the analytics unless the disclosure was shown (terminal detected),
+      // and the machine is not detected to be a bot.
+      if (analytics.firstRun) {
+        analytics.enabled = true;
+      }
+      analytics.close();
+      return exitCode;
     }
   }
 }

pkg/dartdev/lib/src/analytics.dart

@@ -57,7 +57,7 @@ Analytics createAnalyticsInstance(bool disableAnalytics) {
   }
 
   if (disableAnalytics) {
-    // Dartdev tests pass a hidden 'disable-dartdev-analytics' flag which is
+    // Dartdev tests pass a hidden 'no-analytics' flag which is
     // handled here.
     //
     // Also, stdout.hasTerminal is checked; if there is no terminal we infer

pkg/dartdev/lib/src/utils.dart

@@ -38,31 +38,6 @@ String trimEnd(String s, String suffix) {
   return s;
 }
 
-/// Static util methods used in dartdev to potentially modify the order of the
-/// arguments passed into dartdev.
-class PubUtils {
-  /// If [doModifyArgs] returns true, then this method returns a modified copy
-  /// of the argument list, 'help' is removed from the interior of the list, and
-  /// '--help' is added to the end of the list of arguments. This method returns
-  /// a modified copy of the list, the list itself is not modified.
-  static List<String> modifyArgs(List<String> args) => List.from(args)
-    ..remove('help')
-    ..add('--help');
-
-  /// If ... help pub ..., and no other verb (such as 'analyze') appears before
-  /// the ... help pub ... in the argument list, then return true.
-  static bool shouldModifyArgs(List<String> args, List<String> allCmds) =>
-      args != null &&
-      allCmds != null &&
-      args.isNotEmpty &&
-      allCmds.isNotEmpty &&
-      args.firstWhere((arg) => allCmds.contains(arg), orElse: () => '') ==
-          'help' &&
-      args.contains('help') &&
-      args.contains('pub') &&
-      args.indexOf('help') + 1 == args.indexOf('pub');
-}
-
 extension FileSystemEntityExtension on FileSystemEntity {
   String get name => p.basename(path);