From 618883f734f8af5df2a0e7ae47dec970fff016f8 Mon Sep 17 00:00:00 2001 From: Bob Nystrom Date: Mon, 7 Oct 2024 18:22:13 -0700 Subject: [PATCH] Use the latest language version if we can't find a package. (#1573) Use the latest language version if we can't find a package. In the new tall-style behavior where we look for a surrounding package config to infer the language version of each file, we have to decide what to do when that look-up process fails (either because there is no package config, or it's malformed). Initially, I had it error out on that file. That's consistent with the library API where the formatter *requires* a language version before it will do anything. But it's not consistent with the language spec and our other tools. For them, if there is no valid surrounding package config, the file should be treated as the latest language version. This PR does that. It also fixes the catastrophic UX that the current code has which is that when a file can't have its language version inferred... it is silently skipped without telling the user anything. Oops. Now it just formats the file as expected. --- lib/src/io.dart | 16 ++++------ lib/src/testing/test_file_system.dart | 22 +++++++------- test/cli/language_version_test.dart | 43 +++++++++++++++++++-------- 3 files changed, 47 insertions(+), 34 deletions(-) diff --git a/lib/src/io.dart b/lib/src/io.dart index b4808265..3089c4b0 100644 --- a/lib/src/io.dart +++ b/lib/src/io.dart @@ -35,12 +35,10 @@ Future formatStdin( 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. + // If they didn't specify a version or a path, or couldn't find a package + // surrounding the path, then default to the latest version. languageVersion ??= DartFormatter.latestLanguageVersion; // Determine the page width. @@ -164,14 +162,12 @@ Future _processFile( 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 { - // If they didn't specify a version or a path, default to the latest. - languageVersion ??= DartFormatter.latestLanguageVersion; } + // If they didn't specify a version and we couldn't find a surrounding + // package, then default to the latest version. + languageVersion ??= DartFormatter.latestLanguageVersion; + // Determine the page width. var pageWidth = options.pageWidth; if (pageWidth == null && cache != null) { diff --git a/lib/src/testing/test_file_system.dart b/lib/src/testing/test_file_system.dart index b951b49f..1bc86441 100644 --- a/lib/src/testing/test_file_system.dart +++ b/lib/src/testing/test_file_system.dart @@ -18,27 +18,29 @@ final class TestFileSystem implements FileSystem { } @override - Future fileExists(FileSystemPath path) async => - _files.containsKey(path.testPath); + Future fileExists(covariant TestFileSystemPath path) async => + _files.containsKey(path._path); @override - Future join(FileSystemPath from, String to) async { + Future join( + covariant TestFileSystemPath from, String to) async { // If it's an absolute path, discard [from]. if (to.startsWith('|')) return TestFileSystemPath(to); - return TestFileSystemPath('${from.testPath}|$to'); + return TestFileSystemPath('${from._path}|$to'); } @override - Future parentDirectory(FileSystemPath path) async { - var parts = path.testPath.split('|'); + Future parentDirectory( + covariant TestFileSystemPath path) async { + var parts = path._path.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; + Future readFile(covariant TestFileSystemPath path) async { + if (_files[path._path] case var contents?) return contents; throw Exception('No file at "$path".'); } } @@ -51,7 +53,3 @@ final class TestFileSystemPath implements FileSystemPath { @override String toString() => _path; } - -extension on FileSystemPath { - String get testPath => (this as TestFileSystemPath)._path; -} diff --git a/test/cli/language_version_test.dart b/test/cli/language_version_test.dart index 05d72f80..e7ea7bb2 100644 --- a/test/cli/language_version_test.dart +++ b/test/cli/language_version_test.dart @@ -26,10 +26,12 @@ extension type Meters(int value) { } '''; - test('defaults to latest language version if omitted', () async { + test('uses latest language version if no surrounding package', () async { await d.dir('code', [d.file('a.dart', extensionTypeBefore)]).create(); - var process = await runFormatterOnDir(); + // Enable the experiment to be sure that we're opting in to the new + // package config search. + var process = await runFormatterOnDir(['--enable-experiment=tall-style']); await process.shouldExit(0); await d.dir('code', [d.file('a.dart', extensionTypeAfter)]).validate(); @@ -160,24 +162,26 @@ main() { ]).validate(); }); - test('malformed', () async { + test('use the latest version if the package config is 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() {}'), + d.file('main.dart', 'main() {var (a,b)=(1,2);}'), ]).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']); + var process = await runFormatterOnDir(['--enable-experiment=tall-style']); + await process.shouldExit(0); - expect( - await process.stderr.next, - allOf(startsWith('Could not read package configuration for'), - contains(p.join('foo', 'main.dart')))); - await process.shouldExit(65); + // Formats the file. + await d.dir('foo', [ + d.file('main.dart', ''' +main() { + var (a, b) = (1, 2); +} +''') + ]).validate(); }); }); @@ -235,5 +239,20 @@ main() { expect(await process.stdout.next, '}'); await process.shouldExit(0); }); + + test('use latest language version if no surrounding package', () async { + await d.dir('foo', []).create(); + + var process = await runFormatter( + ['--enable-experiment=tall-style', '--stdin-name=foo/main.dart']); + // Use some relatively recent syntax. + process.stdin.writeln('main() {var (a,b)=(1,2);}'); + await process.stdin.close(); + + expect(await process.stdout.next, 'main() {'); + expect(await process.stdout.next, ' var (a, b) = (1, 2);'); + expect(await process.stdout.next, '}'); + await process.shouldExit(0); + }); }); }