From e66a8a4fa3bb8b48e6b83c3fa95d3410d8dc0863 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Wed, 6 Nov 2024 19:41:00 +0000 Subject: [PATCH] analyzer plugins: parse version and path keys in YAML This enables plugin sources to be specified in one of three ways: * as a YamlScalar, like `plugin_name: ^1.2.3` * with a 'version' key, like `plugin_name:\n version: ^1.2.3`, which allows diagnostic configurations to be alongside. * with a 'path' key, like `plugin_name:\n path: foo/bar`, which allows diagnostic configurations to be alongside. We introduce a PluginSource with a `toYaml` method to convert the values back into YAML for the generates pubspec. We update PluginPackageGenerator to use PluginSource.toYaml. Change-Id: Ic5f372f0339edd6aa638dd9b568a83f41a023b08 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/393900 Commit-Queue: Samuel Rawlins Reviewed-by: Phil Quitslund Reviewed-by: Brian Wilkerson --- .../lib/src/plugin/plugin_watcher.dart | 5 +- .../lib/src/plugin2/generator.dart | 28 +++---- .../test/src/plugin2/generator_test.dart | 69 ++++++++++++----- .../test/src/plugin_server_error_test.dart | 1 + .../test/src/plugin_server_test.dart | 1 + .../lib/dart/analysis/analysis_options.dart | 74 ++++++++++++++++++- .../src/dart/analysis/analysis_options.dart | 40 ++++++++-- pkg/analyzer/lib/src/task/options.dart | 3 + 8 files changed, 171 insertions(+), 50 deletions(-) diff --git a/pkg/analysis_server/lib/src/plugin/plugin_watcher.dart b/pkg/analysis_server/lib/src/plugin/plugin_watcher.dart index 34ec5b28dd1b..236332c52706 100644 --- a/pkg/analysis_server/lib/src/plugin/plugin_watcher.dart +++ b/pkg/analysis_server/lib/src/plugin/plugin_watcher.dart @@ -41,8 +41,7 @@ class PluginWatcher implements DriverWatcher { var contextRoot = driver.analysisContext!.contextRoot; _driverInfo[driver] = _DriverInfo( contextRoot, [contextRoot.root.path, _getSdkPath(driver)]); - var enabledPlugins = driver.enabledLegacyPluginNames; - for (var hostPackageName in enabledPlugins) { + for (var hostPackageName in driver.enabledLegacyPluginNames) { // // Determine whether the package exists and defines a plugin. // @@ -66,6 +65,8 @@ class PluginWatcher implements DriverWatcher { manager.addPluginToContextRoot( driver.analysisContext!.contextRoot, pluginPath); } + + // TODO(srawlins): Generate package for plugin configurations, with PluginPackageGenerator. } /// The context manager has just removed the given analysis [driver]. diff --git a/pkg/analysis_server/lib/src/plugin2/generator.dart b/pkg/analysis_server/lib/src/plugin2/generator.dart index cab35484b55d..725da063f852 100644 --- a/pkg/analysis_server/lib/src/plugin2/generator.dart +++ b/pkg/analysis_server/lib/src/plugin2/generator.dart @@ -2,6 +2,8 @@ // 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:analyzer/dart/analysis/analysis_options.dart'; + /// This class can generate various files to make up the shared plugin package. class PluginPackageGenerator { /// The plugin configuration, a map of plugin names to each plugin's @@ -9,9 +11,9 @@ class PluginPackageGenerator { /// /// This typically stems from plugin configuration in an analysis options /// file. - final Map _pluginConfiguration; + final List _pluginConfigurations; - PluginPackageGenerator(this._pluginConfiguration); + PluginPackageGenerator(this._pluginConfigurations); /// Generates the Dart entrpoint file which is to be spawned in a Dart /// isolate by the analysis server. @@ -20,8 +22,8 @@ class PluginPackageGenerator { "'package:analysis_server_plugin/src/plugin_server.dart'", "'package:analyzer/file_system/physical_file_system.dart'", "'package:analyzer_plugin/starter.dart'", - for (var name in _pluginConfiguration.keys) - "'package:$name/main.dart' as $name", + for (var configuration in _pluginConfigurations) + "'package:${configuration.name}/main.dart' as ${configuration.name}", ]; var buffer = StringBuffer("import 'dart:isolate';"); @@ -36,8 +38,8 @@ Future main(List args, SendPort sendPort) async { plugins: [ '''); // TODO(srawlins): Format with the formatter, for readability. - for (var name in _pluginConfiguration.keys) { - buffer.writeln(' $name.plugin,'); + for (var configuration in _pluginConfigurations) { + buffer.writeln(' ${configuration.name}.plugin,'); } buffer.write(''' ], @@ -60,18 +62,8 @@ version: 0.0.1 dependencies: '''); - for (var MapEntry(key: name, value: configuration) - in _pluginConfiguration.entries) { - switch (configuration) { - case String(): - buffer.writeln(' $name: $configuration'); - case Map(): - if (configuration case {'path': String pathValue}) { - buffer.writeln(' $name:\n path: $pathValue'); - } else if (configuration case {'git': String gitValue}) { - buffer.writeln(' $name:\n git: $gitValue'); - } - } + for (var configuration in _pluginConfigurations) { + buffer.write(configuration.sourceYaml()); } return buffer.toString(); diff --git a/pkg/analysis_server/test/src/plugin2/generator_test.dart b/pkg/analysis_server/test/src/plugin2/generator_test.dart index 7dfd3d710d2f..b3b57854f7a2 100644 --- a/pkg/analysis_server/test/src/plugin2/generator_test.dart +++ b/pkg/analysis_server/test/src/plugin2/generator_test.dart @@ -3,6 +3,7 @@ // BSD-style license that can be found in the LICENSE file. import 'package:analysis_server/src/plugin2/generator.dart'; +import 'package:analyzer/src/generated/engine.dart'; import 'package:test/test.dart'; import 'package:test_reflective_loader/test_reflective_loader.dart'; @@ -15,10 +16,16 @@ void main() { @reflectiveTest class GeneratorTest { void test_entrypointImportsPluginEntrypoints() { - var pluginPackageGenerator = PluginPackageGenerator({ - 'no_bools': '^1.0.0', - 'no_ints': '^1.2.0', - }); + var pluginPackageGenerator = PluginPackageGenerator([ + PluginConfiguration( + name: 'no_bools', + source: VersionedPluginSource(constraint: '^1.0.0'), + ), + PluginConfiguration( + name: 'no_ints', + source: VersionedPluginSource(constraint: '^1.2.0'), + ), + ]); expect( pluginPackageGenerator.generateEntrypoint(), contains(''' @@ -29,10 +36,16 @@ import 'package:no_ints/main.dart' as no_ints; } void test_entrypointListsPluginInstances() { - var pluginPackageGenerator = PluginPackageGenerator({ - 'no_bools': '^1.0.0', - 'no_ints': '^1.2.0', - }); + var pluginPackageGenerator = PluginPackageGenerator([ + PluginConfiguration( + name: 'no_bools', + source: VersionedPluginSource(constraint: '^1.0.0'), + ), + PluginConfiguration( + name: 'no_ints', + source: VersionedPluginSource(constraint: '^1.2.0'), + ), + ]); expect( pluginPackageGenerator.generateEntrypoint(), contains(''' @@ -45,24 +58,34 @@ import 'package:no_ints/main.dart' as no_ints; } void test_pubspecContainsGitDependencies() { - var pluginPackageGenerator = PluginPackageGenerator({ - 'no_bools': {'git': 'https://example.com/example.git'}, - }); + var pluginPackageGenerator = PluginPackageGenerator([ + PluginConfiguration( + name: 'no_bools', + source: GitPluginSource(url: 'https://example.com/example.git'), + ), + ]); expect( pluginPackageGenerator.generatePubspec(), contains(''' dependencies: no_bools: - git: https://example.com/example.git + git: + url: https://example.com/example.git '''), ); } void test_pubspecContainsPathDependencies() { - var pluginPackageGenerator = PluginPackageGenerator({ - 'no_bools': {'path': '../no_bools_plugin'}, - 'no_ints': {'path': 'tools/no_ints_plugin'}, - }); + var pluginPackageGenerator = PluginPackageGenerator([ + PluginConfiguration( + name: 'no_bools', + source: PathPluginSource(path: '../no_bools_plugin'), + ), + PluginConfiguration( + name: 'no_ints', + source: PathPluginSource(path: 'tools/no_ints_plugin'), + ), + ]); expect( pluginPackageGenerator.generatePubspec(), contains(''' @@ -76,10 +99,16 @@ dependencies: } void test_pubspecContainsVersionedDependencies() { - var pluginPackageGenerator = PluginPackageGenerator({ - 'no_bools': '^1.0.0', - 'no_ints': '^1.2.0', - }); + var pluginPackageGenerator = PluginPackageGenerator([ + PluginConfiguration( + name: 'no_bools', + source: VersionedPluginSource(constraint: '^1.0.0'), + ), + PluginConfiguration( + name: 'no_ints', + source: VersionedPluginSource(constraint: '^1.2.0'), + ), + ]); expect( pluginPackageGenerator.generatePubspec(), contains(''' diff --git a/pkg/analysis_server_plugin/test/src/plugin_server_error_test.dart b/pkg/analysis_server_plugin/test/src/plugin_server_error_test.dart index e2340a976e93..39c8fee5c678 100644 --- a/pkg/analysis_server_plugin/test/src/plugin_server_error_test.dart +++ b/pkg/analysis_server_plugin/test/src/plugin_server_error_test.dart @@ -36,6 +36,7 @@ class PluginServerErrorTest extends PluginServerTestBase { newAnalysisOptionsYamlFile(packagePath, ''' plugins: no_bools: + path: some/path diagnostics: - no_bools '''); diff --git a/pkg/analysis_server_plugin/test/src/plugin_server_test.dart b/pkg/analysis_server_plugin/test/src/plugin_server_test.dart index 8bc4dabc66fc..9904dbbb87ff 100644 --- a/pkg/analysis_server_plugin/test/src/plugin_server_test.dart +++ b/pkg/analysis_server_plugin/test/src/plugin_server_test.dart @@ -199,6 +199,7 @@ class PluginServerTest extends PluginServerTestBase { var buffer = StringBuffer(''' plugins: no_literals: + path: some/path diagnostics: '''); for (var MapEntry(key: diagnosticName, value: isEnabled) diff --git a/pkg/analyzer/lib/dart/analysis/analysis_options.dart b/pkg/analyzer/lib/dart/analysis/analysis_options.dart index 4f2901eeb626..30031464f26e 100644 --- a/pkg/analyzer/lib/dart/analysis/analysis_options.dart +++ b/pkg/analyzer/lib/dart/analysis/analysis_options.dart @@ -73,12 +73,55 @@ abstract class AnalysisOptions { bool isLintEnabled(String name); } +final class GitPluginSource implements PluginSource { + final String _url; + + final String? _path; + + final String? _ref; + + GitPluginSource({required String url, String? path, String? ref}) + : _url = url, + _path = path, + _ref = ref; + + @override + String toYaml({required String name}) { + var buffer = StringBuffer() + ..writeln(' $name:') + ..writeln(' git:') + ..writeln(' url: $_url'); + if (_ref != null) { + buffer.writeln(' ref: $_ref'); + } + if (_path != null) { + buffer.writeln(' path: $_path'); + } + return buffer.toString(); + } +} + +final class PathPluginSource implements PluginSource { + final String _path; + + PathPluginSource({required String path}) : _path = path; + + @override + String toYaml({required String name}) => ''' + $name: + path: $_path +'''; +} + /// The configuration of a Dart Analysis Server plugin, as specified by /// analysis options. final class PluginConfiguration { /// The name of the plugin being configured. final String name; + /// The source of the plugin being configured. + final PluginSource source; + /// The list of specified [DiagnosticConfig]s. final Map diagnosticConfigs; @@ -87,7 +130,34 @@ final class PluginConfiguration { PluginConfiguration({ required this.name, - required this.diagnosticConfigs, - required this.isEnabled, + required this.source, + this.diagnosticConfigs = const {}, + this.isEnabled = true, }); + + String sourceYaml() => source.toYaml(name: name); +} + +/// A description of the source of a plugin. + +/// We support all of the source formats documented at +/// https://dart.dev/tools/pub/dependencies. +sealed class PluginSource { + /// Returns the YAML-formatted source, using [name] as a key, for writing into + /// a pubspec 'dependencies' section. + String toYaml({required String name}); +} + +/// A plugin source using a version constraint, hosted either at pub.dev or +/// another host. +// TODO(srawlins): Support a different 'hosted' URL. +final class VersionedPluginSource implements PluginSource { + /// The specified version constraint. + final String _constraint; + + VersionedPluginSource({required String constraint}) + : _constraint = constraint; + + @override + String toYaml({required String name}) => ' $name: $_constraint\n'; } diff --git a/pkg/analyzer/lib/src/dart/analysis/analysis_options.dart b/pkg/analyzer/lib/src/dart/analysis/analysis_options.dart index 262ff039ba21..f81be6da3fc9 100644 --- a/pkg/analyzer/lib/src/dart/analysis/analysis_options.dart +++ b/pkg/analyzer/lib/src/dart/analysis/analysis_options.dart @@ -205,11 +205,14 @@ final class AnalysisOptionsBuilder { return; } var pluginName = nameNode.toString(); - if (pluginNode is YamlScalar) { - // TODO(srawlins): There may be value in storing the version constraint, - // `value`. + + // If the plugin name just maps to a String, then that is the version + // constraint; use it and move on. + if (pluginNode case YamlScalar(:String value)) { pluginConfigurations.add(PluginConfiguration( - name: pluginName, diagnosticConfigs: const {}, isEnabled: true)); + name: pluginName, + source: VersionedPluginSource(constraint: value), + )); return; } @@ -217,18 +220,39 @@ final class AnalysisOptionsBuilder { return; } - var diagnostics = pluginNode.valueAt(AnalyzerOptions.diagnostics); - if (diagnostics == null) { + // Grab either the source value from 'version', 'git', or 'path'. In the + // erroneous case that multiple are specified, just take the first. A + // warning should be reported by `OptionsFileValidator`. + // TODO(srawlins): In adition to 'version' and 'path', try 'git'. + + PluginSource? source; + var versionSource = pluginNode.valueAt(AnalyzerOptions.version); + if (versionSource case YamlScalar(:String value)) { + // TODO(srawlins): Handle the 'hosted' key. + source = VersionedPluginSource(constraint: value); + } else { + var pathSource = pluginNode.valueAt(AnalyzerOptions.path); + if (pathSource case YamlScalar(:String value)) { + source = PathPluginSource(path: value); + } + } + + if (source == null) { + // Either the source data is malformed, or neither 'version' nor 'git' + // was provided. A warning should be reported by OptionsFileValidator. return; } - var diagnosticConfigurations = parseDiagnosticsSection(diagnostics); + var diagnostics = pluginNode.valueAt(AnalyzerOptions.diagnostics); + var diagnosticConfigurations = diagnostics == null + ? const {} + : parseDiagnosticsSection(diagnostics); pluginConfigurations.add(PluginConfiguration( name: pluginName, + source: source, diagnosticConfigs: diagnosticConfigurations, // TODO(srawlins): Implement `enabled: false`. - isEnabled: true, )); }); } diff --git a/pkg/analyzer/lib/src/task/options.dart b/pkg/analyzer/lib/src/task/options.dart index ea16d6ee97e0..cbbea02a3f77 100644 --- a/pkg/analyzer/lib/src/task/options.dart +++ b/pkg/analyzer/lib/src/task/options.dart @@ -285,6 +285,8 @@ class AnalyzerOptions { /// Plugin options. static const String diagnostics = 'diagnostics'; + static const String path = 'path'; + static const String version = 'version'; static const String propagateLinterExceptions = 'propagate-linter-exceptions'; @@ -849,6 +851,7 @@ class OptionsFileValidator { sdkVersionConstraint: sdkVersionConstraint, sourceIsOptionsForContextRoot: sourceIsOptionsForContextRoot, ), + // TODO(srawlins): validate the top-level 'plugins' section. ]; List validate(YamlMap options) {