[analyzer plugin] Dependency overrides can be relative

The "plugins" can specify "dependency_overrides", e.g. with a path to
make it work in the sdk.
Currently (before this CL) a relative path is just copied verbatim
though (into something like $HOME/.dartServer/.plugin_manager/...) and
thus doesn't work.
This CL parses it the same way as the plugin are otherwise parsed and
makes the path absolute (again like the plugins are otherwise done).

Change-Id: Ia3e450126ef5bfa3bd0a8b1b31aded954252da46
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/449281
Reviewed-by: Samuel Rawlins <[email protected]>
Commit-Queue: Jens Johansen <[email protected]>

Author: jensjoha

pkg/analysis_server/lib/src/plugin2/generator.dart

@@ -13,11 +13,11 @@ class PluginPackageGenerator {
   /// file.
   final List<PluginConfiguration> _configurations;
 
-  final String? _dependencyOverrides;
+  final Map<String, PluginSource>? _dependencyOverrides;
 
   PluginPackageGenerator({
     required List<PluginConfiguration> configurations,
-    String? dependencyOverrides,
+    Map<String, PluginSource>? dependencyOverrides,
   }) : _configurations = configurations,
        _dependencyOverrides = dependencyOverrides;
 
@@ -79,10 +79,10 @@ dependencies:
     }
 
     if (_dependencyOverrides != null) {
-      buffer.write('''
-dependency_overrides:
-$_dependencyOverrides
-''');
+      buffer.write('dependency_overrides:\n');
+      for (var configuration in _dependencyOverrides.entries) {
+        buffer.write(configuration.value.toYaml(name: configuration.key));
+      }
     }
 
     return buffer.toString();

pkg/analysis_server/test/src/plugin2/generator_test.dart

@@ -69,19 +69,18 @@ import 'package:no_ints/main.dart' as no_ints;
           source: VersionedPluginSource(constraint: '^1.0.0'),
         ),
       ],
-      dependencyOverrides: '''
-    dep_one: 2.0.0
-    dep_two:
-      path: /aaa/bbb/ccc
-''',
+      dependencyOverrides: {
+        'dep_one': VersionedPluginSource(constraint: '2.0.0'),
+        'dep_two': PathPluginSource(path: '/aaa/bbb/ccc'),
+      },
     );
     expect(
       pluginPackageGenerator.generatePubspec(),
       contains('''
 dependency_overrides:
-    dep_one: 2.0.0
-    dep_two:
-      path: /aaa/bbb/ccc
+  dep_one: 2.0.0
+  dep_two:
+    path: /aaa/bbb/ccc
 '''),
     );
   }

pkg/analyzer/lib/src/dart/analysis/analysis_options.dart

@@ -223,7 +223,7 @@ final class AnalysisOptionsBuilder {
     }
 
     var configurations = <PluginConfiguration>[];
-    String? dependencyOverrides;
+    Map<String, PluginSource>? dependencyOverrides;
 
     plugins.nodes.forEach((nameNode, pluginNode) {
       if (nameNode is! YamlScalar) {
@@ -233,63 +233,26 @@ final class AnalysisOptionsBuilder {
       var pluginName = nameNode.toString();
       if (pluginName == 'dependency_overrides') {
         // This is a magic key; not the name of a plugin.
-        var indent = pluginNode.span.start.column;
-        dependencyOverrides = ' ' * indent + pluginNode.span.text;
-      }
-
-      // 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)) {
-        configurations.add(
-          PluginConfiguration(
-            name: pluginName,
-            source: VersionedPluginSource(constraint: value),
-          ),
-        );
-        return;
-      }
+        if (pluginNode is! YamlMap) return;
+        pluginNode.nodes.forEach((nameNode, dependencyOverride) {
+          if (nameNode is! YamlScalar) {
+            return;
+          }
+          var source = _getSource(dependencyOverride, resourceProvider);
+          if (source == null) return;
+          (dependencyOverrides ??= {})[nameNode.toString()] = source;
+        });
 
-      if (pluginNode is! YamlMap) {
         return;
       }
 
-      // 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(AnalysisOptionsFile.version);
-      if (versionSource case YamlScalar(:String value)) {
-        // TODO(srawlins): Handle the 'hosted' key.
-        source = VersionedPluginSource(constraint: value);
-      } else {
-        var pathSource = pluginNode.valueAt(AnalysisOptionsFile.path);
-        if (pathSource case YamlScalar(value: String pathValue)) {
-          var file = this.file;
-          assert(
-            file != null,
-            "AnalysisOptionsImpl must be initialized with a non-null 'file' if "
-            'plugins are specified with path constraints.',
-          );
-          if (file != null &&
-              resourceProvider != null &&
-              resourceProvider.pathContext.isRelative(pathValue)) {
-            // We need to store the absolute path, before this value is used in
-            // a synthetic pub package.
-            pathValue = resourceProvider.pathContext.join(
-              file.parent.path,
-              pathValue,
-            );
-            pathValue = resourceProvider.pathContext.normalize(pathValue);
-          }
-          source = PathPluginSource(path: pathValue);
-        }
-      }
+      var source = _getSource(pluginNode, resourceProvider);
+      if (source == null) return;
 
-      if (source == null) {
-        // Either the source data is malformed, or neither 'version' nor 'git'
-        // was provided. A warning should be reported by OptionsFileValidator.
+      if (pluginNode is! YamlMap) {
+        configurations.add(
+          PluginConfiguration(name: pluginName, source: source),
+        );
         return;
       }
 
@@ -348,6 +311,53 @@ final class AnalysisOptionsBuilder {
       stringValues.map((name) => name.toUpperCase()),
     );
   }
+
+  PluginSource? _getSource(
+    YamlNode pluginNode,
+    ResourceProvider? resourceProvider,
+  ) {
+    // If it just maps to a String, then that is the version constraint.
+    if (pluginNode case YamlScalar(:String value)) {
+      return VersionedPluginSource(constraint: value);
+    }
+
+    if (pluginNode is! YamlMap) {
+      return 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'.
+
+    var versionSource = pluginNode.valueAt(AnalysisOptionsFile.version);
+    if (versionSource case YamlScalar(:String value)) {
+      // TODO(srawlins): Handle the 'hosted' key.
+      return VersionedPluginSource(constraint: value);
+    }
+    var pathSource = pluginNode.valueAt(AnalysisOptionsFile.path);
+    if (pathSource case YamlScalar(value: String pathValue)) {
+      var file = this.file;
+      assert(
+        file != null,
+        "AnalysisOptionsImpl must be initialized with a non-null 'file' if "
+        'plugins are specified with path constraints.',
+      );
+      if (file != null &&
+          resourceProvider != null &&
+          resourceProvider.pathContext.isRelative(pathValue)) {
+        // We need to store the absolute path, before this value is used in
+        // a synthetic pub package.
+        pathValue = resourceProvider.pathContext.join(
+          file.parent.path,
+          pathValue,
+        );
+        pathValue = resourceProvider.pathContext.normalize(pathValue);
+      }
+      return PathPluginSource(path: pathValue);
+    }
+    return null;
+  }
 }
 
 /// A set of analysis options used to control the behavior of an analysis
@@ -622,7 +632,24 @@ class AnalysisOptionsImpl implements AnalysisOptions {
           buffer.addInt(diagnosticConfig.severity.index);
         }
         if (pluginsOptions.dependencyOverrides case var dependencyOverrides?) {
-          buffer.addString(dependencyOverrides);
+          for (var pluginDependencyOverrideEntry
+              in dependencyOverrides.entries.sortedBy((entry) => entry.key)) {
+            buffer.addString(pluginDependencyOverrideEntry.key);
+            switch (pluginDependencyOverrideEntry.value) {
+              case GitPluginSource source:
+                buffer.addString(source._url);
+                if (source._path case var path?) {
+                  buffer.addString(path);
+                }
+                if (source._ref case var ref?) {
+                  buffer.addString(ref);
+                }
+              case PathPluginSource source:
+                buffer.addString(source._path);
+              case VersionedPluginSource source:
+                buffer.addString(source._constraint);
+            }
+          }
         }
       }
 
@@ -759,7 +786,7 @@ final class PluginsOptions {
   final List<PluginConfiguration> configurations;
 
   /// The dependency overrides, if specified.
-  final String? dependencyOverrides;
+  final Map<String, PluginSource>? dependencyOverrides;
 
   PluginsOptions({
     required this.configurations,

pkg/analyzer/test/src/options/analysis_options_test.dart

@@ -12,6 +12,7 @@ import 'package:analyzer/src/file_system/file_system.dart';
 import 'package:analyzer/src/generated/source.dart';
 import 'package:analyzer/src/lint/registry.dart';
 import 'package:analyzer_testing/utilities/extensions/resource_provider.dart';
+import 'package:collection/collection.dart';
 import 'package:linter/src/rules.dart';
 import 'package:test/test.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
@@ -319,6 +320,56 @@ code-style:
     expect(analysisOptions.codeStyleOptions.useFormatter, true);
   }
 
+  test_plugins_dependency_overrides_relative() {
+    var analysisOptions = parseOptions('''
+plugins:
+  plugin_one:
+    path: foo/bar
+  dependency_overrides:
+    some_package1:
+      path: ../some_package1
+    some_package2:
+      path: sub_folder/some_package2
+''');
+
+    var dependencyOverrides =
+        analysisOptions.pluginsOptions.dependencyOverrides;
+    expect(dependencyOverrides, isNotNull);
+    expect(dependencyOverrides, hasLength(2));
+    var package1 = dependencyOverrides!.entries.singleWhereOrNull(
+      (entry) => entry.key == 'some_package1',
+    );
+    var package2 = dependencyOverrides.entries.singleWhereOrNull(
+      (entry) => entry.key == 'some_package2',
+    );
+    expect(package1, isNotNull);
+    expect(package1!.key, 'some_package1');
+    expect(
+      package1.value,
+      isA<PathPluginSource>().having(
+        (e) => e.toYaml(name: 'some_package1'),
+        'toYaml',
+        '''
+  some_package1:
+    path: ${convertPath('/some_package1')}
+''',
+      ),
+    );
+    expect(package2, isNotNull);
+    expect(package2!.key, 'some_package2');
+    expect(
+      package2.value,
+      isA<PathPluginSource>().having(
+        (e) => e.toYaml(name: 'some_package2'),
+        'toYaml',
+        '''
+  some_package2:
+    path: ${convertPath('/project/sub_folder/some_package2')}
+''',
+      ),
+    );
+  }
+
   test_plugins_pathConstraint() {
     var analysisOptions = parseOptions('''
 plugins: