From 76830e1fb727a2af0889cdc640ef621ca891056d Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Tue, 1 Oct 2024 15:00:51 -0700 Subject: [PATCH 1/6] Add code for working with analysis_options.yaml files. This can find, read, and merge them. It handles `include` inside the file. It works using an abstraction over the file system so that (in theory at least), this code could be harvested and made reusable by other tools that need to work with analysis_options.yaml files but don't want to work directly with files on disk. It also includes an implementation of that abstraction using dart:io. Right now, this is all just private inside dart_style, but is organized so that it would be easy to pull out into a separate package later if we want. --- .../analysis_options_file.dart | 65 ++++++++ lib/src/analysis_options/file_system.dart | 45 ++++++ lib/src/analysis_options/io_file_system.dart | 56 +++++++ lib/src/analysis_options/merge_options.dart | 65 ++++++++ lib/src/testing/test_file_system.dart | 53 ++++++ pubspec.yaml | 2 +- .../analysis_options_file_test.dart | 153 ++++++++++++++++++ .../analysis_options/io_file_system_test.dart | 136 ++++++++++++++++ test/analysis_options/merge_options_test.dart | 111 +++++++++++++ 9 files changed, 685 insertions(+), 1 deletion(-) create mode 100644 lib/src/analysis_options/analysis_options_file.dart create mode 100644 lib/src/analysis_options/file_system.dart create mode 100644 lib/src/analysis_options/io_file_system.dart create mode 100644 lib/src/analysis_options/merge_options.dart create mode 100644 lib/src/testing/test_file_system.dart create mode 100644 test/analysis_options/analysis_options_file_test.dart create mode 100644 test/analysis_options/io_file_system_test.dart create mode 100644 test/analysis_options/merge_options_test.dart diff --git a/lib/src/analysis_options/analysis_options_file.dart b/lib/src/analysis_options/analysis_options_file.dart new file mode 100644 index 00000000..2b61c6a3 --- /dev/null +++ b/lib/src/analysis_options/analysis_options_file.dart @@ -0,0 +1,65 @@ +// Copyright (c) 2024, 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. +import 'package:yaml/yaml.dart'; + +import 'file_system.dart'; +import 'merge_options.dart'; + +/// The analysis options configuration is a dynamically-typed JSON-like data +/// structure. +/// +/// (It's JSON-*like* and not JSON because maps in it may have non-string keys.) +typedef AnalysisOptions = Map; + +/// 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 +/// finding one, returns an empty [YamlMap]. +/// +/// 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. +Future findAnalysisOptions( + FileSystem fileSystem, FileSystemPath directory) async { + while (true) { + var optionsPath = await fileSystem.join(directory, 'analysis_options.yaml'); + if (await fileSystem.fileExists(optionsPath)) { + return readAnalysisOptions(fileSystem, optionsPath); + } + + var parent = await fileSystem.parentDirectory(directory); + if (parent == null) break; + directory = parent; + } + + // If we get here, we didn't find an analysis_options.yaml. + return const {}; +} + +Future readAnalysisOptions( + FileSystem fileSystem, FileSystemPath optionsPath) async { + var yaml = loadYamlNode(await fileSystem.readFile(optionsPath)); + + // Lower the YAML to a regular map. + if (yaml is! YamlMap) return const {}; + var options = {...yaml}; + + // If there is an `include:` key, then load that and merge it with these + // options. + if (options['include'] case String include) { + options.remove('include'); + + // The include path may be relative to the directory containing the current + // options file. + var includePath = await fileSystem.join( + (await fileSystem.parentDirectory(optionsPath))!, include); + var includeFile = await readAnalysisOptions(fileSystem, includePath); + options = merge(includeFile, options) as AnalysisOptions; + } + + return options; +} diff --git a/lib/src/analysis_options/file_system.dart b/lib/src/analysis_options/file_system.dart new file mode 100644 index 00000000..4c346bc9 --- /dev/null +++ b/lib/src/analysis_options/file_system.dart @@ -0,0 +1,45 @@ +// Copyright (c) 2024, 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. + +/// Abstraction over a file system. +/// +/// Implement this if you want to control how this package locates and reads +/// files. +abstract interface class FileSystem { + /// Returns `true` if there is a file at [path]. + Future fileExists(FileSystemPath path); + + /// Joins [from] and [to] into a single path with appropriate path separators. + /// + /// Note that [to] may be an absolute path or a "package:" URI and an + /// implementation of [join()] should be prepared to handle that by ignoring + /// [from] and generating a [FileSystemPath] for [to]. + Future join(FileSystemPath from, String to); + + /// Returns a path for the directory containing [path]. + /// + /// If [path] is a root path, then returns `null`. + Future parentDirectory(FileSystemPath path); + + /// Returns the series of directories surrounding [path], from innermost out. + /// + /// If [path] is itself a directory, then it should be the first directory + /// yielded by this. Otherwise, the stream should begin with the directory + /// containing that file. + // Stream parentDirectories(FileSystemPath path); + + /// Reads the contents of the file as [path], which should exist and contain + /// UTF-8 encoded text. + Future readFile(FileSystemPath path); +} + +/// Abstraction over a file or directory in a [FileSystem]. +/// +/// An implementation of [FileSystem] should have a corresponding implementation +/// of this class. It can safely assume that any instances of this passed in to +/// the class were either directly created as instances of the implementation +/// class by the host application, or were returned by methods on that same +/// [FileSystem] object. Thus it is safe for an implementation of [FileSystem] +/// to downcast instances of this to its expected implementation type. +abstract interface class FileSystemPath {} diff --git a/lib/src/analysis_options/io_file_system.dart b/lib/src/analysis_options/io_file_system.dart new file mode 100644 index 00000000..38272d9d --- /dev/null +++ b/lib/src/analysis_options/io_file_system.dart @@ -0,0 +1,56 @@ +// Copyright (c) 2024, 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. + +import 'dart:io'; + +import 'package:path/path.dart' as p; + +import 'file_system.dart'; + +/// An implementation of [FileSystem] using `dart:io`. +class IOFileSystem implements FileSystem { + Future makePath(String path) async { + // TODO(rnystrom): If [path] is a "package:" URI, then look for a + // surrounding package_config.json and resolve the URI to a file path using + // that. + return IOFileSystemPath._(path); + } + + @override + Future fileExists(FileSystemPath path) => File(path.ioPath).exists(); + + @override + Future join(FileSystemPath from, String to) => + makePath(p.join(from.ioPath, to)); + + @override + Future parentDirectory(FileSystemPath path) async { + // Make [path] absolute (if not already) so that we can walk outside of the + // literal path string passed. + var result = p.dirname(p.absolute(path.ioPath)); + + // If the parent directory is the same as [path], we must be at the root. + if (result == path.ioPath) return null; + + return makePath(result); + } + + @override + Future readFile(FileSystemPath path) => + File(path.ioPath).readAsString(); +} + +/// An abstraction over a file path string, used by [IOFileSystem]. +/// +/// To create an instance of this, use [IOFileSystem.makePath()]. +class IOFileSystemPath implements FileSystemPath { + /// The underlying physical file system path. + final String path; + + IOFileSystemPath._(this.path); +} + +extension on FileSystemPath { + String get ioPath => (this as IOFileSystemPath).path; +} diff --git a/lib/src/analysis_options/merge_options.dart b/lib/src/analysis_options/merge_options.dart new file mode 100644 index 00000000..c3acbfe9 --- /dev/null +++ b/lib/src/analysis_options/merge_options.dart @@ -0,0 +1,65 @@ +// Copyright (c) 2024, 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. + +/// Merges a [defaults] options set with an [overrides] options set using +/// simple override semantics, suitable for merging two configurations where +/// one defines default values that are added to (and possibly overridden) by an +/// overriding one. +/// +/// The merge rules are: +/// +/// * Lists are concatenated without duplicates. +/// * A list of strings is promoted to a map of strings to `true` when merged +/// with another map of strings to booleans. For example `['opt1', 'opt2']` +/// is promoted to `{'opt1': true, 'opt2': true}`. +/// * Maps unioned. When both have the same key, the corresponding values are +/// merged, recursively. +/// * Otherwise, a non-`null` override replaces a default value. +Object? merge(Object? defaults, Object? overrides) { + return switch ((defaults, overrides)) { + (List(isAllStrings: true) && var list, Map(isToBools: true)) => + merge(_promoteList(list), overrides), + (Map(isToBools: true), List(isAllStrings: true) && var list) => + merge(defaults, _promoteList(list)), + (Map defaultsMap, Map overridesMap) => _mergeMap(defaultsMap, overridesMap), + (List defaultsList, List overridesList) => + _mergeList(defaultsList, overridesList), + (_, null) => + // Default to override, unless the overriding value is `null`. + defaults, + _ => overrides, + }; +} + +/// Promote a list of strings to a map of those strings to `true`. +Map _promoteList(List list) { + return {for (var element in list) element: true}; +} + +/// Merge lists, avoiding duplicates. +List _mergeList(List defaults, List overrides) { + // Add them both to a set so that the overrides replace the defaults. + return {...defaults, ...overrides}.toList(); +} + +/// Merge maps (recursively). +Map _mergeMap( + Map defaults, Map overrides) { + var merged = {...defaults}; + + overrides.forEach((key, value) { + merged.update(key, (defaultValue) => merge(defaultValue, value), + ifAbsent: () => value); + }); + + return merged; +} + +extension on List { + bool get isAllStrings => every((e) => e is String); +} + +extension on Map { + bool get isToBools => values.every((v) => v is bool); +} diff --git a/lib/src/testing/test_file_system.dart b/lib/src/testing/test_file_system.dart new file mode 100644 index 00000000..73cf32e4 --- /dev/null +++ b/lib/src/testing/test_file_system.dart @@ -0,0 +1,53 @@ +// Copyright (c) 2024, 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. + +import '../analysis_options/file_system.dart'; + +/// A simulated file system for tests. +/// +/// Uses `|` as directory separator to make sure that the implementating code +/// calling into this doesn't assume a directory separator character. +class TestFileSystem implements FileSystem { + final Map _files = {}; + + TestFileSystem([Map? files]) { + if (files != null) _files.addAll(files); + } + + @override + Future fileExists(FileSystemPath path) async => + _files.containsKey(path.testPath); + + @override + Future join(FileSystemPath from, String to) async { + return TestFileSystemPath('${from.testPath}|$to'); + } + + @override + Future parentDirectory(FileSystemPath path) async { + var parts = path.testPath.split('|'); + if (parts.length == 1) return null; + + return TestFileSystemPath(parts.sublist(0, parts.length - 1).join('|')); + } + + @override + Future readFile(FileSystemPath path) async { + if (_files[path.testPath] case var contents?) return contents; + throw Exception('No file at "$path".'); + } +} + +class TestFileSystemPath implements FileSystemPath { + final String _path; + + TestFileSystemPath(this._path); + + @override + String toString() => _path; +} + +extension on FileSystemPath { + String get testPath => (this as TestFileSystemPath)._path; +} diff --git a/pubspec.yaml b/pubspec.yaml index f6d98b09..c1ad20e9 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -16,6 +16,7 @@ dependencies: path: ^1.0.0 pub_semver: ">=1.4.4 <3.0.0" source_span: ^1.4.0 + yaml: ^3.1.2 dev_dependencies: dart_flutter_team_lints: ^3.1.0 @@ -23,4 +24,3 @@ dev_dependencies: test: ^1.24.6 test_descriptor: ^2.0.0 test_process: ^2.0.0 - yaml: ">=2.0.0 <4.0.0" diff --git a/test/analysis_options/analysis_options_file_test.dart b/test/analysis_options/analysis_options_file_test.dart new file mode 100644 index 00000000..4cccdf9e --- /dev/null +++ b/test/analysis_options/analysis_options_file_test.dart @@ -0,0 +1,153 @@ +// Copyright (c) 2024, 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. + +import 'package:dart_style/src/analysis_options/analysis_options_file.dart'; +import 'package:dart_style/src/testing/test_file_system.dart'; +import 'package:test/test.dart'; + +void main() { + group('findAnalysisOptionsoptions()', () { + test('returns an empty map if no analysis options is found', () async { + var testFS = TestFileSystem(); + var options = + await findAnalysisOptions(testFS, TestFileSystemPath('dir|sub')); + expect(options, isEmpty); + }); + + test('finds a file in the given directory', () async { + var testFS = TestFileSystem({ + 'dir|analysis_options.yaml': _makeOptions(pageWidth: 100), + }); + + var options = + await findAnalysisOptions(testFS, TestFileSystemPath('dir')); + expect(_pageWidth(options), equals(100)); + }); + + test('stops at the nearest analysis options file', () async { + var testFS = TestFileSystem({ + 'dir|analysis_options.yaml': _makeOptions(pageWidth: 120), + 'dir|sub|analysis_options.yaml': _makeOptions(pageWidth: 100) + }); + + var options = + await findAnalysisOptions(testFS, TestFileSystemPath('dir|sub')); + expect(_pageWidth(options), equals(100)); + }); + + test('uses the nearest file even if it doesn\'t have the setting', + () async { + var testFS = TestFileSystem({ + 'dir|analysis_options.yaml': _makeOptions(pageWidth: 120), + 'dir|sub|analysis_options.yaml': _makeOptions() + }); + + var options = + await findAnalysisOptions(testFS, TestFileSystemPath('dir|sub')); + expect(_pageWidth(options), isNull); + }); + }); + + group('readAnalysisOptionsoptions()', () { + test('reads an analysis options file', () async { + var testFS = TestFileSystem({'file.yaml': _makeOptions(pageWidth: 120)}); + + var options = + await readAnalysisOptions(testFS, TestFileSystemPath('file.yaml')); + expect(_pageWidth(options), 120); + }); + + test('merges included files', () async { + var testFS = TestFileSystem({ + 'dir|a.yaml': _makeOptions(include: 'b.yaml', other: { + 'a': 'from a', + 'ab': 'from a', + 'ac': 'from a', + 'abc': 'from a', + }), + 'dir|b.yaml': _makeOptions(include: 'c.yaml', other: { + 'ab': 'from b', + 'abc': 'from b', + 'b': 'from b', + 'bc': 'from b', + }), + 'dir|c.yaml': _makeOptions(other: { + 'ac': 'from c', + 'abc': 'from c', + 'bc': 'from c', + 'c': 'from c', + }), + }); + + var options = + await readAnalysisOptions(testFS, TestFileSystemPath('dir|a.yaml')); + expect(options['a'], 'from a'); + expect(options['ab'], 'from a'); + expect(options['ac'], 'from a'); + expect(options['abc'], 'from a'); + expect(options['b'], 'from b'); + expect(options['bc'], 'from b'); + expect(options['c'], 'from c'); + }); + + test('removes the include key after merging', () async { + var testFS = TestFileSystem({ + 'dir|main.yaml': _makeOptions(pageWidth: 120, include: 'a.yaml'), + 'dir|a.yaml': _makeOptions(other: {'a': 123}), + }); + + var options = await readAnalysisOptions( + testFS, TestFileSystemPath('dir|main.yaml')); + expect(options['include'], isNull); + }); + + test('locates includes relative to the parent directory', () async { + var testFS = TestFileSystem({ + 'dir|a.yaml': _makeOptions(include: 'sub|b.yaml', other: { + 'a': 'from a', + }), + 'dir|sub|b.yaml': _makeOptions(include: 'more|c.yaml', other: { + 'b': 'from b', + }), + 'dir|sub|more|c.yaml': _makeOptions(other: { + 'c': 'from c', + }), + }); + + var options = + await readAnalysisOptions(testFS, TestFileSystemPath('dir|a.yaml')); + expect(options['a'], 'from a'); + expect(options['b'], 'from b'); + expect(options['c'], 'from c'); + }); + }); +} + +String _makeOptions( + {int? pageWidth, String? include, Map? other}) { + var result = StringBuffer(); + + if (include != null) { + result.writeln('include: $include'); + } + + if (pageWidth != null) { + result.writeln('formatter:'); + result.writeln(' page_width: $pageWidth'); + } + + if (other != null) { + other.forEach((key, value) { + result.writeln('$key:'); + result.writeln(' $value'); + }); + } + + return result.toString(); +} + +/// Reads the `formatter/page_width` key from [options] if present and returns +/// it or `null` if not found. +Object? _pageWidth(AnalysisOptions options) => + (options['formatter'] as Map?)?['page_width']; diff --git a/test/analysis_options/io_file_system_test.dart b/test/analysis_options/io_file_system_test.dart new file mode 100644 index 00000000..93ac6a64 --- /dev/null +++ b/test/analysis_options/io_file_system_test.dart @@ -0,0 +1,136 @@ +// Copyright (c) 2024, 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. + +import 'package:dart_style/src/analysis_options/file_system.dart'; +import 'package:dart_style/src/analysis_options/io_file_system.dart'; +import 'package:path/path.dart' as p; +import 'package:test/test.dart'; +import 'package:test_descriptor/test_descriptor.dart' as d; + +void main() { + group('makePath()', () { + test('creates a relative path', () async { + var fs = IOFileSystem(); + expect( + (await fs.makePath('relative/path.txt')).path, 'relative/path.txt'); + }); + + test('creates an absolute path', () async { + var fs = IOFileSystem(); + var absolutePath = p.style == p.Style.posix ? '/abs' : 'C:\\abs'; + expect((await fs.makePath(absolutePath)).path, absolutePath); + }); + }); + + group('fileExists()', () { + test('returns whether a file exists at that path', () async { + await d.dir('dir', [ + d.file('exists.txt', 'contents'), + ]).create(); + + var fs = IOFileSystem(); + expect( + await fs.fileExists( + await fs.makePath(p.join(d.sandbox, 'dir', 'exists.txt'))), + isTrue); + expect( + await fs.fileExists( + await fs.makePath(p.join(d.sandbox, 'dir', 'nope.txt'))), + isFalse); + }); + + test('returns false if the entry at that path is not a file', () async { + await d.dir('dir', [ + d.dir('sub', []), + ]).create(); + + var fs = IOFileSystem(); + expect( + await fs + .fileExists(await fs.makePath(p.join(d.sandbox, 'dir', 'sub'))), + isFalse); + }); + }); + + group('join()', () { + test('joins paths', () async { + var fs = IOFileSystem(); + expect((await fs.join(await fs.makePath('dir'), 'file.txt')).ioPath, + p.join('dir', 'file.txt')); + }); + + test('joins an absolute path', () async { + var fs = IOFileSystem(); + + var absolutePath = p.style == p.Style.posix ? '/abs' : 'C:\\abs'; + expect((await fs.join(await fs.makePath('dir'), absolutePath)).ioPath, + absolutePath); + }); + }); + + group('parentDirectory()', () { + var fs = IOFileSystem(); + + // Wrap [path] in an IOFileSystemPath, get the parent directory, and unwrap + // the result (which might be null). + Future parent(String path) async => + (await fs.parentDirectory(await fs.makePath(path))).ioPath; + + test('returns the containing directory', () async { + expect(await parent(p.join('dir', 'sub', 'file.txt')), + p.absolute(p.join('dir', 'sub'))); + + expect(await parent(p.join('dir', 'sub')), p.absolute(p.join('dir'))); + }); + + test('returns null at the root directory (POSIX)', () async { + var rootPath = p.style == p.Style.posix ? '/' : 'C:\\'; + expect(await parent(rootPath), null); + }); + }); + + group('readFile()', () { + test('reads a file', () async { + await d.dir('dir', [ + d.file('some_file.txt', 'contents'), + d.dir('sub', [ + d.file('another.txt', 'more'), + ]), + ]).create(); + + var fs = IOFileSystem(); + expect( + await fs.readFile( + await fs.makePath(p.join(d.sandbox, 'dir', 'some_file.txt'))), + 'contents'); + expect( + await fs.readFile(await fs + .makePath(p.join(d.sandbox, 'dir', 'sub', 'another.txt'))), + 'more'); + }); + + test('treats relative paths as relative to the CWD', () async { + await d.dir('dir', [ + d.file('some_file.txt', 'contents'), + d.dir('sub', [ + d.file('another.txt', 'more'), + ]), + ]).create(); + + var fs = IOFileSystem(); + expect( + await fs.readFile( + await fs.makePath(p.join(d.sandbox, 'dir', 'some_file.txt'))), + 'contents'); + expect( + await fs.readFile(await fs + .makePath(p.join(d.sandbox, 'dir', 'sub', 'another.txt'))), + 'more'); + }); + }); +} + +extension on FileSystemPath? { + String? get ioPath => (this as IOFileSystemPath?)?.path; +} diff --git a/test/analysis_options/merge_options_test.dart b/test/analysis_options/merge_options_test.dart new file mode 100644 index 00000000..acf9adc2 --- /dev/null +++ b/test/analysis_options/merge_options_test.dart @@ -0,0 +1,111 @@ +// Copyright (c) 2024, 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. + +import 'package:dart_style/src/analysis_options/merge_options.dart'; +import 'package:test/test.dart'; + +void main() { + group('merge()', () { + test('Map', () { + _testMerge( + { + 'one': true, + 'two': false, + 'three': { + 'nested': {'four': true, 'six': true} + } + }, + { + 'three': { + 'nested': {'four': false, 'five': true}, + 'five': true + }, + 'seven': true + }, + { + 'one': true, + 'two': false, + 'three': { + 'nested': {'four': false, 'five': true, 'six': true}, + 'five': true + }, + 'seven': true + }, + ); + }); + + test('List', () { + _testMerge( + [1, 2, 3], + [2, 3, 4, 5], + [1, 2, 3, 4, 5], + ); + }); + + test('List with promotion', () { + _testMerge( + ['one', 'two', 'three'], + {'three': false, 'four': true}, + {'one': true, 'two': true, 'three': false, 'four': true}, + ); + _testMerge( + {'one': false, 'two': false}, + ['one', 'three'], + {'one': true, 'two': false, 'three': true}, + ); + }); + + test('Map with list promotion', () { + _testMerge( + { + 'one': ['a', 'b', 'c'] + }, + { + 'one': {'a': true, 'b': false} + }, + { + 'one': {'a': true, 'b': false, 'c': true} + }, + ); + }); + + test('Map with no promotion', () { + _testMerge( + { + 'one': ['a', 'b', 'c'] + }, + { + 'one': {'a': 'foo', 'b': 'bar'} + }, + { + 'one': {'a': 'foo', 'b': 'bar'} + }, + ); + }); + + test('Map with no promotion 2', () { + _testMerge( + { + 'one': {'a': 'foo', 'b': 'bar'} + }, + { + 'one': ['a', 'b', 'c'] + }, + { + 'one': ['a', 'b', 'c'] + }, + ); + }); + + test('Other values', () { + _testMerge(1, 2, 2); + _testMerge(1, 'foo', 'foo'); + _testMerge({'foo': 1}, 'foo', 'foo'); + }); + }); +} + +void _testMerge(Object defaults, Object overrides, Object expected) { + expect(merge(defaults, overrides), equals(expected)); +} From 81d0b28b1406c0ea78e5c0bbfb1e99b0896481be Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Tue, 1 Oct 2024 15:02:26 -0700 Subject: [PATCH 2/6] Enable "unnecessary_final" lint and fix violations. --- analysis_options.yaml | 27 +++++++++++++++++++ lib/src/short/line_splitting/solve_state.dart | 2 +- lib/src/short/source_visitor.dart | 2 +- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/analysis_options.yaml b/analysis_options.yaml index 694552bd..0a70fc41 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -3,3 +3,30 @@ include: package:dart_flutter_team_lints/analysis_options.yaml analyzer: errors: comment_references: ignore +linter: + rules: + # Either "unnecessary_final" or "prefer_final_locals" should be used so + # that the codebase consistently uses either "var" or "final" for local + # variables. Choosing the former because the latter also requires "final" + # even on local variables and pattern variables that have type annotations, + # as in: + # + # final Object upcast = 123; + # //^^^ Unnecessarily verbose. + # + # switch (json) { + # case final List list: ... + # // ^^^^^ Unnecessarily verbose. + # } + # + # Using "unnecessary_final" allows those to be: + # + # Object upcast = 123; + # + # switch (json) { + # case List list: ... + # } + # + # Also, making local variables non-final is consistent with parameters, + # which are also non-final. + - unnecessary_final diff --git a/lib/src/short/line_splitting/solve_state.dart b/lib/src/short/line_splitting/solve_state.dart index 6fdda7f9..8abab11f 100644 --- a/lib/src/short/line_splitting/solve_state.dart +++ b/lib/src/short/line_splitting/solve_state.dart @@ -525,7 +525,7 @@ class SolveState { var unboundConstraints = >{}; for (var unbound in _unboundRules) { // Lazily create and add the set to the constraints only if needed. - late final disallowedValues = unboundConstraints[unbound] = {}; + late var disallowedValues = unboundConstraints[unbound] = {}; for (var bound in unbound.constrainedRules) { if (!_boundRules.contains(bound)) continue; diff --git a/lib/src/short/source_visitor.dart b/lib/src/short/source_visitor.dart index 4b964af7..cb68f1b1 100644 --- a/lib/src/short/source_visitor.dart +++ b/lib/src/short/source_visitor.dart @@ -2473,7 +2473,7 @@ class SourceVisitor extends ThrowingAstVisitor { token(node.leftParenthesis); - final rule = PositionalRule(null, argumentCount: 1); + var rule = PositionalRule(null, argumentCount: 1); builder.startRule(rule); rule.beforeArgument(zeroSplit()); From aea8bec891777b81d989e511728d8659a35bc2ac Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Tue, 1 Oct 2024 15:32:23 -0700 Subject: [PATCH 3/6] Split cli_test.dart into separate files. It was getting pretty huge. I didn't want to stuff more tests in there for CLI integration of analysis_options, so splitting it up into smaller files first. There are no meaningful changes here, just moving code around. --- test/cli/cli_test.dart | 221 +++++++++ test/cli/language_version_test.dart | 239 ++++++++++ test/cli/output_test.dart | 215 +++++++++ test/cli/stdin_test.dart | 57 +++ test/cli_test.dart | 685 ---------------------------- 5 files changed, 732 insertions(+), 685 deletions(-) create mode 100644 test/cli/cli_test.dart create mode 100644 test/cli/language_version_test.dart create mode 100644 test/cli/output_test.dart create mode 100644 test/cli/stdin_test.dart delete mode 100644 test/cli_test.dart diff --git a/test/cli/cli_test.dart b/test/cli/cli_test.dart new file mode 100644 index 00000000..b9fec5a6 --- /dev/null +++ b/test/cli/cli_test.dart @@ -0,0 +1,221 @@ +// Copyright (c) 2024, 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. + +import 'dart:convert'; + +import 'package:path/path.dart' as p; +import 'package:test/test.dart'; +import 'package:test_descriptor/test_descriptor.dart' as d; + +import '../utils.dart'; + +void main() { + compileFormatter(); + + group('given file paths', () { + test('formats a directory', () async { + await d.dir('code', [ + d.file('a.dart', unformattedSource), + d.file('b.dart', formattedSource), + d.file('c.dart', unformattedSource) + ]).create(); + + var process = await runFormatterOnDir(); + expect( + await process.stdout.next, 'Formatted ${p.join('code', 'a.dart')}'); + expect( + await process.stdout.next, 'Formatted ${p.join('code', 'c.dart')}'); + expect(await process.stdout.next, + startsWith(r'Formatted 3 files (2 changed)')); + await process.shouldExit(0); + + // Overwrites the files. + await d.dir('code', [d.file('a.dart', formattedSource)]).validate(); + await d.dir('code', [d.file('c.dart', formattedSource)]).validate(); + }); + + test('formats multiple paths', () async { + await d.dir('code', [ + d.dir('subdir', [ + d.file('a.dart', unformattedSource), + ]), + d.file('b.dart', unformattedSource), + d.file('c.dart', unformattedSource) + ]).create(); + + var process = await runFormatter( + [p.join('code', 'subdir'), p.join('code', 'c.dart')]); + expect(await process.stdout.next, + 'Formatted ${p.join('code', 'subdir', 'a.dart')}'); + expect( + await process.stdout.next, 'Formatted ${p.join('code', 'c.dart')}'); + expect(await process.stdout.next, + startsWith(r'Formatted 2 files (2 changed)')); + await process.shouldExit(0); + + // Overwrites the selected files. + await d.dir('code', [ + d.dir('subdir', [ + d.file('a.dart', formattedSource), + ]), + d.file('b.dart', unformattedSource), + d.file('c.dart', formattedSource) + ]).validate(); + }); + }); + + test('exits with 64 on a command line argument error', () async { + var process = await runFormatter(['-wat']); + await process.shouldExit(64); + }); + + test('exits with 65 on a parse error', () async { + await d.dir('code', [d.file('a.dart', 'herp derp i are a dart')]).create(); + + var process = await runFormatterOnDir(); + await process.shouldExit(65); + }); + + test('--version prints the version number', () async { + var process = await runFormatter(['--version']); + + // Match something roughly semver-like. + expect(await process.stdout.next, matches(RegExp(r'\d+\.\d+\.\d+.*'))); + await process.shouldExit(0); + }); + + group('--help', () { + test('non-verbose shows description and common options', () async { + var process = await runFormatter(['--help']); + expect( + await process.stdout.next, 'Idiomatically format Dart source code.'); + await expectLater(process.stdout, emitsThrough(contains('-o, --output'))); + await expectLater(process.stdout, neverEmits(contains('--summary'))); + await process.shouldExit(0); + }); + + test('verbose shows description and all options', () async { + var process = await runFormatter(['--help', '--verbose']); + expect( + await process.stdout.next, 'Idiomatically format Dart source code.'); + await expectLater(process.stdout, emitsThrough(contains('-o, --output'))); + await expectLater(process.stdout, emitsThrough(contains('--show'))); + await expectLater(process.stdout, emitsThrough(contains('--summary'))); + await process.shouldExit(0); + }); + }); + + test('--verbose errors if not used with --help', () async { + var process = await runFormatterOnDir(['--verbose']); + expect(await process.stderr.next, 'Can only use --verbose with --help.'); + await process.shouldExit(64); + }); + + group('--indent', () { + test('sets the leading indentation of the output', () async { + var process = await runFormatter(['--indent=3']); + process.stdin.writeln("main() {'''"); + process.stdin.writeln("a flush left multi-line string''';}"); + await process.stdin.close(); + + expect(await process.stdout.next, ' main() {'); + expect(await process.stdout.next, " '''"); + expect(await process.stdout.next, "a flush left multi-line string''';"); + expect(await process.stdout.next, ' }'); + await process.shouldExit(0); + }); + + test('errors if the indent is not a non-negative number', () async { + var process = await runFormatter(['--indent=notanum']); + await process.shouldExit(64); + + process = await runFormatter(['--indent=-4']); + await process.shouldExit(64); + }); + }); + + group('--set-exit-if-changed', () { + test('gives exit code 0 if there are no changes', () async { + await d.dir('code', [d.file('a.dart', formattedSource)]).create(); + + var process = await runFormatterOnDir(['--set-exit-if-changed']); + await process.shouldExit(0); + }); + + test('gives exit code 1 if there are changes', () async { + await d.dir('code', [d.file('a.dart', unformattedSource)]).create(); + + var process = await runFormatterOnDir(['--set-exit-if-changed']); + await process.shouldExit(1); + }); + + test('gives exit code 1 if there are changes when not writing', () async { + await d.dir('code', [d.file('a.dart', unformattedSource)]).create(); + + var process = + await runFormatterOnDir(['--set-exit-if-changed', '--show=none']); + await process.shouldExit(1); + }); + }); + + group('--selection', () { + test('errors if given path', () async { + var process = await runFormatter(['--selection', 'path']); + await process.shouldExit(64); + }); + + test('errors on wrong number of components', () async { + var process = await runFormatter(['--selection', '1']); + await process.shouldExit(64); + + process = await runFormatter(['--selection', '1:2:3']); + await process.shouldExit(64); + }); + + test('errors on non-integer component', () async { + var process = await runFormatter(['--selection', '1:2.3']); + await process.shouldExit(64); + }); + + test('updates selection', () async { + var process = await runFormatter(['--output=json', '--selection=6:10']); + process.stdin.writeln(unformattedSource); + await process.stdin.close(); + + var json = jsonEncode({ + 'path': 'stdin', + 'source': formattedSource, + 'selection': {'offset': 5, 'length': 9} + }); + + expect(await process.stdout.next, json); + await process.shouldExit(); + }); + }); + + group('--enable-experiment', () { + test('passes experiment flags to parser', () async { + var process = + await runFormatter(['--enable-experiment=test-experiment,variance']); + process.stdin.writeln('class Writer {}'); + await process.stdin.close(); + + // The formatter doesn't actually support formatting variance annotations, + // but we want to test that the experiment flags are passed all the way + // to the parser, so just test that it parses the variance annotation + // without errors and then fails to format. + expect(await process.stderr.next, + 'Hit a bug in the formatter when formatting stdin.'); + expect(await process.stderr.next, + 'Please report at: github.com/dart-lang/dart_style/issues'); + expect(await process.stderr.next, + 'The formatter produced unexpected output. Input was:'); + expect(await process.stderr.next, 'class Writer {}'); + expect(await process.stderr.next, ''); + expect(await process.stderr.next, 'Which formatted to:'); + expect(await process.stderr.next, 'class Writer {}'); + await process.shouldExit(70); + }); + }); +} diff --git a/test/cli/language_version_test.dart b/test/cli/language_version_test.dart new file mode 100644 index 00000000..a9e08d31 --- /dev/null +++ b/test/cli/language_version_test.dart @@ -0,0 +1,239 @@ +// Copyright (c) 2024, 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. + +import 'package:path/path.dart' as p; +import 'package:test/test.dart'; +import 'package:test_descriptor/test_descriptor.dart' as d; + +import '../utils.dart'; + +void main() { + compileFormatter(); + + group('--language-version', () { + // It's hard to validate that the formatter uses the *exact* latest + // language version supported by the formatter, but at least test that a + // new-ish language feature can be parsed. + const extensionTypeBefore = ''' +extension type Meters(int value) { + Meters operator+(Meters other) => Meters(value+other.value); +}'''; + + const extensionTypeAfter = ''' +extension type Meters(int value) { + Meters operator +(Meters other) => Meters(value + other.value); +} +'''; + + test('defaults to latest language version if omitted', () async { + await d.dir('code', [d.file('a.dart', extensionTypeBefore)]).create(); + + var process = await runFormatterOnDir(); + await process.shouldExit(0); + + await d.dir('code', [d.file('a.dart', extensionTypeAfter)]).validate(); + }); + + test('uses the given language version', () async { + const before = 'main() { switch (o) { case 1+2: break; } }'; + + const after = ''' +main() { + switch (o) { + case 1 + 2: + break; + } +} +'''; + + await d.dir('code', [d.file('a.dart', before)]).create(); + + // Use an older language version where `1 + 2` was still a valid switch + // case. + var process = await runFormatterOnDir(['--language-version=2.19']); + await process.shouldExit(0); + + await d.dir('code', [d.file('a.dart', after)]).validate(); + }); + + test('uses the latest language version if "latest"', () async { + await d.dir('code', [d.file('a.dart', extensionTypeBefore)]).create(); + + var process = await runFormatterOnDir(['--language-version=latest']); + await process.shouldExit(0); + + await d.dir('code', [d.file('a.dart', extensionTypeAfter)]).validate(); + }); + + test("errors if the language version can't be parsed", () async { + var process = await runFormatter(['--language-version=123']); + await process.shouldExit(64); + }); + }); + + group('package config', () { + // TODO(rnystrom): Remove this test when the experiment ships. + test('no package search if experiment is off', () async { + // Put the file in a directory with a malformed package config. If we + // search for it, we should get an error. + await d.dir('foo', [ + d.dir('.dart_tool', [ + d.file('package_config.json', 'this no good json is bad json'), + ]), + d.file('main.dart', 'main(){ }'), + ]).create(); + + var process = await runFormatterOnDir(); + await process.shouldExit(0); + + // Should format the file without any error reading the package config. + await d.dir('foo', [d.file('main.dart', 'main() {}\n')]).validate(); + }); + + test('no package search if language version is specified', () async { + // Put the file in a directory with a malformed package config. If we + // search for it, we should get an error. + await d.dir('foo', [ + d.dir('.dart_tool', [ + d.file('package_config.json', 'this no good json is bad json'), + ]), + d.file('main.dart', 'main(){ }'), + ]).create(); + + var process = await runFormatterOnDir( + ['--language-version=latest', '--enable-experiment=tall-style']); + await process.shouldExit(0); + + // Should format the file without any error reading the package config. + await d.dir('foo', [d.file('main.dart', 'main() {}\n')]).validate(); + }); + + test('default to language version of surrounding package', () async { + // The package config sets the language version to 3.1, but the switch + // case uses a syntax which is valid in earlier versions of Dart but an + // error in 3.0 and later. Verify that the error is reported. + await d.dir('foo', [ + packageConfig('foo', 3, 1), + d.file('main.dart', 'main() { switch (o) { case 1 + 2: break; } }'), + ]).create(); + + var path = p.join(d.sandbox, 'foo', 'main.dart'); + // TODO(rnystrom): Remove experiment flag when it ships. + var process = + await runFormatter([path, '--enable-experiment=tall-style']); + + expect(await process.stderr.next, + 'Could not format because the source could not be parsed:'); + expect(await process.stderr.next, ''); + expect(await process.stderr.next, contains('main.dart')); + await process.shouldExit(65); + }); + + test('language version comment overrides package default', () async { + // The package config sets the language version to 3.1, but the switch + // case uses a syntax which is valid in earlier versions of Dart but an + // error in 3.0 and later. Verify that no error is reported since this + // file opts to the older version. + await d.dir('foo', [ + packageConfig('foo', 3, 1), + d.file('main.dart', ''' + // @dart=2.19 + main() { switch (obj) { case 1 + 2: // Error in 3.1. + } } + '''), + ]).create(); + + var process = await runFormatterOnDir(); + await process.shouldExit(0); + + // Formats the file. + await d.dir('foo', [ + d.file('main.dart', ''' +// @dart=2.19 +main() { + switch (obj) { + case 1 + 2: // Error in 3.1. + } +} +''') + ]).validate(); + }); + + test('malformed', () async { + await d.dir('foo', [ + d.dir('.dart_tool', [ + d.file('package_config.json', 'this no good json is bad json'), + ]), + d.file('main.dart', 'main() {}'), + ]).create(); + + var path = p.join(d.sandbox, 'foo', 'main.dart'); + // TODO(rnystrom): Remove experiment flag when it ships. + var process = + await runFormatter([path, '--enable-experiment=tall-style']); + + expect( + await process.stderr.next, + allOf(startsWith('Could not read package configuration for'), + contains(p.join('foo', 'main.dart')))); + await process.shouldExit(65); + }); + }); + + group('stdin', () { + test('infers language version from surrounding package', () async { + // The package config sets the language version to 3.1, but the switch + // case uses a syntax which is valid in earlier versions of Dart but an + // error in 3.0 and later. Verify that the error is reported. + await d.dir('foo', [ + packageConfig('foo', 2, 19), + ]).create(); + + var process = await runFormatter( + ['--enable-experiment=tall-style', '--stdin-name=foo/main.dart']); + // Write a switch whose syntax is valid in 2.19, but an error in later + // versions. + process.stdin.writeln('main() { switch (o) { case 1 + 2: break; } }'); + await process.stdin.close(); + + expect(await process.stdout.next, 'main() {'); + expect(await process.stdout.next, ' switch (o) {'); + expect(await process.stdout.next, ' case 1 + 2:'); + expect(await process.stdout.next, ' break;'); + expect(await process.stdout.next, ' }'); + expect(await process.stdout.next, '}'); + await process.shouldExit(0); + }); + + test('no package search if language version is specified', () async { + // Put the stdin-name in a directory with a malformed package config. If + // we search for it, we should get an error. + await d.dir('foo', [ + d.dir('.dart_tool', [ + d.file('package_config.json', 'this no good json is bad json'), + ]), + d.file('main.dart', 'main(){ }'), + ]).create(); + + var process = await runFormatter([ + '--language-version=2.19', + '--enable-experiment=tall-style', + '--stdin-name=foo/main.dart' + ]); + + // Write a switch whose syntax is valid in 2.19, but an error in later + // versions. + process.stdin.writeln('main() { switch (o) { case 1 + 2: break; } }'); + await process.stdin.close(); + + expect(await process.stdout.next, 'main() {'); + expect(await process.stdout.next, ' switch (o) {'); + expect(await process.stdout.next, ' case 1 + 2:'); + expect(await process.stdout.next, ' break;'); + expect(await process.stdout.next, ' }'); + expect(await process.stdout.next, '}'); + await process.shouldExit(0); + }); + }); +} diff --git a/test/cli/output_test.dart b/test/cli/output_test.dart new file mode 100644 index 00000000..04e5bdb3 --- /dev/null +++ b/test/cli/output_test.dart @@ -0,0 +1,215 @@ +// Copyright (c) 2024, 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. + +import 'dart:convert'; + +import 'package:path/path.dart' as p; +import 'package:test/test.dart'; +import 'package:test_descriptor/test_descriptor.dart' as d; + +import '../utils.dart'; + +void main() { + compileFormatter(); + + group('--show', () { + test('all shows all files', () async { + await d.dir('code', [ + d.file('a.dart', unformattedSource), + d.file('b.dart', formattedSource), + d.file('c.dart', unformattedSource) + ]).create(); + + var process = await runFormatterOnDir(['--show=all']); + expect( + await process.stdout.next, 'Formatted ${p.join('code', 'a.dart')}'); + expect( + await process.stdout.next, 'Unchanged ${p.join('code', 'b.dart')}'); + expect( + await process.stdout.next, 'Formatted ${p.join('code', 'c.dart')}'); + expect(await process.stdout.next, + startsWith(r'Formatted 3 files (2 changed)')); + await process.shouldExit(0); + }); + + test('none shows nothing', () async { + await d.dir('code', [ + d.file('a.dart', unformattedSource), + d.file('b.dart', formattedSource), + d.file('c.dart', unformattedSource) + ]).create(); + + var process = await runFormatterOnDir(['--show=none']); + expect(await process.stdout.next, + startsWith(r'Formatted 3 files (2 changed)')); + await process.shouldExit(0); + }); + + test('changed shows changed files', () async { + await d.dir('code', [ + d.file('a.dart', unformattedSource), + d.file('b.dart', formattedSource), + d.file('c.dart', unformattedSource) + ]).create(); + + var process = await runFormatterOnDir(['--show=changed']); + expect( + await process.stdout.next, 'Formatted ${p.join('code', 'a.dart')}'); + expect( + await process.stdout.next, 'Formatted ${p.join('code', 'c.dart')}'); + expect(await process.stdout.next, + startsWith(r'Formatted 3 files (2 changed)')); + await process.shouldExit(0); + }); + }); + + group('--output', () { + group('show', () { + test('prints only formatted output by default', () async { + await d.dir('code', [ + d.file('a.dart', unformattedSource), + d.file('b.dart', formattedSource) + ]).create(); + + var process = await runFormatterOnDir(['--output=show']); + expect(await process.stdout.next, formattedOutput); + expect(await process.stdout.next, formattedOutput); + expect(await process.stdout.next, + startsWith(r'Formatted 2 files (1 changed)')); + await process.shouldExit(0); + + // Does not overwrite files. + await d.dir('code', [d.file('a.dart', unformattedSource)]).validate(); + }); + + test('with --show=all prints all files and names first', () async { + await d.dir('code', [ + d.file('a.dart', unformattedSource), + d.file('b.dart', formattedSource) + ]).create(); + + var process = await runFormatterOnDir(['--output=show', '--show=all']); + expect( + await process.stdout.next, 'Changed ${p.join('code', 'a.dart')}'); + expect(await process.stdout.next, formattedOutput); + expect( + await process.stdout.next, 'Unchanged ${p.join('code', 'b.dart')}'); + expect(await process.stdout.next, formattedOutput); + expect(await process.stdout.next, + startsWith(r'Formatted 2 files (1 changed)')); + await process.shouldExit(0); + + // Does not overwrite files. + await d.dir('code', [d.file('a.dart', unformattedSource)]).validate(); + }); + + test('with --show=changed prints only changed files', () async { + await d.dir('code', [ + d.file('a.dart', unformattedSource), + d.file('b.dart', formattedSource) + ]).create(); + + var process = + await runFormatterOnDir(['--output=show', '--show=changed']); + expect( + await process.stdout.next, 'Changed ${p.join('code', 'a.dart')}'); + expect(await process.stdout.next, formattedOutput); + expect(await process.stdout.next, + startsWith(r'Formatted 2 files (1 changed)')); + await process.shouldExit(0); + + // Does not overwrite files. + await d.dir('code', [d.file('a.dart', unformattedSource)]).validate(); + }); + }); + + group('json', () { + test('writes each output as json', () async { + await d.dir('code', [ + d.file('a.dart', unformattedSource), + d.file('b.dart', unformattedSource) + ]).create(); + + var jsonA = jsonEncode({ + 'path': p.join('code', 'a.dart'), + 'source': formattedSource, + 'selection': {'offset': -1, 'length': -1} + }); + + var jsonB = jsonEncode({ + 'path': p.join('code', 'b.dart'), + 'source': formattedSource, + 'selection': {'offset': -1, 'length': -1} + }); + + var process = await runFormatterOnDir(['--output=json']); + + expect(await process.stdout.next, jsonA); + expect(await process.stdout.next, jsonB); + await process.shouldExit(); + }); + + test('errors if the summary is not none', () async { + var process = + await runFormatterOnDir(['--output=json', '--summary=line']); + await process.shouldExit(64); + }); + }); + + group('none', () { + test('with --show=all prints only names', () async { + await d.dir('code', [ + d.file('a.dart', unformattedSource), + d.file('b.dart', formattedSource) + ]).create(); + + var process = await runFormatterOnDir(['--output=none', '--show=all']); + expect( + await process.stdout.next, 'Changed ${p.join('code', 'a.dart')}'); + expect( + await process.stdout.next, 'Unchanged ${p.join('code', 'b.dart')}'); + expect(await process.stdout.next, + startsWith(r'Formatted 2 files (1 changed)')); + await process.shouldExit(0); + + // Does not overwrite files. + await d.dir('code', [d.file('a.dart', unformattedSource)]).validate(); + }); + + test('with --show=changed prints only changed names', () async { + await d.dir('code', [ + d.file('a.dart', unformattedSource), + d.file('b.dart', formattedSource) + ]).create(); + + var process = + await runFormatterOnDir(['--output=none', '--show=changed']); + expect( + await process.stdout.next, 'Changed ${p.join('code', 'a.dart')}'); + expect(await process.stdout.next, + startsWith(r'Formatted 2 files (1 changed)')); + await process.shouldExit(0); + + // Does not overwrite files. + await d.dir('code', [d.file('a.dart', unformattedSource)]).validate(); + }); + }); + }); + + group('--summary', () { + test('line', () async { + await d.dir('code', [ + d.file('a.dart', unformattedSource), + d.file('b.dart', formattedSource) + ]).create(); + + var process = await runFormatterOnDir(['--summary=line']); + expect( + await process.stdout.next, 'Formatted ${p.join('code', 'a.dart')}'); + expect(await process.stdout.next, + matches(r'Formatted 2 files \(1 changed\) in \d+\.\d+ seconds.')); + await process.shouldExit(0); + }); + }); +} diff --git a/test/cli/stdin_test.dart b/test/cli/stdin_test.dart new file mode 100644 index 00000000..da24c7d6 --- /dev/null +++ b/test/cli/stdin_test.dart @@ -0,0 +1,57 @@ +// Copyright (c) 2024, 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. + +import 'package:path/path.dart' as p; +import 'package:test/test.dart'; + +import '../utils.dart'; + +void main() { + compileFormatter(); + + group('stdin', () { + test('errors on --output=write', () async { + var process = await runFormatter(['--output=write']); + await process.shouldExit(64); + }); + + test('exits with 65 on parse error', () async { + var process = await runFormatter(); + process.stdin.writeln('herp derp i are a dart'); + await process.stdin.close(); + await process.shouldExit(65); + }); + + test('reads from stdin', () async { + var process = await runFormatter(); + process.stdin.writeln(unformattedSource); + await process.stdin.close(); + + // No trailing newline at the end. + expect(await process.stdout.next, formattedOutput); + await process.shouldExit(0); + }); + }); + + group('--stdin-name', () { + test('errors if also given path', () async { + var process = await runFormatter(['--stdin-name=name', 'path']); + await process.shouldExit(64); + }); + + test('used in error messages', () async { + var path = p.join('some', 'path.dart'); + var process = await runFormatter(['--stdin-name=$path']); + process.stdin.writeln('herp'); + await process.stdin.close(); + + expect(await process.stderr.next, + 'Could not format because the source could not be parsed:'); + expect(await process.stderr.next, ''); + expect(await process.stderr.next, contains(path)); + await process.stderr.cancel(); + await process.shouldExit(65); + }); + }); +} diff --git a/test/cli_test.dart b/test/cli_test.dart deleted file mode 100644 index 610aa1db..00000000 --- a/test/cli_test.dart +++ /dev/null @@ -1,685 +0,0 @@ -// Copyright (c) 2024, 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. - -import 'dart:convert'; - -import 'package:path/path.dart' as p; -import 'package:test/test.dart'; -import 'package:test_descriptor/test_descriptor.dart' as d; - -import 'utils.dart'; - -void main() { - compileFormatter(); - - test('formats a directory', () async { - await d.dir('code', [ - d.file('a.dart', unformattedSource), - d.file('b.dart', formattedSource), - d.file('c.dart', unformattedSource) - ]).create(); - - var process = await runFormatterOnDir(); - expect(await process.stdout.next, 'Formatted ${p.join('code', 'a.dart')}'); - expect(await process.stdout.next, 'Formatted ${p.join('code', 'c.dart')}'); - expect(await process.stdout.next, - startsWith(r'Formatted 3 files (2 changed)')); - await process.shouldExit(0); - - // Overwrites the files. - await d.dir('code', [d.file('a.dart', formattedSource)]).validate(); - await d.dir('code', [d.file('c.dart', formattedSource)]).validate(); - }); - - test('formats multiple paths', () async { - await d.dir('code', [ - d.dir('subdir', [ - d.file('a.dart', unformattedSource), - ]), - d.file('b.dart', unformattedSource), - d.file('c.dart', unformattedSource) - ]).create(); - - var process = await runFormatter( - [p.join('code', 'subdir'), p.join('code', 'c.dart')]); - expect(await process.stdout.next, - 'Formatted ${p.join('code', 'subdir', 'a.dart')}'); - expect(await process.stdout.next, 'Formatted ${p.join('code', 'c.dart')}'); - expect(await process.stdout.next, - startsWith(r'Formatted 2 files (2 changed)')); - await process.shouldExit(0); - - // Overwrites the selected files. - await d.dir('code', [ - d.dir('subdir', [ - d.file('a.dart', formattedSource), - ]), - d.file('b.dart', unformattedSource), - d.file('c.dart', formattedSource) - ]).validate(); - }); - - test('exits with 64 on a command line argument error', () async { - var process = await runFormatter(['-wat']); - await process.shouldExit(64); - }); - - test('exits with 65 on a parse error', () async { - await d.dir('code', [d.file('a.dart', 'herp derp i are a dart')]).create(); - - var process = await runFormatterOnDir(); - await process.shouldExit(65); - }); - - group('--show', () { - test('all shows all files', () async { - await d.dir('code', [ - d.file('a.dart', unformattedSource), - d.file('b.dart', formattedSource), - d.file('c.dart', unformattedSource) - ]).create(); - - var process = await runFormatterOnDir(['--show=all']); - expect( - await process.stdout.next, 'Formatted ${p.join('code', 'a.dart')}'); - expect( - await process.stdout.next, 'Unchanged ${p.join('code', 'b.dart')}'); - expect( - await process.stdout.next, 'Formatted ${p.join('code', 'c.dart')}'); - expect(await process.stdout.next, - startsWith(r'Formatted 3 files (2 changed)')); - await process.shouldExit(0); - }); - - test('none shows nothing', () async { - await d.dir('code', [ - d.file('a.dart', unformattedSource), - d.file('b.dart', formattedSource), - d.file('c.dart', unformattedSource) - ]).create(); - - var process = await runFormatterOnDir(['--show=none']); - expect(await process.stdout.next, - startsWith(r'Formatted 3 files (2 changed)')); - await process.shouldExit(0); - }); - - test('changed shows changed files', () async { - await d.dir('code', [ - d.file('a.dart', unformattedSource), - d.file('b.dart', formattedSource), - d.file('c.dart', unformattedSource) - ]).create(); - - var process = await runFormatterOnDir(['--show=changed']); - expect( - await process.stdout.next, 'Formatted ${p.join('code', 'a.dart')}'); - expect( - await process.stdout.next, 'Formatted ${p.join('code', 'c.dart')}'); - expect(await process.stdout.next, - startsWith(r'Formatted 3 files (2 changed)')); - await process.shouldExit(0); - }); - }); - - group('--output', () { - group('show', () { - test('prints only formatted output by default', () async { - await d.dir('code', [ - d.file('a.dart', unformattedSource), - d.file('b.dart', formattedSource) - ]).create(); - - var process = await runFormatterOnDir(['--output=show']); - expect(await process.stdout.next, formattedOutput); - expect(await process.stdout.next, formattedOutput); - expect(await process.stdout.next, - startsWith(r'Formatted 2 files (1 changed)')); - await process.shouldExit(0); - - // Does not overwrite files. - await d.dir('code', [d.file('a.dart', unformattedSource)]).validate(); - }); - - test('with --show=all prints all files and names first', () async { - await d.dir('code', [ - d.file('a.dart', unformattedSource), - d.file('b.dart', formattedSource) - ]).create(); - - var process = await runFormatterOnDir(['--output=show', '--show=all']); - expect( - await process.stdout.next, 'Changed ${p.join('code', 'a.dart')}'); - expect(await process.stdout.next, formattedOutput); - expect( - await process.stdout.next, 'Unchanged ${p.join('code', 'b.dart')}'); - expect(await process.stdout.next, formattedOutput); - expect(await process.stdout.next, - startsWith(r'Formatted 2 files (1 changed)')); - await process.shouldExit(0); - - // Does not overwrite files. - await d.dir('code', [d.file('a.dart', unformattedSource)]).validate(); - }); - - test('with --show=changed prints only changed files', () async { - await d.dir('code', [ - d.file('a.dart', unformattedSource), - d.file('b.dart', formattedSource) - ]).create(); - - var process = - await runFormatterOnDir(['--output=show', '--show=changed']); - expect( - await process.stdout.next, 'Changed ${p.join('code', 'a.dart')}'); - expect(await process.stdout.next, formattedOutput); - expect(await process.stdout.next, - startsWith(r'Formatted 2 files (1 changed)')); - await process.shouldExit(0); - - // Does not overwrite files. - await d.dir('code', [d.file('a.dart', unformattedSource)]).validate(); - }); - }); - - group('json', () { - test('writes each output as json', () async { - await d.dir('code', [ - d.file('a.dart', unformattedSource), - d.file('b.dart', unformattedSource) - ]).create(); - - var jsonA = jsonEncode({ - 'path': p.join('code', 'a.dart'), - 'source': formattedSource, - 'selection': {'offset': -1, 'length': -1} - }); - - var jsonB = jsonEncode({ - 'path': p.join('code', 'b.dart'), - 'source': formattedSource, - 'selection': {'offset': -1, 'length': -1} - }); - - var process = await runFormatterOnDir(['--output=json']); - - expect(await process.stdout.next, jsonA); - expect(await process.stdout.next, jsonB); - await process.shouldExit(); - }); - - test('errors if the summary is not none', () async { - var process = - await runFormatterOnDir(['--output=json', '--summary=line']); - await process.shouldExit(64); - }); - }); - - group('none', () { - test('with --show=all prints only names', () async { - await d.dir('code', [ - d.file('a.dart', unformattedSource), - d.file('b.dart', formattedSource) - ]).create(); - - var process = await runFormatterOnDir(['--output=none', '--show=all']); - expect( - await process.stdout.next, 'Changed ${p.join('code', 'a.dart')}'); - expect( - await process.stdout.next, 'Unchanged ${p.join('code', 'b.dart')}'); - expect(await process.stdout.next, - startsWith(r'Formatted 2 files (1 changed)')); - await process.shouldExit(0); - - // Does not overwrite files. - await d.dir('code', [d.file('a.dart', unformattedSource)]).validate(); - }); - - test('with --show=changed prints only changed names', () async { - await d.dir('code', [ - d.file('a.dart', unformattedSource), - d.file('b.dart', formattedSource) - ]).create(); - - var process = - await runFormatterOnDir(['--output=none', '--show=changed']); - expect( - await process.stdout.next, 'Changed ${p.join('code', 'a.dart')}'); - expect(await process.stdout.next, - startsWith(r'Formatted 2 files (1 changed)')); - await process.shouldExit(0); - - // Does not overwrite files. - await d.dir('code', [d.file('a.dart', unformattedSource)]).validate(); - }); - }); - }); - - group('--summary', () { - test('line', () async { - await d.dir('code', [ - d.file('a.dart', unformattedSource), - d.file('b.dart', formattedSource) - ]).create(); - - var process = await runFormatterOnDir(['--summary=line']); - expect( - await process.stdout.next, 'Formatted ${p.join('code', 'a.dart')}'); - expect(await process.stdout.next, - matches(r'Formatted 2 files \(1 changed\) in \d+\.\d+ seconds.')); - await process.shouldExit(0); - }); - }); - - test('--version prints the version number', () async { - var process = await runFormatter(['--version']); - - // Match something roughly semver-like. - expect(await process.stdout.next, matches(RegExp(r'\d+\.\d+\.\d+.*'))); - await process.shouldExit(0); - }); - - group('--help', () { - test('non-verbose shows description and common options', () async { - var process = await runFormatter(['--help']); - expect( - await process.stdout.next, 'Idiomatically format Dart source code.'); - await expectLater(process.stdout, emitsThrough(contains('-o, --output'))); - await expectLater(process.stdout, neverEmits(contains('--summary'))); - await process.shouldExit(0); - }); - - test('verbose shows description and all options', () async { - var process = await runFormatter(['--help', '--verbose']); - expect( - await process.stdout.next, 'Idiomatically format Dart source code.'); - await expectLater(process.stdout, emitsThrough(contains('-o, --output'))); - await expectLater(process.stdout, emitsThrough(contains('--show'))); - await expectLater(process.stdout, emitsThrough(contains('--summary'))); - await process.shouldExit(0); - }); - }); - - test('--verbose errors if not used with --help', () async { - var process = await runFormatterOnDir(['--verbose']); - expect(await process.stderr.next, 'Can only use --verbose with --help.'); - await process.shouldExit(64); - }); - - group('--indent', () { - test('sets the leading indentation of the output', () async { - var process = await runFormatter(['--indent=3']); - process.stdin.writeln("main() {'''"); - process.stdin.writeln("a flush left multi-line string''';}"); - await process.stdin.close(); - - expect(await process.stdout.next, ' main() {'); - expect(await process.stdout.next, " '''"); - expect(await process.stdout.next, "a flush left multi-line string''';"); - expect(await process.stdout.next, ' }'); - await process.shouldExit(0); - }); - - test('errors if the indent is not a non-negative number', () async { - var process = await runFormatter(['--indent=notanum']); - await process.shouldExit(64); - - process = await runFormatter(['--indent=-4']); - await process.shouldExit(64); - }); - }); - - group('--set-exit-if-changed', () { - test('gives exit code 0 if there are no changes', () async { - await d.dir('code', [d.file('a.dart', formattedSource)]).create(); - - var process = await runFormatterOnDir(['--set-exit-if-changed']); - await process.shouldExit(0); - }); - - test('gives exit code 1 if there are changes', () async { - await d.dir('code', [d.file('a.dart', unformattedSource)]).create(); - - var process = await runFormatterOnDir(['--set-exit-if-changed']); - await process.shouldExit(1); - }); - - test('gives exit code 1 if there are changes when not writing', () async { - await d.dir('code', [d.file('a.dart', unformattedSource)]).create(); - - var process = - await runFormatterOnDir(['--set-exit-if-changed', '--show=none']); - await process.shouldExit(1); - }); - }); - - group('--selection', () { - test('errors if given path', () async { - var process = await runFormatter(['--selection', 'path']); - await process.shouldExit(64); - }); - - test('errors on wrong number of components', () async { - var process = await runFormatter(['--selection', '1']); - await process.shouldExit(64); - - process = await runFormatter(['--selection', '1:2:3']); - await process.shouldExit(64); - }); - - test('errors on non-integer component', () async { - var process = await runFormatter(['--selection', '1:2.3']); - await process.shouldExit(64); - }); - - test('updates selection', () async { - var process = await runFormatter(['--output=json', '--selection=6:10']); - process.stdin.writeln(unformattedSource); - await process.stdin.close(); - - var json = jsonEncode({ - 'path': 'stdin', - 'source': formattedSource, - 'selection': {'offset': 5, 'length': 9} - }); - - expect(await process.stdout.next, json); - await process.shouldExit(); - }); - }); - - group('--stdin-name', () { - test('errors if also given path', () async { - var process = await runFormatter(['--stdin-name=name', 'path']); - await process.shouldExit(64); - }); - - test('infers language version from surrounding package', () async { - // The package config sets the language version to 3.1, but the switch - // case uses a syntax which is valid in earlier versions of Dart but an - // error in 3.0 and later. Verify that the error is reported. - await d.dir('foo', [ - packageConfig('foo', 2, 19), - ]).create(); - - var process = await runFormatter( - ['--enable-experiment=tall-style', '--stdin-name=foo/main.dart']); - // Write a switch whose syntax is valid in 2.19, but an error in later - // versions. - process.stdin.writeln('main() { switch (o) { case 1 + 2: break; } }'); - await process.stdin.close(); - - expect(await process.stdout.next, 'main() {'); - expect(await process.stdout.next, ' switch (o) {'); - expect(await process.stdout.next, ' case 1 + 2:'); - expect(await process.stdout.next, ' break;'); - expect(await process.stdout.next, ' }'); - expect(await process.stdout.next, '}'); - await process.shouldExit(0); - }); - - test('no package search if language version is specified', () async { - // Put the stdin-name in a directory with a malformed package config. If - // we search for it, we should get an error. - await d.dir('foo', [ - d.dir('.dart_tool', [ - d.file('package_config.json', 'this no good json is bad json'), - ]), - d.file('main.dart', 'main(){ }'), - ]).create(); - - var process = await runFormatter([ - '--language-version=2.19', - '--enable-experiment=tall-style', - '--stdin-name=foo/main.dart' - ]); - - // Write a switch whose syntax is valid in 2.19, but an error in later - // versions. - process.stdin.writeln('main() { switch (o) { case 1 + 2: break; } }'); - await process.stdin.close(); - - expect(await process.stdout.next, 'main() {'); - expect(await process.stdout.next, ' switch (o) {'); - expect(await process.stdout.next, ' case 1 + 2:'); - expect(await process.stdout.next, ' break;'); - expect(await process.stdout.next, ' }'); - expect(await process.stdout.next, '}'); - await process.shouldExit(0); - }); - }); - - group('language version', () { - // It's hard to validate that the formatter uses the *exact* latest - // language version supported by the formatter, but at least test that a - // new-ish language feature can be parsed. - const extensionTypeBefore = ''' -extension type Meters(int value) { - Meters operator+(Meters other) => Meters(value+other.value); -}'''; - - const extensionTypeAfter = ''' -extension type Meters(int value) { - Meters operator +(Meters other) => Meters(value + other.value); -} -'''; - - test('defaults to latest language version if omitted', () async { - await d.dir('code', [d.file('a.dart', extensionTypeBefore)]).create(); - - var process = await runFormatterOnDir(); - await process.shouldExit(0); - - await d.dir('code', [d.file('a.dart', extensionTypeAfter)]).validate(); - }); - - test('uses the given language version', () async { - const before = 'main() { switch (o) { case 1+2: break; } }'; - - const after = ''' -main() { - switch (o) { - case 1 + 2: - break; - } -} -'''; - - await d.dir('code', [d.file('a.dart', before)]).create(); - - // Use an older language version where `1 + 2` was still a valid switch - // case. - var process = await runFormatterOnDir(['--language-version=2.19']); - await process.shouldExit(0); - - await d.dir('code', [d.file('a.dart', after)]).validate(); - }); - - test('uses the latest language version if "latest"', () async { - await d.dir('code', [d.file('a.dart', extensionTypeBefore)]).create(); - - var process = await runFormatterOnDir(['--language-version=latest']); - await process.shouldExit(0); - - await d.dir('code', [d.file('a.dart', extensionTypeAfter)]).validate(); - }); - - test("errors if the language version can't be parsed", () async { - var process = await runFormatter(['--language-version=123']); - await process.shouldExit(64); - }); - }); - - group('--enable-experiment', () { - test('passes experiment flags to parser', () async { - var process = - await runFormatter(['--enable-experiment=test-experiment,variance']); - process.stdin.writeln('class Writer {}'); - await process.stdin.close(); - - // The formatter doesn't actually support formatting variance annotations, - // but we want to test that the experiment flags are passed all the way - // to the parser, so just test that it parses the variance annotation - // without errors and then fails to format. - expect(await process.stderr.next, - 'Hit a bug in the formatter when formatting stdin.'); - expect(await process.stderr.next, - 'Please report at: github.com/dart-lang/dart_style/issues'); - expect(await process.stderr.next, - 'The formatter produced unexpected output. Input was:'); - expect(await process.stderr.next, 'class Writer {}'); - expect(await process.stderr.next, ''); - expect(await process.stderr.next, 'Which formatted to:'); - expect(await process.stderr.next, 'class Writer {}'); - await process.shouldExit(70); - }); - }); - - group('with no paths', () { - test('errors on --output=write', () async { - var process = await runFormatter(['--output=write']); - await process.shouldExit(64); - }); - - test('exits with 65 on parse error', () async { - var process = await runFormatter(); - process.stdin.writeln('herp derp i are a dart'); - await process.stdin.close(); - await process.shouldExit(65); - }); - - test('reads from stdin', () async { - var process = await runFormatter(); - process.stdin.writeln(unformattedSource); - await process.stdin.close(); - - // No trailing newline at the end. - expect(await process.stdout.next, formattedOutput); - await process.shouldExit(0); - }); - - test('allows specifying stdin path name', () async { - var path = p.join('some', 'path.dart'); - var process = await runFormatter(['--stdin-name=$path']); - process.stdin.writeln('herp'); - await process.stdin.close(); - - expect(await process.stderr.next, - 'Could not format because the source could not be parsed:'); - expect(await process.stderr.next, ''); - expect(await process.stderr.next, contains(path)); - await process.stderr.cancel(); - await process.shouldExit(65); - }); - }); - - group('package config', () { - // TODO(rnystrom): Remove this test when the experiment ships. - test('no package search if experiment is off', () async { - // Put the file in a directory with a malformed package config. If we - // search for it, we should get an error. - await d.dir('foo', [ - d.dir('.dart_tool', [ - d.file('package_config.json', 'this no good json is bad json'), - ]), - d.file('main.dart', 'main(){ }'), - ]).create(); - - var process = await runFormatterOnDir(); - await process.shouldExit(0); - - // Should format the file without any error reading the package config. - await d.dir('foo', [d.file('main.dart', 'main() {}\n')]).validate(); - }); - - test('no package search if language version is specified', () async { - // Put the file in a directory with a malformed package config. If we - // search for it, we should get an error. - await d.dir('foo', [ - d.dir('.dart_tool', [ - d.file('package_config.json', 'this no good json is bad json'), - ]), - d.file('main.dart', 'main(){ }'), - ]).create(); - - var process = await runFormatterOnDir( - ['--language-version=latest', '--enable-experiment=tall-style']); - await process.shouldExit(0); - - // Should format the file without any error reading the package config. - await d.dir('foo', [d.file('main.dart', 'main() {}\n')]).validate(); - }); - - test('default to language version of surrounding package', () async { - // The package config sets the language version to 3.1, but the switch - // case uses a syntax which is valid in earlier versions of Dart but an - // error in 3.0 and later. Verify that the error is reported. - await d.dir('foo', [ - packageConfig('foo', 3, 1), - d.file('main.dart', 'main() { switch (o) { case 1 + 2: break; } }'), - ]).create(); - - var path = p.join(d.sandbox, 'foo', 'main.dart'); - // TODO(rnystrom): Remove experiment flag when it ships. - var process = - await runFormatter([path, '--enable-experiment=tall-style']); - - expect(await process.stderr.next, - 'Could not format because the source could not be parsed:'); - expect(await process.stderr.next, ''); - expect(await process.stderr.next, contains('main.dart')); - await process.shouldExit(65); - }); - - test('language version comment overrides package default', () async { - // The package config sets the language version to 3.1, but the switch - // case uses a syntax which is valid in earlier versions of Dart but an - // error in 3.0 and later. Verify that no error is reported since this - // file opts to the older version. - await d.dir('foo', [ - packageConfig('foo', 3, 1), - d.file('main.dart', ''' - // @dart=2.19 - main() { switch (obj) { case 1 + 2: // Error in 3.1. - } } - '''), - ]).create(); - - var process = await runFormatterOnDir(); - await process.shouldExit(0); - - // Formats the file. - await d.dir('foo', [ - d.file('main.dart', ''' -// @dart=2.19 -main() { - switch (obj) { - case 1 + 2: // Error in 3.1. - } -} -''') - ]).validate(); - }); - - test('malformed', () async { - await d.dir('foo', [ - d.dir('.dart_tool', [ - d.file('package_config.json', 'this no good json is bad json'), - ]), - d.file('main.dart', 'main() {}'), - ]).create(); - - var path = p.join(d.sandbox, 'foo', 'main.dart'); - // TODO(rnystrom): Remove experiment flag when it ships. - var process = - await runFormatter([path, '--enable-experiment=tall-style']); - - expect( - await process.stderr.next, - allOf(startsWith('Could not read package configuration for'), - contains(p.join('foo', 'main.dart')))); - await process.shouldExit(65); - }); - }); -} From f56469b2ddcbd0315d205213cc0de2dd0aa32fd0 Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Thu, 3 Oct 2024 16:08:05 -0700 Subject: [PATCH 4/6] Look up page width of surrounding analysis_options.yaml file. When formatting from the CLI using files or stdin with a --stdin-name option, looks in the surrounding directories for an analysis_options.yaml file. If found and the file specifies a formatter page width, uses that. Handles includes and merging as well. Doesn't handle "package:" includes yet. --- lib/src/cli/format_command.dart | 9 +- lib/src/cli/formatter_options.dart | 9 +- lib/src/config_cache.dart | 140 ++++++++++ lib/src/dart_formatter.dart | 6 +- lib/src/io.dart | 82 +++--- lib/src/language_version_cache.dart | 68 ----- .../analysis_options_file_test.dart | 56 ++-- test/cli/language_version_test.dart | 6 +- test/cli/page_width_test.dart | 180 +++++++++++++ test/config_cache_test.dart | 249 ++++++++++++++++++ test/language_version_cache_test.dart | 140 ---------- test/utils.dart | 75 +++++- 12 files changed, 719 insertions(+), 301 deletions(-) create mode 100644 lib/src/config_cache.dart delete mode 100644 lib/src/language_version_cache.dart create mode 100644 test/cli/page_width_test.dart create mode 100644 test/config_cache_test.dart delete mode 100644 test/language_version_cache_test.dart diff --git a/lib/src/cli/format_command.dart b/lib/src/cli/format_command.dart index 5618261f..c59f71b2 100644 --- a/lib/src/cli/format_command.dart +++ b/lib/src/cli/format_command.dart @@ -189,9 +189,12 @@ class FormatCommand extends Command { } } - var pageWidth = int.tryParse(argResults['line-length'] as String) ?? - usageException('--line-length must be an integer, was ' - '"${argResults['line-length']}".'); + int? pageWidth; + if (argResults.wasParsed('line-length')) { + pageWidth = int.tryParse(argResults['line-length'] as String) ?? + usageException('--line-length must be an integer, was ' + '"${argResults['line-length']}".'); + } var indent = int.tryParse(argResults['indent'] as String) ?? usageException('--indent must be an integer, was ' diff --git a/lib/src/cli/formatter_options.dart b/lib/src/cli/formatter_options.dart index d20e4238..43140f01 100644 --- a/lib/src/cli/formatter_options.dart +++ b/lib/src/cli/formatter_options.dart @@ -24,8 +24,11 @@ class FormatterOptions { final int indent; /// The number of columns that formatted output should be constrained to fit - /// within. - final int pageWidth; + /// within or `null` if not specified. + /// + /// If omitted, the formatter defaults to a page width of + /// [DartFormatter.defaultPageWidth]. + final int? pageWidth; /// Whether symlinks should be traversed when formatting a directory. final bool followLinks; @@ -49,7 +52,7 @@ class FormatterOptions { FormatterOptions( {this.languageVersion, this.indent = 0, - this.pageWidth = 80, + this.pageWidth, this.followLinks = false, this.show = Show.changed, this.output = Output.write, diff --git a/lib/src/config_cache.dart b/lib/src/config_cache.dart new file mode 100644 index 00000000..af3ac489 --- /dev/null +++ b/lib/src/config_cache.dart @@ -0,0 +1,140 @@ +// Copyright (c) 2024, 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. +import 'dart:async'; +import 'dart:io'; + +import 'package:package_config/package_config.dart'; +import 'package:pub_semver/pub_semver.dart'; + +import 'analysis_options/analysis_options_file.dart'; +import 'analysis_options/io_file_system.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. +/// +/// 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 +/// will share the same package config, so this caches a previously read +/// config for each directory. +/// +/// (When formatting dart_style on a Mac laptop, it would spend as much time +/// looking for package configs for each file as it did formatting if we don't +/// cache. Caching makes it ~10x faster to find the config for each file.) +/// +/// This class also directly caches the language versions and page widths that +/// are then inferred from the package config and analysis_options.yaml files. +class ConfigCache { + /// The previously cached package config for all files immediately within a + /// given directory. + final Map _directoryConfigs = {}; + + /// The previously cached default language version for all files immediately + /// within a given directory. + /// + /// The version may be `null` if we formatted a file in that directory and + /// discovered that there is no surrounding package. + final Map _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 _directoryPageWidths = {}; + + final IOFileSystem _fileSystem = IOFileSystem(); + + /// Looks for a package surrounding [file] and, if found, returns the default + /// language version specified by that package. + Future findLanguageVersion(File file, String displayPath) async { + // Use the cached version (which may be `null`) if present. + var directory = file.parent.path; + if (_directoryVersions.containsKey(directory)) { + return _directoryVersions[directory]; + } + + // Otherwise, walk the file system and look for it. + var config = + await _findPackageConfig(file, displayPath, forLanguageVersion: true); + if (config?.packageOf(file.absolute.uri)?.languageVersion + case var languageVersion?) { + // Store the version as pub_semver's [Version] type because that's + // what the analyzer parser uses, which is where the version + // ultimately gets used. + var version = Version(languageVersion.major, languageVersion.minor, 0); + return _directoryVersions[directory] = version; + } + + // We weren't able to resolve this file's version, so don't try again. + return _directoryVersions[directory] = null; + } + + /// Looks for an "analysis_options.yaml" file surrounding [file] and, if + /// found and valid, returns the page width specified by that config file. + /// + /// Otherwise returns `null`. + /// + /// The schema looks like: + /// + /// formatter: + /// page_width: 123 + Future findPageWidth(File file) async { + // Use the cached version (which may be `null`) if present. + var directory = file.parent.path; + if (_directoryPageWidths.containsKey(directory)) { + return _directoryPageWidths[directory]; + } + + // Look for a surrounding analysis_options.yaml file. + var options = await findAnalysisOptions( + _fileSystem, await _fileSystem.makePath(file.path)); + + if (options['formatter'] case Map formatter) { + if (formatter['page_width'] case int pageWidth) { + return _directoryPageWidths[directory] = pageWidth; + } + } + + // 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; + } + + /// Look for and cache the nearest package surrounding [file]. + Future _findPackageConfig(File file, String displayPath, + {required bool forLanguageVersion}) async { + Profile.begin('look up package config'); + try { + // Use the cached one (which might be `null`) if we have it. + var directory = file.parent.path; + if (_directoryConfigs.containsKey(directory)) { + return _directoryConfigs[directory]; + } + + // Otherwise, walk the file system and look for it. If we fail to find it, + // store `null` so that we don't look again in that same directory. + return _directoryConfigs[directory] = + await findPackageConfig(file.parent); + } catch (error) { + // We need a language version, so report an error if we can't find one. + // We don't need a page width because we happily use the default, so say + // nothing in that case. + if (forLanguageVersion) { + stderr.writeln('Could not read package configuration for ' + '$displayPath:\n$error'); + stderr.writeln('To avoid searching for a package configuration, ' + 'specify a language version using "--language-version".'); + } + return null; + } finally { + Profile.end('look up package config'); + } + } +} diff --git a/lib/src/dart_formatter.dart b/lib/src/dart_formatter.dart index 9394448d..7e5401be 100644 --- a/lib/src/dart_formatter.dart +++ b/lib/src/dart_formatter.dart @@ -27,6 +27,10 @@ class DartFormatter { /// version of the formatter. static final latestLanguageVersion = Version(3, 3, 0); + /// The page width that the formatter tries to fit code inside if no other + /// width is specified. + static const defaultPageWidth = 80; + /// The Dart language version that formatted code should be parsed as. /// /// Note that a `// @dart=` comment inside the code overrides this. @@ -64,7 +68,7 @@ class DartFormatter { int? pageWidth, int? indent, List? experimentFlags}) - : pageWidth = pageWidth ?? 80, + : pageWidth = pageWidth ?? defaultPageWidth, indent = indent ?? 0, experimentFlags = [...?experimentFlags]; diff --git a/lib/src/io.dart b/lib/src/io.dart index b2def3d0..b4808265 100644 --- a/lib/src/io.dart +++ b/lib/src/io.dart @@ -6,13 +6,12 @@ import 'dart:convert'; import 'dart:io'; import 'package:path/path.dart' as p; -import 'package:pub_semver/pub_semver.dart'; import 'cli/formatter_options.dart'; +import 'config_cache.dart'; import 'constants.dart'; import 'dart_formatter.dart'; import 'exceptions.dart'; -import 'language_version_cache.dart'; import 'source_code.dart'; /// Reads and formats input from stdin until closed. @@ -26,21 +25,35 @@ Future formatStdin( selectionLength = selection[1]; } + ConfigCache? cache; + // TODO(rnystrom): Remove the experiment check when the experiment ships. + if (options.experimentFlags.contains(tallStyleExperimentFlag)) { + cache = ConfigCache(); + } + var languageVersion = options.languageVersion; - if (languageVersion == null && path != null) { - // TODO(rnystrom): Remove the experiment check when the experiment ships. - if (options.experimentFlags.contains(tallStyleExperimentFlag)) { - var cache = LanguageVersionCache(); - languageVersion = await cache.find(File(path), path); - - // If the lookup failed, don't try to parse the code. - if (languageVersion == null) return; - } + if (languageVersion == null && path != null && cache != null) { + // We have a stdin-name, so look for a surrounding package config. + languageVersion = await cache.findLanguageVersion(File(path), path); + + // If the lookup failed, don't try to parse the code. + if (languageVersion == null) return; } // If they didn't specify a version or a path, default to the latest. languageVersion ??= DartFormatter.latestLanguageVersion; + // Determine the page width. + var pageWidth = options.pageWidth; + if (pageWidth == null && path != null && cache != null) { + // We have a stdin-name, so look for a surrounding analyisis_options.yaml. + pageWidth = await cache.findPageWidth(File(path)); + } + + // Use a default page width if we don't have a specified one and couldn't + // find a configured one. + pageWidth ??= DartFormatter.defaultPageWidth; + var name = path ?? 'stdin'; var completer = Completer(); @@ -49,7 +62,7 @@ Future formatStdin( var formatter = DartFormatter( languageVersion: languageVersion!, indent: options.indent, - pageWidth: options.pageWidth, + pageWidth: pageWidth, experimentFlags: options.experimentFlags); try { options.beforeFile(null, name); @@ -81,12 +94,11 @@ $stack'''); Future formatPaths(FormatterOptions options, List paths) async { // If the user didn't specify a language version, then look for surrounding // package configs so we know what language versions to use for the files. - LanguageVersionCache? cache; - if (options.languageVersion == null) { - // TODO(rnystrom): Remove the experiment check when the experiment ships. - if (options.experimentFlags.contains(tallStyleExperimentFlag)) { - cache = LanguageVersionCache(); - } + ConfigCache? cache; + // TODO(rnystrom): Remove the experiment check when the experiment ships and + // make cache non-nullable. + if (options.experimentFlags.contains(tallStyleExperimentFlag)) { + cache = ConfigCache(); } for (var path in paths) { @@ -114,8 +126,8 @@ Future formatPaths(FormatterOptions options, List paths) async { /// /// Returns `true` if successful or `false` if an error occurred in any of the /// files. -Future _processDirectory(LanguageVersionCache? cache, - FormatterOptions options, Directory directory) async { +Future _processDirectory( + ConfigCache? cache, FormatterOptions options, Directory directory) async { var success = true; var entries = @@ -144,28 +156,36 @@ Future _processDirectory(LanguageVersionCache? cache, /// /// Returns `true` if successful or `false` if an error occurred. Future _processFile( - LanguageVersionCache? cache, FormatterOptions options, File file, + ConfigCache? cache, FormatterOptions options, File file, {String? displayPath}) async { displayPath ??= file.path; - // Determine what language version to use. If we have a language version - // cache, that implies that we should use the surrounding package config to - // infer the file's language version. Otherwise, use the user-provided - // version. - Version? languageVersion; - if (cache != null) { - languageVersion = await cache.find(file, displayPath); + // Determine what language version to use. + var languageVersion = options.languageVersion; + if (languageVersion == null && cache != null) { + languageVersion = await cache.findLanguageVersion(file, displayPath); // If the lookup failed, don't try to parse the file. if (languageVersion == null) return false; } else { - languageVersion = options.languageVersion; + // If they didn't specify a version or a path, default to the latest. + languageVersion ??= DartFormatter.latestLanguageVersion; } + // Determine the page width. + var pageWidth = options.pageWidth; + if (pageWidth == null && cache != null) { + pageWidth = await cache.findPageWidth(file); + } + + // Use a default page width if we don't have a specified one and couldn't + // find a configured one. + pageWidth ??= DartFormatter.defaultPageWidth; + var formatter = DartFormatter( - languageVersion: languageVersion ?? DartFormatter.latestLanguageVersion, + languageVersion: languageVersion, indent: options.indent, - pageWidth: options.pageWidth, + pageWidth: pageWidth, experimentFlags: options.experimentFlags); try { diff --git a/lib/src/language_version_cache.dart b/lib/src/language_version_cache.dart deleted file mode 100644 index 14e04d50..00000000 --- a/lib/src/language_version_cache.dart +++ /dev/null @@ -1,68 +0,0 @@ -// Copyright (c) 2024, 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. -import 'dart:async'; -import 'dart:io'; - -import 'package:package_config/package_config.dart'; -import 'package:pub_semver/pub_semver.dart'; - -import 'profile.dart'; - -/// Caches the default language version that should be used for files within -/// directories. -/// -/// The default language version for a Dart file is found by walking the parent -/// directories of the file being formatted to look for a -/// `.dart_tool/package_config.json` file. When found, we cache the result for -/// the formatted file's parent directory. This way, when formatting multiple -/// files in the same directory, we don't have to look for and read the package -/// config multiple times, which is slow. -/// -/// (When formatting dart_style on a Mac laptop, it would spend as much time -/// looking for package configs for each file as it did formatting if we don't -/// cache. Caching makes it ~10x faster to find the language version for each -/// file.) -class LanguageVersionCache { - /// The previously cached default language version for all files immediately - /// within a given directory. - /// - /// The version may be `null` if we formatted a file in that directory and - /// discovered that there is no surrounding package. - final Map _directoryVersions = {}; - - /// Looks for a package surrounding [file] and, if found, returns the default - /// language version specified by that package. - Future find(File file, String displayPath) async { - Profile.begin('look up package config'); - try { - // Use the cached version (which may be `null`) if present. - var directory = file.parent.path; - if (_directoryVersions.containsKey(directory)) { - return _directoryVersions[directory]; - } - - // Otherwise, walk the file system and look for it. - var config = await findPackageConfig(file.parent); - if (config?.packageOf(file.absolute.uri)?.languageVersion - case var languageVersion?) { - // Store the version as pub_semver's [Version] type because that's - // what the analyzer parser uses, which is where the version - // ultimately gets used. - var version = Version(languageVersion.major, languageVersion.minor, 0); - return _directoryVersions[directory] = version; - } - - // We weren't able to resolve this file's directory, so don't try again. - return _directoryVersions[directory] = null; - } catch (error) { - stderr.writeln('Could not read package configuration for ' - '$displayPath:\n$error'); - stderr.writeln('To avoid searching for a package configuration, ' - 'specify a language version using "--language-version".'); - return null; - } finally { - Profile.end('look up package config'); - } - } -} diff --git a/test/analysis_options/analysis_options_file_test.dart b/test/analysis_options/analysis_options_file_test.dart index 4cccdf9e..9795919b 100644 --- a/test/analysis_options/analysis_options_file_test.dart +++ b/test/analysis_options/analysis_options_file_test.dart @@ -6,6 +6,8 @@ import 'package:dart_style/src/analysis_options/analysis_options_file.dart'; import 'package:dart_style/src/testing/test_file_system.dart'; import 'package:test/test.dart'; +import '../utils.dart'; + void main() { group('findAnalysisOptionsoptions()', () { test('returns an empty map if no analysis options is found', () async { @@ -17,7 +19,7 @@ void main() { test('finds a file in the given directory', () async { var testFS = TestFileSystem({ - 'dir|analysis_options.yaml': _makeOptions(pageWidth: 100), + 'dir|analysis_options.yaml': analysisOptions(pageWidth: 100), }); var options = @@ -27,8 +29,8 @@ void main() { test('stops at the nearest analysis options file', () async { var testFS = TestFileSystem({ - 'dir|analysis_options.yaml': _makeOptions(pageWidth: 120), - 'dir|sub|analysis_options.yaml': _makeOptions(pageWidth: 100) + 'dir|analysis_options.yaml': analysisOptions(pageWidth: 120), + 'dir|sub|analysis_options.yaml': analysisOptions(pageWidth: 100) }); var options = @@ -39,8 +41,8 @@ void main() { test('uses the nearest file even if it doesn\'t have the setting', () async { var testFS = TestFileSystem({ - 'dir|analysis_options.yaml': _makeOptions(pageWidth: 120), - 'dir|sub|analysis_options.yaml': _makeOptions() + 'dir|analysis_options.yaml': analysisOptions(pageWidth: 120), + 'dir|sub|analysis_options.yaml': analysisOptions() }); var options = @@ -49,9 +51,10 @@ void main() { }); }); - group('readAnalysisOptionsoptions()', () { + group('readAnalysisOptionsOptions()', () { test('reads an analysis options file', () async { - var testFS = TestFileSystem({'file.yaml': _makeOptions(pageWidth: 120)}); + var testFS = + TestFileSystem({'file.yaml': analysisOptions(pageWidth: 120)}); var options = await readAnalysisOptions(testFS, TestFileSystemPath('file.yaml')); @@ -60,19 +63,19 @@ void main() { test('merges included files', () async { var testFS = TestFileSystem({ - 'dir|a.yaml': _makeOptions(include: 'b.yaml', other: { + 'dir|a.yaml': analysisOptions(include: 'b.yaml', other: { 'a': 'from a', 'ab': 'from a', 'ac': 'from a', 'abc': 'from a', }), - 'dir|b.yaml': _makeOptions(include: 'c.yaml', other: { + 'dir|b.yaml': analysisOptions(include: 'c.yaml', other: { 'ab': 'from b', 'abc': 'from b', 'b': 'from b', 'bc': 'from b', }), - 'dir|c.yaml': _makeOptions(other: { + 'dir|c.yaml': analysisOptions(other: { 'ac': 'from c', 'abc': 'from c', 'bc': 'from c', @@ -93,8 +96,8 @@ void main() { test('removes the include key after merging', () async { var testFS = TestFileSystem({ - 'dir|main.yaml': _makeOptions(pageWidth: 120, include: 'a.yaml'), - 'dir|a.yaml': _makeOptions(other: {'a': 123}), + 'dir|main.yaml': analysisOptions(pageWidth: 120, include: 'a.yaml'), + 'dir|a.yaml': analysisOptions(other: {'a': 123}), }); var options = await readAnalysisOptions( @@ -104,13 +107,13 @@ void main() { test('locates includes relative to the parent directory', () async { var testFS = TestFileSystem({ - 'dir|a.yaml': _makeOptions(include: 'sub|b.yaml', other: { + 'dir|a.yaml': analysisOptions(include: 'sub|b.yaml', other: { 'a': 'from a', }), - 'dir|sub|b.yaml': _makeOptions(include: 'more|c.yaml', other: { + 'dir|sub|b.yaml': analysisOptions(include: 'more|c.yaml', other: { 'b': 'from b', }), - 'dir|sub|more|c.yaml': _makeOptions(other: { + 'dir|sub|more|c.yaml': analysisOptions(other: { 'c': 'from c', }), }); @@ -124,29 +127,6 @@ void main() { }); } -String _makeOptions( - {int? pageWidth, String? include, Map? other}) { - var result = StringBuffer(); - - if (include != null) { - result.writeln('include: $include'); - } - - if (pageWidth != null) { - result.writeln('formatter:'); - result.writeln(' page_width: $pageWidth'); - } - - if (other != null) { - other.forEach((key, value) { - result.writeln('$key:'); - result.writeln(' $value'); - }); - } - - return result.toString(); -} - /// Reads the `formatter/page_width` key from [options] if present and returns /// it or `null` if not found. Object? _pageWidth(AnalysisOptions options) => diff --git a/test/cli/language_version_test.dart b/test/cli/language_version_test.dart index a9e08d31..05d72f80 100644 --- a/test/cli/language_version_test.dart +++ b/test/cli/language_version_test.dart @@ -114,7 +114,7 @@ main() { // case uses a syntax which is valid in earlier versions of Dart but an // error in 3.0 and later. Verify that the error is reported. await d.dir('foo', [ - packageConfig('foo', 3, 1), + packageConfig('foo', version: '3.1'), d.file('main.dart', 'main() { switch (o) { case 1 + 2: break; } }'), ]).create(); @@ -136,7 +136,7 @@ main() { // error in 3.0 and later. Verify that no error is reported since this // file opts to the older version. await d.dir('foo', [ - packageConfig('foo', 3, 1), + packageConfig('foo', version: '3.1'), d.file('main.dart', ''' // @dart=2.19 main() { switch (obj) { case 1 + 2: // Error in 3.1. @@ -187,7 +187,7 @@ main() { // case uses a syntax which is valid in earlier versions of Dart but an // error in 3.0 and later. Verify that the error is reported. await d.dir('foo', [ - packageConfig('foo', 2, 19), + packageConfig('foo', version: '2.19'), ]).create(); var process = await runFormatter( diff --git a/test/cli/page_width_test.dart b/test/cli/page_width_test.dart new file mode 100644 index 00000000..c4d36bf9 --- /dev/null +++ b/test/cli/page_width_test.dart @@ -0,0 +1,180 @@ +// Copyright (c) 2024, 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. + +import 'dart:convert'; + +import 'package:test/test.dart'; +import 'package:test_descriptor/test_descriptor.dart' as d; + +import '../utils.dart'; + +void main() { + compileFormatter(); + + test('no options search if experiment is off', () async { + await d.dir('foo', [ + analysisOptionsFile(pageWidth: 20), + d.file('main.dart', _unformatted), + ]).create(); + + var process = await runFormatterOnDir(); + await process.shouldExit(0); + + // Should format the file at the default width. + await d.dir('foo', [d.file('main.dart', _formatted80)]).validate(); + }); + + test('no options search if page width is specified on the CLI', () async { + await d.dir('foo', [ + analysisOptionsFile(pageWidth: 20), + d.file('main.dart', _unformatted), + ]).create(); + + var process = await runFormatterOnDir([ + '--language-version=latest', // Error to not have language version. + '--line-length=30', + '--enable-experiment=tall-style' + ]); + await process.shouldExit(0); + + // Should format the file at 30, not 20 or 80. + await d.dir('foo', [d.file('main.dart', _formatted30)]).validate(); + }); + + test('default to page width of surrounding options', () async { + await _testWithOptions( + { + 'formatter': {'page_width': 20} + }, + expectedWidth: 20, + ); + }); + + test('use default page width on invalid analysis options', () async { + await _testWithOptions({'unrelated': 'stuff'}, expectedWidth: 80); + await _testWithOptions({'formatter': 'not a map'}, expectedWidth: 80); + await _testWithOptions({ + 'formatter': {'no': 'page_width'} + }, expectedWidth: 80); + await _testWithOptions( + { + 'formatter': {'page_width': 'not an int'} + }, + expectedWidth: 80, + ); + }); + + test('get page width from included options file', () async { + await d.dir('foo', [ + analysisOptionsFile(include: 'other.yaml'), + analysisOptionsFile(name: 'other.yaml', include: 'sub/third.yaml'), + d.dir('sub', [ + analysisOptionsFile(name: 'third.yaml', pageWidth: 30), + ]), + d.file('main.dart', _unformatted), + ]).create(); + + var process = await runFormatterOnDir([ + '--language-version=latest', // Error to not have language version. + '--enable-experiment=tall-style' + ]); + await process.shouldExit(0); + + // Should format the file at 30. + await d.dir('foo', [d.file('main.dart', _formatted30)]).validate(); + }); + + group('stdin', () { + test('use page width from surrounding package', () async { + await d.dir('foo', [ + analysisOptionsFile(pageWidth: 30), + ]).create(); + + var process = await runFormatter([ + '--language-version=latest', // Error to not have language version. + '--enable-experiment=tall-style', + '--stdin-name=foo/main.dart', + ]); + + process.stdin.writeln(_unformatted); + await process.stdin.close(); + + // Formats at page width 30. + expect(await process.stdout.next, 'var x ='); + expect(await process.stdout.next, ' operand +'); + expect(await process.stdout.next, ' another * andAnother;'); + await process.shouldExit(0); + }); + + test('no options search if page width is specified', () async { + await d.dir('foo', [ + analysisOptionsFile(pageWidth: 20), + d.file('main.dart', _unformatted), + ]).create(); + + var process = await runFormatter([ + '--language-version=latest', + '--enable-experiment=tall-style', + '--line-length=30', + '--stdin-name=foo/main.dart' + ]); + + process.stdin.writeln(_unformatted); + await process.stdin.close(); + + // Formats at page width 30, not 20. + expect(await process.stdout.next, 'var x ='); + expect(await process.stdout.next, ' operand +'); + expect(await process.stdout.next, ' another * andAnother;'); + await process.shouldExit(0); + }); + }); +} + +const _unformatted = ''' +var x=operand+another*andAnother; +'''; + +const _formatted20 = ''' +var x = + operand + + another * + andAnother; +'''; + +const _formatted30 = ''' +var x = + operand + + another * andAnother; +'''; + +const _formatted80 = ''' +var x = operand + another * andAnother; +'''; + +/// Test that formatting a file with surrounding analysis_options.yaml +/// containing [options] formats the input with a page width of [expectedWidth]. +Future _testWithOptions(Object? options, + {required int expectedWidth}) async { + var expected = switch (expectedWidth) { + 20 => _formatted20, + 30 => _formatted30, + 80 => _formatted80, + _ => throw ArgumentError('Expected width must be 20, 30, or 80.'), + }; + + await d.dir('foo', [ + d.FileDescriptor('analysis_options.yaml', jsonEncode(options)), + d.file('main.dart', _unformatted), + ]).create(); + + var process = await runFormatterOnDir([ + '--language-version=latest', // Error to not have language version. + '--enable-experiment=tall-style' + ]); + await process.shouldExit(0); + + // Should format the file at the expected width. + await d.dir('foo', [d.file('main.dart', expected)]).validate(); +} diff --git a/test/config_cache_test.dart b/test/config_cache_test.dart new file mode 100644 index 00000000..27d08486 --- /dev/null +++ b/test/config_cache_test.dart @@ -0,0 +1,249 @@ +// Copyright (c) 2024, 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. +import 'dart:convert'; +import 'dart:io'; + +import 'package:dart_style/src/config_cache.dart'; +import 'package:path/path.dart' as p; +import 'package:pub_semver/pub_semver.dart'; +import 'package:test/test.dart'; +import 'package:test_descriptor/test_descriptor.dart' as d; + +import 'utils.dart'; + +void main() { + group('findLanguageVersion()', () { + test('no surrounding package config', () async { + // Note: In theory this test could fail if machine it's run on happens to + // have a `.dart_tool` directory containing a package config in one of the + // parent directories of the system temporary directory. + + await d.dir('dir', [d.file('main.dart', 'f() {}')]).create(); + + var cache = ConfigCache(); + await _expectNullVersion(cache, 'dir/main.dart'); + }); + + test('language version from package config', () async { + await d.dir('foo', [ + packageConfig('foo', version: '3.4'), + d.file('main.dart', 'f() {}'), + ]).create(); + + var cache = ConfigCache(); + await _expectVersion(cache, 'foo/main.dart', 3, 4); + }); + + test('multiple packages in directory', () async { + await d.dir('parent', [ + _makePackage('foo', '3.4'), + _makePackage('bar', '3.5'), + ]).create(); + + var cache = ConfigCache(); + await _expectVersion(cache, 'parent/foo/main.dart', 3, 4); + await _expectVersion(cache, 'parent/bar/main.dart', 3, 5); + }); + + test('multiple files in same package', () async { + await _makePackage('foo', '3.4', [ + d.file('main.dart', 'f() {}'), + d.dir('sub', [ + d.file('another.dart', 'f() {}'), + d.dir('further', [ + d.file('third.dart', 'f() {}'), + ]), + ]), + ]).create(); + + var cache = ConfigCache(); + await _expectVersion(cache, 'foo/main.dart', 3, 4); + await _expectVersion(cache, 'foo/sub/another.dart', 3, 4); + await _expectVersion(cache, 'foo/sub/further/third.dart', 3, 4); + }); + + test('some files in package, some not', () async { + await d.dir('parent', [ + _makePackage('foo', '3.4'), + d.file('outside.dart', 'f() {}'), + d.dir('sub', [ + d.file('another.dart', 'f() {}'), + ]), + ]).create(); + + var cache = ConfigCache(); + await _expectVersion(cache, 'parent/foo/main.dart', 3, 4); + await _expectNullVersion(cache, 'parent/outside.dart'); + await _expectNullVersion(cache, 'parent/sub/another.dart'); + }); + + test('non-existent file', () async { + await d.dir('dir', []).create(); + + var cache = ConfigCache(); + await _expectNullVersion(cache, 'dir/does_not_exist.dart'); + }); + + test('non-existent directory', () async { + await d.dir('dir', []).create(); + + var cache = ConfigCache(); + await _expectNullVersion(cache, 'dir/does/not/exist.dart'); + }); + + test('nested package', () async { + await _makePackage('outer', '3.4', [ + d.file('out_main.dart', 'f() {}'), + _makePackage('inner', '3.5', [ + d.file('in_main.dart', 'f() {}'), + ]) + ]).create(); + + var cache = ConfigCache(); + await _expectVersion(cache, 'outer/out_main.dart', 3, 4); + await _expectVersion(cache, 'outer/inner/in_main.dart', 3, 5); + }); + }); + + group('findPageWidth()', () { + test('null page width if no surrounding options', () async { + await d.dir('dir', [ + d.file('main.dart', 'main() {}'), + ]).create(); + + var cache = ConfigCache(); + expect(await cache.findPageWidth(_expectedFile('dir/main.dart')), isNull); + }); + + test('use page width of surrounding options', () async { + await d.dir('dir', [ + analysisOptionsFile(pageWidth: 20), + d.file('main.dart', 'main() {}'), + ]).create(); + + await _expectWidth(width: 20); + }); + + test('use page width of indirectly surrounding options', () async { + await d.dir('dir', [ + analysisOptionsFile(pageWidth: 30), + d.dir('some', [ + d.dir('sub', [ + d.dir('directory', [ + d.file('main.dart', 'f() {}'), + ]), + ]), + ]), + ]).create(); + + await _expectWidth(file: 'dir/some/sub/directory/main.dart', width: 30); + }); + + test('null page width if no "formatter" key in options', () async { + await d.dir('dir', [ + d.FileDescriptor( + 'analysis_options.yaml', + jsonEncode({'unrelated': 'stuff'}), + ), + d.file('main.dart', 'main() {}'), + ]).create(); + + await _expectWidth(width: null); + }); + + test('null page width if "formatter" is not a map', () async { + await d.dir('dir', [ + d.FileDescriptor( + 'analysis_options.yaml', + jsonEncode({'formatter': 'not a map'}), + ), + d.file('main.dart', 'main() {}'), + ]).create(); + + await _expectWidth(width: null); + }); + + test('null page width if no "page_width" key in formatter', () async { + await d.dir('dir', [ + d.FileDescriptor( + 'analysis_options.yaml', + jsonEncode({ + 'formatter': {'no': 'page_width'} + }), + ), + d.file('main.dart', 'main() {}'), + ]).create(); + + await _expectWidth(width: null); + }); + + test('null page width if no "page_width" not an int', () async { + await d.dir('dir', [ + d.FileDescriptor( + 'analysis_options.yaml', + jsonEncode({ + 'formatter': {'page_width': 'not an int'} + }), + ), + d.file('main.dart', 'main() {}'), + ]).create(); + + await _expectWidth(width: null); + }); + + test('take page width from included options file', () async { + await d.dir('dir', [ + analysisOptionsFile(include: 'other.yaml'), + analysisOptionsFile(name: 'other.yaml', include: 'sub/third.yaml'), + d.dir('sub', [ + analysisOptionsFile(name: 'third.yaml', pageWidth: 30), + ]), + d.file('main.dart', 'main() {}'), + ]).create(); + + await _expectWidth(width: 30); + }); + }); +} + +Future _expectVersion( + ConfigCache cache, String path, int major, int minor) async { + expect(await cache.findLanguageVersion(_expectedFile(path), path), + Version(major, minor, 0)); +} + +Future _expectNullVersion(ConfigCache cache, String path) async { + expect(await cache.findLanguageVersion(_expectedFile(path), path), null); +} + +/// Test that a [file] with some some surrounding analysis_options.yaml is +/// interpreted as having the given page [width]. +Future _expectWidth( + {String file = 'dir/main.dart', required int? width}) async { + var cache = ConfigCache(); + expect(await cache.findPageWidth(_expectedFile(file)), width); +} + +/// Normalize path separators to the host OS separator since that's what the +/// cache uses. +File _expectedFile(String path) => File( + p.joinAll([d.sandbox, ...p.posix.split(path)]), + ); + +/// Create a test package with [packageName] containing a package config with +/// language version [major].[minor]. +/// +/// If [files] is given, then the package contains those files, otherwise it +/// contains a default `main.dart` file. +d.DirectoryDescriptor _makePackage( + String packageName, + String version, [ + List? files, +]) { + files ??= [d.file('main.dart', 'f() {}')]; + return d.dir(packageName, [ + packageConfig(packageName, version: version), + ...files, + ]); +} diff --git a/test/language_version_cache_test.dart b/test/language_version_cache_test.dart deleted file mode 100644 index 71f847d5..00000000 --- a/test/language_version_cache_test.dart +++ /dev/null @@ -1,140 +0,0 @@ -// Copyright (c) 2024, 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. -import 'dart:io'; - -import 'package:dart_style/src/language_version_cache.dart'; -import 'package:path/path.dart' as p; -import 'package:pub_semver/pub_semver.dart'; -import 'package:test/test.dart'; -import 'package:test_descriptor/test_descriptor.dart' as d; - -import 'utils.dart'; - -const _source = 'f() {}'; - -void main() { - test('no surrounding package config', () async { - // Note: In theory this test could fail if machine it's run on happens to - // have a `.dart_tool` directory containing a package config in one of the - // parent directories of the system temporary directory. - - await d.dir('dir', [d.file('main.dart', _source)]).create(); - - var cache = LanguageVersionCache(); - await _expectNullVersion(cache, 'dir/main.dart'); - }); - - test('language version from package config', () async { - await d.dir('foo', [ - packageConfig('foo', 3, 4), - d.file('main.dart', _source), - ]).create(); - - var cache = LanguageVersionCache(); - await _expectVersion(cache, 'foo/main.dart', 3, 4); - }); - - test('multiple packages in directory', () async { - await d.dir('parent', [ - _makePackage('foo', 3, 4), - _makePackage('bar', 3, 5), - ]).create(); - - var cache = LanguageVersionCache(); - await _expectVersion(cache, 'parent/foo/main.dart', 3, 4); - await _expectVersion(cache, 'parent/bar/main.dart', 3, 5); - }); - - test('multiple files in same package', () async { - await _makePackage('foo', 3, 4, [ - d.file('main.dart', _source), - d.dir('sub', [ - d.file('another.dart', _source), - d.dir('further', [ - d.file('third.dart', _source), - ]), - ]), - ]).create(); - - var cache = LanguageVersionCache(); - await _expectVersion(cache, 'foo/main.dart', 3, 4); - await _expectVersion(cache, 'foo/sub/another.dart', 3, 4); - await _expectVersion(cache, 'foo/sub/further/third.dart', 3, 4); - }); - - test('some files in package, some not', () async { - await d.dir('parent', [ - _makePackage('foo', 3, 4), - d.file('outside.dart', _source), - d.dir('sub', [ - d.file('another.dart', _source), - ]), - ]).create(); - - var cache = LanguageVersionCache(); - await _expectVersion(cache, 'parent/foo/main.dart', 3, 4); - await _expectNullVersion(cache, 'parent/outside.dart'); - await _expectNullVersion(cache, 'parent/sub/another.dart'); - }); - - test('non-existent file', () async { - await d.dir('dir', []).create(); - - var cache = LanguageVersionCache(); - await _expectNullVersion(cache, 'dir/does_not_exist.dart'); - }); - - test('non-existent directory', () async { - await d.dir('dir', []).create(); - - var cache = LanguageVersionCache(); - await _expectNullVersion(cache, 'dir/does/not/exist.dart'); - }); - - test('nested package', () async { - await _makePackage('outer', 3, 4, [ - d.file('out_main.dart', _source), - _makePackage('inner', 3, 5, [ - d.file('in_main.dart', _source), - ]) - ]).create(); - - var cache = LanguageVersionCache(); - await _expectVersion(cache, 'outer/out_main.dart', 3, 4); - await _expectVersion(cache, 'outer/inner/in_main.dart', 3, 5); - }); -} - -Future _expectVersion( - LanguageVersionCache cache, String path, int major, int minor) async { - expect(await cache.find(_expectedFile(path), path), Version(major, minor, 0)); -} - -Future _expectNullVersion(LanguageVersionCache cache, String path) async { - expect(await cache.find(_expectedFile(path), path), null); -} - -/// Normalize path separators to the host OS separator since that's what the -/// cache uses. -File _expectedFile(String path) => File( - p.joinAll([d.sandbox, ...p.posix.split(path)]), - ); - -/// Create a test package with [packageName] containing a package config with -/// language version [major].[minor]. -/// -/// If [files] is given, then the package contains those files, otherwise it -/// contains a default `main.dart` file. -d.DirectoryDescriptor _makePackage( - String packageName, - int major, - int minor, [ - List? files, -]) { - files ??= [d.file('main.dart', _source)]; - return d.dir(packageName, [ - packageConfig(packageName, major, minor), - ...files, - ]); -} diff --git a/test/utils.dart b/test/utils.dart index b36cde16..e35c1296 100644 --- a/test/utils.dart +++ b/test/utils.dart @@ -2,6 +2,7 @@ // 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. +import 'dart:convert'; import 'dart:io'; import 'package:dart_style/dart_style.dart'; @@ -168,20 +169,66 @@ void _testFile(TestFile testFile) { } /// Create a test `.dart_tool` directory with a package config for a package -/// with [packageName] and language version [major].[minor]. -d.DirectoryDescriptor packageConfig(String packageName, int major, int minor) { - var config = ''' - { - "configVersion": 2, - "packages": [ - { - "name": "$packageName", - "rootUri": "../", - "packageUri": "lib/", - "languageVersion": "$major.$minor" - } +/// with [rootPackageName] and language version [major].[minor]. +/// +/// If [packages] is given, it should be a map from package names to root URIs +/// for each package. +d.DirectoryDescriptor packageConfig(String rootPackageName, + {String version = '3.5', Map? packages}) { + Map package(String name, String rootUri) => { + 'name': name, + 'rootUri': rootUri, + 'packageUri': 'lib/', + 'languageVersion': version + }; + + var config = { + 'configVersion': 2, + 'packages': [ + package(rootPackageName, '../'), + if (packages != null) + for (var name in packages.keys) package(name, packages[name]!), ] - }'''; + }; + + return d.dir('.dart_tool', [ + d.file('package_config.json', jsonEncode(config)), + ]); +} + +/// Creates the YAML string contents of an analysis options file. +/// +/// If [pageWidth] is given, then the result has a "formatter" section to +/// specify the page width. If [include] is given, then adds an "include" key +/// to include another analysis options file. If [other] is given, then those +/// are added as other top-level keys in the YAML. +String analysisOptions( + {int? pageWidth, String? include, Map? other}) { + var yaml = StringBuffer(); + + if (include != null) { + yaml.writeln('include: $include'); + } + + if (pageWidth != null) { + yaml.writeln('formatter:'); + yaml.writeln(' page_width: $pageWidth'); + } + + if (other != null) { + other.forEach((key, value) { + yaml.writeln('$key:'); + yaml.writeln(' $value'); + }); + } + + return yaml.toString(); +} - return d.dir('.dart_tool', [d.file('package_config.json', config)]); +/// Creates a file named "analysis_options.yaml" containing the given YAML +/// options to configure the [pageWidth] and [include] file, if any. +d.FileDescriptor analysisOptionsFile( + {String name = 'analysis_options.yaml', int? pageWidth, String? include}) { + var yaml = analysisOptions(pageWidth: pageWidth, include: include); + return d.FileDescriptor(name, yaml.toString()); } From 09ec8630aa48b110ed3a1987ec8d9ea1e80c9fc4 Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Thu, 3 Oct 2024 17:04:04 -0700 Subject: [PATCH 5/6] Handle "package:" URIs in analysis options includes. --- .../analysis_options_file.dart | 59 ++++++++- lib/src/analysis_options/file_system.dart | 5 +- lib/src/analysis_options/io_file_system.dart | 8 +- lib/src/config_cache.dart | 33 ++++- lib/src/testing/test_file_system.dart | 4 + .../analysis_options_file_test.dart | 50 +++++++ test/cli/page_width_test.dart | 57 ++++++++ test/config_cache_test.dart | 123 ++++++++++++++++++ 8 files changed, 320 insertions(+), 19 deletions(-) diff --git a/lib/src/analysis_options/analysis_options_file.dart b/lib/src/analysis_options/analysis_options_file.dart index 2b61c6a3..31648df8 100644 --- a/lib/src/analysis_options/analysis_options_file.dart +++ b/lib/src/analysis_options/analysis_options_file.dart @@ -12,6 +12,11 @@ import 'merge_options.dart'; /// (It's JSON-*like* and not JSON because maps in it may have non-string keys.) typedef AnalysisOptions = Map; +/// Interface for taking a "package:" URI that may appear in an analysis +/// options file's "include" key and resolving it to a file path which can be +/// passed to [FileSystem.join()]. +typedef ResolvePackageUri = Future Function(Uri packageUri); + /// Reads an `analysis_options.yaml` file in [directory] or in the nearest /// surrounding folder that contains that file using [fileSystem]. /// @@ -23,12 +28,18 @@ typedef AnalysisOptions = Map; /// [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. +/// +/// If there any "package:" includes, then they are resolved to file paths +/// using [resolvePackageUri]. If [resolvePackageUri] is omitted, an exception +/// is thrown if any "package:" includes are found. Future findAnalysisOptions( - FileSystem fileSystem, FileSystemPath directory) async { + FileSystem fileSystem, FileSystemPath directory, + {ResolvePackageUri? resolvePackageUri}) async { while (true) { var optionsPath = await fileSystem.join(directory, 'analysis_options.yaml'); if (await fileSystem.fileExists(optionsPath)) { - return readAnalysisOptions(fileSystem, optionsPath); + return readAnalysisOptions(fileSystem, optionsPath, + resolvePackageUri: resolvePackageUri); } var parent = await fileSystem.parentDirectory(directory); @@ -40,8 +51,18 @@ Future findAnalysisOptions( return const {}; } +/// Uses [fileSystem] to read the analysis options file at [optionsPath]. +/// +/// If there any "package:" includes, then they are resolved to file paths +/// using [resolvePackageUri]. If [resolvePackageUri] is omitted, an exception +/// is thrown if any "package:" includes are found. Future readAnalysisOptions( - FileSystem fileSystem, FileSystemPath optionsPath) async { + FileSystem fileSystem, FileSystemPath optionsPath, + {ResolvePackageUri? resolvePackageUri}) async => + _readAnalysisOptions(fileSystem, resolvePackageUri, optionsPath); + +Future _readAnalysisOptions(FileSystem fileSystem, + ResolvePackageUri? resolvePackageUri, FileSystemPath optionsPath) async { var yaml = loadYamlNode(await fileSystem.readFile(optionsPath)); // Lower the YAML to a regular map. @@ -53,13 +74,43 @@ Future readAnalysisOptions( if (options['include'] case String include) { options.remove('include'); + // If the include path is "package:", resolve it to a file path first. + var includeUri = Uri.tryParse(include); + if (includeUri != null && includeUri.scheme == 'package') { + if (resolvePackageUri != null) { + var filePath = await resolvePackageUri(includeUri); + if (filePath != null) { + include = filePath; + } else { + throw PackageResolutionException( + 'Failed to resolve package URI "$include" in include.'); + } + } else { + throw PackageResolutionException( + 'Couldn\'t resolve package URI "$include" in include because ' + 'no package resolver was provided.'); + } + } + // The include path may be relative to the directory containing the current // options file. var includePath = await fileSystem.join( (await fileSystem.parentDirectory(optionsPath))!, include); - var includeFile = await readAnalysisOptions(fileSystem, includePath); + var includeFile = + await _readAnalysisOptions(fileSystem, resolvePackageUri, includePath); options = merge(includeFile, options) as AnalysisOptions; } return options; } + +/// Exception thrown when an analysis options file contains a "package:" URI in +/// an include and resolving the URI to a file path failed. +class PackageResolutionException implements Exception { + final String _message; + + PackageResolutionException(this._message); + + @override + String toString() => _message; +} diff --git a/lib/src/analysis_options/file_system.dart b/lib/src/analysis_options/file_system.dart index 4c346bc9..80af5770 100644 --- a/lib/src/analysis_options/file_system.dart +++ b/lib/src/analysis_options/file_system.dart @@ -12,9 +12,8 @@ abstract interface class FileSystem { /// Joins [from] and [to] into a single path with appropriate path separators. /// - /// Note that [to] may be an absolute path or a "package:" URI and an - /// implementation of [join()] should be prepared to handle that by ignoring - /// [from] and generating a [FileSystemPath] for [to]. + /// Note that [to] may be an absolute path implementation of [join()] should + /// be prepared to handle that by ignoring [from]. Future join(FileSystemPath from, String to); /// Returns a path for the directory containing [path]. diff --git a/lib/src/analysis_options/io_file_system.dart b/lib/src/analysis_options/io_file_system.dart index 38272d9d..996675ab 100644 --- a/lib/src/analysis_options/io_file_system.dart +++ b/lib/src/analysis_options/io_file_system.dart @@ -10,12 +10,8 @@ import 'file_system.dart'; /// An implementation of [FileSystem] using `dart:io`. class IOFileSystem implements FileSystem { - Future makePath(String path) async { - // TODO(rnystrom): If [path] is a "package:" URI, then look for a - // surrounding package_config.json and resolve the URI to a file path using - // that. - return IOFileSystemPath._(path); - } + Future makePath(String path) async => + IOFileSystemPath._(path); @override Future fileExists(FileSystemPath path) => File(path.ioPath).exists(); diff --git a/lib/src/config_cache.dart b/lib/src/config_cache.dart index af3ac489..edd12d98 100644 --- a/lib/src/config_cache.dart +++ b/lib/src/config_cache.dart @@ -91,14 +91,21 @@ class ConfigCache { return _directoryPageWidths[directory]; } - // Look for a surrounding analysis_options.yaml file. - var options = await findAnalysisOptions( - _fileSystem, await _fileSystem.makePath(file.path)); + try { + // Look for a surrounding analysis_options.yaml file. + var options = await findAnalysisOptions( + _fileSystem, await _fileSystem.makePath(file.path), + resolvePackageUri: (uri) => _resolvePackageUri(file, uri)); - if (options['formatter'] case Map formatter) { - if (formatter['page_width'] case int pageWidth) { - return _directoryPageWidths[directory] = pageWidth; + if (options['formatter'] case Map formatter) { + if (formatter['page_width'] case int pageWidth) { + return _directoryPageWidths[directory] = pageWidth; + } } + } 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. } // If we get here, the options file either doesn't specify the page width, @@ -137,4 +144,18 @@ class ConfigCache { Profile.end('look up package config'); } } + + /// Resolves a "package:" [packageUri] using the nearest package config file + /// surrounding [file]. + /// + /// If there is no package config file around [file], or the package config + /// doesn't contain the package for [packageUri], returns `null`. Otherwise, + /// returns an absolute file path for where [packageUri] can be found on disk. + Future _resolvePackageUri(File file, Uri packageUri) async { + var config = + await _findPackageConfig(file, file.path, forLanguageVersion: false); + if (config == null) return null; + + return config.resolve(packageUri)?.toFilePath(); + } } diff --git a/lib/src/testing/test_file_system.dart b/lib/src/testing/test_file_system.dart index 73cf32e4..c60b7f20 100644 --- a/lib/src/testing/test_file_system.dart +++ b/lib/src/testing/test_file_system.dart @@ -8,6 +8,8 @@ import '../analysis_options/file_system.dart'; /// /// Uses `|` as directory separator to make sure that the implementating code /// calling into this doesn't assume a directory separator character. +/// +/// A path starting with `|` is considered absolute for purposes of joining. class TestFileSystem implements FileSystem { final Map _files = {}; @@ -21,6 +23,8 @@ class TestFileSystem implements FileSystem { @override Future join(FileSystemPath from, String to) async { + // If it's an absolute path, discard [from]. + if (to.startsWith('|')) return TestFileSystemPath(to); return TestFileSystemPath('${from.testPath}|$to'); } diff --git a/test/analysis_options/analysis_options_file_test.dart b/test/analysis_options/analysis_options_file_test.dart index 9795919b..65d95012 100644 --- a/test/analysis_options/analysis_options_file_test.dart +++ b/test/analysis_options/analysis_options_file_test.dart @@ -124,6 +124,56 @@ void main() { expect(options['b'], 'from b'); expect(options['c'], 'from c'); }); + + test('resolves "package:" includes', () async { + var testFS = TestFileSystem({ + 'dir|a.yaml': + analysisOptions(include: 'package:b/b_options.yaml', other: { + 'a': 'from a', + }), + '|b|b_options.yaml': + analysisOptions(include: 'package:c/c_options.yaml', other: { + 'b': 'from b', + }), + '|c|c_options.yaml': analysisOptions(other: { + 'c': 'from c', + }), + }); + + Future resolve(Uri packageUri) async { + return '|${packageUri.pathSegments.join('|')}'; + } + + var options = await readAnalysisOptions( + testFS, TestFileSystemPath('dir|a.yaml'), + resolvePackageUri: resolve); + expect(options['a'], 'from a'); + expect(options['b'], 'from b'); + expect(options['c'], 'from c'); + }); + + test('throws on a "package:" include with no package resolver', () async { + var testFS = TestFileSystem({ + 'options.yaml': analysisOptions(include: 'package:foo/options.yaml'), + }); + + expect(readAnalysisOptions(testFS, TestFileSystemPath('options.yaml')), + throwsA(isA())); + }); + + test('throws on a "package:" resolution failure', () async { + var testFS = TestFileSystem({ + 'options.yaml': analysisOptions(include: 'package:foo/options.yaml'), + }); + + // A resolver that always fails. + Future failingResolver(Uri uri) async => null; + + expect( + readAnalysisOptions(testFS, TestFileSystemPath('options.yaml'), + resolvePackageUri: failingResolver), + throwsA(isA())); + }); }); } diff --git a/test/cli/page_width_test.dart b/test/cli/page_width_test.dart index c4d36bf9..1a8b2f66 100644 --- a/test/cli/page_width_test.dart +++ b/test/cli/page_width_test.dart @@ -85,6 +85,63 @@ void main() { await d.dir('foo', [d.file('main.dart', _formatted30)]).validate(); }); + test('resolve "package:" includes', () async { + await d.dir('dir', [ + d.dir('foo', [ + packageConfig('foo', packages: { + 'bar': '../../bar', + 'baz': '../../baz', + }), + analysisOptionsFile(include: 'package:bar/analysis_options.yaml'), + d.file('main.dart', _unformatted), + ]), + d.dir('bar', [ + d.dir('lib', [ + analysisOptionsFile(include: 'package:baz/analysis_options.yaml'), + ]), + ]), + d.dir('baz', [ + d.dir('lib', [ + analysisOptionsFile(pageWidth: 30), + ]), + ]), + ]).create(); + + var process = await runFormatterOnDir([ + '--language-version=latest', // Error to not have language version. + '--enable-experiment=tall-style' + ]); + await process.shouldExit(0); + + // Should format the file at 30. + await d.dir('dir', [ + d.dir('foo', [d.file('main.dart', _formatted30)]) + ]).validate(); + }); + + test('ignore "package:" resolution errors', () async { + await d.dir('dir', [ + d.dir('foo', [ + packageConfig('foo', packages: { + 'bar': '../../bar', + }), + analysisOptionsFile(include: 'package:not_bar/analysis_options.yaml'), + d.file('main.dart', _unformatted), + ]), + ]).create(); + + var process = await runFormatterOnDir([ + '--language-version=latest', // Error to not have language version. + '--enable-experiment=tall-style' + ]); + await process.shouldExit(0); + + // Should format the file at 80. + await d.dir('dir', [ + d.dir('foo', [d.file('main.dart', _formatted80)]) + ]).validate(); + }); + group('stdin', () { test('use page width from surrounding package', () async { await d.dir('foo', [ diff --git a/test/config_cache_test.dart b/test/config_cache_test.dart index 27d08486..9c33bda5 100644 --- a/test/config_cache_test.dart +++ b/test/config_cache_test.dart @@ -204,6 +204,129 @@ void main() { await _expectWidth(width: 30); }); + + test('resolve "package:" includes', () async { + await d.dir('dir', [ + d.dir('foo', [ + packageConfig('foo', packages: { + 'bar': '../../bar', + 'baz': '../../baz', + }), + analysisOptionsFile(include: 'package:bar/analysis_options.yaml'), + d.file('main.dart', 'main() {}'), + ]), + d.dir('bar', [ + d.dir('lib', [ + analysisOptionsFile(include: 'package:baz/analysis_options.yaml'), + ]), + ]), + d.dir('baz', [ + d.dir('lib', [ + analysisOptionsFile(pageWidth: 30), + ]), + ]), + ]).create(); + + var cache = ConfigCache(); + expect(await cache.findPageWidth(_expectedFile('dir/foo/main.dart')), 30); + }); + + test('use the root file\'s config for transitive "package:" includes', + () async { + // This tests a tricky edge case. Consider: + // + // Package my_app has analysis_options.yaml: + // + // include: "package:foo/options.yaml" + // + // my_app also has a package config that resolves bar to `bar_1.0.0/`. + // + // Package foo has analysis_options.yaml: + // + // include: "package:bar/options.yaml" + // + // foo also has a package config that resolves bar to `bar_2.0.0/`. + // + // Package bar_1.0.0 has options.yaml with a page width of 40. + // Package bar_2.0.0 has options.yaml with a page width of 60. + // + // Which page width do files in my_app get? If we resolve every "package:" + // include using the package config surrounding the analysis options file + // containing that include, you will get 60. If we resolve every + // "package:" include using the package config surrounding the original + // source file that we're formatting, you'll get 40. + // + // The answer we want is 40. A file is being formatted in the context of + // some package and we want that package's own transitive dependency solve + // to be used for analysis options, look up, not the dependency solves of + // those dependencies. + await d.dir('dir', [ + d.dir('foo', [ + packageConfig('foo', packages: { + 'bar': '../../bar', + 'baz': '../../baz', + }), + analysisOptionsFile(include: 'package:bar/analysis_options.yaml'), + d.file('main.dart', 'main() {}'), + ]), + d.dir('bar', [ + packageConfig('foo', packages: { + 'baz': '../../evil_baz', + }), + d.dir('lib', [ + analysisOptionsFile(include: 'package:baz/analysis_options.yaml'), + ]), + ]), + d.dir('baz', [ + d.dir('lib', [ + analysisOptionsFile(pageWidth: 30), + ]), + ]), + d.dir('evil_baz', [ + d.dir('lib', [ + analysisOptionsFile(pageWidth: 666), + ]), + ]), + ]).create(); + + var cache = ConfigCache(); + expect(await cache.findPageWidth(_expectedFile('dir/foo/main.dart')), 30); + }); + + test('nested package', () async { + // Both packages have a package config for resolving "bar" but each + // resolves to a different "bar" directory. Test that when resolving a + // "package:bar" include, we use the nearest surrounding package config. + await d.dir('dir', [ + d.dir('outer', [ + packageConfig('outer', packages: { + 'bar': '../../outer_bar', + }), + d.dir('inner', [ + packageConfig('inner', packages: { + 'bar': '../../../inner_bar', + }), + analysisOptionsFile(include: 'package:bar/analysis_options.yaml'), + d.file('main.dart', 'f() {}'), + ]), + analysisOptionsFile(include: 'package:bar/analysis_options.yaml'), + d.file('main.dart', 'f() {}'), + ]), + d.dir('outer_bar', [ + d.dir('lib', [analysisOptionsFile(pageWidth: 20)]) + ]), + d.dir('inner_bar', [ + d.dir('lib', [analysisOptionsFile(pageWidth: 30)]) + ]), + ]).create(); + + var cache = ConfigCache(); + expect( + await cache.findPageWidth(_expectedFile('dir/outer/main.dart')), 20); + expect( + await cache.findPageWidth(_expectedFile('dir/outer/inner/main.dart')), + 30); + }); }); } From 23637d18f50bb3835e597aa053ed5a85052c5977 Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Mon, 7 Oct 2024 15:51:20 -0700 Subject: [PATCH 6/6] Apply review feedback. --- .../analysis_options_file.dart | 17 +++++++------- lib/src/analysis_options/file_system.dart | 8 +++---- lib/src/analysis_options/io_file_system.dart | 22 +++++++++---------- .../analysis_options_file_test.dart | 13 +++++++++-- 4 files changed, 33 insertions(+), 27 deletions(-) diff --git a/lib/src/analysis_options/analysis_options_file.dart b/lib/src/analysis_options/analysis_options_file.dart index 31648df8..1b2b041a 100644 --- a/lib/src/analysis_options/analysis_options_file.dart +++ b/lib/src/analysis_options/analysis_options_file.dart @@ -57,16 +57,15 @@ Future findAnalysisOptions( /// using [resolvePackageUri]. If [resolvePackageUri] is omitted, an exception /// is thrown if any "package:" includes are found. Future readAnalysisOptions( - FileSystem fileSystem, FileSystemPath optionsPath, - {ResolvePackageUri? resolvePackageUri}) async => - _readAnalysisOptions(fileSystem, resolvePackageUri, optionsPath); - -Future _readAnalysisOptions(FileSystem fileSystem, - ResolvePackageUri? resolvePackageUri, FileSystemPath optionsPath) async { + FileSystem fileSystem, FileSystemPath optionsPath, + {ResolvePackageUri? resolvePackageUri}) async { var yaml = loadYamlNode(await fileSystem.readFile(optionsPath)); - // Lower the YAML to a regular map. + // If for some reason the YAML isn't a map, consider it malformed and yield + // a default empty map. if (yaml is! YamlMap) return const {}; + + // Lower the YAML to a regular map. var options = {...yaml}; // If there is an `include:` key, then load that and merge it with these @@ -96,8 +95,8 @@ Future _readAnalysisOptions(FileSystem fileSystem, // options file. var includePath = await fileSystem.join( (await fileSystem.parentDirectory(optionsPath))!, include); - var includeFile = - await _readAnalysisOptions(fileSystem, resolvePackageUri, includePath); + var includeFile = await readAnalysisOptions(fileSystem, includePath, + resolvePackageUri: resolvePackageUri); options = merge(includeFile, options) as AnalysisOptions; } diff --git a/lib/src/analysis_options/file_system.dart b/lib/src/analysis_options/file_system.dart index 80af5770..8cae92b2 100644 --- a/lib/src/analysis_options/file_system.dart +++ b/lib/src/analysis_options/file_system.dart @@ -8,18 +8,18 @@ /// files. abstract interface class FileSystem { /// Returns `true` if there is a file at [path]. - Future fileExists(FileSystemPath path); + Future fileExists(covariant FileSystemPath path); /// Joins [from] and [to] into a single path with appropriate path separators. /// /// Note that [to] may be an absolute path implementation of [join()] should /// be prepared to handle that by ignoring [from]. - Future join(FileSystemPath from, String to); + Future join(covariant FileSystemPath from, String to); /// Returns a path for the directory containing [path]. /// /// If [path] is a root path, then returns `null`. - Future parentDirectory(FileSystemPath path); + Future parentDirectory(covariant FileSystemPath path); /// Returns the series of directories surrounding [path], from innermost out. /// @@ -30,7 +30,7 @@ abstract interface class FileSystem { /// Reads the contents of the file as [path], which should exist and contain /// UTF-8 encoded text. - Future readFile(FileSystemPath path); + Future readFile(covariant FileSystemPath path); } /// Abstraction over a file or directory in a [FileSystem]. diff --git a/lib/src/analysis_options/io_file_system.dart b/lib/src/analysis_options/io_file_system.dart index 996675ab..020b1cdd 100644 --- a/lib/src/analysis_options/io_file_system.dart +++ b/lib/src/analysis_options/io_file_system.dart @@ -14,27 +14,29 @@ class IOFileSystem implements FileSystem { IOFileSystemPath._(path); @override - Future fileExists(FileSystemPath path) => File(path.ioPath).exists(); + Future fileExists(covariant IOFileSystemPath path) => + File(path.path).exists(); @override - Future join(FileSystemPath from, String to) => - makePath(p.join(from.ioPath, to)); + Future join(covariant IOFileSystemPath from, String to) => + makePath(p.join(from.path, to)); @override - Future parentDirectory(FileSystemPath path) async { + Future parentDirectory( + covariant IOFileSystemPath path) async { // Make [path] absolute (if not already) so that we can walk outside of the // literal path string passed. - var result = p.dirname(p.absolute(path.ioPath)); + var result = p.dirname(p.absolute(path.path)); // If the parent directory is the same as [path], we must be at the root. - if (result == path.ioPath) return null; + if (result == path.path) return null; return makePath(result); } @override - Future readFile(FileSystemPath path) => - File(path.ioPath).readAsString(); + Future readFile(covariant IOFileSystemPath path) => + File(path.path).readAsString(); } /// An abstraction over a file path string, used by [IOFileSystem]. @@ -46,7 +48,3 @@ class IOFileSystemPath implements FileSystemPath { IOFileSystemPath._(this.path); } - -extension on FileSystemPath { - String get ioPath => (this as IOFileSystemPath).path; -} diff --git a/test/analysis_options/analysis_options_file_test.dart b/test/analysis_options/analysis_options_file_test.dart index 65d95012..dde5ce05 100644 --- a/test/analysis_options/analysis_options_file_test.dart +++ b/test/analysis_options/analysis_options_file_test.dart @@ -9,7 +9,7 @@ import 'package:test/test.dart'; import '../utils.dart'; void main() { - group('findAnalysisOptionsoptions()', () { + group('findAnalysisOptions()', () { test('returns an empty map if no analysis options is found', () async { var testFS = TestFileSystem(); var options = @@ -42,7 +42,8 @@ void main() { () async { var testFS = TestFileSystem({ 'dir|analysis_options.yaml': analysisOptions(pageWidth: 120), - 'dir|sub|analysis_options.yaml': analysisOptions() + 'dir|sub|analysis_options.yaml': + analysisOptions(other: {'other': 'stuff'}) }); var options = @@ -61,6 +62,14 @@ void main() { expect(_pageWidth(options), 120); }); + test('yields an empty map if the file isn\'t a YAML map', () async { + var testFS = TestFileSystem({'file.yaml': '123'}); + var options = + await readAnalysisOptions(testFS, TestFileSystemPath('file.yaml')); + expect(options, isA()); + expect(options, isEmpty); + }); + test('merges included files', () async { var testFS = TestFileSystem({ 'dir|a.yaml': analysisOptions(include: 'b.yaml', other: {