DAS plugins: track lint rule configuration by name

* PluginConfiguration.ruleConfigs is now a Map instead of a List,
  mapping analysis rule names to each RuleConfig. This makes it more
  straightforward (and theoretically more performant) for the Registry
  to determine the set of enabled analysis rules. The primary parsing
  code, `parseLinterSection`, also returns a mapping now.
* This merges seamlessly into `AnalysisOptionsImpl`'s call to
  `parseLinterSection`.

This is most of the refactoring work found in
https://dart-review.googlesource.com/c/sdk/+/392981, but without the
change to allow warnings to be disabled.

Change-Id: I4d1e6791da83ad370c18a15e9e3bb85b1daed58d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/393120
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>

pkg/analysis_server_plugin/lib/src/plugin_server.dart

@@ -256,6 +256,7 @@ class PluginServer {
 
     for (var configuration in analysisOptions.pluginConfigurations) {
       if (!configuration.isEnabled) continue;
+      // TODO(srawlins): Namespace rules by their plugin, to avoid collisions.
       var rules = Registry.ruleRegistry.enabled(configuration.ruleConfigs);
       for (var rule in rules) {
         rule.reporter = errorReporter;

pkg/analysis_server_plugin/test/src/lint_rules.dart

@@ -23,6 +23,23 @@ class NoBoolsRule extends AnalysisRule {
   }
 }
 
+class NoDoublesRule extends AnalysisRule {
+  static const LintCode code = LintCode('no_doubles', 'No doubles message');
+
+  NoDoublesRule()
+      : super(name: 'no_doubles', description: 'No doubles message');
+
+  @override
+  LintCode get lintCode => code;
+
+  @override
+  void registerNodeProcessors(
+      NodeLintRegistry registry, LinterContext context) {
+    var visitor = _NoDoublesVisitor(this);
+    registry.addDoubleLiteral(this, visitor);
+  }
+}
+
 class _NoBoolsVisitor extends SimpleAstVisitor<void> {
   final AnalysisRule rule;
 
@@ -33,3 +50,14 @@ class _NoBoolsVisitor extends SimpleAstVisitor<void> {
     rule.reportLint(node);
   }
 }
+
+class _NoDoublesVisitor extends SimpleAstVisitor<void> {
+  final AnalysisRule rule;
+
+  _NoDoublesVisitor(this.rule);
+
+  @override
+  void visitDoubleLiteral(DoubleLiteral node) {
+    rule.reportLint(node);
+  }
+}

pkg/analysis_server_plugin/test/src/plugin_server_test.dart

@@ -34,19 +34,14 @@ class PluginServerTest extends PluginServerTestBase {
   @override
   Future<void> setUp() async {
     await super.setUp();
-    newAnalysisOptionsYamlFile(packagePath, '''
-plugins:
-  no_bools:
-    rules:
-      - no_bools
-''');
 
     pluginServer = PluginServer(
-        resourceProvider: resourceProvider, plugins: [_NoBoolsPlugin()]);
+        resourceProvider: resourceProvider, plugins: [_NoLiteralsPlugin()]);
     await startPlugin();
   }
 
   Future<void> test_handleAnalysisSetContextRoots() async {
+    writeAnalysisOptionsWithPlugin();
     newFile(filePath, 'bool b = false;');
     await channel
         .sendRequest(protocol.AnalysisSetContextRootsParams([contextRoot]));
@@ -59,6 +54,7 @@ plugins:
   }
 
   Future<void> test_handleEditGetFixes() async {
+    writeAnalysisOptionsWithPlugin();
     newFile(filePath, 'bool b = false;');
     await channel
         .sendRequest(protocol.AnalysisSetContextRootsParams([contextRoot]));
@@ -73,7 +69,33 @@ plugins:
     expect(fixes[0].fixes, hasLength(1));
   }
 
+  Future<void> test_lintRulesAreDisabledByDefault() async {
+    writeAnalysisOptionsWithPlugin();
+    newFile(filePath, 'double x = 3.14;');
+    await channel
+        .sendRequest(protocol.AnalysisSetContextRootsParams([contextRoot]));
+    var paramsQueue = StreamQueue(channel.notifications
+        .map((n) => protocol.AnalysisErrorsParams.fromNotification(n))
+        .where((p) => p.file == filePath));
+    var params = await paramsQueue.next;
+    expect(params.errors, isEmpty);
+  }
+
+  Future<void> test_lintRulesCanBeEnabled() async {
+    writeAnalysisOptionsWithPlugin({'no_doubles': true});
+    newFile(filePath, 'double x = 3.14;');
+    await channel
+        .sendRequest(protocol.AnalysisSetContextRootsParams([contextRoot]));
+    var paramsQueue = StreamQueue(channel.notifications
+        .map((n) => protocol.AnalysisErrorsParams.fromNotification(n))
+        .where((p) => p.file == filePath));
+    var params = await paramsQueue.next;
+    expect(params.errors, hasLength(1));
+    _expectAnalysisError(params.errors.single, message: 'No doubles message');
+  }
+
   Future<void> test_updateContent_addOverlay() async {
+    writeAnalysisOptionsWithPlugin();
     newFile(filePath, 'int b = 7;');
     await channel
         .sendRequest(protocol.AnalysisSetContextRootsParams([contextRoot]));
@@ -93,6 +115,7 @@ plugins:
   }
 
   Future<void> test_updateContent_changeOverlay() async {
+    writeAnalysisOptionsWithPlugin();
     newFile(filePath, 'int b = 7;');
     await channel
         .sendRequest(protocol.AnalysisSetContextRootsParams([contextRoot]));
@@ -120,6 +143,7 @@ plugins:
   }
 
   Future<void> test_updateContent_removeOverlay() async {
+    writeAnalysisOptionsWithPlugin();
     newFile(filePath, 'bool b = false;');
     await channel
         .sendRequest(protocol.AnalysisSetContextRootsParams([contextRoot]));
@@ -145,6 +169,47 @@ plugins:
     _expectAnalysisError(params.errors.single, message: 'No bools message');
   }
 
+  Future<void> test_warningRulesAreEnabledByDefault() async {
+    writeAnalysisOptionsWithPlugin();
+    newFile(filePath, 'bool b = false;');
+    await channel
+        .sendRequest(protocol.AnalysisSetContextRootsParams([contextRoot]));
+    var paramsQueue = StreamQueue(channel.notifications
+        .map((n) => protocol.AnalysisErrorsParams.fromNotification(n))
+        .where((p) => p.file == filePath));
+    var params = await paramsQueue.next;
+    expect(params.errors, hasLength(1));
+    _expectAnalysisError(params.errors.single, message: 'No bools message');
+  }
+
+  Future<void> test_warningRulesCannotBeDisabled() async {
+    // TODO(srawlins): A warning should be reported in the analysis options file
+    // for this.
+    writeAnalysisOptionsWithPlugin({'no_bools': false});
+    newFile(filePath, 'bool b = false;');
+    await channel
+        .sendRequest(protocol.AnalysisSetContextRootsParams([contextRoot]));
+    var paramsQueue = StreamQueue(channel.notifications
+        .map((n) => protocol.AnalysisErrorsParams.fromNotification(n))
+        .where((p) => p.file == filePath));
+    var params = await paramsQueue.next;
+    _expectAnalysisError(params.errors.single, message: 'No bools message');
+  }
+
+  void writeAnalysisOptionsWithPlugin(
+      [Map<String, bool> ruleConfiguration = const {}]) {
+    var buffer = StringBuffer('''
+plugins:
+  no_literals:
+    rules:
+''');
+    for (var MapEntry(key: ruleName, value: isEnabled)
+        in ruleConfiguration.entries) {
+      buffer.writeln('      $ruleName: $isEnabled');
+    }
+    newAnalysisOptionsYamlFile(packagePath, buffer.toString());
+  }
+
   void _expectAnalysisError(protocol.AnalysisError error,
       {required String message}) {
     expect(
@@ -159,10 +224,11 @@ plugins:
   }
 }
 
-class _NoBoolsPlugin extends Plugin {
+class _NoLiteralsPlugin extends Plugin {
   @override
   void register(PluginRegistry registry) {
     registry.registerWarningRule(NoBoolsRule());
+    registry.registerLintRule(NoDoublesRule());
     registry.registerFixForRule(NoBoolsRule.code, _WrapInQuotes.new);
   }
 }

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

@@ -80,7 +80,7 @@ final class PluginConfiguration {
   final String name;
 
   /// The list of specified [RuleConfig]s.
-  final List<RuleConfig> ruleConfigs;
+  final Map<String, RuleConfig> ruleConfigs;
 
   /// Whether the plugin is enabled.
   final bool isEnabled;

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

@@ -209,7 +209,7 @@ final class AnalysisOptionsBuilder {
         // TODO(srawlins): There may be value in storing the version constraint,
         // `value`.
         pluginConfigurations.add(PluginConfiguration(
-            name: pluginName, ruleConfigs: const [], isEnabled: true));
+            name: pluginName, ruleConfigs: const {}, isEnabled: true));
         return;
       }
 

pkg/analyzer/lib/src/lint/config.dart

@@ -6,53 +6,48 @@ import 'package:analyzer/src/task/options.dart';
 import 'package:analyzer/src/util/yaml.dart';
 import 'package:yaml/yaml.dart';
 
-/// Parses [optionsMap] into a list of [RuleConfig]s, returning them, or `null`
-/// if [optionsMap] does not have `linter` map.
-List<RuleConfig>? parseLinterSection(YamlMap optionsMap) {
+/// Parses [optionsMap] into [RuleConfig]s mapped from their names, returning
+/// them, or `null` if [optionsMap] does not have `linter` map.
+Map<String, RuleConfig>? parseLinterSection(YamlMap optionsMap) {
   var options = optionsMap.valueAt('linter');
   // Quick check of basic contract.
   if (options is YamlMap) {
-    var ruleConfigs = <RuleConfig>[];
     var rulesNode = options.valueAt(AnalyzerOptions.rules);
-    if (rulesNode != null) {
-      ruleConfigs.addAll(parseRulesSection(rulesNode));
-    }
-
-    return ruleConfigs;
+    return {
+      if (rulesNode != null) ...parseRulesSection(rulesNode),
+    };
   }
 
   return null;
 }
 
 /// Returns the [RuleConfig]s that are parsed from [value], which can be either
-/// a YAML list or a YAML map.
-List<RuleConfig> parseRulesSection(YamlNode value) {
+/// a YAML list or a YAML map, mapped from each rule's name.
+Map<String, RuleConfig> parseRulesSection(YamlNode value) {
   // For example:
   //
   // ```yaml
   // - unnecessary_getters
   // - camel_case_types
   // ```
   if (value is YamlList) {
-    var ruleConfigs = <RuleConfig>[];
-    for (var ruleNode in value.nodes) {
-      if (ruleNode case YamlScalar(value: String ruleName)) {
-        ruleConfigs.add(RuleConfig._(name: ruleName, isEnabled: true));
-      }
-    }
-    return ruleConfigs;
+    return {
+      for (var ruleNode in value.nodes)
+        if (ruleNode case YamlScalar(value: String ruleName))
+          ruleName: RuleConfig._(name: ruleName, isEnabled: true),
+    };
   }
 
   if (value is! YamlMap) {
-    return const [];
+    return const {};
   }
 
-  var ruleConfigs = <RuleConfig>[];
+  var ruleConfigs = <String, RuleConfig>{};
   value.nodes.forEach((configKey, configValue) {
     if (configKey case YamlScalar(value: String configName)) {
       var ruleConfig = _parseRuleConfig(configKey, configValue);
       if (ruleConfig != null) {
-        ruleConfigs.add(ruleConfig);
+        ruleConfigs[ruleConfig.name] = ruleConfig;
         return;
       }
 
@@ -68,7 +63,7 @@ List<RuleConfig> parseRulesSection(YamlNode value) {
         var ruleConfig =
             _parseRuleConfig(ruleName, ruleValue, group: configName);
         if (ruleConfig != null) {
-          ruleConfigs.add(ruleConfig);
+          ruleConfigs[ruleConfig.name] = ruleConfig;
           return;
         }
       });
@@ -89,7 +84,7 @@ RuleConfig? _parseRuleConfig(dynamic configKey, YamlNode configNode,
   return null;
 }
 
-/// The configuration of a single lint rule within an analysis options file.
+/// The configuration of a single analysis rule within an analysis options file.
 class RuleConfig {
   /// Whether this rule is enabled or disabled in this configuration.
   final bool isEnabled;

pkg/analyzer/lib/src/lint/registry.dart

@@ -39,17 +39,20 @@ class Registry with IterableMixin<AnalysisRule> {
 
   /// Returns a list of the enabled rules.
   ///
-  /// This includes any warning rules, which are enabled by default, and any
-  /// lint rules explicitly enabled by the given [ruleConfigs].
+  /// This includes any warning rules, which are enabled by default and are not
+  /// disabled by the given [ruleConfigs], and any lint rules which are
+  /// explicitly enabled by the given [ruleConfigs].
   ///
   /// For example:
   ///     my_rule: true
   ///
   /// enables `my_rule`.
-  Iterable<AnalysisRule> enabled(List<RuleConfig> ruleConfigs) => [
+  Iterable<AnalysisRule> enabled(Map<String, RuleConfig> ruleConfigs) => [
+        // All warning rules (which cannot be disabled).
         ..._warningRules.values,
+        // All lint rules that have explicitly been enabled.
         ..._lintRules.values
-            .where((rule) => ruleConfigs.any((rc) => rc.enables(rule.name))),
+            .where((rule) => ruleConfigs[rule.name]?.isEnabled ?? false),
       ];
 
   /// Returns the rule with the given [name].

pkg/analyzer/test/src/lint/config_test.dart

@@ -16,7 +16,7 @@ defineTests() {
   /// Process the given option [fileContents] and produce a corresponding
   /// [LintConfig]. Return `null` if [fileContents] is not a YAML map, or
   /// does not have the `linter` child map.
-  List<RuleConfig>? processAnalysisOptionsFile(String fileContents) {
+  Map<String, RuleConfig>? processAnalysisOptionsFile(String fileContents) {
     var yaml = loadYamlNode(fileContents);
     if (yaml is YamlMap) {
       return parseLinterSection(yaml);
@@ -61,7 +61,7 @@ linter:
       unnecessary_getters: false
 ''') as YamlMap)!;
         expect(ruleConfigs, hasLength(1));
-        var ruleConfig = ruleConfigs[0];
+        var ruleConfig = ruleConfigs.values.first;
         expect(ruleConfig.group, 'style_guide');
         expect(ruleConfig.name, 'unnecessary_getters');
         expect(ruleConfig.isEnabled, isFalse);
@@ -86,7 +86,7 @@ linter:
       camel_case_types: true #enable
 ''';
           var ruleConfigs = processAnalysisOptionsFile(src)!;
-          var ruleNames = ruleConfigs.map((rc) => rc.name);
+          var ruleNames = ruleConfigs.values.map((rc) => rc.name);
           expect(ruleNames, hasLength(2));
           expect(ruleNames, contains('unnecessary_getters'));
           expect(ruleNames, contains('camel_case_types'));
@@ -107,7 +107,7 @@ linter:
           var ruleConfigs = processAnalysisOptionsFile(src)!;
           expect(ruleConfigs, hasLength(1));
           // Verify that defaults are enabled.
-          expect(ruleConfigs[0].isEnabled, isTrue);
+          expect(ruleConfigs.values.first.isEnabled, isTrue);
         });
 
         test('rule map (bools)', () {
@@ -121,7 +121,7 @@ linter:
     camel_case_types: true #enable
     unnecessary_getters: false #disable
 ''';
-          var ruleConfigs = processAnalysisOptionsFile(src)!;
+          var ruleConfigs = processAnalysisOptionsFile(src)!.values.toList();
           ruleConfigs.sort(
               (RuleConfig rc1, RuleConfig rc2) => rc1.name.compareTo(rc2.name));
           expect(ruleConfigs, hasLength(2));
@@ -144,7 +144,7 @@ linter:
 
   group('options processing', () {
     group('raw maps', () {
-      List<RuleConfig> parseMap(Map<Object, Object?> map) {
+      Map<String, RuleConfig> parseMap(Map<Object, Object?> map) {
         return parseLinterSection(wrap(map) as YamlMap)!;
       }