From ffc36db613d14be3cad5e192f181f61fd23ac7ac Mon Sep 17 00:00:00 2001 From: Ben Konyi Date: Wed, 4 Nov 2020 00:34:26 +0000 Subject: [PATCH] [ 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 Reviewed-by: Sigurd Meldgaard Reviewed-by: Jonas Jensen --- pkg/dartdev/lib/dartdev.dart | 43 +------------------ pkg/dartdev/lib/src/commands/compile.dart | 31 +++++++++++--- pkg/dartdev/lib/src/commands/pub.dart | 9 ++-- pkg/dartdev/lib/src/commands/run.dart | 10 +---- pkg/dartdev/lib/src/commands/test.dart | 9 ++-- pkg/dartdev/lib/src/events.dart | 3 +- pkg/dartdev/lib/src/experiments.dart | 51 +++++++++++++++++++++++ pkg/dartdev/test/analytics_test.dart | 6 +-- pkg/dartdev/test/commands/flag_test.dart | 12 ------ pkg/dartdev/test/commands/pub_test.dart | 8 ++-- pkg/dartdev/test/commands/run_test.dart | 2 +- runtime/bin/main_options.cc | 38 ----------------- 12 files changed, 100 insertions(+), 122 deletions(-) diff --git a/pkg/dartdev/lib/dartdev.dart b/pkg/dartdev/lib/dartdev.dart index 34f07a4bb1f..6b7cfdc06ae 100644 --- a/pkg/dartdev/lib/dartdev.dart +++ b/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 { 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 { 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 { String get invocation => 'dart [] []'; - void addExperimentalFlags(ArgParser argParser, bool verbose) { - List features = experimentalFeatures; - - Map 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 runCommand(ArgResults topLevelResults) async { final stopwatch = Stopwatch()..start(); @@ -250,18 +227,6 @@ class DartdevRunner extends CommandRunner { ? Logger.verbose(ansi: ansi) : Logger.standard(ansi: ansi); - if (topLevelResults.wasParsed(experimentFlagName)) { - List 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 { 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 { // // Note that this will also conflate short-options and long-options. command?.options?.where(command.wasParsed)?.toList(), - specifiedExperiments: topLevelCommand?.specifiedExperiments, + specifiedExperiments: topLevelResults.enabledExperiments, ), ); } diff --git a/pkg/dartdev/lib/src/commands/compile.dart b/pkg/dartdev/lib/src/commands/compile.dart index 024bea83d68..c0fdea3d94b 100644 --- a/pkg/dartdev/lib/src/commands/compile.dart +++ b/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 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. 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, )); } } diff --git a/pkg/dartdev/lib/src/commands/pub.dart b/pkg/dartdev/lib/src/commands/pub.dart index d77dce64e9b..9f18a82fda3 100644 --- a/pkg/dartdev/lib/src/commands/pub.dart +++ b/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 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), ]; } diff --git a/pkg/dartdev/lib/src/commands/run.dart b/pkg/dartdev/lib/src/commands/run.dart index 52dab76aba4..b350b38181e 100644 --- a/pkg/dartdev/lib/src/commands/run.dart +++ b/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 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 diff --git a/pkg/dartdev/lib/src/commands/test.dart b/pkg/dartdev/lib/src/commands/test.dart index 43117fa69f6..b4fe29c6978 100644 --- a/pkg/dartdev/lib/src/commands/test.dart +++ b/pkg/dartdev/lib/src/commands/test.dart @@ -58,11 +58,14 @@ class TestCommand extends DartdevCommand { _printMissingDepInstructions(isHelpCommand); return 65; } - + List 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, ]; diff --git a/pkg/dartdev/lib/src/events.dart b/pkg/dartdev/lib/src/events.dart index 29395dcd062..b2c936a77bd 100644 --- a/pkg/dartdev/lib/src/events.dart +++ b/pkg/dartdev/lib/src/events.dart @@ -56,7 +56,8 @@ Future 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. diff --git a/pkg/dartdev/lib/src/experiments.dart b/pkg/dartdev/lib/src/experiments.dart index 39219008b15..ebbff079f48 100644 --- a/pkg/dartdev/lib/src/experiments.dart +++ b/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 get experimentalFeatures { features.sort((a, b) => a.enableString.compareTo(b.enableString)); return features; } + +void addExperimentalFlags(ArgParser argParser, bool verbose) { + List features = experimentalFeatures; + + Map 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 get enabledExperiments { + List 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; + } +} diff --git a/pkg/dartdev/test/analytics_test.dart b/pkg/dartdev/test/analytics_test.dart index df4359dd3e9..cb25407f67a 100644 --- a/pkg/dartdev/test/analytics_test.dart +++ b/pkg/dartdev/test/analytics_test.dart @@ -182,10 +182,10 @@ void main() { test('run --enable-experiments', () { final p = project( - mainSrc: 'void main(List args) => print(args)', + mainSrc: 'void main(List 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), [ diff --git a/pkg/dartdev/test/commands/flag_test.dart b/pkg/dartdev/test/commands/flag_test.dart index 1e887b17f55..70660b49fab 100644 --- a/pkg/dartdev/test/commands/flag_test.dart +++ b/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() { diff --git a/pkg/dartdev/test/commands/pub_test.dart b/pkg/dartdev/test/commands/pub_test.dart index e5a1a86716d..14db6be7dfb 100644 --- a/pkg/dartdev/test/commands/pub_test.dart +++ b/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', () { diff --git a/pkg/dartdev/test/commands/run_test.dart b/pkg/dartdev/test/commands/run_test.dart index f33cb83a485..4c0a9c1e676 100644 --- a/pkg/dartdev/test/commands/run_test.dart +++ b/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); diff --git a/runtime/bin/main_options.cc b/runtime/bin/main_options.cc index e6442022bb5..a4f327064f6 100644 --- a/runtime/bin/main_options.cc +++ b/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(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(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) {