linter: extract ruleConfigs earlier

Each call site for `parseLintRuleConfigs`, `processAnalysisOptionsFile`
immediately just want the `ruleConfigs` (which is the only property of a
LintConfig). So instead of passing around LintConfigs, just pass around
the list of RuleConfigs.

(LintConfig can probably be deleted now, but I'll look at that for a
separate change; might need care with google3.)

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

Author: srawlins

pkg/analyzer/lib/src/generated/engine.dart

@@ -506,9 +506,9 @@ class AnalysisOptionsImpl implements AnalysisOptions {
     var formatter = optionsMap.valueAt(AnalyzerOptions.formatter);
     builder._applyFormatterOptions(formatter);
 
-    var lintConfig = parseLintConfig(optionsMap);
-    if (lintConfig != null) {
-      var enabledRules = Registry.ruleRegistry.enabled(lintConfig);
+    var ruleConfigs = parseLintRuleConfigs(optionsMap);
+    if (ruleConfigs != null) {
+      var enabledRules = Registry.ruleRegistry.enabled(ruleConfigs);
       if (enabledRules.isNotEmpty) {
         builder.lint = true;
         builder.lintRules = enabledRules.toList();

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

@@ -8,55 +8,32 @@ import 'package:yaml/yaml.dart';
 
 /// Parses [optionsMap] into a [LintConfig], returning the config, or `null` if
 /// [optionsMap] does not have `linter` map.
-LintConfig? parseLintConfig(YamlMap optionsMap) {
+List<RuleConfig>? parseLintRuleConfigs(YamlMap optionsMap) {
   var options = optionsMap.valueAt('linter');
   // Quick check of basic contract.
   if (options is YamlMap) {
-    return LintConfig.parseMap(options);
+    return LintConfig.parseMap(options).ruleConfigs;
   }
 
   return null;
 }
 
-/// 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.
-LintConfig? processAnalysisOptionsFile(String fileContents, {String? fileUrl}) {
-  var yaml = loadYamlNode(fileContents,
-      sourceUrl: fileUrl != null ? Uri.parse(fileUrl) : null);
-  if (yaml is YamlMap) {
-    return parseLintConfig(yaml);
-  }
-  return null;
-}
-
 /// The configuration of lint rules within an analysis options file.
 class LintConfig {
   final List<RuleConfig> ruleConfigs;
 
-  LintConfig(this.ruleConfigs);
-
-  factory LintConfig.parse(String source, {String? sourceUrl}) {
-    var yaml = loadYamlNode(source,
-        sourceUrl: sourceUrl != null ? Uri.parse(sourceUrl) : null);
-    if (yaml is! YamlMap) {
-      throw StateError("Expected YAML at '$source' to be a Map, but is "
-          '${yaml.runtimeType}.');
-    }
-
-    return LintConfig.parseMap(yaml);
-  }
-
   factory LintConfig.parseMap(YamlMap yaml) {
     var ruleConfigs = <RuleConfig>[];
     var rulesNode = yaml.valueAt(AnalyzerOptions.rules);
     if (rulesNode != null) {
       ruleConfigs.addAll(_ruleConfigs(rulesNode));
     }
 
-    return LintConfig(ruleConfigs);
+    return LintConfig._(ruleConfigs);
   }
 
+  LintConfig._(this.ruleConfigs);
+
   static bool? _asBool(YamlNode scalar) {
     var value = scalar is YamlScalar ? scalar.valueOrThrow : scalar;
     return switch (value) {
@@ -135,8 +112,10 @@ class RuleConfig {
 
   RuleConfig({required this.name, required this.args, this.group});
 
+  /// Returns whether [ruleName] is disabled in this configuration.
   bool disables(String ruleName) =>
       ruleName == name && args['enabled'] == false;
 
+  /// Returns whether [ruleName] is enabled in this configuration.
   bool enables(String ruleName) => ruleName == name && args['enabled'] == true;
 }

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

@@ -39,8 +39,8 @@ class Registry with IterableMixin<LintRule> {
   /// enables `my_rule`.
   ///
   /// Unspecified rules are treated as disabled by default.
-  Iterable<LintRule> enabled(LintConfig config) => rules
-      .where((rule) => config.ruleConfigs.any((rc) => rc.enables(rule.name)));
+  Iterable<LintRule> enabled(List<RuleConfig> ruleConfigs) =>
+      rules.where((rule) => ruleConfigs.any((rc) => rc.enables(rule.name)));
 
   /// Return the lint rule with the given [name].
   LintRule? getRule(String name) => _ruleMap[name];

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

@@ -13,7 +13,28 @@ main() {
 }
 
 defineTests() {
-  const src = """
+  /// 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) {
+    var yaml = loadYamlNode(fileContents);
+    if (yaml is YamlMap) {
+      return parseLintRuleConfigs(yaml);
+    }
+    return null;
+  }
+
+  // In the future, options might be marshaled in maps and passed to rules.
+  //  acme:
+  //    some_rule:
+  //      some_option: # Note this nesting might be arbitrarily complex?
+  //        - param1
+  //        - param2
+
+  group('lint config', () {
+    group('rule', () {
+      test('configs', () {
+        var optionsYaml = loadYamlNode('''
 files:
   include: foo # un-quoted
   exclude:
@@ -25,28 +46,16 @@ rules:
     camel_case_types: true #enable
   pub:
     package_names: false
-""";
-
-// In the future, options might be marshaled in maps and passed to rules.
-//  acme:
-//    some_rule:
-//      some_option: # Note this nesting might be arbitrarily complex?
-//        - param1
-//        - param2
-
-  var config = LintConfig.parse(src);
-
-  group('lint config', () {
-    group('rule', () {
-      test('configs', () {
+''');
+        var config = LintConfig.parseMap(optionsYaml as YamlMap);
         expect(config.ruleConfigs, hasLength(3));
       });
 
       test('config', () {
-        config = LintConfig.parse('''
+        var config = LintConfig.parseMap(loadYamlNode('''
 rules:
   style_guide:
-    unnecessary_getters: false''');
+    unnecessary_getters: false''') as YamlMap);
         expect(config.ruleConfigs, hasLength(1));
         var ruleConfig = config.ruleConfigs[0];
         expect(ruleConfig.group, 'style_guide');
@@ -72,8 +81,8 @@ linter:
       unnecessary_getters: false #disable
       camel_case_types: true #enable
 ''';
-          var config = processAnalysisOptionsFile(src)!;
-          var ruleNames = config.ruleConfigs.map((rc) => rc.name);
+          var ruleConfigs = processAnalysisOptionsFile(src)!;
+          var ruleNames = ruleConfigs.map((rc) => rc.name);
           expect(ruleNames, hasLength(2));
           expect(ruleNames, contains('unnecessary_getters'));
           expect(ruleNames, contains('camel_case_types'));
@@ -91,10 +100,10 @@ linter:
   rules:
     - camel_case_types
 ''';
-          var config = processAnalysisOptionsFile(src)!;
-          expect(config.ruleConfigs.length, 1);
+          var ruleConfigs = processAnalysisOptionsFile(src)!;
+          expect(ruleConfigs, hasLength(1));
           // Verify that defaults are enabled.
-          expect(config.ruleConfigs[0].args['enabled'], isTrue);
+          expect(ruleConfigs[0].args['enabled'], isTrue);
         });
 
         test('rule map (bools)', () {
@@ -108,8 +117,7 @@ linter:
     camel_case_types: true #enable
     unnecessary_getters: false #disable
 ''';
-          var config = processAnalysisOptionsFile(src)!;
-          var ruleConfigs = config.ruleConfigs.toList();
+          var ruleConfigs = processAnalysisOptionsFile(src)!;
           ruleConfigs.sort((RuleConfig rc1, RuleConfig rc2) =>
               rc1.name!.compareTo(rc2.name!));
           expect(ruleConfigs, hasLength(2));
@@ -132,8 +140,8 @@ linter:
 
   group('options processing', () {
     group('raw maps', () {
-      LintConfig parseMap(Map<Object, Object?> map) {
-        return parseLintConfig(wrap(map) as YamlMap)!;
+      List<RuleConfig> parseMap(Map<Object, Object?> map) {
+        return parseLintRuleConfigs(wrap(map) as YamlMap)!;
       }
 
       test('rule list', () {
@@ -143,9 +151,9 @@ linter:
         };
         options['linter'] = lintOptions;
 
-        var config = parseMap(options);
-        expect(config, isNotNull);
-        expect(config.ruleConfigs, hasLength(1));
+        var ruleConfigs = parseMap(options);
+        expect(ruleConfigs, isNotNull);
+        expect(ruleConfigs, hasLength(1));
       });
 
       test('rule map (bool)', () {
@@ -155,9 +163,9 @@ linter:
         };
         options['linter'] = lintOptions;
 
-        var config = parseMap(options);
-        expect(config, isNotNull);
-        expect(config.ruleConfigs, hasLength(1));
+        var ruleConfigs = parseMap(options);
+        expect(ruleConfigs, isNotNull);
+        expect(ruleConfigs, hasLength(1));
       });
 
       test('rule map (string)', () {
@@ -167,9 +175,9 @@ linter:
         };
         options['linter'] = lintOptions;
 
-        var config = parseMap(options);
-        expect(config, isNotNull);
-        expect(config.ruleConfigs, hasLength(1));
+        var ruleConfigs = parseMap(options);
+        expect(ruleConfigs, isNotNull);
+        expect(ruleConfigs, hasLength(1));
       });
 
       test('nested rule map (bool)', () {
@@ -181,9 +189,9 @@ linter:
         };
         options['linter'] = lintOptions;
 
-        var config = parseMap(options);
-        expect(config, isNotNull);
-        expect(config.ruleConfigs, hasLength(1));
+        var ruleConfigs = parseMap(options);
+        expect(ruleConfigs, isNotNull);
+        expect(ruleConfigs, hasLength(1));
       });
 
       test('nested rule map (string)', () {
@@ -195,9 +203,9 @@ linter:
         };
         options['linter'] = lintOptions;
 
-        var config = parseMap(options);
-        expect(config, isNotNull);
-        expect(config.ruleConfigs, hasLength(1));
+        var ruleConfigs = parseMap(options);
+        expect(ruleConfigs, isNotNull);
+        expect(ruleConfigs, hasLength(1));
       });
     });
   });

pkg/linter/tool/benchmark.dart

@@ -15,6 +15,7 @@ import 'package:linter/src/extensions.dart';
 import 'package:linter/src/rules.dart';
 import 'package:linter/src/test_utilities/formatter.dart';
 import 'package:linter/src/test_utilities/test_linter.dart';
+import 'package:yaml/yaml.dart';
 
 import 'lint_sets.dart';
 
@@ -93,7 +94,8 @@ Future<void> runLinter(List<String> args) async {
 
   LinterOptions linterOptions;
   if (configFile is String) {
-    var config = LintConfig.parse(readFile(configFile));
+    var optionsContent = readFile(configFile);
+    var config = LintConfig.parseMap(loadYamlNode(optionsContent) as YamlMap);
     var enabledRules = Registry.ruleRegistry.where((LintRule rule) =>
         !config.ruleConfigs.any((rc) => rc.disables(rule.name)));
 

pkg/linter/tool/lint_sets.dart

@@ -7,6 +7,7 @@ import 'dart:async';
 import 'package:analyzer/src/lint/config.dart';
 import 'package:http/http.dart' as http;
 import 'package:linter/src/utils.dart';
+import 'package:yaml/yaml.dart';
 
 Future<List<String>> get dartCoreLints =>
     _fetchRulesFromGitHub('/dart-lang/lints/blob/main/lib/core.yaml');
@@ -23,10 +24,17 @@ Future<List<String>> get flutterUserLints => _fetchRulesFromGitHub(
 Future<List<String>> _fetchRulesFromGitHub(String optionsPath) async {
   var optionsUrl = Uri.https('raw.githubusercontent.com', optionsPath);
   var req = await http.get(optionsUrl);
-  var config = processAnalysisOptionsFile(req.body);
-  if (config == null) {
+
+  var optionsYaml = loadYamlNode(req.body);
+  if (optionsYaml is! YamlMap) {
+    printToConsole('No YAML map found for: $optionsUrl (SKIPPED)');
+    return [];
+  }
+
+  var ruleConfigs = parseLintRuleConfigs(optionsYaml);
+  if (ruleConfigs == null) {
     printToConsole('No config found for: $optionsUrl (SKIPPED)');
     return [];
   }
-  return config.ruleConfigs.map((r) => r.name).nonNulls.toList(growable: false);
+  return ruleConfigs.map((r) => r.name).nonNulls.toList(growable: false);
 }