analyzer: use typed data in RuleConfig to store enablement

Instead of using a Map of args, with a String key, `'enabled`', just
use a bool field.

Also document the fields on RuleConfig.

Also delete a test that verifies support for bool-like String YAML
keys, like `'true'`. We can just support bool keys.

Change-Id: I5bf3ab32c7e1c9ee683b977e44be88feb06ccb88
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/391683
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Phil Quitslund <[email protected]>

Author: srawlins

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

@@ -6,8 +6,8 @@ import 'package:analyzer/src/task/options.dart';
 import 'package:analyzer/src/util/yaml.dart';
 import 'package:yaml/yaml.dart';
 
-/// Parses [optionsMap] into a [LintConfig], returning the config, or `null` if
-/// [optionsMap] does not have `linter` map.
+/// Parses [optionsMap] into a list of [RuleConfig]s, returning them, or `null`
+/// if [optionsMap] does not have `linter` map.
 List<RuleConfig>? parseLintRuleConfigs(YamlMap optionsMap) {
   var options = optionsMap.valueAt('linter');
   // Quick check of basic contract.
@@ -24,29 +24,16 @@ List<RuleConfig>? parseLintRuleConfigs(YamlMap optionsMap) {
   return null;
 }
 
-/// Returns [scalar] as a [bool], if it can be parsed as one.
-bool? _asBool(YamlNode scalar) {
-  var value = scalar is YamlScalar ? scalar.valueOrThrow : scalar;
-  return switch (value) {
-    bool() => value,
-    'true' => true,
-    'false' => false,
-    _ => null,
-  };
-}
-
-/// Returns [scalar] as a [String], if it can be parsed as one.
-String? _asString(Object scalar) {
-  var value = scalar is YamlScalar ? scalar.value : scalar;
-  return value is String ? value : null;
-}
-
-Map<String, bool> _parseArgs(YamlNode args) {
-  var enabled = _asBool(args);
-  if (enabled != null) {
-    return {'enabled': enabled};
+RuleConfig? _parseRuleConfig(dynamic configKey, YamlNode configNode,
+    {String? group}) {
+  // For example: `{unnecessary_getters: false}`.
+  if (configKey case YamlScalar(value: String ruleName)) {
+    if (configNode case YamlScalar(value: bool isEnabled)) {
+      return RuleConfig._(name: ruleName, isEnabled: isEnabled, group: group);
+    }
   }
-  return {};
+
+  return null;
 }
 
 /// Returns the [RuleConfig]s that are parsed from [value], which can be either
@@ -59,56 +46,65 @@ List<RuleConfig> _ruleConfigs(YamlNode value) {
   // - camel_case_types
   // ```
   if (value is YamlList) {
-    return [
-      for (var rule in value.nodes)
-        RuleConfig(name: _asString(rule), args: {'enabled': true}),
-    ];
-  }
-
-  // style_guide: {unnecessary_getters: false, camel_case_types: true}
-  if (value is YamlMap) {
     var ruleConfigs = <RuleConfig>[];
-    value.nodes.cast<Object, YamlNode>().forEach((key, value) {
-      // For example: `{unnecessary_getters: false}`.
-      var valueAsBool = _asBool(value);
-      if (valueAsBool != null) {
-        ruleConfigs.add(RuleConfig(
-          name: _asString(key),
-          args: {'enabled': valueAsBool},
-        ));
-      }
-
-      // style_guide: {unnecessary_getters: false, camel_case_types: true}
-      if (value is YamlMap) {
-        value.nodes.cast<Object, YamlNode>().forEach((rule, args) {
-          // TODO(brianwilkerson): verify format.
-          // For example: `unnecessary_getters: false`.
-          ruleConfigs.add(RuleConfig(
-            name: _asString(rule),
-            args: _parseArgs(args),
-            group: _asString(key),
-          ));
-        });
+    for (var ruleNode in value.nodes) {
+      if (ruleNode case YamlScalar(value: String ruleName)) {
+        ruleConfigs.add(RuleConfig._(name: ruleName, isEnabled: true));
       }
-    });
+    }
     return ruleConfigs;
   }
 
-  return const [];
+  if (value is! YamlMap) {
+    return const [];
+  }
+
+  var ruleConfigs = <RuleConfig>[];
+  value.nodes.forEach((configKey, configValue) {
+    if (configKey case YamlScalar(value: String configName)) {
+      var ruleConfig = _parseRuleConfig(configKey, configValue);
+      if (ruleConfig != null) {
+        ruleConfigs.add(ruleConfig);
+        return;
+      }
+
+      if (configValue is! YamlMap) {
+        return;
+      }
+      // For example:
+      //
+      // ```yaml
+      // style_guide: {unnecessary_getters: false, camel_case_types: true}
+      // ```
+      configValue.nodes.forEach((ruleName, ruleValue) {
+        var ruleConfig =
+            _parseRuleConfig(ruleName, ruleValue, group: configName);
+        if (ruleConfig != null) {
+          ruleConfigs.add(ruleConfig);
+          return;
+        }
+      });
+    }
+  });
+  return ruleConfigs;
 }
 
 /// The configuration of a single lint rule within an analysis options file.
 class RuleConfig {
-  final Map<String, bool> args;
+  /// Whether this rule is enabled or disabled in this configuration.
+  final bool isEnabled;
+
+  /// The name of the group under which this configuration is found.
   final String? group;
-  final String? name;
 
-  RuleConfig({required this.name, required this.args, this.group});
+  /// The name of the rule.
+  final String name;
+
+  RuleConfig._({required this.name, required this.isEnabled, this.group});
 
   /// Returns whether [ruleName] is disabled in this configuration.
-  bool disables(String ruleName) =>
-      ruleName == name && args['enabled'] == false;
+  bool disables(String ruleName) => ruleName == name && !isEnabled;
 
   /// Returns whether [ruleName] is enabled in this configuration.
-  bool enables(String ruleName) => ruleName == name && args['enabled'] == true;
+  bool enables(String ruleName) => ruleName == name && isEnabled;
 }

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

@@ -64,7 +64,7 @@ linter:
         var ruleConfig = ruleConfigs[0];
         expect(ruleConfig.group, 'style_guide');
         expect(ruleConfig.name, 'unnecessary_getters');
-        expect(ruleConfig.args, {'enabled': false});
+        expect(ruleConfig.isEnabled, isFalse);
         expect(ruleConfig.disables('unnecessary_getters'), isTrue);
       });
     });
@@ -107,7 +107,7 @@ linter:
           var ruleConfigs = processAnalysisOptionsFile(src)!;
           expect(ruleConfigs, hasLength(1));
           // Verify that defaults are enabled.
-          expect(ruleConfigs[0].args['enabled'], isTrue);
+          expect(ruleConfigs[0].isEnabled, isTrue);
         });
 
         test('rule map (bools)', () {
@@ -122,13 +122,13 @@ linter:
     unnecessary_getters: false #disable
 ''';
           var ruleConfigs = processAnalysisOptionsFile(src)!;
-          ruleConfigs.sort((RuleConfig rc1, RuleConfig rc2) =>
-              rc1.name!.compareTo(rc2.name!));
+          ruleConfigs.sort(
+              (RuleConfig rc1, RuleConfig rc2) => rc1.name.compareTo(rc2.name));
           expect(ruleConfigs, hasLength(2));
           expect(ruleConfigs[0].name, 'camel_case_types');
-          expect(ruleConfigs[0].args['enabled'], isTrue);
+          expect(ruleConfigs[0].isEnabled, isTrue);
           expect(ruleConfigs[1].name, 'unnecessary_getters');
-          expect(ruleConfigs[1].args['enabled'], isFalse);
+          expect(ruleConfigs[1].isEnabled, isFalse);
         });
       });
     });
@@ -172,18 +172,6 @@ linter:
         expect(ruleConfigs, hasLength(1));
       });
 
-      test('rule map (string)', () {
-        var options = <Object, Object?>{};
-        var lintOptions = {
-          'rules': {'camel_case_types': 'true'}
-        };
-        options['linter'] = lintOptions;
-
-        var ruleConfigs = parseMap(options);
-        expect(ruleConfigs, isNotNull);
-        expect(ruleConfigs, hasLength(1));
-      });
-
       test('nested rule map (bool)', () {
         var options = <Object, Object?>{};
         var lintOptions = {