[ CLI / VM ] Add --enable-experiment support for 'dart compile', change

where --enable experiment is accepted

Adds --enable-experiment support for:
  - dart compile aot-snapshot
  - dart compile jit-snapshot
  - dart compile js
  - dart compile exe
  - dart compile kernel

Also changes --enable-experiment from a top level CLI flag to a
per-command flag. --enable-experiment flags will now only be
interpreted by the VM if:

1) They are provided before a CLI command (e.g., dart
--enable-experiment=non-nullable analyze) or

2) They are provided with an explicit or implicit run command (e.g.,
dart --enable-experiment=non-nullable foo.dart or dart
--enable-experiment=non-nullable run foo.dart or dart run
--enable-experiment=non-nullable foo.dart)

This should make it more generally clear where --enable-experiment is
accepted and what subcommands can accept the flag. Prior to this change,
providing --enable-experiment anywhere, even for commands without
experiment support, would not raise an error.

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

Change-Id: I5ec48b2dd2bb6db5526185dae2edbca95ef24d9f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/169902
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Sigurd Meldgaard <sigurdm@google.com>
Reviewed-by: Jonas Jensen <jonasfj@google.com>

pkg/dartdev/lib/dartdev.dart

@@ -6,7 +6,6 @@
 import 'dart:io' as io hide exit;
 import 'dart:isolate';
 
-import 'package:analyzer/src/dart/analysis/experiments.dart';
 import 'package:args/args.dart';
 import 'package:args/command_runner.dart';
 import 'package:cli_util/cli_logging.dart';
@@ -172,8 +171,6 @@ class DartdevRunner extends CommandRunner<int> {
     argParser.addFlag('disable-analytics',
         negatable: false, help: 'Disable anonymous analytics.');
 
-    addExperimentalFlags(argParser, verbose);
-
     argParser.addFlag('diagnostics',
         negatable: false, help: 'Show tool diagnostic output.', hide: !verbose);
 
@@ -189,7 +186,7 @@ class DartdevRunner extends CommandRunner<int> {
 
     addCommand(AnalyzeCommand());
     addCommand(CreateCommand(verbose: verbose));
-    addCommand(CompileCommand());
+    addCommand(CompileCommand(verbose: verbose));
     addCommand(FixCommand());
     addCommand(FormatCommand(verbose: verbose));
     addCommand(MigrateCommand(verbose: verbose));
@@ -206,26 +203,6 @@ class DartdevRunner extends CommandRunner<int> {
   String get invocation =>
       'dart [<vm-flags>] <command|dart-file> [<arguments>]';
 
-  void addExperimentalFlags(ArgParser argParser, bool verbose) {
-    List<ExperimentalFeature> features = experimentalFeatures;
-
-    Map<String, String> allowedHelp = {};
-    for (ExperimentalFeature feature in features) {
-      String suffix =
-          feature.isEnabledByDefault ? ' (no-op - enabled by default)' : '';
-      allowedHelp[feature.enableString] = '${feature.documentation}$suffix';
-    }
-
-    argParser.addMultiOption(
-      experimentFlagName,
-      valueHelp: 'experiment',
-      allowedHelp: verbose ? allowedHelp : null,
-      help: 'Enable one or more experimental features '
-          '(see dart.dev/go/experiments).',
-      hide: !verbose,
-    );
-  }
-
   @override
   Future<int> runCommand(ArgResults topLevelResults) async {
     final stopwatch = Stopwatch()..start();
@@ -250,18 +227,6 @@ class DartdevRunner extends CommandRunner<int> {
         ? Logger.verbose(ansi: ansi)
         : Logger.standard(ansi: ansi);
 
-    if (topLevelResults.wasParsed(experimentFlagName)) {
-      List<String> experimentIds = topLevelResults[experimentFlagName];
-      for (ExperimentalFeature feature in experimentalFeatures) {
-        // We allow default true flags, but complain when they are passed in.
-        if (feature.isEnabledByDefault &&
-            experimentIds.contains(feature.enableString)) {
-          print("'${feature.enableString}' is now enabled by default; this "
-              'flag is no longer required.');
-        }
-      }
-    }
-
     var command = topLevelResults.command;
     final commandNames = [];
     while (command != null) {
@@ -276,10 +241,6 @@ class DartdevRunner extends CommandRunner<int> {
       analyticsInstance.sendScreenView(path),
     );
 
-    final topLevelCommand = topLevelResults.command == null
-        ? null
-        : commands[topLevelResults.command.name];
-
     try {
       final exitCode = await super.runCommand(topLevelResults);
 
@@ -299,7 +260,7 @@ class DartdevRunner extends CommandRunner<int> {
                 //
                 // Note that this will also conflate short-options and long-options.
                 command?.options?.where(command.wasParsed)?.toList(),
-            specifiedExperiments: topLevelCommand?.specifiedExperiments,
+            specifiedExperiments: topLevelResults.enabledExperiments,
           ),
         );
       }

pkg/dartdev/lib/src/commands/compile.dart

@@ -1,4 +1,4 @@
-// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
+// Copyright (c) 2019, 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.
 
@@ -9,6 +9,7 @@ import 'package:dart2native/generate.dart';
 import 'package:path/path.dart' as path;
 
 import '../core.dart';
+import '../experiments.dart';
 import '../sdk.dart';
 import '../vm_interop_handler.dart';
 
@@ -45,7 +46,8 @@ bool checkFile(String sourcePath) {
 class CompileJSCommand extends CompileSubcommandCommand {
   static const String cmdName = 'js';
 
-  CompileJSCommand() : super(cmdName, 'Compile Dart to JavaScript.') {
+  CompileJSCommand({bool verbose})
+      : super(cmdName, 'Compile Dart to JavaScript.') {
     argParser
       ..addOption(
         commonOptions['outputFile'].flag,
@@ -58,6 +60,7 @@ class CompileJSCommand extends CompileSubcommandCommand {
         abbr: 'm',
         negatable: false,
       );
+    addExperimentalFlags(argParser, verbose);
   }
 
   @override
@@ -91,6 +94,8 @@ class CompileJSCommand extends CompileSubcommandCommand {
 
     VmInteropHandler.run(sdk.dart2jsSnapshot, [
       '--libraries-spec=$librariesPath',
+      if (argResults.enabledExperiments.isNotEmpty)
+        "--enable-experiment=${argResults.enabledExperiments.join(',')}",
       ...argResults.arguments,
     ]);
 
@@ -112,6 +117,7 @@ class CompileSnapshotCommand extends CompileSubcommandCommand {
     this.help,
     this.fileExt,
     this.formatName,
+    bool verbose,
   }) : super(commandName, 'Compile Dart $help') {
     argParser
       ..addOption(
@@ -119,6 +125,7 @@ class CompileSnapshotCommand extends CompileSubcommandCommand {
         help: commonOptions['outputFile'].help,
         abbr: commonOptions['outputFile'].abbr,
       );
+    addExperimentalFlags(argParser, verbose);
   }
 
   @override
@@ -144,10 +151,14 @@ class CompileSnapshotCommand extends CompileSubcommandCommand {
       outputFile = '$inputWithoutDart.$fileExt';
     }
 
+    final enabledExperiments = argResults.enabledExperiments;
     // Build arguments.
     List<String> args = [];
     args.add('--snapshot-kind=$formatName');
     args.add('--snapshot=${path.canonicalize(outputFile)}');
+    if (enabledExperiments.isNotEmpty) {
+      args.add("--enable-experiment=${enabledExperiments.join(',')}");
+    }
     if (verbose) {
       args.add('-v');
     }
@@ -173,6 +184,7 @@ class CompileNativeCommand extends CompileSubcommandCommand {
     this.commandName,
     this.format,
     this.help,
+    bool verbose,
   }) : super(commandName, 'Compile Dart $help') {
     argParser
       ..addOption(
@@ -195,6 +207,8 @@ For example: dart compile $commandName --packages=/tmp/pkgs main.dart''')
       ..addOption('save-debugging-info', abbr: 'S', valueHelp: 'path', help: '''
 Remove debugging information from the output and save it separately to the specified file.
 <path> can be relative or absolute.''');
+
+    addExperimentalFlags(argParser, verbose);
   }
 
   @override
@@ -225,6 +239,7 @@ Remove debugging information from the output and save it separately to the speci
         defines: argResults['define'],
         packages: argResults['packages'],
         enableAsserts: argResults['enable-asserts'],
+        enableExperiment: argResults.enabledExperiments.join(','),
         debugFile: argResults['save-debugging-info'],
         verbose: verbose,
       );
@@ -245,30 +260,36 @@ abstract class CompileSubcommandCommand extends DartdevCommand {
 
 class CompileCommand extends DartdevCommand {
   static const String cmdName = 'compile';
-
-  CompileCommand() : super(cmdName, 'Compile Dart to various formats.') {
-    addSubcommand(CompileJSCommand());
+  CompileCommand({bool verbose = false})
+      : super(cmdName, 'Compile Dart to various formats.') {
+    addSubcommand(CompileJSCommand(
+      verbose: verbose,
+    ));
     addSubcommand(CompileSnapshotCommand(
       commandName: CompileSnapshotCommand.jitSnapshotCmdName,
       help: 'to a JIT snapshot.',
       fileExt: 'jit',
       formatName: 'app-jit',
+      verbose: verbose,
     ));
     addSubcommand(CompileSnapshotCommand(
       commandName: CompileSnapshotCommand.kernelCmdName,
       help: 'to a kernel snapshot.',
       fileExt: 'dill',
       formatName: 'kernel',
+      verbose: verbose,
     ));
     addSubcommand(CompileNativeCommand(
       commandName: CompileNativeCommand.exeCmdName,
       help: 'to a self-contained executable.',
       format: 'exe',
+      verbose: verbose,
     ));
     addSubcommand(CompileNativeCommand(
       commandName: CompileNativeCommand.aotSnapshotCmdName,
       help: 'to an AOT snapshot.',
       format: 'aot',
+      verbose: verbose,
     ));
   }
 }

pkg/dartdev/lib/src/commands/pub.dart

@@ -63,20 +63,19 @@ class PubCommand extends DartdevCommand {
     final command = sdk.pubSnapshot;
     var args = argResults.arguments;
 
+    final enabledExperiments = argResults.enabledExperiments;
     // Pass any --enable-experiment options along.
-    if (args.isNotEmpty && wereExperimentsSpecified) {
-      List<String> experimentIds = specifiedExperiments;
-
+    if (args.isNotEmpty && enabledExperiments.isNotEmpty) {
       if (args.first == 'run') {
         args = [
           ...args.sublist(0, 1),
-          '--$experimentFlagName=${experimentIds.join(',')}',
+          '--$experimentFlagName=${enabledExperiments.join(',')}',
           ...args.sublist(1),
         ];
       } else if (args.length > 1 && args[0] == 'global' && args[0] == 'run') {
         args = [
           ...args.sublist(0, 2),
-          '--$experimentFlagName=${experimentIds.join(',')}',
+          '--$experimentFlagName=${enabledExperiments.join(',')}',
           ...args.sublist(2),
         ];
       }

pkg/dartdev/lib/src/commands/run.dart

@@ -145,6 +145,7 @@ class RunCommand extends DartdevCommand {
         negatable: false,
         help: 'Enables tracing of library and script loading.',
       );
+    addExperimentalFlags(argParser, verbose);
   }
 
   @override
@@ -216,15 +217,6 @@ class RunCommand extends DartdevCommand {
       }
     }
 
-    // Pass any --enable-experiment options along.
-    if (args.isNotEmpty && wereExperimentsSpecified) {
-      List<String> experimentIds = specifiedExperiments;
-      args = [
-        '--$experimentFlagName=${experimentIds.join(',')}',
-        ...args,
-      ];
-    }
-
     // If the user wants to start a debugging session we need to do some extra
     // work and spawn a Dart Development Service (DDS) instance. DDS is a VM
     // service intermediary which implements the VM service protocol and

pkg/dartdev/lib/src/commands/test.dart

@@ -58,11 +58,14 @@ class TestCommand extends DartdevCommand {
       _printMissingDepInstructions(isHelpCommand);
       return 65;
     }
-
+    List<String> enabledExperiments = [];
+    if (!(testArgs.length == 1 && testArgs[0] == '-h')) {
+      enabledExperiments = argResults.enabledExperiments;
+    }
     final args = [
       'run',
-      if (wereExperimentsSpecified)
-        '--$experimentFlagName=${specifiedExperiments.join(',')}',
+      if (enabledExperiments.isNotEmpty)
+        '--$experimentFlagName=${enabledExperiments.join(',')}',
       'test',
       ...testArgs,
     ];

pkg/dartdev/lib/src/events.dart

@@ -56,7 +56,8 @@ Future<void> sendUsageEvent(
   /// pattern from the flutter cli tool which always passes 'flutter' as the
   /// category.
   final category = _dartdev;
-  commandFlags = commandFlags?.toList() ?? [];
+  commandFlags =
+      commandFlags?.where((e) => e != 'enable-experiment')?.toList() ?? [];
   specifiedExperiments = specifiedExperiments?.toList() ?? [];
 
   // Sort the flag lists to slightly reduce the explosion of possibilities.

pkg/dartdev/lib/src/experiments.dart

@@ -3,6 +3,7 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'package:analyzer/src/dart/analysis/experiments.dart';
+import 'package:args/args.dart';
 
 const experimentFlagName = 'enable-experiment';
 
@@ -14,3 +15,53 @@ List<ExperimentalFeature> get experimentalFeatures {
   features.sort((a, b) => a.enableString.compareTo(b.enableString));
   return features;
 }
+
+void addExperimentalFlags(ArgParser argParser, bool verbose) {
+  List<ExperimentalFeature> features = experimentalFeatures;
+
+  Map<String, String> allowedHelp = {};
+  for (ExperimentalFeature feature in features) {
+    String suffix =
+        feature.isEnabledByDefault ? ' (no-op - enabled by default)' : '';
+    allowedHelp[feature.enableString] = '${feature.documentation}$suffix';
+  }
+
+  argParser.addMultiOption(
+    experimentFlagName,
+    valueHelp: 'experiment',
+    allowedHelp: verbose ? allowedHelp : null,
+    help: 'Enable one or more experimental features '
+        '(see dart.dev/go/experiments).',
+    hide: !verbose,
+  );
+}
+
+extension EnabledExperimentsArg on ArgResults {
+  List<String> get enabledExperiments {
+    List<String> enabledExperiments = [];
+    if (options.contains(experimentFlagName)) {
+      if (wasParsed(experimentFlagName)) {
+        enabledExperiments = this[experimentFlagName];
+      }
+    } else {
+      String experiments = arguments.firstWhere(
+        (e) => e.startsWith('--enable-experiment'),
+        orElse: () => null,
+      );
+      if (experiments == null) {
+        return [];
+      }
+      enabledExperiments = experiments.split('=')[1].split(',');
+    }
+
+    for (ExperimentalFeature feature in experimentalFeatures) {
+      // We allow default true flags, but complain when they are passed in.
+      if (feature.isEnabledByDefault &&
+          enabledExperiments.contains(feature.enableString)) {
+        print("'${feature.enableString}' is now enabled by default; this "
+            'flag is no longer required.');
+      }
+    }
+    return enabledExperiments;
+  }
+}

pkg/dartdev/test/analytics_test.dart

@@ -182,10 +182,10 @@ void main() {
 
     test('run --enable-experiments', () {
       final p = project(
-          mainSrc: 'void main(List<String> args) => print(args)',
+          mainSrc: 'void main(List<String> args) => print(args);',
           logAnalytics: true);
-      final result = p.runSync('--enable-experiment=non-nullable', [
-        'run',
+      final result = p.runSync('run', [
+        '--enable-experiment=non-nullable',
         'lib/main.dart',
       ]);
       expect(extractAnalytics(result), [

pkg/dartdev/test/commands/flag_test.dart

@@ -4,7 +4,6 @@
 
 import 'dart:io';
 
-import 'package:args/args.dart';
 import 'package:args/command_runner.dart';
 import 'package:dartdev/dartdev.dart';
 import 'package:test/test.dart';
@@ -48,17 +47,6 @@ void command() {
       }
     });
   });
-
-  test('enable experiments flag is supported', () {
-    final args = [
-      '--disable-dartdev-analytics',
-      '--enable-experiment=non-nullable'
-    ];
-    final runner = DartdevRunner(args);
-    ArgResults results = runner.parse(args);
-    expect(results['enable-experiment'], isNotEmpty);
-    expect(results['enable-experiment'].first, 'non-nullable');
-  });
 }
 
 void help() {

pkg/dartdev/test/commands/pub_test.dart

@@ -69,20 +69,20 @@ void pub() {
   test('run --enable-experiment', () {
     p = project();
     p.file('bin/main.dart',
-        "void main() { int a; a = null; print('a is \$a.'); }");
+        "void main() { int? a; a = null; print('a is \$a.'); }");
 
     // run 'pub get'
     p.runSync('pub', ['get']);
 
     var result = p.runSync(
-        'pub', ['run', '--enable-experiment=non-nullable', 'main.dart']);
+        'pub', ['run', '--enable-experiment=no-non-nullable', 'main.dart']);
 
     expect(result.exitCode, 254);
     expect(result.stdout, isEmpty);
     expect(
         result.stderr,
-        contains("A value of type 'Null' can't be assigned to a variable of "
-            "type 'int'"));
+        contains('bin/main.dart:1:18: Error: This requires the \'non-nullable\''
+            ' language feature to be enabled.\n'));
   });
 
   test('failure', () {

pkg/dartdev/test/commands/run_test.dart

@@ -145,7 +145,7 @@ void run() {
     expect(
       result.stdout,
       matches(
-          r'Observatory listening on http:\/\/127.0.0.1:8181\/[a-zA-Z0-9]+=\/\n.*'),
+          r'Observatory listening on http:\/\/127.0.0.1:8181\/[a-zA-Z0-9_-]+=\/\n.*'),
     );
     expect(result.stderr, isEmpty);
     expect(result.exitCode, 0);

runtime/bin/main_options.cc

@@ -557,44 +557,6 @@ int Options::ParseArguments(int argc,
 
   vm_options->AddArguments(vm_argv, vm_argc);
 
-#if !defined(DART_PRECOMPILED_RUNTIME)
-  if (!enabled_experiments_.is_empty()) {
-    intptr_t num_experiments = enabled_experiments_.length();
-    if (!(Options::disable_dart_dev() || run_script)) {
-      const char* kEnableExperiment = "--enable-experiment=";
-      int option_size = strlen(kEnableExperiment);
-      for (intptr_t i = 0; i < num_experiments; ++i) {
-        const char* flag = enabled_experiments_.At(i);
-        option_size += strlen(flag);
-        if (i + 1 != num_experiments) {
-          // Account for comma if there's more experiments to add.
-          ++option_size;
-        }
-      }
-      // Make room for null terminator
-      ++option_size;
-
-      char* enabled_experiments_arg = new char[option_size];
-      int offset = snprintf(enabled_experiments_arg, option_size, "%s",
-                            kEnableExperiment);
-      for (intptr_t i = 0; i < num_experiments; ++i) {
-        const char* flag = enabled_experiments_.At(i);
-        const char* kFormat = (i + 1 != num_experiments) ? "%s," : "%s";
-        offset += snprintf(enabled_experiments_arg + offset,
-                           option_size - offset, kFormat, flag);
-        free(const_cast<char*>(flag));
-        ASSERT(offset < option_size);
-      }
-      DartDevIsolate::set_should_run_dart_dev(true);
-      dart_options->AddArgument(enabled_experiments_arg);
-    } else {
-      for (intptr_t i = 0; i < num_experiments; ++i) {
-        free(const_cast<char*>(enabled_experiments_.At(i)));
-      }
-    }
-  }
-#endif  // !defined(DART_PRECOMPILED_RUNTIME)
-
   // If running with dartdev, attempt to parse VM flags which are part of the
   // dartdev command (e.g., --enable-vm-service, --observe, etc).
   if (!run_script) {