[dart2wasm] Fix handling of --define/-D

osa1 · Commit Queue · commit f4b41f09028e · 2025-03-18T04:11:41.000-07:00
When parsing `--define` or `-D` arguments don't split the the value by commas. This is consistent with how dart2js handles `-D`, but inconsistent with how VM handles it. Example: void main() { print(const String.fromEnvironment("FOO")); } When compiled with `dart compile js -DFOO="a, b"` and run, dart2js prints a, b VM prints (when compiled to exe) a Between these two, I think dart2js' behavior is more common, so we follow dart2js. Also update compile_benchmark to avoid splitting a single argument "a b" into "a" and "b" when parsing the arguments and then splicing them back before calling `dart2wasm`. Also update the test runner and ddc batch mode argument parser to handle splitting quoted arguments in `// dart2jsOption = ...` and the same options for ddc and dart2wasm, by moving dart2js's `splitLine` to a new library and reusing it in the test runner and ddc. Fixes flutter/flutter#164873. See also #60341 for relevant future work. Change-Id: Idbdf69072fa212c8e4a390990577eb5a57b49e8a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/415280 Reviewed-by: Martin Kustermann <kustermann@google.com> Commit-Queue: Ömer Ağacan <omersa@google.com>
diff --git a/pkg/compiler/lib/src/dart2js.dart b/pkg/compiler/lib/src/dart2js.dart
@@ -11,6 +11,7 @@ import 'dart:isolate' show Isolate;
 
 // ignore: implementation_imports
 import 'package:front_end/src/api_unstable/dart2js.dart' as fe;
+import 'package:shell_arg_splitter/shell_arg_splitter.dart';
 
 import '../compiler_api.dart' as api;
 import 'commandline_options.dart';
@@ -20,7 +21,6 @@ import 'io/mapped_file.dart';
 import 'options.dart'
     show CompilerOptions, CompilerStage, DumpInfoFormat, FeatureOptions;
 import 'source_file_provider.dart';
-import 'util/command_line.dart';
 import 'util/util.dart' show stackTraceFilePrefix;
 
 const String _defaultSpecificationUri = '../../../../sdk/lib/libraries.json';
diff --git a/pkg/compiler/pubspec.yaml b/pkg/compiler/pubspec.yaml
@@ -21,6 +21,7 @@ dependencies:
   kernel: any
   meta: any
   mmap: any
+  shell_arg_splitter: any
   vm_service: any
 
 # Use 'any' constraints here; we get our versions from the DEPS file.
diff --git a/pkg/dart2wasm/lib/dart2wasm.dart b/pkg/dart2wasm/lib/dart2wasm.dart
@@ -68,7 +68,7 @@ final List<Option> options = [
       "watch", (o, values) => o.translatorOptions.watchPoints = values),
   StringMultiOption(
       "define", (o, values) => o.environment.addAll(processEnvironment(values)),
-      abbr: "D"),
+      abbr: "D", splitCommas: false),
   StringMultiOption(
       "enable-experiment",
       (o, values) =>
diff --git a/pkg/dart2wasm/lib/option.dart b/pkg/dart2wasm/lib/option.dart
@@ -78,10 +78,12 @@ class MultiValueOption<T> extends Option<List<T>> {
       void Function(WasmCompilerOptions o, List<T> v) applyToOptions,
       T Function(dynamic v) converter,
       {Iterable<String>? defaultsTo,
-      String? abbr})
+      String? abbr,
+      bool splitCommas = true})
       : super(
             name,
-            (a) => a.addMultiOption(name, abbr: abbr, defaultsTo: defaultsTo),
+            (a) => a.addMultiOption(name,
+                abbr: abbr, defaultsTo: defaultsTo, splitCommas: splitCommas),
             applyToOptions,
             (vs) => vs.map(converter).cast<T>().toList());
 }
@@ -97,9 +99,9 @@ class IntMultiOption extends MultiValueOption<int> {
 class StringMultiOption extends MultiValueOption<String> {
   StringMultiOption(String name,
       void Function(WasmCompilerOptions o, List<String> v) applyToOptions,
-      {String? abbr, Iterable<String>? defaultsTo})
+      {String? abbr, Iterable<String>? defaultsTo, bool splitCommas = true})
       : super(name, applyToOptions, (v) => v,
-            abbr: abbr, defaultsTo: defaultsTo);
+            abbr: abbr, defaultsTo: defaultsTo, splitCommas: splitCommas);
 }
 
 class UriMultiOption extends MultiValueOption<Uri> {
diff --git a/pkg/dart2wasm/tool/compile_benchmark b/pkg/dart2wasm/tool/compile_benchmark
@@ -133,7 +133,7 @@ while [ $# -gt 0 ]; do
       ;;
 
     --extra-compiler-option=*)
-      DART2WASM_ARGS+=(${1#--extra-compiler-option=})
+      DART2WASM_ARGS+=("${1#--extra-compiler-option=}")
       shift
       ;;
 
@@ -219,7 +219,7 @@ fi
 binaryen_command=("$BINARYEN" "${BINARYEN_FLAGS[@]}" "$WASM_FILE" -o "$WASM_FILE")
 
 if [ -n "$COMPILE_BENCHMARK_BASE_NAME" ]; then
-  measure ${dart2wasm_command[@]}
+  measure "${dart2wasm_command[@]}"
   COMPILER_TIME=$TIME
   COMPILER_MEMORY=$MEMORY
 
@@ -262,6 +262,6 @@ if [ -n "$COMPILE_BENCHMARK_BASE_NAME" ]; then
   echo "$COMPILE_BENCHMARK_BASE_NAME.MemoryUse.Dart2Wasm(MemoryUse): $COMPILER_MEMORY bytes"
   run_if_binaryen_enabled echo "$COMPILE_BENCHMARK_BASE_NAME.MemoryUse.Wasm2WasmOpt(MemoryUse): $BINARYEN_MEMORY bytes"
 else
-  ${dart2wasm_command[@]}
+  "${dart2wasm_command[@]}"
   run_if_binaryen_enabled ${binaryen_command[@]}
 fi
diff --git a/pkg/dartdev/lib/src/commands/compile.dart b/pkg/dartdev/lib/src/commands/compile.dart
@@ -799,6 +799,7 @@ class CompileWasmCommand extends CompileSubcommandCommand {
         help: defineOption.help,
         abbr: defineOption.abbr,
         valueHelp: defineOption.valueHelp,
+        splitCommas: false,
       )
       ..addExperimentalFlags(verbose: verbose);
   }
diff --git a/pkg/dev_compiler/lib/ddc.dart b/pkg/dev_compiler/lib/ddc.dart
@@ -14,6 +14,7 @@ import 'dart:io';
 import 'dart:isolate';
 import 'package:bazel_worker/bazel_worker.dart';
 import 'package:kernel/ast.dart' show clearDummyTreeNodesParentPointer;
+import 'package:shell_arg_splitter/shell_arg_splitter.dart';
 
 import 'src/command/arguments.dart';
 import 'src/command/command.dart';
@@ -129,7 +130,7 @@ class _BatchHelper {
 
   Future<void> _doIteration(ParsedArguments batchArgs, String line) async {
     totalTests++;
-    var args = batchArgs.merge(line.split(RegExp(r'\s+')));
+    var args = batchArgs.merge(splitLine(line));
 
     String outcome;
     try {
diff --git a/pkg/dev_compiler/pubspec.yaml b/pkg/dev_compiler/pubspec.yaml
@@ -18,6 +18,7 @@ dependencies:
   js_shared: any
   kernel: any
   path: any
+  shell_arg_splitter: any
   source_maps: any
   source_span: any
 
diff --git a/pkg/shell_arg_splitter/analysis_options.yaml b/pkg/shell_arg_splitter/analysis_options.yaml
@@ -0,0 +1 @@
+include: package:lints/recommended.yaml
diff --git a/pkg/shell_arg_splitter/lib/shell_arg_splitter.dart b/pkg/shell_arg_splitter/lib/shell_arg_splitter.dart
@@ -2,12 +2,10 @@
 // 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.
 
-library;
-
 /// The accepted escapes in the input of the --batch processor.
 ///
 /// Contrary to Dart strings it does not contain hex escapes (\u or \x).
-const Map<String, String> escapeMapping = {
+const Map<String, String> _escapeMapping = {
   'n': '\n',
   'r': '\r',
   't': '\t',
@@ -57,7 +55,7 @@ List<String> splitLine(String line, {bool windows = false}) {
         i++;
 
         c = line[i];
-        String mapped = escapeMapping[c] ?? c;
+        String mapped = _escapeMapping[c] ?? c;
         buffer.write(mapped);
         continue;
       }
diff --git a/pkg/shell_arg_splitter/pubspec.yaml b/pkg/shell_arg_splitter/pubspec.yaml
@@ -0,0 +1,10 @@
+name: shell_arg_splitter
+# This package is not intended for consumption on pub.dev. DO NOT publish.
+publish_to: none
+
+environment:
+  sdk: '^3.7.0'
+
+dev_dependencies:
+  expect: any
+  lints: any
diff --git a/pkg/shell_arg_splitter/test/command_line_split_test.dart b/pkg/shell_arg_splitter/test/command_line_split_test.dart
@@ -3,9 +3,9 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import "package:expect/expect.dart";
-import 'package:compiler/src/util/command_line.dart';
+import "package:shell_arg_splitter/shell_arg_splitter.dart";
 
-main() {
+void main() {
   Expect.listEquals(["foo", "bar"], splitLine("foo bar"));
   Expect.listEquals(["foo", "bar"], splitLine("foo bar", windows: true));
 
@@ -28,13 +28,23 @@ main() {
     "''bar",
   ], splitLine(r"""foo"'" '"'bar" """, windows: true));
 
-  Expect.listEquals(["foo", "bar"], splitLine("'f''o''o' " + '"b""a""r"'));
+  Expect.listEquals(
+    ["foo", "bar"],
+    splitLine(
+      "'f''o''o' "
+      '"b""a""r"',
+    ),
+  );
   // TODO(johnniwinther): This is not actual Windows behavior: "b""a" is
   // interpreted as b"a but "b""a""r" is interpreted as b"ar.
-  Expect.listEquals([
-    "'f''o''o'",
-    "bar",
-  ], splitLine("'f''o''o' " + '"b""a""r"', windows: true));
+  Expect.listEquals(
+    ["'f''o''o'", "bar"],
+    splitLine(
+      "'f''o''o' "
+      '"b""a""r"',
+      windows: true,
+    ),
+  );
 
   Expect.listEquals([
     "\n",
diff --git a/pkg/test_runner/lib/src/process_queue.dart b/pkg/test_runner/lib/src/process_queue.dart
@@ -1058,6 +1058,7 @@ class BatchRunnerProcess {
   }
 
   String _createArgumentsLine(List<String> arguments, int timeout) {
+    arguments = arguments.map(escapeCommandLineArgument).toList();
     if (_useJson) {
       return "${jsonEncode(arguments)}\n";
     } else {
diff --git a/pkg/test_runner/lib/src/test_file.dart b/pkg/test_runner/lib/src/test_file.dart
@@ -3,6 +3,8 @@
 // BSD-style license that can be found in the LICENSE file.
 import 'dart:io';
 
+import 'package:shell_arg_splitter/shell_arg_splitter.dart';
+
 import 'feature.dart';
 import 'path.dart';
 import 'static_error.dart';
@@ -22,9 +24,6 @@ final _localFileRegExp = RegExp(
     r"""(?:(?:show|hide)\s+\w+\s*(?:,\s*\w+\s*))*;""",
     multiLine: true);
 
-List<String> _splitWords(String s) =>
-    s.split(' ')..removeWhere((s) => s.isEmpty);
-
 List<T> _parseOption<T>(
     String filePath, String contents, String name, T Function(String) convert,
     {bool allowMultiple = false}) {
@@ -37,7 +36,7 @@ List<T> _parseOption<T>(
 
   var options = <T>[];
   for (var match in matches) {
-    for (var option in _splitWords(match[1]!)) {
+    for (var option in splitLine(match[1]!)) {
       options.add(convert(option));
     }
   }
@@ -228,7 +227,7 @@ class TestFile extends _TestFileBase {
     var vmOptions = <List<String>>[];
     var matches = _vmOptionsRegExp.allMatches(contents);
     for (var match in matches) {
-      vmOptions.add(_splitWords(match[1]!));
+      vmOptions.add(splitLine(match[1]!));
     }
     if (vmOptions.isEmpty) vmOptions.add(<String>[]);
 
diff --git a/pkg/test_runner/pubspec.yaml b/pkg/test_runner/pubspec.yaml
@@ -17,6 +17,7 @@ dependencies:
   package_config: any
   path: any
   pool: any
+  shell_arg_splitter: any
   smith: any
   status_file: any
   webdriver: any
diff --git a/tests/web/wasm_js_shared/env_defines_test.dart b/tests/web/wasm_js_shared/env_defines_test.dart
@@ -0,0 +1,14 @@
+// Copyright (c) 2025, 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.
+
+// dart2jsOptions="-DFOO=a, b, c=123" --define=BAR=hi
+// ddcOptions="-DFOO=a, b, c=123" --define=BAR=hi
+// dart2wasmOptions="--extra-compiler-option=-DFOO=a, b, c=123" --define=BAR=hi
+
+import 'package:expect/expect.dart';
+
+void main() {
+  Expect.equals("a, b, c=123", const String.fromEnvironment("FOO"));
+  Expect.equals("hi", const String.fromEnvironment("BAR"));
+}

Original file line number	Diff line number	Diff line change
`@@ -799,6 +799,7 @@ class CompileWasmCommand extends CompileSubcommandCommand {`
`799`	`799`	`help: defineOption.help,`
`800`	`800`	`abbr: defineOption.abbr,`
`801`	`801`	`valueHelp: defineOption.valueHelp,`
	`802`	`+ splitCommas: false,`
`802`	`803`	`)`
`803`	`804`	`..addExperimentalFlags(verbose: verbose);`
`804`	`805`	`}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+include: package:lints/recommended.yaml`