Add an optional configuration to preserve trailing commas. (#1672)

Add an optional configuration to preserve trailing commas.

In the old short formatter, when a delimited construct had a trailing comma, it had two effects:

1.  The construct would be formatted "Flutter style" where the closing bracket would be moved to the next line and the contents indented +2, as opposed to the earlier style where the bracket stays on the same line as the last element and the elements are indented +4).

2.  The construct is forced to split even if it otherwise wouldn't.

The new formatter always uses a Flutter-like style for commas-separated constructors, so trailing commas are no longer needed for point 1.

When it does split a construct, it always adds a trailing comma. This means that point 2 breaks reversibility. Because of that, and because many users don't want to decide for every construct whether it should split or not, the new tall formatter no longer does point 2. It will remove a trailing comma and collapse a construct if it decides to.

However, some users rely heavily on that feature and strongly prefer control over forcing argument lists and other constructs to split.

This PR restores that functionality with an opt-in configuration:

```
formatter:
  trailing_commas: preserve
```

When enabled, if a construct has a trailing comma, the formatter will always split it. Users that preferred that behavior from the old formatter can enable the option and continue to work that way.

This does not mean we are generally moving in the direction of a more configurable formatter. This was a behavior that the formatter already supported and we removed it to the detriment of some users' experience. We're restoring that existing behavior for those users who want it.

Fix #1652.

Author: munificent

CHANGELOG.md

@@ -1,4 +1,4 @@
-## 3.0.2-wip
+## 3.1.0-wip
 
 * Format null-aware elements.
 
@@ -63,10 +63,26 @@
   );
   ```
 
+* Allow preserving trailing commas and forcing the surrounding construct to
+  split even when it would otherwise fit on one line. This is off by default
+  (because it breaks [reversibility][] among other reasons) but can be enabled
+  by adding this to a surrounding `analysis_options.yaml` file:
+
+  ```yaml
+  formatter:
+    trailing_commas: preserve
+  ```
+
+  This is similar to how trailing commas worked in the old short style
+  formatter.
+
 * Don't add a trailing comma in lists that don't allow it, even when there is
   a trailing comment (#1639).
+
 * Add tests for digit separators.
 
+[reversibility]: https://github.com/dart-lang/dart_style/wiki/Reversibility-principle
+
 ## 3.0.1
 
 * Handle trailing commas in for-loop updaters (#1354).

example/format.dart

@@ -30,19 +30,42 @@ void main(List<String> args) {
   _runTest('other/selection.stmt', 2);
 }
 
-void _formatStmt(String source, {bool tall = true, int pageWidth = 40}) {
-  _runFormatter(source, pageWidth, tall: tall, isCompilationUnit: false);
+void _formatStmt(
+  String source, {
+  bool tall = true,
+  int pageWidth = 40,
+  TrailingCommas trailingCommas = TrailingCommas.automate,
+}) {
+  _runFormatter(
+    source,
+    pageWidth,
+    tall: tall,
+    isCompilationUnit: false,
+    trailingCommas: trailingCommas,
+  );
 }
 
-void _formatUnit(String source, {bool tall = true, int pageWidth = 40}) {
-  _runFormatter(source, pageWidth, tall: tall, isCompilationUnit: true);
+void _formatUnit(
+  String source, {
+  bool tall = true,
+  int pageWidth = 40,
+  TrailingCommas trailingCommas = TrailingCommas.automate,
+}) {
+  _runFormatter(
+    source,
+    pageWidth,
+    tall: tall,
+    isCompilationUnit: true,
+    trailingCommas: trailingCommas,
+  );
 }
 
 void _runFormatter(
   String source,
   int pageWidth, {
   required bool tall,
   required bool isCompilationUnit,
+  TrailingCommas trailingCommas = TrailingCommas.automate,
 }) {
   try {
     var formatter = DartFormatter(
@@ -51,6 +74,7 @@ void _runFormatter(
               ? DartFormatter.latestLanguageVersion
               : DartFormatter.latestShortStyleLanguageVersion,
       pageWidth: pageWidth,
+      trailingCommas: trailingCommas,
     );
 
     String result;
@@ -84,13 +108,7 @@ Future<void> _runTest(
 }) async {
   var testFile = await TestFile.read('${tall ? 'tall' : 'short'}/$path');
   var formatTest = testFile.tests.firstWhere((test) => test.line == line);
-
-  var formatter = DartFormatter(
-    languageVersion: formatTest.languageVersion,
-    pageWidth: testFile.pageWidth,
-    indent: formatTest.leadingIndent,
-    experimentFlags: formatTest.experimentFlags,
-  );
+  var formatter = testFile.formatterForTest(formatTest);
 
   var actual = formatter.formatSource(formatTest.input);
 

lib/src/analysis_options/analysis_options_file.dart

@@ -17,14 +17,14 @@ typedef AnalysisOptions = Map<Object?, Object?>;
 /// passed to [FileSystem.join()].
 typedef ResolvePackageUri = Future<String?> Function(Uri packageUri);
 
-/// Reads an `analysis_options.yaml` file in [directory] or in the nearest
+/// Reads an "analysis_options.yaml" file in [directory] or in the nearest
 /// surrounding folder that contains that file using [fileSystem].
 ///
 /// Stops walking parent directories as soon as it finds one that contains an
-/// `analysis_options.yaml` file. If it reaches the root directory without
+/// "analysis_options.yaml" file. If it reaches the root directory without
 /// finding one, returns an empty [YamlMap].
 ///
-/// If an `analysis_options.yaml` file is found, reads it and parses it to a
+/// If an "analysis_options.yaml" file is found, reads it and parses it to a
 /// [YamlMap]. If the map contains an `include` key whose value is a list, then
 /// reads any of the other referenced YAML files and merges them into this one.
 /// Returns the resulting map with the `include` key removed.

lib/src/ast_extensions.dart

@@ -454,6 +454,9 @@ extension PatternExtensions on DartPattern {
 }
 
 extension TokenExtensions on Token {
+  /// Whether the token before this one is a comma.
+  bool get hasCommaBefore => previous?.type == TokenType.COMMA;
+
   /// Whether this token has a preceding comment that is a line comment.
   bool get hasLineCommentBefore {
     for (

lib/src/cli/format_command.dart

@@ -106,6 +106,22 @@ final class FormatCommand extends Command<int> {
       hide: true,
     );
 
+    argParser.addOption(
+      'trailing-commas',
+      help: 'How trailing commas in input affect formatting.',
+      defaultsTo: 'automate',
+      allowedHelp: {
+        'automate':
+            'The formatter adds and removes trailing commas based on\n'
+            'its decision to split the surrounding construct.',
+        'preserve':
+            'A trailing comma forces the surrounding construct to split.\n'
+            'The formatter will add a trailing comma when it splits a\n'
+            'construct but will not remove one.',
+      },
+      hide: !verbose,
+    );
+
     argParser.addOption(
       'indent',
       abbr: 'i',
@@ -255,6 +271,19 @@ final class FormatCommand extends Command<int> {
       }
     }
 
+    TrailingCommas? trailingCommas;
+    if (argResults.wasParsed('trailing-commas')) {
+      // We check the values explicitly here instead of using `allowedValues`
+      // from [ArgParser] because this provides a better error message.
+      trailingCommas = switch (argResults['trailing-commas']) {
+        'automate' => TrailingCommas.automate,
+        'preserve' => TrailingCommas.preserve,
+        var mode => usageException(
+          '--trailing-commas must be "automate" or "preserve", was "$mode".',
+        ),
+      };
+    }
+
     var indent =
         int.tryParse(argResults['indent'] as String) ??
         usageException(
@@ -300,6 +329,7 @@ final class FormatCommand extends Command<int> {
       languageVersion: languageVersion,
       indent: indent,
       pageWidth: pageWidth,
+      trailingCommas: trailingCommas,
       followLinks: followLinks,
       show: show,
       output: output,

lib/src/cli/formatter_options.dart

@@ -6,15 +6,17 @@ import 'dart:io';
 
 import 'package:pub_semver/pub_semver.dart';
 
+import '../dart_formatter.dart';
 import '../source_code.dart';
 import 'output.dart';
 import 'show.dart';
 import 'summary.dart';
 
 // Note: The following line of code is modified by tool/grind.dart.
-const dartStyleVersion = '3.0.2-wip';
+const dartStyleVersion = '3.1.0-wip';
 
-/// Global options that affect how the formatter produces and uses its outputs.
+/// Global options parsed from the command line that affect how the formatter
+/// produces and uses its outputs.
 final class FormatterOptions {
   /// The language version formatted code should be parsed at or `null` if not
   /// specified.
@@ -30,6 +32,9 @@ final class FormatterOptions {
   /// [DartFormatter.defaultPageWidth].
   final int? pageWidth;
 
+  /// How trailing commas in the input source code affect formatting.
+  final TrailingCommas? trailingCommas;
+
   /// Whether symlinks should be traversed when formatting a directory.
   final bool followLinks;
 
@@ -53,6 +58,7 @@ final class FormatterOptions {
     this.languageVersion,
     this.indent = 0,
     this.pageWidth,
+    this.trailingCommas,
     this.followLinks = false,
     this.show = Show.changed,
     this.output = Output.write,

lib/src/config_cache.dart

@@ -9,13 +9,14 @@ import 'package:pub_semver/pub_semver.dart';
 
 import 'analysis_options/analysis_options_file.dart';
 import 'analysis_options/io_file_system.dart';
+import 'dart_formatter.dart';
 import 'profile.dart';
 
 /// Caches the nearest surrounding package config file for files in directories.
 ///
 /// The formatter reads `.dart_tool/package_config.json` files in order to
 /// determine the default language version of files in that package and to
-/// resolve "package:" URIs in `analysis_options.yaml` files.
+/// resolve "package:" URIs in "analysis_options.yaml" files.
 ///
 /// Walking the file system to find the package config and then reading it off
 /// disk is very slow. We know that every formatted file in the same directory
@@ -40,13 +41,9 @@ final class ConfigCache {
   /// discovered that there is no surrounding package.
   final Map<String, Version?> _directoryVersions = {};
 
-  /// The previously cached page width for all files immediately within a given
-  /// directory.
-  ///
-  /// The width may be `null` if we formatted a file in that directory and
-  /// discovered that there is no surrounding analysis options that sets a
-  /// page width.
-  final Map<String, int?> _directoryPageWidths = {};
+  /// The previously cached configured options for all files immediately within
+  /// a given directory.
+  final Map<String, _FormatterOptions> _directoryOptions = {};
 
   final IOFileSystem _fileSystem = IOFileSystem();
 
@@ -89,35 +86,77 @@ final class ConfigCache {
   ///     formatter:
   ///       page_width: 123
   Future<int?> findPageWidth(File file) async {
-    // Use the cached version (which may be `null`) if present.
+    return (await _findFormatterOptions(file)).pageWidth;
+  }
+
+  /// Looks for an "analysis_options.yaml" file surrounding [file] and, if
+  /// found and valid, returns the trailing comma handling specified by that
+  /// config file.
+  ///
+  /// Otherwise returns `null`.
+  ///
+  /// The schema looks like:
+  ///
+  ///     formatter:
+  ///       trailing_commas: preserve # Or "automate".
+  Future<TrailingCommas?> findTrailingCommas(File file) async {
+    return (await _findFormatterOptions(file)).trailingCommas;
+  }
+
+  /// Looks for an "analysis_options.yaml" file surrounding [file] and, if
+  /// found and valid, returns the configured options.
+  ///
+  /// If no options file could be found or it doesn't contain a "formatter" key
+  /// whose value is a map, returns a default set of options where all settings
+  /// are `null`.
+  Future<_FormatterOptions> _findFormatterOptions(File file) async {
+    // Use the cached version if present.
     var directory = file.parent.path;
-    if (_directoryPageWidths.containsKey(directory)) {
-      return _directoryPageWidths[directory];
-    }
+    if (_directoryOptions[directory] case var options?) return options;
+
+    int? pageWidth;
+    TrailingCommas? trailingCommas;
 
     try {
-      // Look for a surrounding analysis_options.yaml file.
-      var options = await findAnalysisOptions(
+      // Look for a surrounding "analysis_options.yaml" file.
+      var optionsFile = await findAnalysisOptions(
         _fileSystem,
         await _fileSystem.makePath(file.path),
         resolvePackageUri: (uri) => _resolvePackageUri(file, uri),
       );
 
-      if (options['formatter'] case Map<Object?, Object?> formatter) {
-        if (formatter['page_width'] case int pageWidth) {
-          return _directoryPageWidths[directory] = pageWidth;
+      if (optionsFile['formatter'] case Map<Object?, Object?> formatter) {
+        if (formatter case {'page_width': int width}) {
+          pageWidth = width;
+        }
+
+        if (formatter case {'trailing_commas': var commas}) {
+          switch (commas) {
+            case 'automate':
+              trailingCommas = TrailingCommas.automate;
+            case 'preserve':
+              trailingCommas = TrailingCommas.preserve;
+            default:
+              stderr.writeln(
+                'Warning: "trailing_commas" option should be "automate" or '
+                '"preserve", but was "$commas".',
+              );
+          }
         }
       }
-    } on PackageResolutionException {
-      // Silently ignore any errors coming from the processing the analyis
-      // options. If there are any issues, we just use the default page width
-      // and keep going.
+    } on PackageResolutionException catch (exception) {
+      // Report the error, but use the default settings and keep going.
+      stderr.writeln(
+        'Warning: Package resolution error when reading '
+        '"analysis_options.yaml" file:\n$exception',
+      );
     }
 
-    // If we get here, the options file either doesn't specify the page width,
-    // or specifies it in some invalid way. When that happens, silently ignore
-    // the config file and use the default width.
-    return _directoryPageWidths[directory] = null;
+    // Cache whichever options we found (or `null` if we didn't find them).
+    return _directoryOptions[directory] = _FormatterOptions(
+      pageWidth,
+      trailingCommas,
+    );
   }
 
   /// Look for and cache the nearest package surrounding [file].
@@ -176,3 +215,17 @@ final class ConfigCache {
     return config.resolve(packageUri)?.toFilePath();
   }
 }
+
+/// The formatter options that can be configured in the "analysis_options.yaml"
+/// file.
+final class _FormatterOptions {
+  /// The configured page width, or `null` if there is no options file or the
+  /// options file doesn't specify it.
+  final int? pageWidth;
+
+  /// The configured comma handling, or `null` if there is no options file or
+  /// the options file doesn't specify it.
+  final TrailingCommas? trailingCommas;
+
+  _FormatterOptions(this.pageWidth, this.trailingCommas);
+}